[PATCH v2 1/3] [media] mt9v032: Add reset and standby gpios

[PATCH v2 1/3] [media] mt9v032: Add reset and standby gpios

[Markus Pargmann]

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);
+
 	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;
-- 
2.6.2

[PATCH v2 2/3] [media] mt9v032: Do not unset master_mode

[Markus Pargmann]

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 | 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;
-- 
2.6.2

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

[Markus Pargmann]

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, 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 @@
 #define		MT9V032_TEST_PATTERN_GRAY_DIAGONAL	(3 << 11)
 #define		MT9V032_TEST_PATTERN_ENABLE		(1 << 13)
 #define		MT9V032_TEST_PATTERN_FLIP		(1 << 14)
+#define MT9V032_AEGC_DESIRED_BIN			0xa5
+#define MT9V032_AEC_UPDATE_FREQUENCY			0xa6
+#define MT9V032_AEC_LPF					0xa8
+#define MT9V032_AGC_UPDATE_FREQUENCY			0xa9
+#define MT9V032_AGC_LPF					0xaa
 #define MT9V032_AEC_AGC_ENABLE				0xaf
 #define		MT9V032_AEC_ENABLE			(1 << 0)
 #define		MT9V032_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_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_FREQUENCY,
+				    ctrl->val);
+
+	case V4L2_CID_AGC_UPDATE_INTERVAL:
+		return regmap_write(map, MT9V032_AGC_UPDATE_FREQUENCY,
+				    ctrl->val);
+
+	case V4L2_CID_AEC_MAX_SHUTTER_WIDTH:
+		return regmap_write(map,
+				    mt9v032->model->data->aec_max_shutter_reg,
+				    ctrl->val);
 	}
 
 	return 0;
@@ -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",
+		.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,
+	.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,
+};
+
 /* -----------------------------------------------------------------------------
  * V4L2 subdev core operations
  */
@@ -986,7 +1129,8 @@ static int mt9v032_probe(struct i2c_client *client,
 	mt9v032->pdata = pdata;
 	mt9v032->model = (const void *)did->driver_data;
 
-	v4l2_ctrl_handler_init(&mt9v032->ctrls, 10);
+	v4l2_ctrl_handler_init(&mt9v032->ctrls, 11 +
+			       ARRAY_SIZE(mt9v032_aegc_controls));
 
 	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
@@ -1015,6 +1159,13 @@ static int mt9v032_probe(struct i2c_client *client,
 	mt9v032->test_pattern_color = v4l2_ctrl_new_custom(&mt9v032->ctrls,
 				      &mt9v032_test_pattern_color, NULL);
 
+	v4l2_ctrl_new_custom(&mt9v032->ctrls,
+			     mt9v032->model->data->aec_max_shutter_v4l2_ctrl,
+			     NULL);
+	for (i = 0; i != ARRAY_SIZE(mt9v032_aegc_controls); ++i)
+		v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_aegc_controls[i],
+				     NULL);
+
 	v4l2_ctrl_cluster(2, &mt9v032->test_pattern);
 
 	mt9v032->pixel_rate =
-- 
2.6.2

Re: [PATCH v2 1/3] [media] mt9v032: Add reset and standby gpios

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] [media] mt9v032: Do not unset master_mode

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] [media] mt9v032: Add reset and standby gpios

[Markus Pargmann]

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

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;
> 
> 

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

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

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

[Laurent Pinchart]

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]

-- 
Regards,

Laurent Pinchart

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

[Markus Pargmann]

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

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]
> 
> 

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

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

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

[Laurent Pinchart]

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]

-- 
Regards,

Laurent Pinchart

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

[Markus Pargmann]

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

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]
> 
> 

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

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