Message ID | 1450104113-6392-1-git-send-email-mpa@pengutronix.de |
---|---|
State | Not Applicable, archived |
Headers | show |
Hi Markus, Thank you for the patch. On Monday 14 December 2015 15:41:51 Markus Pargmann wrote: > Add optional reset and standby gpios. The reset gpio is used to reset > the chip in power_on(). > > The standby gpio is not used currently. It is just unset, so the chip is > not in standby. > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > Acked-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > drivers/media/i2c/mt9v032.c | 28 +++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index > 202565313e82..100f0ae43269 100644 > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > @@ -20,6 +20,8 @@ Optional Properties: > > - link-frequencies: List of allowed link frequencies in Hz. Each frequency > is expressed as a 64-bit big-endian integer. > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. > +- standby-gpios: GPIO handle which is connected to the standby pin of the > chip. > > For further reading on port node refer to > Documentation/devicetree/bindings/media/video-interfaces.txt. > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index a68ce94ee097..c1bc564a0979 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -14,6 +14,7 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/gpio/consumer.h> > #include <linux/i2c.h> > #include <linux/log2.h> > #include <linux/mutex.h> > @@ -251,6 +252,8 @@ struct mt9v032 { > > struct regmap *regmap; > struct clk *clk; > + struct gpio_desc *reset_gpio; > + struct gpio_desc *standby_gpio; > > struct mt9v032_platform_data *pdata; > const struct mt9v032_model_info *model; > @@ -312,16 +315,31 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > struct regmap *map = mt9v032->regmap; > int ret; > > + if (mt9v032->reset_gpio) > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); > + gpiod_set_value_cansleep() already checks whether the gpiod is NULL, you don't need to duplicate the check here. Apart from that, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> No need to resubmit I'll fix this when applying. > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); > if (ret < 0) > return ret; > > + /* System clock has to be enabled before releasing the reset */ > ret = clk_prepare_enable(mt9v032->clk); > if (ret) > return ret; > > udelay(1); > > + if (mt9v032->reset_gpio) { > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > + > + /* After releasing reset we need to wait 10 clock cycles > + * before accessing the sensor over I2C. As the minimum SYSCLK > + * frequency is 13MHz, waiting 1µs will be enough in the worst > + * case. > + */ > + udelay(1); > + } > + > /* Reset the chip and stop data read out */ > ret = regmap_write(map, MT9V032_RESET, 1); > if (ret < 0) > @@ -954,6 +972,16 @@ static int mt9v032_probe(struct i2c_client *client, > if (IS_ERR(mt9v032->clk)) > return PTR_ERR(mt9v032->clk); > > + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(mt9v032->reset_gpio)) > + return PTR_ERR(mt9v032->reset_gpio); > + > + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v032->standby_gpio)) > + return PTR_ERR(mt9v032->standby_gpio); > + > mutex_init(&mt9v032->power_lock); > mt9v032->pdata = pdata; > mt9v032->model = (const void *)did->driver_data;
Hi Markus, Thank you for the patch. On Monday 14 December 2015 15:41:52 Markus Pargmann wrote: > The power_on function of the driver resets the chip and sets the > CHIP_CONTROL register to 0. This switches the operating mode to slave. > The s_stream function sets the correct mode. But this caused problems on > a board where the camera chip is operated as master. The camera started > after a random amount of time streaming an image, I observed between 10 > and 300 seconds. > > The STRFM_OUT and STLN_OUT pins are not connected on this board which > may cause some issues in slave mode. I could not find any documentation > about this. > > Keeping the chip in master mode after the reset helped to fix this > issue for me. > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> and applied to my tree. > --- > drivers/media/i2c/mt9v032.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index c1bc564a0979..cc16acf001de 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -349,7 +349,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > if (ret < 0) > return ret; > > - return regmap_write(map, MT9V032_CHIP_CONTROL, 0); > + return regmap_write(map, MT9V032_CHIP_CONTROL, > + MT9V032_CHIP_CONTROL_MASTER_MODE); > } > > static void mt9v032_power_off(struct mt9v032 *mt9v032) > @@ -421,8 +422,7 @@ __mt9v032_get_pad_crop(struct mt9v032 *mt9v032, struct > v4l2_subdev_pad_config *c > > static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable) > { > - const u16 mode = MT9V032_CHIP_CONTROL_MASTER_MODE > - | MT9V032_CHIP_CONTROL_DOUT_ENABLE > + const u16 mode = MT9V032_CHIP_CONTROL_DOUT_ENABLE > > | MT9V032_CHIP_CONTROL_SEQUENTIAL; > > struct mt9v032 *mt9v032 = to_mt9v032(subdev); > struct v4l2_rect *crop = &mt9v032->crop;
Hi, On Monday 14 December 2015 21:26:25 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Monday 14 December 2015 15:41:51 Markus Pargmann wrote: > > Add optional reset and standby gpios. The reset gpio is used to reset > > the chip in power_on(). > > > > The standby gpio is not used currently. It is just unset, so the chip is > > not in standby. > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > Acked-by: Rob Herring <robh@kernel.org> > > --- > > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > > drivers/media/i2c/mt9v032.c | 28 +++++++++++++++++++ > > 2 files changed, 30 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index > > 202565313e82..100f0ae43269 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > @@ -20,6 +20,8 @@ Optional Properties: > > > > - link-frequencies: List of allowed link frequencies in Hz. Each frequency > > is expressed as a 64-bit big-endian integer. > > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. > > +- standby-gpios: GPIO handle which is connected to the standby pin of the > > chip. > > > > For further reading on port node refer to > > Documentation/devicetree/bindings/media/video-interfaces.txt. > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index a68ce94ee097..c1bc564a0979 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > @@ -14,6 +14,7 @@ > > > > #include <linux/clk.h> > > #include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > #include <linux/i2c.h> > > #include <linux/log2.h> > > #include <linux/mutex.h> > > @@ -251,6 +252,8 @@ struct mt9v032 { > > > > struct regmap *regmap; > > struct clk *clk; > > + struct gpio_desc *reset_gpio; > > + struct gpio_desc *standby_gpio; > > > > struct mt9v032_platform_data *pdata; > > const struct mt9v032_model_info *model; > > @@ -312,16 +315,31 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > > struct regmap *map = mt9v032->regmap; > > int ret; > > > > + if (mt9v032->reset_gpio) > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); > > + > > gpiod_set_value_cansleep() already checks whether the gpiod is NULL, you don't > need to duplicate the check here. > > Apart from that, > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > No need to resubmit I'll fix this when applying. Ok, thank you. Best Regards, Markus > > > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); > > if (ret < 0) > > return ret; > > > > + /* System clock has to be enabled before releasing the reset */ > > ret = clk_prepare_enable(mt9v032->clk); > > if (ret) > > return ret; > > > > udelay(1); > > > > + if (mt9v032->reset_gpio) { > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > > + > > + /* After releasing reset we need to wait 10 clock cycles > > + * before accessing the sensor over I2C. As the minimum SYSCLK > > + * frequency is 13MHz, waiting 1µs will be enough in the worst > > + * case. > > + */ > > + udelay(1); > > + } > > + > > /* Reset the chip and stop data read out */ > > ret = regmap_write(map, MT9V032_RESET, 1); > > if (ret < 0) > > @@ -954,6 +972,16 @@ static int mt9v032_probe(struct i2c_client *client, > > if (IS_ERR(mt9v032->clk)) > > return PTR_ERR(mt9v032->clk); > > > > + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(mt9v032->reset_gpio)) > > + return PTR_ERR(mt9v032->reset_gpio); > > + > > + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(mt9v032->standby_gpio)) > > + return PTR_ERR(mt9v032->standby_gpio); > > + > > mutex_init(&mt9v032->power_lock); > > mt9v032->pdata = pdata; > > mt9v032->model = (const void *)did->driver_data; > >
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 <mpa@pengutronix.de> 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 = V4L2_CTRL_TYPE_INTEGER, > + .name = "aec_update_interval", > + .min = 0, > + .max = 16, > + .step = 1, > + .def = 2, > + .flags = 0, > + }, { > + .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 = 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 ? > + .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]
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 <mpa@pengutronix.de> > > 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 = 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 = V4L2_CTRL_TYPE_INTEGER, > > + .name = "aec_update_interval", > > + .min = 0, > > + .max = 16, > > + .step = 1, > > + .def = 2, > > + .flags = 0, > > + }, { > > + .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 = 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. 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] > >
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 <mpa@pengutronix.de> > > > > 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]
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 <mpa@pengutronix.de> > > > > > > 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] > >
diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index 202565313e82..100f0ae43269 100644 --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt @@ -20,6 +20,8 @@ Optional Properties: - link-frequencies: List of allowed link frequencies in Hz. Each frequency is expressed as a 64-bit big-endian integer. +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. +- standby-gpios: GPIO handle which is connected to the standby pin of the chip. For further reading on port node refer to Documentation/devicetree/bindings/media/video-interfaces.txt. diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c index a68ce94ee097..c1bc564a0979 100644 --- a/drivers/media/i2c/mt9v032.c +++ b/drivers/media/i2c/mt9v032.c @@ -14,6 +14,7 @@ #include <linux/clk.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/log2.h> #include <linux/mutex.h> @@ -251,6 +252,8 @@ struct mt9v032 { struct regmap *regmap; struct clk *clk; + struct gpio_desc *reset_gpio; + struct gpio_desc *standby_gpio; struct mt9v032_platform_data *pdata; const struct mt9v032_model_info *model; @@ -312,16 +315,31 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) struct regmap *map = mt9v032->regmap; int ret; + if (mt9v032->reset_gpio) + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); + ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); if (ret < 0) return ret; + /* System clock has to be enabled before releasing the reset */ ret = clk_prepare_enable(mt9v032->clk); if (ret) return ret; udelay(1); + if (mt9v032->reset_gpio) { + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); + + /* After releasing reset we need to wait 10 clock cycles + * before accessing the sensor over I2C. As the minimum SYSCLK + * frequency is 13MHz, waiting 1µs will be enough in the worst + * case. + */ + udelay(1); + } + /* Reset the chip and stop data read out */ ret = regmap_write(map, MT9V032_RESET, 1); if (ret < 0) @@ -954,6 +972,16 @@ static int mt9v032_probe(struct i2c_client *client, if (IS_ERR(mt9v032->clk)) return PTR_ERR(mt9v032->clk); + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(mt9v032->reset_gpio)) + return PTR_ERR(mt9v032->reset_gpio); + + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", + GPIOD_OUT_LOW); + if (IS_ERR(mt9v032->standby_gpio)) + return PTR_ERR(mt9v032->standby_gpio); + mutex_init(&mt9v032->power_lock); mt9v032->pdata = pdata; mt9v032->model = (const void *)did->driver_data;