| Message ID | 1446815625-18413-1-git-send-email-mpa@pengutronix.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Nov 06, 2015 at 02:13:43PM +0100, 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> > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ Acked-by: Rob Herring <robh@kernel.org> > drivers/media/i2c/mt9v032.c | 23 ++++++++++++++++++++++ > 2 files changed, 25 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..4aefde9634f5 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -24,6 +24,7 @@ > #include <linux/videodev2.h> > #include <linux/v4l2-mediabus.h> > #include <linux/module.h> > +#include <linux/gpio/consumer.h> > > #include <media/mt9v032.h> > #include <media/v4l2-ctrls.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,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > struct regmap *map = mt9v032->regmap; > int ret; > > + 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); > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > + > + /* > + * After releasing reset, it can take up to 1us until the chip is done > + */ > + udelay(1); > + > /* Reset the chip and stop data read out */ > ret = regmap_write(map, MT9V032_RESET, 1); > if (ret < 0) > @@ -954,6 +967,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; > -- > 2.6.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, Thank you for the patch. On Friday 06 November 2015 14:13:43 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. We could use a gpio hog for this, but given that the standby signal should eventually get used, and given that specifying it in DT is a good hardware description, that looks good to me. > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > drivers/media/i2c/mt9v032.c | 23 +++++++++++++++++++ > 2 files changed, 25 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..4aefde9634f5 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -24,6 +24,7 @@ > #include <linux/videodev2.h> > #include <linux/v4l2-mediabus.h> > #include <linux/module.h> > +#include <linux/gpio/consumer.h> module.h escaped my vigilance, but let's try to keep headers alphabetically sorted. > > #include <media/mt9v032.h> > #include <media/v4l2-ctrls.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,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > struct regmap *map = mt9v032->regmap; > int ret; > > + 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 */ Nitpicking, the driver capitalizes the first letter of comments. > ret = clk_prepare_enable(mt9v032->clk); > if (ret) > return ret; > > udelay(1); > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > + > + /* > + * After releasing reset, it can take up to 1us until the chip is done > + */ > + udelay(1); > + The delay isn't necessary if there's no reset GPIO. How about if (mt9v032->reset_gpio) { gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); /* After releasing reset, it can take up to 1us until the * chip is done. */ udelay(1); } And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should be safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a comment, maybe as /* 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); If you're fine with these changes there's no need to resubmit the patch, I can fix it when applying it to my tree. > /* Reset the chip and stop data read out */ > ret = regmap_write(map, MT9V032_RESET, 1); > if (ret < 0) > @@ -954,6 +967,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 Friday 06 November 2015 14:13:44 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> > --- > drivers/media/i2c/mt9v032.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index 4aefde9634f5..943c3f39ea73 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -344,7 +344,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); This makes sense, but shouldn't you also fix the mt9v032_s_stream() function then ? It clears the MT9V032_CHIP_CONTROL_MASTER_MODE bit when turning the stream off. > } > > static void mt9v032_power_off(struct mt9v032 *mt9v032)
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 <mpa@pengutronix.de> > --- > 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_WIDTH 0xad 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_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. > }; > }; > > @@ -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 > + * next exp = Current exp +- (Calculated new exp / 2^LPF) I know this comes directly from the datasheet, but it doesn't make too much sense to me. I wonder whether the correct formula wouldn't be 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) > { > @@ -712,6 +755,28 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl) > break; > } > return regmap_write(map, MT9V032_TEST_PATTERN, data); > + > + case V4L2_CID_DESIRED_BIN: > + return regmap_write(map, MT9V032_DESIRED_BIN, ctrl->val); > + > + case V4L2_CID_AEC_LPF: > + return regmap_write(map, MT9V032_AEC_LPF, ctrl->val); > + > + case V4L2_CID_AGC_LPF: > + return regmap_write(map, MT9V032_AGC_LPF, ctrl->val); > + > + case V4L2_CID_AEC_UPDATE_INTERVAL: > + return regmap_write(map, MT9V032_AEC_UPDATE_INTERVAL, > + ctrl->val); > + > + case V4L2_CID_AGC_UPDATE_INTERVAL: > + return regmap_write(map, MT9V032_AGC_UPDATE_INTERVAL, > + ctrl->val); > + > + case V4L2_CID_AEC_MAX_SHUTTER_WIDTH: > + return regmap_write(map, > + mt9v032->model->data->aec_max_shutter_reg, > + ctrl->val); > } > > return 0; > @@ -741,6 +806,78 @@ static const struct v4l2_ctrl_config > mt9v032_test_pattern_color = { .flags = 0, > }; > > +static const struct v4l2_ctrl_config mt9v032_desired_bin = { > + .ops = &mt9v032_ctrl_ops, > + .id = V4L2_CID_DESIRED_BIN, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "aec_agc_desired_bin", Please use proper controls names. > + .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 ? > + .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)); > > v4l2_ctrl_cluster(2, &mt9v032->test_pattern);
Hi Laurent, On Monday 09 November 2015 14:46:42 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Friday 06 November 2015 14:13:44 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> > > --- > > drivers/media/i2c/mt9v032.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index 4aefde9634f5..943c3f39ea73 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > @@ -344,7 +344,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); > > This makes sense, but shouldn't you also fix the mt9v032_s_stream() function > then ? It clears the MT9V032_CHIP_CONTROL_MASTER_MODE bit when turning the > stream off. Oh yes, thanks. Will fix it for the next version. Best Regards, Markus
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 <mpa@pengutronix.de> > > --- > > 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. Ok, fixed for next version. > > > +#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. Yes they are called differently, fixed. > > Could you please keep the registers sorted numerically ? Yes sorry. > > > #define MT9V032_AEC_AGC_ENABLE 0xaf > > #define MT9V032_AEC_ENABLE (1 << 0) > > #define MT9V032_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 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 ? It seems to be the 'row-time'. So it should be a linear relationship to the overall exposure time. > > > +#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 > > + * next exp = Current exp +- (Calculated new exp / 2^LPF) > > I know this comes directly from the datasheet, but it doesn't make too much > sense to me. I wonder whether the correct formula wouldn't be > > next exp = current exp + ((calculated new exp - current exp) / 2^LPF) Yes exactly, thanks. > > > + */ > > +#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) > > { > > @@ -712,6 +755,28 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl) > > break; > > } > > return regmap_write(map, MT9V032_TEST_PATTERN, data); > > + > > + case V4L2_CID_DESIRED_BIN: > > + return regmap_write(map, MT9V032_DESIRED_BIN, ctrl->val); > > + > > + case V4L2_CID_AEC_LPF: > > + return regmap_write(map, MT9V032_AEC_LPF, ctrl->val); > > + > > + case V4L2_CID_AGC_LPF: > > + return regmap_write(map, MT9V032_AGC_LPF, ctrl->val); > > + > > + case V4L2_CID_AEC_UPDATE_INTERVAL: > > + return regmap_write(map, MT9V032_AEC_UPDATE_INTERVAL, > > + ctrl->val); > > + > > + case V4L2_CID_AGC_UPDATE_INTERVAL: > > + return regmap_write(map, MT9V032_AGC_UPDATE_INTERVAL, > > + ctrl->val); > > + > > + case V4L2_CID_AEC_MAX_SHUTTER_WIDTH: > > + return regmap_write(map, > > + mt9v032->model->data->aec_max_shutter_reg, > > + ctrl->val); > > } > > > > return 0; > > @@ -741,6 +806,78 @@ static const struct v4l2_ctrl_config > > mt9v032_test_pattern_color = { .flags = 0, > > }; > > > > +static const struct v4l2_ctrl_config mt9v032_desired_bin = { > > + .ops = &mt9v032_ctrl_ops, > > + .id = V4L2_CID_DESIRED_BIN, > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .name = "aec_agc_desired_bin", > > Please use proper controls names. Sorry I don't really know what you mean? For me these are proper names. > > > + .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
Hi, On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Friday 06 November 2015 14:13:43 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. > > We could use a gpio hog for this, but given that the standby signal should > eventually get used, and given that specifying it in DT is a good hardware > description, that looks good to me. > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > > drivers/media/i2c/mt9v032.c | 23 +++++++++++++++++++ > > 2 files changed, 25 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..4aefde9634f5 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > @@ -24,6 +24,7 @@ > > #include <linux/videodev2.h> > > #include <linux/v4l2-mediabus.h> > > #include <linux/module.h> > > +#include <linux/gpio/consumer.h> > > module.h escaped my vigilance, but let's try to keep headers alphabetically > sorted. > > > > #include <media/mt9v032.h> > > #include <media/v4l2-ctrls.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,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > > struct regmap *map = mt9v032->regmap; > > int ret; > > > > + 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 */ > > Nitpicking, the driver capitalizes the first letter of comments. > > > ret = clk_prepare_enable(mt9v032->clk); > > if (ret) > > return ret; > > > > udelay(1); > > > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > > + > > + /* > > + * After releasing reset, it can take up to 1us until the chip is done > > + */ > > + udelay(1); > > + > > The delay isn't necessary if there's no reset GPIO. How about > > if (mt9v032->reset_gpio) { > gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > > /* After releasing reset, it can take up to 1us until the > * chip is done. > */ > udelay(1); > } > > And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should be > safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a > comment, maybe as > > /* 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); > > If you're fine with these changes there's no need to resubmit the patch, I can > fix it when applying it to my tree. Thanks, I am fine with all your changes. But as there will be a v2 for the other two patches I could as well send an updated version if you wish. Thanks, Markus
Hi Markus, On Monday 09 November 2015 16:33:03 Markus Pargmann wrote: > On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote: > > On Friday 06 November 2015 14:13:43 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. > > > > We could use a gpio hog for this, but given that the standby signal should > > eventually get used, and given that specifying it in DT is a good hardware > > description, that looks good to me. > > > >> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > >> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > >> --- > >> > >> .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > >> drivers/media/i2c/mt9v032.c | 23 +++++++++++++++ > >> 2 files changed, 25 insertions(+) [snip] > > If you're fine with these changes there's no need to resubmit the patch, I > > can fix it when applying it to my tree. > > Thanks, I am fine with all your changes. But as there will be a v2 for the > other two patches I could as well send an updated version if you wish. As you wish, both options are fine with me.
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 > >
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..4aefde9634f5 100644 --- a/drivers/media/i2c/mt9v032.c +++ b/drivers/media/i2c/mt9v032.c @@ -24,6 +24,7 @@ #include <linux/videodev2.h> #include <linux/v4l2-mediabus.h> #include <linux/module.h> +#include <linux/gpio/consumer.h> #include <media/mt9v032.h> #include <media/v4l2-ctrls.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,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) struct regmap *map = mt9v032->regmap; int ret; + 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); + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); + + /* + * After releasing reset, it can take up to 1us until the chip is done + */ + udelay(1); + /* Reset the chip and stop data read out */ ret = regmap_write(map, MT9V032_RESET, 1); if (ret < 0) @@ -954,6 +967,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;