Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Markus,
Thank you for the patch.
On Friday 06 November 2015 14:13:45 Markus Pargmann wrote:
> This patch adds V4L2 controls for Auto Exposure Control and Auto Gain
> Control settings. These settings include low pass filter, update
> frequency of these settings and the update interval for those units.
>
> Signed-off-by: Markus Pargmann
> ---
> drivers/media/i2c/mt9v032.c | 153 +
> 1 file changed, 153 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 943c3f39ea73..978ae8cbb0cc 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -133,9 +133,16 @@
> #define MT9V032_TEST_PATTERN_GRAY_DIAGONAL (3 << 11)
> #define MT9V032_TEST_PATTERN_ENABLE (1 << 13)
> #define MT9V032_TEST_PATTERN_FLIP (1 << 14)
> +#define MT9V032_AEC_LPF 0xa8
> +#define MT9V032_AGC_LPF 0xaa
> +#define MT9V032_DESIRED_BIN 0xa5
To better match the datasheet, could you call this MT9V032_AEGC_DESIRED_BIN ?
Same comment for the related control name.
> +#define MT9V032_AEC_UPDATE_INTERVAL 0xa6
> +#define MT9V032_AGC_UPDATE_INTERVAL 0xa9
Simalarly I'd call these two registers MT9V032_AEC_UPDATE_FREQUENCY and
MT9V032_AGC_UPDATE_FREQUENCY as that's how they're named in the datasheet (at
least the version I have). It makes sense to keep using interval in the
control names though, as that's how they operate.
Could you please keep the registers sorted numerically ?
> #define MT9V032_AEC_AGC_ENABLE 0xaf
> #define MT9V032_AEC_ENABLE (1 << 0)
> #define MT9V032_AGC_ENABLE (1 << 1)
> +#define MT9V024_AEC_MAX_SHUTTER_WIDTH0xad
As other registers specific to the MT9V024 and MT9V034 use the MT9V034 prefix,
could you do so here as well ?
Would it make sense to add the minimum shutter width too ?
> +#define MT9V032_AEC_MAX_SHUTTER_WIDTH0xbd
> #define MT9V032_THERMAL_INFO 0xc1
>
> enum mt9v032_model {
> @@ -162,6 +169,7 @@ struct mt9v032_model_data {
> unsigned int min_shutter;
> unsigned int max_shutter;
> unsigned int pclk_reg;
> + unsigned int aec_max_shutter_reg;
> };
>
> struct mt9v032_model_info {
> @@ -185,6 +193,7 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { .min_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> .max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
> .pclk_reg = MT9V032_PIXEL_CLOCK,
> + .aec_max_shutter_reg = MT9V032_AEC_MAX_SHUTTER_WIDTH,
> }, {
> /* MT9V024, MT9V034 */
> .min_row_time = 690,
> @@ -194,6 +203,7 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { .min_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MIN,
> .max_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MAX,
> .pclk_reg = MT9V034_PIXEL_CLOCK,
> + .aec_max_shutter_reg = MT9V024_AEC_MAX_SHUTTER_WIDTH,
> },
> };
>
> @@ -265,6 +275,12 @@ struct mt9v032 {
> struct {
> struct v4l2_ctrl *test_pattern;
> struct v4l2_ctrl *test_pattern_color;
> + struct v4l2_ctrl *desired_bin;
> + struct v4l2_ctrl *aec_lpf;
> + struct v4l2_ctrl *agc_lpf;
> + struct v4l2_ctrl *aec_update_interval;
> + struct v4l2_ctrl *agc_update_interval;
> + struct v4l2_ctrl *aec_max_shutter_width;
You don't need to store all those controls in the mt9v032 structure as you
don't use the pointers anywhere. The reason why the test_pattern and
test_pattern_color controls are stored there is that they both affect the same
register and are thus grouped into a control cluster.
> };
> };
>
> @@ -643,6 +659,33 @@ static int mt9v032_set_selection(struct v4l2_subdev
> *subdev, */
>
> #define V4L2_CID_TEST_PATTERN_COLOR (V4L2_CID_USER_BASE | 0x1001)
> +/*
> + * Value between 1 and 64 to set the desired bin. This is effectively a
> measure
> + * of how bright the image is supposed to be. Both AGC and AEC try to reach
> + * this.
> + */
Do you know what the value represents exactly ? Does it have a linear
relationship with the overall image luminance ? Is it related to image binning
at all ?
> +#define V4L2_CID_DESIRED_BIN (V4L2_CID_USER_BASE | 0x1002)
> +/*
> + * LPF is the low pass filter capability of the chip. Both AEC and AGC have
> + * this setting. This limits the speed in which AGC/AEC adjust their
> settings.
> + * Possible values are 0-2. 0 means no LPF. For 1 and 2 this equation is
> used:
> + * if |(Calculated new exp - current exp)| > (current exp / 4)
> + * next exp = Calculated new exp
> + * else
>
Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi,
On Monday 09 November 2015 15:22:06 Laurent Pinchart wrote:
> Hi Markus,
>
> Thank you for the patch.
>
> On Friday 06 November 2015 14:13:45 Markus Pargmann wrote:
> > This patch adds V4L2 controls for Auto Exposure Control and Auto Gain
> > Control settings. These settings include low pass filter, update
> > frequency of these settings and the update interval for those units.
> >
> > Signed-off-by: Markus Pargmann
> > ---
> > drivers/media/i2c/mt9v032.c | 153 +
> > 1 file changed, 153 insertions(+)
> >
> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> > index 943c3f39ea73..978ae8cbb0cc 100644
> > --- a/drivers/media/i2c/mt9v032.c
> > +++ b/drivers/media/i2c/mt9v032.c
> > @@ -133,9 +133,16 @@
> > #defineMT9V032_TEST_PATTERN_GRAY_DIAGONAL (3 << 11)
> > #defineMT9V032_TEST_PATTERN_ENABLE (1 << 13)
> > #defineMT9V032_TEST_PATTERN_FLIP (1 << 14)
> > +#define MT9V032_AEC_LPF0xa8
> > +#define MT9V032_AGC_LPF0xaa
> > +#define MT9V032_DESIRED_BIN0xa5
>
> To better match the datasheet, could you call this MT9V032_AEGC_DESIRED_BIN ?
> Same comment for the related control name.
Ok, fixed for next version.
>
> > +#define MT9V032_AEC_UPDATE_INTERVAL0xa6
> > +#define MT9V032_AGC_UPDATE_INTERVAL0xa9
>
> Simalarly I'd call these two registers MT9V032_AEC_UPDATE_FREQUENCY and
> MT9V032_AGC_UPDATE_FREQUENCY as that's how they're named in the datasheet (at
> least the version I have). It makes sense to keep using interval in the
> control names though, as that's how they operate.
Yes they are called differently, fixed.
>
> Could you please keep the registers sorted numerically ?
Yes sorry.
>
> > #define MT9V032_AEC_AGC_ENABLE 0xaf
> > #defineMT9V032_AEC_ENABLE (1 << 0)
> > #defineMT9V032_AGC_ENABLE (1 << 1)
> > +#define MT9V024_AEC_MAX_SHUTTER_WIDTH 0xad
>
> As other registers specific to the MT9V024 and MT9V034 use the MT9V034
> prefix,
> could you do so here as well ?
Yes, fixed.
>
> Would it make sense to add the minimum shutter width too ?
Yes perhaps, I personally just needed the extra exposure to get the image
brighter. However I don't have any information about the minimum register. For
mt9v032 this seems to be hardwired to 1 and for mt9v024 this is just mentioned
in text without further information.
>
> > +#define MT9V032_AEC_MAX_SHUTTER_WIDTH 0xbd
> > #define MT9V032_THERMAL_INFO 0xc1
> >
> > enum mt9v032_model {
> > @@ -162,6 +169,7 @@ struct mt9v032_model_data {
> > unsigned int min_shutter;
> > unsigned int max_shutter;
> > unsigned int pclk_reg;
> > + unsigned int aec_max_shutter_reg;
> > };
> >
> > struct mt9v032_model_info {
> > @@ -185,6 +193,7 @@ static const struct mt9v032_model_data
> > mt9v032_model_data[] = { .min_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
> > .max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
> > .pclk_reg = MT9V032_PIXEL_CLOCK,
> > + .aec_max_shutter_reg = MT9V032_AEC_MAX_SHUTTER_WIDTH,
> > }, {
> > /* MT9V024, MT9V034 */
> > .min_row_time = 690,
> > @@ -194,6 +203,7 @@ static const struct mt9v032_model_data
> > mt9v032_model_data[] = { .min_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MIN,
> > .max_shutter = MT9V034_TOTAL_SHUTTER_WIDTH_MAX,
> > .pclk_reg = MT9V034_PIXEL_CLOCK,
> > + .aec_max_shutter_reg = MT9V024_AEC_MAX_SHUTTER_WIDTH,
> > },
> > };
> >
> > @@ -265,6 +275,12 @@ struct mt9v032 {
> > struct {
> > struct v4l2_ctrl *test_pattern;
> > struct v4l2_ctrl *test_pattern_color;
> > + struct v4l2_ctrl *desired_bin;
> > + struct v4l2_ctrl *aec_lpf;
> > + struct v4l2_ctrl *agc_lpf;
> > + struct v4l2_ctrl *aec_update_interval;
> > + struct v4l2_ctrl *agc_update_interval;
> > + struct v4l2_ctrl *aec_max_shutter_width;
>
> You don't need to store all those controls in the mt9v032 structure as you
> don't use the pointers anywhere. The reason why the test_pattern and
> test_pattern_color controls are stored there is that they both affect the
> same
> register and are thus grouped into a control cluster.
Thanks, indeed, removed.
>
> > };
> > };
> >
> > @@ -643,6 +659,33 @@ static int mt9v032_set_selection(struct v4l2_subdev
> > *subdev, */
> >
> > #define V4L2_CID_TEST_PATTERN_COLOR(V4L2_CID_USER_BASE | 0x1001)
> > +/*
> > + * Value between 1 and 64 to set the desired bin. This is effectively a
> > measure
> > + * of how bright the image is supposed to be. Both AGC and AEC try to r
Re: [PATCH 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
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
