[PATCH v2 0/5] mt9v032: add support for v4l2-async, mt9v02x, and regmap

[PATCH v2 0/5] mt9v032: add support for v4l2-async, mt9v02x, and regmap

[Philipp Zabel]

Hi,

This series contains the fixed up mt9v032 patches I recently sent,
plus an additional patch to use regmap for the i2c register access.

regards
Philipp

Philipp Zabel (5):
  [media] mt9v032: reset is self clearing
  [media] mt9v032: register v4l2 asynchronous subdevice
  [media] mt9v032: do not clear reserved bits in read mode register
  [media] mt9v032: add support for mt9v022 and mt9v024
  [media] mt9v032: use regmap

 drivers/media/i2c/Kconfig   |   1 +
 drivers/media/i2c/mt9v032.c | 168 +++++++++++++++++++++++++-------------------
 2 files changed, 95 insertions(+), 74 deletions(-)

-- 
2.0.0.rc2

[PATCH v2 1/5] [media] mt9v032: reset is self clearing

[Philipp Zabel]

According to the publicly available MT9V032 data sheet, the reset bits are self
clearing and the reset register always reads 0. The reset will be asserted for
15 SYSCLK cycles. Instead of writing 0 to the register, wait using ndelay.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/i2c/mt9v032.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index f04d0bb..29d8d8f 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -315,6 +315,7 @@ mt9v032_update_hblank(struct mt9v032 *mt9v032)
 static int mt9v032_power_on(struct mt9v032 *mt9v032)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	unsigned long rate;
 	int ret;
 
 	ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk);
@@ -332,9 +333,9 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_RESET, 0);
-	if (ret < 0)
-		return ret;
+	/* Wait 15 SYSCLK cycles, 564 ns @ 26.6 MHz */
+	rate = clk_get_rate(mt9v032->clk);
+	ndelay(DIV_ROUND_UP(15 * 125000000, rate >> 3));
 
 	return mt9v032_write(client, MT9V032_CHIP_CONTROL, 0);
 }
-- 
2.0.0.rc2

Re: [PATCH v2 1/5] [media] mt9v032: reset is self clearing

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 03 June 2014 11:35:51 Philipp Zabel wrote:
> According to the publicly available MT9V032 data sheet, the reset bits are
> self clearing and the reset register always reads 0. The reset will be
> asserted for 15 SYSCLK cycles. Instead of writing 0 to the register, wait
> using ndelay.

On the other hand, revision D of the datasheet states on page 71 ("Appendix A 
- Serial Configurations, Figure 46: Stand-Alone Topology") that the typical 
configuration of the sensor includes issuing a soft reset with 

4. Issue a soft reset (set R0x0C[0] = 1 followed by R0x0C[0] = 0.

I wonder whether it wouldn't be safer to keep the register write. Do you see 
any adverse effect of keeping it ?

> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/i2c/mt9v032.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index f04d0bb..29d8d8f 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -315,6 +315,7 @@ mt9v032_update_hblank(struct mt9v032 *mt9v032)
>  static int mt9v032_power_on(struct mt9v032 *mt9v032)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> +	unsigned long rate;
>  	int ret;
> 
>  	ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk);
> @@ -332,9 +333,9 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = mt9v032_write(client, MT9V032_RESET, 0);
> -	if (ret < 0)
> -		return ret;
> +	/* Wait 15 SYSCLK cycles, 564 ns @ 26.6 MHz */
> +	rate = clk_get_rate(mt9v032->clk);
> +	ndelay(DIV_ROUND_UP(15 * 125000000, rate >> 3));
> 
>  	return mt9v032_write(client, MT9V032_CHIP_CONTROL, 0);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/5] [media] mt9v032: reset is self clearing

[Philipp Zabel]

Hi Laurent,

Am Mittwoch, den 04.06.2014, 16:49 +0200 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Tuesday 03 June 2014 11:35:51 Philipp Zabel wrote:
> > According to the publicly available MT9V032 data sheet, the reset bits are
> > self clearing and the reset register always reads 0. The reset will be
> > asserted for 15 SYSCLK cycles. Instead of writing 0 to the register, wait
> > using ndelay.
> 
> On the other hand, revision D of the datasheet states on page 71 ("Appendix A 
> - Serial Configurations, Figure 46: Stand-Alone Topology") that the typical 
> configuration of the sensor includes issuing a soft reset with 
> 
> 4. Issue a soft reset (set R0x0C[0] = 1 followed by R0x0C[0] = 0.
> 
> I wonder whether it wouldn't be safer to keep the register write. Do you see 
> any adverse effect of keeping it ?

No, writing 0 doesn't have any effect. Since the datasheet suggests
doing this, let's just drop this patch and keep the code as is.

regards
Philipp

[PATCH v2 2/5] [media] mt9v032: register v4l2 asynchronous subdevice

[Philipp Zabel]

Add support for registering the sensor subdevice using the v4l2-async API.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Fixed cleanup and error handling
---
 drivers/media/i2c/mt9v032.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 29d8d8f..83ae8ca6d 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -985,10 +985,20 @@ static int mt9v032_probe(struct i2c_client *client,
 
 	mt9v032->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_init(&mt9v032->subdev.entity, 1, &mt9v032->pad, 0);
+	if (ret < 0)
+		goto err_entity;
 
+	mt9v032->subdev.dev = &client->dev;
+	ret = v4l2_async_register_subdev(&mt9v032->subdev);
 	if (ret < 0)
-		v4l2_ctrl_handler_free(&mt9v032->ctrls);
+		goto err_async;
+
+	return 0;
 
+err_async:
+	media_entity_cleanup(&mt9v032->subdev.entity);
+err_entity:
+	v4l2_ctrl_handler_free(&mt9v032->ctrls);
 	return ret;
 }
 
@@ -997,6 +1007,7 @@ static int mt9v032_remove(struct i2c_client *client)
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
 
+	v4l2_async_unregister_subdev(subdev);
 	v4l2_ctrl_handler_free(&mt9v032->ctrls);
 	v4l2_device_unregister_subdev(subdev);
 	media_entity_cleanup(&subdev->entity);
-- 
2.0.0.rc2

Re: [PATCH v2 2/5] [media] mt9v032: register v4l2 asynchronous subdevice

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 03 June 2014 11:35:52 Philipp Zabel wrote:
> Add support for registering the sensor subdevice using the v4l2-async API.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
>  - Fixed cleanup and error handling
> ---
>  drivers/media/i2c/mt9v032.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 29d8d8f..83ae8ca6d 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -985,10 +985,20 @@ static int mt9v032_probe(struct i2c_client *client,
> 
>  	mt9v032->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_init(&mt9v032->subdev.entity, 1, &mt9v032->pad, 0);
> +	if (ret < 0)
> +		goto err_entity;
> 
> +	mt9v032->subdev.dev = &client->dev;
> +	ret = v4l2_async_register_subdev(&mt9v032->subdev);
>  	if (ret < 0)
> -		v4l2_ctrl_handler_free(&mt9v032->ctrls);
> +		goto err_async;
> +
> +	return 0;
> 
> +err_async:
> +	media_entity_cleanup(&mt9v032->subdev.entity);

media_entity_cleanup() can safely be called on an unintialized entity, 
provided the memory has been zeroed. You could thus merge the err_async and 
err_entity labels into a single error label.

> +err_entity:
> +	v4l2_ctrl_handler_free(&mt9v032->ctrls);
>  	return ret;
>  }
> 
> @@ -997,6 +1007,7 @@ static int mt9v032_remove(struct i2c_client *client)
>  	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
>  	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> 
> +	v4l2_async_unregister_subdev(subdev);
>  	v4l2_ctrl_handler_free(&mt9v032->ctrls);
>  	v4l2_device_unregister_subdev(subdev);
>  	media_entity_cleanup(&subdev->entity);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] [media] mt9v032: register v4l2 asynchronous subdevice

[Philipp Zabel]

Hi Laurent,

Am Mittwoch, den 04.06.2014, 16:16 +0200 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Tuesday 03 June 2014 11:35:52 Philipp Zabel wrote:
> > Add support for registering the sensor subdevice using the v4l2-async API.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v1:
> >  - Fixed cleanup and error handling
> > ---
> >  drivers/media/i2c/mt9v032.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> > index 29d8d8f..83ae8ca6d 100644
> > --- a/drivers/media/i2c/mt9v032.c
> > +++ b/drivers/media/i2c/mt9v032.c
> > @@ -985,10 +985,20 @@ static int mt9v032_probe(struct i2c_client *client,
> > 
> >  	mt9v032->pad.flags = MEDIA_PAD_FL_SOURCE;
> >  	ret = media_entity_init(&mt9v032->subdev.entity, 1, &mt9v032->pad, 0);
> > +	if (ret < 0)
> > +		goto err_entity;
> > 
> > +	mt9v032->subdev.dev = &client->dev;
> > +	ret = v4l2_async_register_subdev(&mt9v032->subdev);
> >  	if (ret < 0)
> > -		v4l2_ctrl_handler_free(&mt9v032->ctrls);
> > +		goto err_async;
> > +
> > +	return 0;
> > 
> > +err_async:
> > +	media_entity_cleanup(&mt9v032->subdev.entity);
> 
> media_entity_cleanup() can safely be called on an unintialized entity, 
> provided the memory has been zeroed. You could thus merge the err_async and 
> err_entity labels into a single error label.

Alright, I'll update this patch accordingly.

regards
Philipp

[PATCH v2 3/5] [media] mt9v032: do not clear reserved bits in read mode register

[Philipp Zabel]

The read mode register bits 8 and 9 are set and marked as reserved.
Don't clear them.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Add MT9V032_READ_MODE_RESERVED #define
---
 drivers/media/i2c/mt9v032.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 83ae8ca6d..d969663 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -87,6 +87,7 @@
 #define		MT9V032_READ_MODE_COLUMN_FLIP		(1 << 5)
 #define		MT9V032_READ_MODE_DARK_COLUMNS		(1 << 6)
 #define		MT9V032_READ_MODE_DARK_ROWS		(1 << 7)
+#define		MT9V032_READ_MODE_RESERVED		0x0300
 #define MT9V032_PIXEL_OPERATION_MODE			0x0f
 #define		MT9V034_PIXEL_OPERATION_MODE_HDR	(1 << 0)
 #define		MT9V034_PIXEL_OPERATION_MODE_COLOR	(1 << 1)
@@ -415,6 +416,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 	struct i2c_client *client = v4l2_get_subdevdata(subdev);
 	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
 	struct v4l2_rect *crop = &mt9v032->crop;
+	unsigned int read_mode;
 	unsigned int hbin;
 	unsigned int vbin;
 	int ret;
@@ -425,9 +427,13 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 	/* Configure the window size and row/column bin */
 	hbin = fls(mt9v032->hratio) - 1;
 	vbin = fls(mt9v032->vratio) - 1;
-	ret = mt9v032_write(client, MT9V032_READ_MODE,
-			    hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
-			    vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
+	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
+	if (read_mode < 0)
+		return read_mode;
+	read_mode &= MT9V032_READ_MODE_RESERVED;
+	read_mode |= hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
+		     vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT;
+	ret = mt9v032_write(client, MT9V032_READ_MODE, read_mode);
 	if (ret < 0)
 		return ret;
 
-- 
2.0.0.rc2

Re: [PATCH v2 3/5] [media] mt9v032: do not clear reserved bits in read mode register

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 03 June 2014 11:35:53 Philipp Zabel wrote:
> The read mode register bits 8 and 9 are set and marked as reserved.
> Don't clear them.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
> Changes since v1:
>  - Add MT9V032_READ_MODE_RESERVED #define
> ---
>  drivers/media/i2c/mt9v032.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 83ae8ca6d..d969663 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -87,6 +87,7 @@
>  #define		MT9V032_READ_MODE_COLUMN_FLIP		(1 << 5)
>  #define		MT9V032_READ_MODE_DARK_COLUMNS		(1 << 6)
>  #define		MT9V032_READ_MODE_DARK_ROWS		(1 << 7)
> +#define		MT9V032_READ_MODE_RESERVED		0x0300
>  #define MT9V032_PIXEL_OPERATION_MODE			0x0f
>  #define		MT9V034_PIXEL_OPERATION_MODE_HDR	(1 << 0)
>  #define		MT9V034_PIXEL_OPERATION_MODE_COLOR	(1 << 1)
> @@ -415,6 +416,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev,
> int enable) struct i2c_client *client = v4l2_get_subdevdata(subdev);
>  	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
>  	struct v4l2_rect *crop = &mt9v032->crop;
> +	unsigned int read_mode;
>  	unsigned int hbin;
>  	unsigned int vbin;
>  	int ret;
> @@ -425,9 +427,13 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev,
> int enable) /* Configure the window size and row/column bin */
>  	hbin = fls(mt9v032->hratio) - 1;
>  	vbin = fls(mt9v032->vratio) - 1;
> -	ret = mt9v032_write(client, MT9V032_READ_MODE,
> -			    hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> -			    vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
> +	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> +	if (read_mode < 0)
> +		return read_mode;
> +	read_mode &= MT9V032_READ_MODE_RESERVED;
> +	read_mode |= hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> +		     vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT;
> +	ret = mt9v032_write(client, MT9V032_READ_MODE, read_mode);
>  	if (ret < 0)
>  		return ret;

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/5] [media] mt9v032: add support for mt9v022 and mt9v024

[Philipp Zabel]

>From the looks of it, mt9v022 and mt9v032 are very similar,
as are mt9v024 and mt9v034. With minimal changes it is possible
to support mt9v02[24] with the same driver.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Removed code changes for fault pixel correction bit on mt9v02x
---
 drivers/media/i2c/mt9v032.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index d969663..cb7c6df 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -1,5 +1,5 @@
 /*
- * Driver for MT9V032 CMOS Image Sensor from Micron
+ * Driver for MT9V022, MT9V024, MT9V032, and MT9V034 CMOS Image Sensors
  *
  * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com>
  *
@@ -134,8 +134,12 @@
 #define MT9V032_THERMAL_INFO				0xc1
 
 enum mt9v032_model {
-	MT9V032_MODEL_V032_COLOR,
-	MT9V032_MODEL_V032_MONO,
+	MT9V032_MODEL_V022_COLOR,	/* MT9V022IX7ATC */
+	MT9V032_MODEL_V022_MONO,	/* MT9V022IX7ATM */
+	MT9V032_MODEL_V024_COLOR,	/* MT9V024IA7XTC */
+	MT9V032_MODEL_V024_MONO,	/* MT9V024IA7XTM */
+	MT9V032_MODEL_V032_COLOR,	/* MT9V032C12STM */
+	MT9V032_MODEL_V032_MONO,	/* MT9V032C12STC */
 	MT9V032_MODEL_V034_COLOR,
 	MT9V032_MODEL_V034_MONO,
 };
@@ -161,14 +165,14 @@ struct mt9v032_model_info {
 };
 
 static const struct mt9v032_model_version mt9v032_versions[] = {
-	{ MT9V032_CHIP_ID_REV1, "MT9V032 rev1/2" },
-	{ MT9V032_CHIP_ID_REV3, "MT9V032 rev3" },
-	{ MT9V034_CHIP_ID_REV1, "MT9V034 rev1" },
+	{ MT9V032_CHIP_ID_REV1, "MT9V022/MT9V032 rev1/2" },
+	{ MT9V032_CHIP_ID_REV3, "MT9V022/MT9V032 rev3" },
+	{ MT9V034_CHIP_ID_REV1, "MT9V024/MT9V034 rev1" },
 };
 
 static const struct mt9v032_model_data mt9v032_model_data[] = {
 	{
-		/* MT9V032 revisions 1/2/3 */
+		/* MT9V022, MT9V032 revisions 1/2/3 */
 		.min_row_time = 660,
 		.min_hblank = MT9V032_HORIZONTAL_BLANKING_MIN,
 		.min_vblank = MT9V032_VERTICAL_BLANKING_MIN,
@@ -177,7 +181,7 @@ static const struct mt9v032_model_data mt9v032_model_data[] = {
 		.max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
 		.pclk_reg = MT9V032_PIXEL_CLOCK,
 	}, {
-		/* MT9V034 */
+		/* MT9V024, MT9V034 */
 		.min_row_time = 690,
 		.min_hblank = MT9V034_HORIZONTAL_BLANKING_MIN,
 		.min_vblank = MT9V034_VERTICAL_BLANKING_MIN,
@@ -189,6 +193,22 @@ static const struct mt9v032_model_data mt9v032_model_data[] = {
 };
 
 static const struct mt9v032_model_info mt9v032_models[] = {
+	[MT9V032_MODEL_V022_COLOR] = {
+		.data = &mt9v032_model_data[0],
+		.color = true,
+	},
+	[MT9V032_MODEL_V022_MONO] = {
+		.data = &mt9v032_model_data[0],
+		.color = false,
+	},
+	[MT9V032_MODEL_V024_COLOR] = {
+		.data = &mt9v032_model_data[1],
+		.color = true,
+	},
+	[MT9V032_MODEL_V024_MONO] = {
+		.data = &mt9v032_model_data[1],
+		.color = false,
+	},
 	[MT9V032_MODEL_V032_COLOR] = {
 		.data = &mt9v032_model_data[0],
 		.color = true,
@@ -1022,6 +1042,10 @@ static int mt9v032_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id mt9v032_id[] = {
+	{ "mt9v022", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V022_COLOR] },
+	{ "mt9v022m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V022_MONO] },
+	{ "mt9v024", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V024_COLOR] },
+	{ "mt9v024m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V024_MONO] },
 	{ "mt9v032", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V032_COLOR] },
 	{ "mt9v032m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V032_MONO] },
 	{ "mt9v034", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V034_COLOR] },
-- 
2.0.0.rc2

Re: [PATCH v2 4/5] [media] mt9v032: add support for mt9v022 and mt9v024

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 03 June 2014 11:35:54 Philipp Zabel wrote:
> From the looks of it, mt9v022 and mt9v032 are very similar,
> as are mt9v024 and mt9v034. With minimal changes it is possible
> to support mt9v02[24] with the same driver.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
> Changes since v1:
>  - Removed code changes for fault pixel correction bit on mt9v02x
> ---
>  drivers/media/i2c/mt9v032.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index d969663..cb7c6df 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -1,5 +1,5 @@
>  /*
> - * Driver for MT9V032 CMOS Image Sensor from Micron
> + * Driver for MT9V022, MT9V024, MT9V032, and MT9V034 CMOS Image Sensors
>   *
>   * Copyright (C) 2010, Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> *
> @@ -134,8 +134,12 @@
>  #define MT9V032_THERMAL_INFO				0xc1
> 
>  enum mt9v032_model {
> -	MT9V032_MODEL_V032_COLOR,
> -	MT9V032_MODEL_V032_MONO,
> +	MT9V032_MODEL_V022_COLOR,	/* MT9V022IX7ATC */
> +	MT9V032_MODEL_V022_MONO,	/* MT9V022IX7ATM */
> +	MT9V032_MODEL_V024_COLOR,	/* MT9V024IA7XTC */
> +	MT9V032_MODEL_V024_MONO,	/* MT9V024IA7XTM */
> +	MT9V032_MODEL_V032_COLOR,	/* MT9V032C12STM */
> +	MT9V032_MODEL_V032_MONO,	/* MT9V032C12STC */
>  	MT9V032_MODEL_V034_COLOR,
>  	MT9V032_MODEL_V034_MONO,
>  };
> @@ -161,14 +165,14 @@ struct mt9v032_model_info {
>  };
> 
>  static const struct mt9v032_model_version mt9v032_versions[] = {
> -	{ MT9V032_CHIP_ID_REV1, "MT9V032 rev1/2" },
> -	{ MT9V032_CHIP_ID_REV3, "MT9V032 rev3" },
> -	{ MT9V034_CHIP_ID_REV1, "MT9V034 rev1" },
> +	{ MT9V032_CHIP_ID_REV1, "MT9V022/MT9V032 rev1/2" },
> +	{ MT9V032_CHIP_ID_REV3, "MT9V022/MT9V032 rev3" },
> +	{ MT9V034_CHIP_ID_REV1, "MT9V024/MT9V034 rev1" },
>  };
> 
>  static const struct mt9v032_model_data mt9v032_model_data[] = {
>  	{
> -		/* MT9V032 revisions 1/2/3 */
> +		/* MT9V022, MT9V032 revisions 1/2/3 */
>  		.min_row_time = 660,
>  		.min_hblank = MT9V032_HORIZONTAL_BLANKING_MIN,
>  		.min_vblank = MT9V032_VERTICAL_BLANKING_MIN,
> @@ -177,7 +181,7 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { .max_shutter = MT9V032_TOTAL_SHUTTER_WIDTH_MAX,
>  		.pclk_reg = MT9V032_PIXEL_CLOCK,
>  	}, {
> -		/* MT9V034 */
> +		/* MT9V024, MT9V034 */
>  		.min_row_time = 690,
>  		.min_hblank = MT9V034_HORIZONTAL_BLANKING_MIN,
>  		.min_vblank = MT9V034_VERTICAL_BLANKING_MIN,
> @@ -189,6 +193,22 @@ static const struct mt9v032_model_data
> mt9v032_model_data[] = { };
> 
>  static const struct mt9v032_model_info mt9v032_models[] = {
> +	[MT9V032_MODEL_V022_COLOR] = {
> +		.data = &mt9v032_model_data[0],
> +		.color = true,
> +	},
> +	[MT9V032_MODEL_V022_MONO] = {
> +		.data = &mt9v032_model_data[0],
> +		.color = false,
> +	},
> +	[MT9V032_MODEL_V024_COLOR] = {
> +		.data = &mt9v032_model_data[1],
> +		.color = true,
> +	},
> +	[MT9V032_MODEL_V024_MONO] = {
> +		.data = &mt9v032_model_data[1],
> +		.color = false,
> +	},
>  	[MT9V032_MODEL_V032_COLOR] = {
>  		.data = &mt9v032_model_data[0],
>  		.color = true,
> @@ -1022,6 +1042,10 @@ static int mt9v032_remove(struct i2c_client *client)
>  }
> 
>  static const struct i2c_device_id mt9v032_id[] = {
> +	{ "mt9v022", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V022_COLOR] },
> +	{ "mt9v022m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V022_MONO] },
> +	{ "mt9v024", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V024_COLOR] },
> +	{ "mt9v024m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V024_MONO] },
>  	{ "mt9v032", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V032_COLOR] },
>  	{ "mt9v032m", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V032_MONO] },
>  	{ "mt9v034", (kernel_ulong_t)&mt9v032_models[MT9V032_MODEL_V034_COLOR] },

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/5] [media] mt9v032: use regmap

[Philipp Zabel]

This switches all register accesses to use regmap. It allows to
use the regmap cache, tracing, and debug register dump facilities,
and removes the need to open code read-modify-writes.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
This patch was not included before.
---
 drivers/media/i2c/Kconfig   |   1 +
 drivers/media/i2c/mt9v032.c | 112 ++++++++++++++++++--------------------------
 2 files changed, 46 insertions(+), 67 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 441053b..f40b4cf 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -551,6 +551,7 @@ config VIDEO_MT9V032
 	tristate "Micron MT9V032 sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 	depends on MEDIA_CAMERA_SUPPORT
+	select REGMAP_I2C
 	---help---
 	  This is a Video4Linux2 sensor-level driver for the Micron
 	  MT9V032 752x480 CMOS sensor.
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index cb7c6df..e756d50 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -17,6 +17,7 @@
 #include <linux/i2c.h>
 #include <linux/log2.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <linux/v4l2-mediabus.h>
@@ -245,6 +246,7 @@ struct mt9v032 {
 	struct mutex power_lock;
 	int power_count;
 
+	struct regmap *regmap;
 	struct clk *clk;
 
 	struct mt9v032_platform_data *pdata;
@@ -252,7 +254,6 @@ struct mt9v032 {
 	const struct mt9v032_model_version *version;
 
 	u32 sysclk;
-	u16 chip_control;
 	u16 aec_agc;
 	u16 hblank;
 	struct {
@@ -266,40 +267,10 @@ static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd)
 	return container_of(sd, struct mt9v032, subdev);
 }
 
-static int mt9v032_read(struct i2c_client *client, const u8 reg)
-{
-	s32 data = i2c_smbus_read_word_swapped(client, reg);
-	dev_dbg(&client->dev, "%s: read 0x%04x from 0x%02x\n", __func__,
-		data, reg);
-	return data;
-}
-
-static int mt9v032_write(struct i2c_client *client, const u8 reg,
-			 const u16 data)
-{
-	dev_dbg(&client->dev, "%s: writing 0x%04x to 0x%02x\n", __func__,
-		data, reg);
-	return i2c_smbus_write_word_swapped(client, reg, data);
-}
-
-static int mt9v032_set_chip_control(struct mt9v032 *mt9v032, u16 clear, u16 set)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
-	u16 value = (mt9v032->chip_control & ~clear) | set;
-	int ret;
-
-	ret = mt9v032_write(client, MT9V032_CHIP_CONTROL, value);
-	if (ret < 0)
-		return ret;
-
-	mt9v032->chip_control = value;
-	return 0;
-}
-
 static int
 mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	struct regmap *map = mt9v032->regmap;
 	u16 value = mt9v032->aec_agc;
 	int ret;
 
@@ -308,7 +279,7 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
 	else
 		value &= ~which;
 
-	ret = mt9v032_write(client, MT9V032_AEC_AGC_ENABLE, value);
+	ret = regmap_write(map, MT9V032_AEC_AGC_ENABLE, value);
 	if (ret < 0)
 		return ret;
 
@@ -319,7 +290,6 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
 static int
 mt9v032_update_hblank(struct mt9v032 *mt9v032)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
 	struct v4l2_rect *crop = &mt9v032->crop;
 	unsigned int min_hblank = mt9v032->model->data->min_hblank;
 	unsigned int hblank;
@@ -330,12 +300,13 @@ mt9v032_update_hblank(struct mt9v032 *mt9v032)
 			   min_hblank);
 	hblank = max_t(unsigned int, mt9v032->hblank, min_hblank);
 
-	return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING, hblank);
+	return regmap_write(mt9v032->regmap, MT9V032_HORIZONTAL_BLANKING,
+			    hblank);
 }
 
 static int mt9v032_power_on(struct mt9v032 *mt9v032)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	struct regmap *map = mt9v032->regmap;
 	unsigned long rate;
 	int ret;
 
@@ -350,7 +321,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
 	udelay(1);
 
 	/* Reset the chip and stop data read out */
-	ret = mt9v032_write(client, MT9V032_RESET, 1);
+	ret = regmap_write(map, MT9V032_RESET, 1);
 	if (ret < 0)
 		return ret;
 
@@ -358,7 +329,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
 	rate = clk_get_rate(mt9v032->clk);
 	ndelay(DIV_ROUND_UP(15 * 125000000, rate >> 3));
 
-	return mt9v032_write(client, MT9V032_CHIP_CONTROL, 0);
+	return regmap_write(map, MT9V032_CHIP_CONTROL, 0);
 }
 
 static void mt9v032_power_off(struct mt9v032 *mt9v032)
@@ -368,7 +339,7 @@ static void mt9v032_power_off(struct mt9v032 *mt9v032)
 
 static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	struct regmap *map = mt9v032->regmap;
 	int ret;
 
 	if (!on) {
@@ -382,14 +353,14 @@ static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
 
 	/* Configure the pixel clock polarity */
 	if (mt9v032->pdata && mt9v032->pdata->clk_pol) {
-		ret = mt9v032_write(client, mt9v032->model->data->pclk_reg,
+		ret = regmap_write(map, mt9v032->model->data->pclk_reg,
 				MT9V032_PIXEL_CLOCK_INV_PXL_CLK);
 		if (ret < 0)
 			return ret;
 	}
 
 	/* Disable the noise correction algorithm and restore the controls. */
-	ret = mt9v032_write(client, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
+	ret = regmap_write(map, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
 	if (ret < 0)
 		return ret;
 
@@ -433,43 +404,39 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 	const u16 mode = MT9V032_CHIP_CONTROL_MASTER_MODE
 		       | MT9V032_CHIP_CONTROL_DOUT_ENABLE
 		       | MT9V032_CHIP_CONTROL_SEQUENTIAL;
-	struct i2c_client *client = v4l2_get_subdevdata(subdev);
 	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
 	struct v4l2_rect *crop = &mt9v032->crop;
-	unsigned int read_mode;
+	struct regmap *map = mt9v032->regmap;
 	unsigned int hbin;
 	unsigned int vbin;
 	int ret;
 
 	if (!enable)
-		return mt9v032_set_chip_control(mt9v032, mode, 0);
+		return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, 0);
 
 	/* Configure the window size and row/column bin */
 	hbin = fls(mt9v032->hratio) - 1;
 	vbin = fls(mt9v032->vratio) - 1;
-	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
-	if (read_mode < 0)
-		return read_mode;
-	read_mode &= MT9V032_READ_MODE_RESERVED;
-	read_mode |= hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
-		     vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT;
-	ret = mt9v032_write(client, MT9V032_READ_MODE, read_mode);
+	ret = regmap_update_bits(map, MT9V032_READ_MODE,
+				 ~MT9V032_READ_MODE_RESERVED,
+				 hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
+				 vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_COLUMN_START, crop->left);
+	ret = regmap_write(map, MT9V032_COLUMN_START, crop->left);
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_ROW_START, crop->top);
+	ret = regmap_write(map, MT9V032_ROW_START, crop->top);
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_WINDOW_WIDTH, crop->width);
+	ret = regmap_write(map, MT9V032_WINDOW_WIDTH, crop->width);
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_WINDOW_HEIGHT, crop->height);
+	ret = regmap_write(map, MT9V032_WINDOW_HEIGHT, crop->height);
 	if (ret < 0)
 		return ret;
 
@@ -478,7 +445,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 		return ret;
 
 	/* Switch to master "normal" mode */
-	return mt9v032_set_chip_control(mt9v032, 0, mode);
+	return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, mode);
 }
 
 static int mt9v032_enum_mbus_code(struct v4l2_subdev *subdev,
@@ -660,7 +627,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9v032 *mt9v032 =
 			container_of(ctrl->handler, struct mt9v032, ctrls);
-	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	struct regmap *map = mt9v032->regmap;
 	u32 freq;
 	u16 data;
 
@@ -670,23 +637,23 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
 					      ctrl->val);
 
 	case V4L2_CID_GAIN:
-		return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
+		return regmap_write(map, MT9V032_ANALOG_GAIN, ctrl->val);
 
 	case V4L2_CID_EXPOSURE_AUTO:
 		return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
 					      !ctrl->val);
 
 	case V4L2_CID_EXPOSURE:
-		return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
-				     ctrl->val);
+		return regmap_write(map, MT9V032_TOTAL_SHUTTER_WIDTH,
+				    ctrl->val);
 
 	case V4L2_CID_HBLANK:
 		mt9v032->hblank = ctrl->val;
 		return mt9v032_update_hblank(mt9v032);
 
 	case V4L2_CID_VBLANK:
-		return mt9v032_write(client, MT9V032_VERTICAL_BLANKING,
-				     ctrl->val);
+		return regmap_write(map, MT9V032_VERTICAL_BLANKING,
+				    ctrl->val);
 
 	case V4L2_CID_PIXEL_RATE:
 	case V4L2_CID_LINK_FREQ:
@@ -723,7 +690,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
 			     | MT9V032_TEST_PATTERN_FLIP;
 			break;
 		}
-		return mt9v032_write(client, MT9V032_TEST_PATTERN, data);
+		return regmap_write(map, MT9V032_TEST_PATTERN, data);
 	}
 
 	return 0;
@@ -791,7 +758,7 @@ static int mt9v032_registered(struct v4l2_subdev *subdev)
 	struct i2c_client *client = v4l2_get_subdevdata(subdev);
 	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
 	unsigned int i;
-	s32 version;
+	u32 version;
 	int ret;
 
 	dev_info(&client->dev, "Probing MT9V032 at address 0x%02x\n",
@@ -804,10 +771,10 @@ static int mt9v032_registered(struct v4l2_subdev *subdev)
 	}
 
 	/* Read and check the sensor version */
-	version = mt9v032_read(client, MT9V032_CHIP_VERSION);
-	if (version < 0) {
+	ret = regmap_read(mt9v032->regmap, MT9V032_CHIP_VERSION, &version);
+	if (ret < 0) {
 		dev_err(&client->dev, "Failed reading chip version\n");
-		return version;
+		return ret;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(mt9v032_versions); ++i) {
@@ -894,6 +861,13 @@ static const struct v4l2_subdev_internal_ops mt9v032_subdev_internal_ops = {
 	.close = mt9v032_close,
 };
 
+static const struct regmap_config mt9v032_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xff,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 /* -----------------------------------------------------------------------------
  * Driver initialization and probing
  */
@@ -917,6 +891,10 @@ static int mt9v032_probe(struct i2c_client *client,
 	if (!mt9v032)
 		return -ENOMEM;
 
+	mt9v032->regmap = devm_regmap_init_i2c(client, &mt9v032_regmap_config);
+	if (IS_ERR(mt9v032->regmap))
+		return PTR_ERR(mt9v032->regmap);
+
 	mt9v032->clk = devm_clk_get(&client->dev, NULL);
 	if (IS_ERR(mt9v032->clk))
 		return PTR_ERR(mt9v032->clk);
-- 
2.0.0.rc2

Re: [PATCH v2 5/5] [media] mt9v032: use regmap

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Tuesday 03 June 2014 11:35:55 Philipp Zabel wrote:
> This switches all register accesses to use regmap. It allows to
> use the regmap cache, tracing, and debug register dump facilities,
> and removes the need to open code read-modify-writes.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

This looks good to me, but I have two small questions:

- How does regmap handle endianness ? It seems to hardcode a big endian byte 
order, which is fortunately what we need here. I suppose you've successfully 
tested this patch :-)

- How does regmap handle the register cache ? Will it try to populate it when 
initialized, or will it only read registers on demand due to a read or an 
update bits operation ?

> ---
> This patch was not included before.
> ---
>  drivers/media/i2c/Kconfig   |   1 +
>  drivers/media/i2c/mt9v032.c | 112 +++++++++++++++++------------------------
>  2 files changed, 46 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 441053b..f40b4cf 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -551,6 +551,7 @@ config VIDEO_MT9V032
>  	tristate "Micron MT9V032 sensor support"
>  	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>  	depends on MEDIA_CAMERA_SUPPORT
> +	select REGMAP_I2C
>  	---help---
>  	  This is a Video4Linux2 sensor-level driver for the Micron
>  	  MT9V032 752x480 CMOS sensor.
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index cb7c6df..e756d50 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -17,6 +17,7 @@
>  #include <linux/i2c.h>
>  #include <linux/log2.h>
>  #include <linux/mutex.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
>  #include <linux/v4l2-mediabus.h>
> @@ -245,6 +246,7 @@ struct mt9v032 {
>  	struct mutex power_lock;
>  	int power_count;
> 
> +	struct regmap *regmap;
>  	struct clk *clk;
> 
>  	struct mt9v032_platform_data *pdata;
> @@ -252,7 +254,6 @@ struct mt9v032 {
>  	const struct mt9v032_model_version *version;
> 
>  	u32 sysclk;
> -	u16 chip_control;
>  	u16 aec_agc;
>  	u16 hblank;
>  	struct {
> @@ -266,40 +267,10 @@ static struct mt9v032 *to_mt9v032(struct v4l2_subdev
> *sd) return container_of(sd, struct mt9v032, subdev);
>  }
> 
> -static int mt9v032_read(struct i2c_client *client, const u8 reg)
> -{
> -	s32 data = i2c_smbus_read_word_swapped(client, reg);
> -	dev_dbg(&client->dev, "%s: read 0x%04x from 0x%02x\n", __func__,
> -		data, reg);
> -	return data;
> -}
> -
> -static int mt9v032_write(struct i2c_client *client, const u8 reg,
> -			 const u16 data)
> -{
> -	dev_dbg(&client->dev, "%s: writing 0x%04x to 0x%02x\n", __func__,
> -		data, reg);
> -	return i2c_smbus_write_word_swapped(client, reg, data);
> -}
> -
> -static int mt9v032_set_chip_control(struct mt9v032 *mt9v032, u16 clear, u16
> set) -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> -	u16 value = (mt9v032->chip_control & ~clear) | set;
> -	int ret;
> -
> -	ret = mt9v032_write(client, MT9V032_CHIP_CONTROL, value);
> -	if (ret < 0)
> -		return ret;
> -
> -	mt9v032->chip_control = value;
> -	return 0;
> -}
> -
>  static int
>  mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> +	struct regmap *map = mt9v032->regmap;
>  	u16 value = mt9v032->aec_agc;
>  	int ret;
> 
> @@ -308,7 +279,7 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> which, int enable) else
>  		value &= ~which;
> 
> -	ret = mt9v032_write(client, MT9V032_AEC_AGC_ENABLE, value);
> +	ret = regmap_write(map, MT9V032_AEC_AGC_ENABLE, value);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -319,7 +290,6 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> which, int enable) static int
>  mt9v032_update_hblank(struct mt9v032 *mt9v032)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
>  	struct v4l2_rect *crop = &mt9v032->crop;
>  	unsigned int min_hblank = mt9v032->model->data->min_hblank;
>  	unsigned int hblank;
> @@ -330,12 +300,13 @@ mt9v032_update_hblank(struct mt9v032 *mt9v032)
>  			   min_hblank);
>  	hblank = max_t(unsigned int, mt9v032->hblank, min_hblank);
> 
> -	return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING, hblank);
> +	return regmap_write(mt9v032->regmap, MT9V032_HORIZONTAL_BLANKING,
> +			    hblank);
>  }
> 
>  static int mt9v032_power_on(struct mt9v032 *mt9v032)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> +	struct regmap *map = mt9v032->regmap;
>  	unsigned long rate;
>  	int ret;
> 
> @@ -350,7 +321,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
>  	udelay(1);
> 
>  	/* Reset the chip and stop data read out */
> -	ret = mt9v032_write(client, MT9V032_RESET, 1);
> +	ret = regmap_write(map, MT9V032_RESET, 1);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -358,7 +329,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
>  	rate = clk_get_rate(mt9v032->clk);
>  	ndelay(DIV_ROUND_UP(15 * 125000000, rate >> 3));
> 
> -	return mt9v032_write(client, MT9V032_CHIP_CONTROL, 0);
> +	return regmap_write(map, MT9V032_CHIP_CONTROL, 0);
>  }
> 
>  static void mt9v032_power_off(struct mt9v032 *mt9v032)
> @@ -368,7 +339,7 @@ static void mt9v032_power_off(struct mt9v032 *mt9v032)
> 
>  static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> +	struct regmap *map = mt9v032->regmap;
>  	int ret;
> 
>  	if (!on) {
> @@ -382,14 +353,14 @@ static int __mt9v032_set_power(struct mt9v032
> *mt9v032, bool on)
> 
>  	/* Configure the pixel clock polarity */
>  	if (mt9v032->pdata && mt9v032->pdata->clk_pol) {
> -		ret = mt9v032_write(client, mt9v032->model->data->pclk_reg,
> +		ret = regmap_write(map, mt9v032->model->data->pclk_reg,
>  				MT9V032_PIXEL_CLOCK_INV_PXL_CLK);
>  		if (ret < 0)
>  			return ret;
>  	}
> 
>  	/* Disable the noise correction algorithm and restore the controls. */
> -	ret = mt9v032_write(client, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
> +	ret = regmap_write(map, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -433,43 +404,39 @@ static int mt9v032_s_stream(struct v4l2_subdev
> *subdev, int enable) const u16 mode = MT9V032_CHIP_CONTROL_MASTER_MODE
> 
>  		       | MT9V032_CHIP_CONTROL_DOUT_ENABLE
>  		       | MT9V032_CHIP_CONTROL_SEQUENTIAL;
> 
> -	struct i2c_client *client = v4l2_get_subdevdata(subdev);
>  	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
>  	struct v4l2_rect *crop = &mt9v032->crop;
> -	unsigned int read_mode;
> +	struct regmap *map = mt9v032->regmap;
>  	unsigned int hbin;
>  	unsigned int vbin;
>  	int ret;
> 
>  	if (!enable)
> -		return mt9v032_set_chip_control(mt9v032, mode, 0);
> +		return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, 0);
> 
>  	/* Configure the window size and row/column bin */
>  	hbin = fls(mt9v032->hratio) - 1;
>  	vbin = fls(mt9v032->vratio) - 1;
> -	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> -	if (read_mode < 0)
> -		return read_mode;
> -	read_mode &= MT9V032_READ_MODE_RESERVED;
> -	read_mode |= hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> -		     vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT;
> -	ret = mt9v032_write(client, MT9V032_READ_MODE, read_mode);
> +	ret = regmap_update_bits(map, MT9V032_READ_MODE,
> +				 ~MT9V032_READ_MODE_RESERVED,
> +				 hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> +				 vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = mt9v032_write(client, MT9V032_COLUMN_START, crop->left);
> +	ret = regmap_write(map, MT9V032_COLUMN_START, crop->left);
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = mt9v032_write(client, MT9V032_ROW_START, crop->top);
> +	ret = regmap_write(map, MT9V032_ROW_START, crop->top);
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = mt9v032_write(client, MT9V032_WINDOW_WIDTH, crop->width);
> +	ret = regmap_write(map, MT9V032_WINDOW_WIDTH, crop->width);
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = mt9v032_write(client, MT9V032_WINDOW_HEIGHT, crop->height);
> +	ret = regmap_write(map, MT9V032_WINDOW_HEIGHT, crop->height);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -478,7 +445,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev,
> int enable) return ret;
> 
>  	/* Switch to master "normal" mode */
> -	return mt9v032_set_chip_control(mt9v032, 0, mode);
> +	return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, mode);
>  }
> 
>  static int mt9v032_enum_mbus_code(struct v4l2_subdev *subdev,
> @@ -660,7 +627,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9v032 *mt9v032 =
>  			container_of(ctrl->handler, struct mt9v032, ctrls);
> -	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> +	struct regmap *map = mt9v032->regmap;
>  	u32 freq;
>  	u16 data;
> 
> @@ -670,23 +637,23 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
>  					      ctrl->val);
> 
>  	case V4L2_CID_GAIN:
> -		return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
> +		return regmap_write(map, MT9V032_ANALOG_GAIN, ctrl->val);
> 
>  	case V4L2_CID_EXPOSURE_AUTO:
>  		return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
>  					      !ctrl->val);
> 
>  	case V4L2_CID_EXPOSURE:
> -		return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
> -				     ctrl->val);
> +		return regmap_write(map, MT9V032_TOTAL_SHUTTER_WIDTH,
> +				    ctrl->val);
> 
>  	case V4L2_CID_HBLANK:
>  		mt9v032->hblank = ctrl->val;
>  		return mt9v032_update_hblank(mt9v032);
> 
>  	case V4L2_CID_VBLANK:
> -		return mt9v032_write(client, MT9V032_VERTICAL_BLANKING,
> -				     ctrl->val);
> +		return regmap_write(map, MT9V032_VERTICAL_BLANKING,
> +				    ctrl->val);
> 
>  	case V4L2_CID_PIXEL_RATE:
>  	case V4L2_CID_LINK_FREQ:
> @@ -723,7 +690,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> 
>  			     | MT9V032_TEST_PATTERN_FLIP;
> 
>  			break;
>  		}
> -		return mt9v032_write(client, MT9V032_TEST_PATTERN, data);
> +		return regmap_write(map, MT9V032_TEST_PATTERN, data);
>  	}
> 
>  	return 0;
> @@ -791,7 +758,7 @@ static int mt9v032_registered(struct v4l2_subdev
> *subdev) struct i2c_client *client = v4l2_get_subdevdata(subdev);
>  	struct mt9v032 *mt9v032 = to_mt9v032(subdev);
>  	unsigned int i;
> -	s32 version;
> +	u32 version;
>  	int ret;
> 
>  	dev_info(&client->dev, "Probing MT9V032 at address 0x%02x\n",
> @@ -804,10 +771,10 @@ static int mt9v032_registered(struct v4l2_subdev
> *subdev) }
> 
>  	/* Read and check the sensor version */
> -	version = mt9v032_read(client, MT9V032_CHIP_VERSION);
> -	if (version < 0) {
> +	ret = regmap_read(mt9v032->regmap, MT9V032_CHIP_VERSION, &version);
> +	if (ret < 0) {
>  		dev_err(&client->dev, "Failed reading chip version\n");
> -		return version;
> +		return ret;
>  	}
> 
>  	for (i = 0; i < ARRAY_SIZE(mt9v032_versions); ++i) {
> @@ -894,6 +861,13 @@ static const struct v4l2_subdev_internal_ops
> mt9v032_subdev_internal_ops = { .close = mt9v032_close,
>  };
> 
> +static const struct regmap_config mt9v032_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xff,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Driver initialization and probing
>   */
> @@ -917,6 +891,10 @@ static int mt9v032_probe(struct i2c_client *client,
>  	if (!mt9v032)
>  		return -ENOMEM;
> 
> +	mt9v032->regmap = devm_regmap_init_i2c(client, &mt9v032_regmap_config);
> +	if (IS_ERR(mt9v032->regmap))
> +		return PTR_ERR(mt9v032->regmap);
> +
>  	mt9v032->clk = devm_clk_get(&client->dev, NULL);
>  	if (IS_ERR(mt9v032->clk))
>  		return PTR_ERR(mt9v032->clk);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] [media] mt9v032: use regmap

[Philipp Zabel]

Hi Laurent,

Am Mittwoch, den 04.06.2014, 17:44 +0200 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Tuesday 03 June 2014 11:35:55 Philipp Zabel wrote:
> > This switches all register accesses to use regmap. It allows to
> > use the regmap cache, tracing, and debug register dump facilities,
> > and removes the need to open code read-modify-writes.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> This looks good to me, but I have two small questions:
> 
> - How does regmap handle endianness ? It seems to hardcode a big endian byte 
> order, which is fortunately what we need here.

We could set regmap_config->val_format_endian = REGMAP_ENDIAN_BIG.
This defaults to big endian unless the regmap_bus says otherwise
(regmap-i2c doesn't).

> I suppose you've successfully tested this patch :-)

Yes.

> - How does regmap handle the register cache ? Will it try to populate it when 
> initialized, or will it only read registers on demand due to a read or an 
> update bits operation ?

That depends on the cache implementation. regcache-rbtree has a
cache_present bitmap per node. As long as the corresponding bit is not
set, regcache_read will return -ENOENT and regmap_read will then do an
actual register read (and store the result in the cache).

regcache-flat doesn't have this at all, so it would be necessary to
provide initial register values in the driver or explicitly read back
from the hardware during initialization. This is also be possible with
the rbtree cache.

regards
Philipp

Re: [PATCH v2 5/5] [media] mt9v032: use regmap

[Laurent Pinchart]

Hi Philipp,

On Wednesday 04 June 2014 18:45:28 Philipp Zabel wrote:
> Am Mittwoch, den 04.06.2014, 17:44 +0200 schrieb Laurent Pinchart:
> > On Tuesday 03 June 2014 11:35:55 Philipp Zabel wrote:
> > > This switches all register accesses to use regmap. It allows to
> > > use the regmap cache, tracing, and debug register dump facilities,
> > > and removes the need to open code read-modify-writes.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > This looks good to me, but I have two small questions:
> > 
> > - How does regmap handle endianness ? It seems to hardcode a big endian
> > byte order, which is fortunately what we need here.
> 
> We could set regmap_config->val_format_endian = REGMAP_ENDIAN_BIG.
> This defaults to big endian unless the regmap_bus says otherwise
> (regmap-i2c doesn't).

OK, thank you for the information.

> > I suppose you've successfully tested this patch :-)
> 
> Yes.
> 
> > - How does regmap handle the register cache ? Will it try to populate it
> > when initialized, or will it only read registers on demand due to a read
> > or an update bits operation ?
> 
> That depends on the cache implementation. regcache-rbtree has a
> cache_present bitmap per node. As long as the corresponding bit is not
> set, regcache_read will return -ENOENT and regmap_read will then do an
> actual register read (and store the result in the cache).
> 
> regcache-flat doesn't have this at all, so it would be necessary to
> provide initial register values in the driver or explicitly read back
> from the hardware during initialization. This is also be possible with
> the rbtree cache.

That sounds good to me. I'll apply your rebased version.

-- 
Regards,

Laurent Pinchart