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
