Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC

[Markus Pargmann <mpa@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4804 bytes --]

Hi Laurent,

On Monday 09 November 2015 16:25:02 Markus Pargmann wrote:
> On Monday 09 November 2015 15:22:06 Laurent Pinchart wrote:
[...]
> > 
> > Please use proper controls names.
> 
> Sorry I don't really know what you mean? For me these are proper names.

Could you give me a hint how these names should look like?

Thanks,

Markus

> 
> > 
> > > +	.min		= 1,
> > > +	.max		= 64,
> > > +	.step		= 1,
> > > +	.def		= 58,
> > > +	.flags		= 0,
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_config mt9v032_aec_lpf = {
> > > +	.ops		= &mt9v032_ctrl_ops,
> > > +	.id		= V4L2_CID_AEC_LPF,
> > > +	.type		= V4L2_CTRL_TYPE_INTEGER,
> > > +	.name		= "aec_lpf",
> > > +	.min		= 0,
> > > +	.max		= 2,
> > > +	.step		= 1,
> > > +	.def		= 0,
> > > +	.flags		= 0,
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_config mt9v032_agc_lpf = {
> > > +	.ops		= &mt9v032_ctrl_ops,
> > > +	.id		= V4L2_CID_AGC_LPF,
> > > +	.type		= V4L2_CTRL_TYPE_INTEGER,
> > > +	.name		= "agc_lpf",
> > > +	.min		= 0,
> > > +	.max		= 2,
> > > +	.step		= 1,
> > > +	.def		= 2,
> > > +	.flags		= 0,
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_config mt9v032_aec_update_interval = {
> > > +	.ops		= &mt9v032_ctrl_ops,
> > > +	.id		= V4L2_CID_AEC_UPDATE_INTERVAL,
> > > +	.type		= V4L2_CTRL_TYPE_INTEGER,
> > > +	.name		= "aec_update_interval",
> > > +	.min		= 0,
> > > +	.max		= 16,
> > > +	.step		= 1,
> > > +	.def		= 2,
> > > +	.flags		= 0,
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_config mt9v032_agc_update_interval = {
> > > +	.ops		= &mt9v032_ctrl_ops,
> > > +	.id		= V4L2_CID_AGC_UPDATE_INTERVAL,
> > > +	.type		= V4L2_CTRL_TYPE_INTEGER,
> > > +	.name		= "agc_update_interval",
> > > +	.min		= 0,
> > > +	.max		= 16,
> > > +	.step		= 1,
> > > +	.def		= 2,
> > > +	.flags		= 0,
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_config mt9v032_aec_max_shutter_width = {
> > > +	.ops		= &mt9v032_ctrl_ops,
> > > +	.id		= V4L2_CID_AEC_MAX_SHUTTER_WIDTH,
> > > +	.type		= V4L2_CTRL_TYPE_INTEGER,
> > > +	.name		= "aec_max_shutter_width",
> > > +	.min		= 1,
> > > +	.max		= MT9V034_TOTAL_SHUTTER_WIDTH_MAX,
> > 
> > Isn't the maximum value 2047 for the MT9V0[23]2 ?
> 
> Oh right, these differ by 2. Not really much but will fix it.
> 
> > 
> > > +	.step		= 1,
> > > +	.def		= MT9V032_TOTAL_SHUTTER_WIDTH_DEF,
> > > +	.flags		= 0,
> > > +};
> > > +
> > >  /*
> > > ---------------------------------------------------------------------------
> > > -- * V4L2 subdev core operations
> > >   */
> > > @@ -1010,6 +1147,22 @@ static int mt9v032_probe(struct i2c_client *client,
> > >  				mt9v032_test_pattern_menu);
> > >  	mt9v032->test_pattern_color = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > >  				      &mt9v032_test_pattern_color, NULL);
> > > +	mt9v032->desired_bin = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						    &mt9v032_desired_bin,
> > > +						    NULL);
> > > +	mt9v032->aec_lpf = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						&mt9v032_aec_lpf, NULL);
> > > +	mt9v032->agc_lpf = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						&mt9v032_agc_lpf, NULL);
> > > +	mt9v032->aec_update_interval = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						&mt9v032_aec_update_interval,
> > > +						NULL);
> > > +	mt9v032->agc_update_interval = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						&mt9v032_agc_update_interval,
> > > +						NULL);
> > > +	mt9v032->aec_max_shutter_width = v4l2_ctrl_new_custom(&mt9v032->ctrls,
> > > +						&mt9v032_aec_max_shutter_width,
> > > +						NULL);
> > 
> > As there's no need to store the control pointers I would create an array of 
> > struct v4l2_ctrl_config above instead of defining one variable per control, 
> > and then loop over the array here.
> > 
> >         for (i = 0; i < ARRAY_SIZE(mt9v032_aegc_controls); ++i)
> >                 v4l2_ctrl_new_custom(&mt9v032->ctrls,
> >                                      &mt9v032_aegc_controls[i]);
> > 
> > You should also update the above v4l2_ctrl_handler_init() call to take the new 
> > controls into account, as that will improve performances of the control 
> > framework.
> > 
> >         v4l2_ctrl_handler_init(&mt9v032->ctrls,
> >                                10 + ARRAY_SIZE(mt9v032_aegc_controls));
> 
> Fixed as well. Will send a new version as soon as the proper naming is clear to
> me.
> 
> Thanks,
> 
> Markus
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]