diff mbox

[5/5] add module parameters for default values

Message ID 1517950905-5015-6-git-send-email-floe@butterbrot.org (mailing list archive)
State New, archived
Headers show

Commit Message

Florian Echtler Feb. 6, 2018, 9:01 p.m. UTC 
To allow setting custom parameters for the sensor directly at startup, the
three primary controls are exposed as module parameters in this patch.

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Hans Verkuil Feb. 6, 2018, 9:31 p.m. UTC | #1 
On 02/06/2018 10:01 PM, Florian Echtler wrote:
> To allow setting custom parameters for the sensor directly at startup, the
> three primary controls are exposed as module parameters in this patch.
> 
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
>  drivers/input/touchscreen/sur40.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 66ef7e6..d1fcb95 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -167,6 +167,17 @@ struct sur40_image_header {
>  #define SUR40_BACKLIGHT_MIN 0x00
>  #define SUR40_BACKLIGHT_DEF 0x01
>  
> +/* module parameters */
> +static uint brightness = SUR40_BRIGHTNESS_DEF;
> +module_param(brightness, uint, 0644);
> +MODULE_PARM_DESC(brightness, "set default brightness");
> +static uint contrast = SUR40_CONTRAST_DEF;
> +module_param(contrast, uint, 0644);
> +MODULE_PARM_DESC(contrast, "set default contrast");
> +static uint gain = SUR40_GAIN_DEF;
> +module_param(gain, uint, 0644);
> +MODULE_PARM_DESC(contrast, "set default gain");

contrast -> gain

Isn't 'initial gain' better than 'default gain'?

If I load this module with gain=X, will the gain control also
start off at X? I didn't see any code for that.

It might be useful to add the allowed range in the description.
E.g.: "set initial gain, range=0-255". Perhaps mention even the
default value, but I'm not sure if that's really needed.

Regards,

	Hans

> +
>  static const struct v4l2_pix_format sur40_pix_format[] = {
>  	{
>  		.pixelformat = V4L2_TCH_FMT_TU08,
> @@ -374,6 +385,11 @@ static void sur40_open(struct input_polled_dev *polldev)
>  
>  	dev_dbg(sur40->dev, "open\n");
>  	sur40_init(sur40);
> +
> +	/* set default values */
> +	sur40_set_irlevel(sur40, brightness & 0xFF);
> +	sur40_set_vsvideo(sur40, ((contrast & 0x0F) << 4) | (gain & 0x0F));
> +	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
>  }
>  
>  /* Disable device, polling has stopped. */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Echtler Feb. 7, 2018, 8:33 a.m. UTC | #2 
On 06.02.2018 22:31, Hans Verkuil wrote:
> On 02/06/2018 10:01 PM, Florian Echtler wrote:
>> To allow setting custom parameters for the sensor directly at startup, the
>> three primary controls are exposed as module parameters in this patch.
>>
>> +/* module parameters */
>> +static uint brightness = SUR40_BRIGHTNESS_DEF;
>> +module_param(brightness, uint, 0644);
>> +MODULE_PARM_DESC(brightness, "set default brightness");
>> +static uint contrast = SUR40_CONTRAST_DEF;
>> +module_param(contrast, uint, 0644);
>> +MODULE_PARM_DESC(contrast, "set default contrast");
>> +static uint gain = SUR40_GAIN_DEF;
>> +module_param(gain, uint, 0644);
>> +MODULE_PARM_DESC(contrast, "set default gain");
>
> contrast -> gain

Ah, typo. Thanks, will fix that.

> Isn't 'initial gain' better than 'default gain'?

Probably correct, yes.

> If I load this module with gain=X, will the gain control also
> start off at X? I didn't see any code for that.

This reminds me: how can I get/set the control from inside the driver?
Should I use something like the following:

struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&sur40->ctrls, V4L2_CID_BRIGHTNESS);
int val = v4l2_ctrl_g_ctrl(ctrl);
// modify val...
v4l2_ctrl_s_ctrl(ctrl, val);

> It might be useful to add the allowed range in the description.
> E.g.: "set initial gain, range=0-255". Perhaps mention even the
> default value, but I'm not sure if that's really needed.

Good point, though - right now the code directly sets the registers without any
clamping, I guess it would be better to call the control framework as mentioned
above?

Best regards, Florian
Hans Verkuil Feb. 7, 2018, 9:02 a.m. UTC | #3 
On 02/07/18 09:33, Florian Echtler wrote:
> On 06.02.2018 22:31, Hans Verkuil wrote:
>> On 02/06/2018 10:01 PM, Florian Echtler wrote:
>>> To allow setting custom parameters for the sensor directly at startup, the
>>> three primary controls are exposed as module parameters in this patch.
>>>
>>> +/* module parameters */
>>> +static uint brightness = SUR40_BRIGHTNESS_DEF;
>>> +module_param(brightness, uint, 0644);
>>> +MODULE_PARM_DESC(brightness, "set default brightness");
>>> +static uint contrast = SUR40_CONTRAST_DEF;
>>> +module_param(contrast, uint, 0644);
>>> +MODULE_PARM_DESC(contrast, "set default contrast");
>>> +static uint gain = SUR40_GAIN_DEF;
>>> +module_param(gain, uint, 0644);
>>> +MODULE_PARM_DESC(contrast, "set default gain");
>>
>> contrast -> gain
> 
> Ah, typo. Thanks, will fix that.
> 
>> Isn't 'initial gain' better than 'default gain'?
> 
> Probably correct, yes.
> 
>> If I load this module with gain=X, will the gain control also
>> start off at X? I didn't see any code for that.
> 
> This reminds me: how can I get/set the control from inside the driver?
> Should I use something like the following:
> 
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&sur40->ctrls, V4L2_CID_BRIGHTNESS);
> int val = v4l2_ctrl_g_ctrl(ctrl);
> // modify val...
> v4l2_ctrl_s_ctrl(ctrl, val);

Yes, that's correct. Usually drivers store the ctrl in their state struct
when they create the control. That way you don't have to find it.

> 
>> It might be useful to add the allowed range in the description.
>> E.g.: "set initial gain, range=0-255". Perhaps mention even the
>> default value, but I'm not sure if that's really needed.
> 
> Good point, though - right now the code directly sets the registers without any
> clamping, I guess it would be better to call the control framework as mentioned
> above?

Easiest is just to use this value for the default when you create the
control. Just clamp it first.

E.g.:

static uint gain = SUR40_GAIN_DEF;
module_param(gain, uint, 0644);

...

gain = clamp(gain, SUR40_GAIN_MIN, SUR40_GAIN_MAX);
v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_GAIN,
	  SUR40_GAIN_MIN, SUR40_GAIN_MAX, 1, gain);

You need to clamp gain first, otherwise v4l2_ctrl_new_std would fail
if the given default value is out of range.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 66ef7e6..d1fcb95 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -167,6 +167,17 @@  struct sur40_image_header {
 #define SUR40_BACKLIGHT_MIN 0x00
 #define SUR40_BACKLIGHT_DEF 0x01
 
+/* module parameters */
+static uint brightness = SUR40_BRIGHTNESS_DEF;
+module_param(brightness, uint, 0644);
+MODULE_PARM_DESC(brightness, "set default brightness");
+static uint contrast = SUR40_CONTRAST_DEF;
+module_param(contrast, uint, 0644);
+MODULE_PARM_DESC(contrast, "set default contrast");
+static uint gain = SUR40_GAIN_DEF;
+module_param(gain, uint, 0644);
+MODULE_PARM_DESC(contrast, "set default gain");
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
 		.pixelformat = V4L2_TCH_FMT_TU08,
@@ -374,6 +385,11 @@  static void sur40_open(struct input_polled_dev *polldev)
 
 	dev_dbg(sur40->dev, "open\n");
 	sur40_init(sur40);
+
+	/* set default values */
+	sur40_set_irlevel(sur40, brightness & 0xFF);
+	sur40_set_vsvideo(sur40, ((contrast & 0x0F) << 4) | (gain & 0x0F));
+	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
 }
 
 /* Disable device, polling has stopped. */