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

[PATCH 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>
---
 .../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>
 
 #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

[PATCH 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 | 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);
 }
 
 static void mt9v032_power_off(struct mt9v032 *mt9v032)
-- 
2.6.1

[PATCH 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, 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
+#define MT9V032_AEC_UPDATE_INTERVAL			0xa6
+#define MT9V032_AGC_UPDATE_INTERVAL			0xa9
 #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
+#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;
 	};
 };
 
@@ -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.
+ */
+#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)
+ */
+#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",
+	.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,
+	.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);
 
 	v4l2_ctrl_cluster(2, &mt9v032->test_pattern);
 
-- 
2.6.1

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

[Rob Herring]

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

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Markus Pargmann]

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

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

-- 
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 3/3] [media] mt9v032: Add V4L2 controls for AEC and AGC

[Markus Pargmann]

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

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

-- 
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 1/3] [media] mt9v032: Add reset and standby gpios

[Markus Pargmann]

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

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

-- 
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 1/3] [media] mt9v032: Add reset and standby gpios

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Markus Pargmann]

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

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

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