Re: [PATCH v2 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Laurent, On Tuesday 29 December 2015 11:38:39 Laurent Pinchart wrote: > Hi Markus, > > On Wednesday 16 December 2015 11:14:28 Markus Pargmann wrote: > > On Wednesday 16 December 2015 09:47:58 Laurent Pinchart wrote: > > > On Monday 14 December 2015 15:41:53 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 > > > > > > Please see below for a few comments. If you agree about them there's no > > > need to resubmit, I'll fix the patch when applying. > > > > Most of them are fine, I commented on the open ones. > > > > >> --- > > >> > > >> drivers/media/i2c/mt9v032.c | 153 - > > >> 1 file changed, 152 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > >> index cc16acf001de..6cbc3b87eda9 100644 > > >> --- a/drivers/media/i2c/mt9v032.c > > >> +++ b/drivers/media/i2c/mt9v032.c > > [snip] > > > >> +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= MT9V032_TOTAL_SHUTTER_WIDTH_MAX, > > > > > > According the the MT9V032 datasheet I have, the maximum value is 2047 > > > while MT9V032_TOTAL_SHUTTER_WIDTH_MAX is defined as 32767. Do you have any > > > information that would hint for an error in the datasheet ? > > > > The register is defined as having 15 bits. I simply assumed that the already > > defined TOTAL_SHUTTER_WIDTH_MAX would apply for this register as well. At > > least it should end up controlling the same property of the chip. I didn't > > test this on mt9v032 but on mt9v024. > > According to the MT9V032 datasheet > (http://www.onsemi.com/pub/Collateral/MT9V032-D.PDF) the maximum shutter > width > in AEC mode is limited to 2047. That is documented both in the Maximum Total > Shutter Width register legal values and in the "Automatic Gain Control and > Automatic Exposure Control" section: > > "The exposure is measured in row-time by reading R0xBB. The exposure range is > 1 to 2047." > > I assume that the the AEC unit limits the shutter width to 2047 lines and > that > it's thus pointless to set the maximum total shutter width to a higher value. > Whether doing so could have any adverse effect I don't know, but better be > same than sorry. If you agree we should limit the value to 2047 I can fix > this. Yes, I agree. It would be great if you fix this. Thanks, Markus > > > >> +.step = 1, > > >> +.def= MT9V032_TOTAL_SHUTTER_WIDTH_DEF, > > >> +.flags = 0, > > >> +}; > > >> + > > >> +static const struct v4l2_ctrl_config mt9v034_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, > > >> +.step = 1, > > >> +.def= MT9V032_TOTAL_SHUTTER_WIDTH_DEF, > > >> +.flags = 0, > > >> +}; > > > > > > [snip] > > -- 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- | signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Markus, On Wednesday 16 December 2015 11:14:28 Markus Pargmann wrote: > On Wednesday 16 December 2015 09:47:58 Laurent Pinchart wrote: > > On Monday 14 December 2015 15:41:53 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 > > > > Please see below for a few comments. If you agree about them there's no > > need to resubmit, I'll fix the patch when applying. > > Most of them are fine, I commented on the open ones. > > >> --- > >> > >> drivers/media/i2c/mt9v032.c | 153 - > >> 1 file changed, 152 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > >> index cc16acf001de..6cbc3b87eda9 100644 > >> --- a/drivers/media/i2c/mt9v032.c > >> +++ b/drivers/media/i2c/mt9v032.c [snip] > >> +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= MT9V032_TOTAL_SHUTTER_WIDTH_MAX, > > > > According the the MT9V032 datasheet I have, the maximum value is 2047 > > while MT9V032_TOTAL_SHUTTER_WIDTH_MAX is defined as 32767. Do you have any > > information that would hint for an error in the datasheet ? > > The register is defined as having 15 bits. I simply assumed that the already > defined TOTAL_SHUTTER_WIDTH_MAX would apply for this register as well. At > least it should end up controlling the same property of the chip. I didn't > test this on mt9v032 but on mt9v024. According to the MT9V032 datasheet (http://www.onsemi.com/pub/Collateral/MT9V032-D.PDF) the maximum shutter width in AEC mode is limited to 2047. That is documented both in the Maximum Total Shutter Width register legal values and in the "Automatic Gain Control and Automatic Exposure Control" section: "The exposure is measured in row-time by reading R0xBB. The exposure range is 1 to 2047." I assume that the the AEC unit limits the shutter width to 2047 lines and that it's thus pointless to set the maximum total shutter width to a higher value. Whether doing so could have any adverse effect I don't know, but better be same than sorry. If you agree we should limit the value to 2047 I can fix this. > >> + .step = 1, > >> + .def= MT9V032_TOTAL_SHUTTER_WIDTH_DEF, > >> + .flags = 0, > >> +}; > >> + > >> +static const struct v4l2_ctrl_config mt9v034_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, > >> + .step = 1, > >> + .def= MT9V032_TOTAL_SHUTTER_WIDTH_DEF, > >> + .flags = 0, > >> +}; > > > > [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Laurent, On Wednesday 16 December 2015 09:47:58 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Monday 14 December 2015 15:41:53 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 > > Please see below for a few comments. If you agree about them there's no need > to resubmit, I'll fix the patch when applying. Most of them are fine, I commented on the open ones. > > > --- > > drivers/media/i2c/mt9v032.c | 153 - > > 1 file changed, 152 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index cc16acf001de..6cbc3b87eda9 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > [snip] > > > enum mt9v032_model { > > @@ -162,6 +169,8 @@ struct mt9v032_model_data { > > unsigned int min_shutter; > > unsigned int max_shutter; > > unsigned int pclk_reg; > > + unsigned int aec_max_shutter_reg; > > + const struct v4l2_ctrl_config * const aec_max_shutter_v4l2_ctrl; > > }; > > > > struct mt9v032_model_info { > > @@ -175,6 +184,9 @@ static const struct mt9v032_model_version > > mt9v032_versions[] = { { MT9V034_CHIP_ID_REV1, "MT9V024/MT9V034 rev1" }, > > }; > > > > +static const struct v4l2_ctrl_config mt9v032_aec_max_shutter_width; > > +static const struct v4l2_ctrl_config mt9v034_aec_max_shutter_width; > > We can avoid forward declarations by moving the mt9v032_model_data array > further down in the driver. > > > static const struct mt9v032_model_data mt9v032_model_data[] = { > > { > > /* MT9V022, MT9V032 revisions 1/2/3 */ > > [snip] > > > @@ 647,6 +663,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. > > + */ > > +#define V4L2_CID_AEGC_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 > > + * next exp = Current exp + ((Calculated new exp - current > > exp) / > 2^LPF) > > Over 80 columns, you can fix it by just reducing the indentation by one tab. > > > + */ > > +#define V4L2_CID_AEC_LPF (V4L2_CID_USER_BASE | 0x1003) > > +#define V4L2_CID_AGC_LPF (V4L2_CID_USER_BASE | 0x1004) > > +/* > > + * Value between 0 and 15. This is the number of frames being skipped > > before > > + * updating the auto exposure/gain. > > + */ > > +#define V4L2_CID_AEC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1005) > > +#define V4L2_CID_AGC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1006) > > +/* > > + * Maximum shutter width used for AEC. > > + */ > > +#define V4L2_CID_AEC_MAX_SHUTTER_WIDTH (V4L2_CID_USER_BASE | 0x1007) > > [snip] > > > @@ -745,6 +810,84 @@ static const struct v4l2_ctrl_config > > mt9v032_test_pattern_color = { .flags = 0, > > }; > > > > +static const struct v4l2_ctrl_config mt9v032_aegc_controls[] = { > > + { > > + .ops= &mt9v032_ctrl_ops, > > + .id = V4L2_CID_AEGC_DESIRED_BIN, > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .name = "aec_agc_desired_bin", > > I forgot to reply to your e-mail asking what proper controls names would be, > sorry. > > V4L2 control names contain spaces and use uppercase as needed. This one could > be "AEC/AGC Desired Bin" for instance. Ah I see. I was just wondering as v4l2-ctl showed everything with lowercase letters and underscores. But with a closer look it seems something between driver and v4l2-ctl translates them from uppercase/spaces to lowercase/underscores. So yes that's fine then and makes sense. > > > + .min= 1, > > + .max= 64, > > + .step = 1, > > + .def= 58, > > + .flags = 0, > > + }, { > > + .ops= &mt9v032_ctrl_ops, > > + .id = V4L2_CID_AEC_LPF, > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .name = "aec_lpf", > > + .min= 0, > > + .max= 2, > > + .step
Re: [PATCH v2 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
Hi Markus, Thank you for the patch. On Monday 14 December 2015 15:41:53 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 Please see below for a few comments. If you agree about them there's no need to resubmit, I'll fix the patch when applying. > --- > drivers/media/i2c/mt9v032.c | 153 - > 1 file changed, 152 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index cc16acf001de..6cbc3b87eda9 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c [snip] > enum mt9v032_model { > @@ -162,6 +169,8 @@ struct mt9v032_model_data { > unsigned int min_shutter; > unsigned int max_shutter; > unsigned int pclk_reg; > + unsigned int aec_max_shutter_reg; > + const struct v4l2_ctrl_config * const aec_max_shutter_v4l2_ctrl; > }; > > struct mt9v032_model_info { > @@ -175,6 +184,9 @@ static const struct mt9v032_model_version > mt9v032_versions[] = { { MT9V034_CHIP_ID_REV1, "MT9V024/MT9V034 rev1" }, > }; > > +static const struct v4l2_ctrl_config mt9v032_aec_max_shutter_width; > +static const struct v4l2_ctrl_config mt9v034_aec_max_shutter_width; We can avoid forward declarations by moving the mt9v032_model_data array further down in the driver. > static const struct mt9v032_model_data mt9v032_model_data[] = { > { > /* MT9V022, MT9V032 revisions 1/2/3 */ [snip] > @@ 647,6 +663,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. > + */ > +#define V4L2_CID_AEGC_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 > + * next exp = Current exp + ((Calculated new exp - current exp) / 2^LPF) Over 80 columns, you can fix it by just reducing the indentation by one tab. > + */ > +#define V4L2_CID_AEC_LPF (V4L2_CID_USER_BASE | 0x1003) > +#define V4L2_CID_AGC_LPF (V4L2_CID_USER_BASE | 0x1004) > +/* > + * Value between 0 and 15. This is the number of frames being skipped > before > + * updating the auto exposure/gain. > + */ > +#define V4L2_CID_AEC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1005) > +#define V4L2_CID_AGC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1006) > +/* > + * Maximum shutter width used for AEC. > + */ > +#define V4L2_CID_AEC_MAX_SHUTTER_WIDTH (V4L2_CID_USER_BASE | 0x1007) [snip] > @@ -745,6 +810,84 @@ static const struct v4l2_ctrl_config > mt9v032_test_pattern_color = { .flags = 0, > }; > > +static const struct v4l2_ctrl_config mt9v032_aegc_controls[] = { > + { > + .ops= &mt9v032_ctrl_ops, > + .id = V4L2_CID_AEGC_DESIRED_BIN, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "aec_agc_desired_bin", I forgot to reply to your e-mail asking what proper controls names would be, sorry. V4L2 control names contain spaces and use uppercase as needed. This one could be "AEC/AGC Desired Bin" for instance. > + .min= 1, > + .max= 64, > + .step = 1, > + .def= 58, > + .flags = 0, > + }, { > + .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, > + }, { > + .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, > + }, { > + .ops= &mt9v032_ctrl_ops, > + .id = V4L2_CID_AEC_UPDATE_INTERVAL, > + .type
[PATCH v2 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC
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, 152 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c index cc16acf001de..6cbc3b87eda9 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_AEGC_DESIRED_BIN 0xa5 +#define MT9V032_AEC_UPDATE_FREQUENCY 0xa6 +#define MT9V032_AEC_LPF0xa8 +#define MT9V032_AGC_UPDATE_FREQUENCY 0xa9 +#define MT9V032_AGC_LPF0xaa #define MT9V032_AEC_AGC_ENABLE 0xaf #defineMT9V032_AEC_ENABLE (1 << 0) #defineMT9V032_AGC_ENABLE (1 << 1) +#define MT9V034_AEC_MAX_SHUTTER_WIDTH 0xad +#define MT9V032_AEC_MAX_SHUTTER_WIDTH 0xbd #define MT9V032_THERMAL_INFO 0xc1 enum mt9v032_model { @@ -162,6 +169,8 @@ struct mt9v032_model_data { unsigned int min_shutter; unsigned int max_shutter; unsigned int pclk_reg; + unsigned int aec_max_shutter_reg; + const struct v4l2_ctrl_config * const aec_max_shutter_v4l2_ctrl; }; struct mt9v032_model_info { @@ -175,6 +184,9 @@ static const struct mt9v032_model_version mt9v032_versions[] = { { MT9V034_CHIP_ID_REV1, "MT9V024/MT9V034 rev1" }, }; +static const struct v4l2_ctrl_config mt9v032_aec_max_shutter_width; +static const struct v4l2_ctrl_config mt9v034_aec_max_shutter_width; + static const struct mt9v032_model_data mt9v032_model_data[] = { { /* MT9V022, MT9V032 revisions 1/2/3 */ @@ -185,6 +197,8 @@ 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, + .aec_max_shutter_v4l2_ctrl = &mt9v032_aec_max_shutter_width, }, { /* MT9V024, MT9V034 */ .min_row_time = 690, @@ -194,6 +208,8 @@ 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 = MT9V034_AEC_MAX_SHUTTER_WIDTH, + .aec_max_shutter_v4l2_ctrl = &mt9v034_aec_max_shutter_width, }, }; @@ -647,6 +663,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. + */ +#define V4L2_CID_AEGC_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 + * next exp = Current exp + ((Calculated new exp - current exp) / 2^LPF) + */ +#define V4L2_CID_AEC_LPF (V4L2_CID_USER_BASE | 0x1003) +#define V4L2_CID_AGC_LPF (V4L2_CID_USER_BASE | 0x1004) +/* + * Value between 0 and 15. This is the number of frames being skipped before + * updating the auto exposure/gain. + */ +#define V4L2_CID_AEC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1005) +#define V4L2_CID_AGC_UPDATE_INTERVAL (V4L2_CID_USER_BASE | 0x1006) +/* + * Maximum shutter width used for AEC. + */ +#define V4L2_CID_AEC_MAX_SHUTTER_WIDTH (V4L2_CID_USER_BASE | 0x1007) static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl) { @@ -716,6 +759,28 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl) break; } return regmap_write(map, MT9V032_TEST_PATTERN, data); + + case V4L2_CID_AEGC_DESIRED_BIN: + return regmap_write(map, MT9V032_AEGC_DESIRED_BIN, ctrl->val); + + case V4L2_CID_AEC_LPF: + return regmap_w