[RFC,media] mt9v032: Add support for mt9v022 and mt9v024

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>
---
 drivers/media/i2c/mt9v032.c | 56 +++++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 14 deletions(-)
  

Hi Philipp,

On Mon, 26 May 2014, 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.

Are you aware of drivers/media/i2c/soc_camera/mt9v022.c? With this patch 
you'd duplicate support for both mt9v022 and mt9v024, which doesn't look 
like a good idea to me.

Thanks
Guennadi

> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/i2c/mt9v032.c | 56 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 052e754..617b77f 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>
>   *
> @@ -68,6 +68,7 @@
>  #define		MT9V032_CHIP_CONTROL_MASTER_MODE	(1 << 3)
>  #define		MT9V032_CHIP_CONTROL_DOUT_ENABLE	(1 << 7)
>  #define		MT9V032_CHIP_CONTROL_SEQUENTIAL		(1 << 8)
> +#define		MT9V024_CHIP_CONTROL_DEF_PIX_CORRECT	(1 << 9)
>  #define MT9V032_SHUTTER_WIDTH1				0x08
>  #define MT9V032_SHUTTER_WIDTH2				0x09
>  #define MT9V032_SHUTTER_WIDTH_CONTROL			0x0a
> @@ -133,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,
>  };
> @@ -160,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,
> @@ -176,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,
> @@ -188,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,
> @@ -415,7 +436,6 @@ 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;
> @@ -426,11 +446,9 @@ 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;
> -	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> -	read_mode &= ~0xff; /* bits 0x300 are reserved, on MT9V024 */
> -	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 = mt9v032_write(client, MT9V032_READ_MODE,
> +			    hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> +			    vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -902,6 +920,12 @@ static int mt9v032_probe(struct i2c_client *client,
>  	mt9v032->pdata = pdata;
>  	mt9v032->model = (const void *)did->driver_data;
>  
> +	/* Keep defective pixel correction enabled on MT9V024 */
> +	ret = mt9v032_read(client, MT9V032_CHIP_CONTROL);
> +	if (ret < 0)
> +		return ret;
> +	mt9v032->chip_control = ret & MT9V024_CHIP_CONTROL_DEF_PIX_CORRECT;
> +
>  	v4l2_ctrl_handler_init(&mt9v032->ctrls, 10);
>  
>  	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> @@ -1015,6 +1039,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Guennadi,

Am Dienstag, den 27.05.2014, 21:48 +0200 schrieb Guennadi Liakhovetski:
> Hi Philipp,
> 
> On Mon, 26 May 2014, 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.
> 
> Are you aware of drivers/media/i2c/soc_camera/mt9v022.c?

Yes. Unfortunately this driver can't be used in a system without
soc_camera. It uses soc_camera helpers and doesn't implement pad ops
among others.

> With this patch you'd duplicate support for both mt9v022 and mt9v024,
> which doesn't look like a good idea to me.

While this is true, given that the mt9v02x/3x sensors are so similar,
the support is already duplicated in all but name.
Would you suggest we should try to merge the mt9v032 and mt9v022
drivers?

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Wed, 28 May 2014, Philipp Zabel wrote:

> Hi Guennadi,
> 
> Am Dienstag, den 27.05.2014, 21:48 +0200 schrieb Guennadi Liakhovetski:
> > Hi Philipp,
> > 
> > On Mon, 26 May 2014, 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.
> > 
> > Are you aware of drivers/media/i2c/soc_camera/mt9v022.c?
> 
> Yes. Unfortunately this driver can't be used in a system without
> soc_camera. It uses soc_camera helpers and doesn't implement pad ops
> among others.

As I mentioned many times, this compatibility is a matter of someone just 
needing and finally doing this. If you need this, please, extend the 
mt9v022 driver to also work with non soc-camera hosts, if you need any 
help - please feel free to ask, I can send you my conversion code, that 
I've done for ov772x, but never managed to finalise testing, 
unfortunately.

> > With this patch you'd duplicate support for both mt9v022 and mt9v024,
> > which doesn't look like a good idea to me.
> 
> While this is true, given that the mt9v02x/3x sensors are so similar,
> the support is already duplicated in all but name.
> Would you suggest we should try to merge the mt9v032 and mt9v022
> drivers?

Out of 3 options:

1. extend mt9v022 to work with non soc-camera hosts
2. extend mt9v032 to also support mt9v022 and mt9v024
3. merge both mt9v022 and mt9v032 drivers

option 2 seems the worst to me. I'm ok with either 1 or 3, whereas 3 is 
more difficult than 1.

Thanks
Guennadi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hello,

On Wednesday 28 May 2014 12:07:57 Guennadi Liakhovetski wrote:
> On Wed, 28 May 2014, Philipp Zabel wrote:
> > Am Dienstag, den 27.05.2014, 21:48 +0200 schrieb Guennadi Liakhovetski:
> > > On Mon, 26 May 2014, 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.
> > > 
> > > Are you aware of drivers/media/i2c/soc_camera/mt9v022.c?
> > 
> > Yes. Unfortunately this driver can't be used in a system without
> > soc_camera. It uses soc_camera helpers and doesn't implement pad ops
> > among others.
> 
> As I mentioned many times, this compatibility is a matter of someone just
> needing and finally doing this. If you need this, please, extend the
> mt9v022 driver to also work with non soc-camera hosts, if you need any
> help - please feel free to ask, I can send you my conversion code, that
> I've done for ov772x, but never managed to finalise testing,
> unfortunately.
>
> > > With this patch you'd duplicate support for both mt9v022 and mt9v024,
> > > which doesn't look like a good idea to me.
> > 
> > While this is true, given that the mt9v02x/3x sensors are so similar,
> > the support is already duplicated in all but name.
> > Would you suggest we should try to merge the mt9v032 and mt9v022
> > drivers?
> 
> Out of 3 options:
> 
> 1. extend mt9v022 to work with non soc-camera hosts
> 2. extend mt9v032 to also support mt9v022 and mt9v024
> 3. merge both mt9v022 and mt9v032 drivers
> 
> option 2 seems the worst to me. I'm ok with either 1 or 3, whereas 3 is
> more difficult than 1.

This topic has been discussed over and over. It indeed "just" requires someone 
to do it, although it might be more complex than that sounds.

We need to fix the infrastructure to make sensor drivers completely unaware of 
soc-camera. This isn't about extending the mt9v022 driver to work with non 
soc-camera hosts, it's about fixing soc-camera not to require any change to 
sensor drivers. Philipp, if you have time to work on that, we can discuss what 
needs to be done.

On the sensor side, we should have a single driver for the mt9v022, 024 and 
032 sensors. I would vote for merging the two drivers into 
drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not being 
soc-camera specific.
  

Hi Philipp,

Thank you for the patch.

On Monday 26 May 2014 16:03:05 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>
> ---
>  drivers/media/i2c/mt9v032.c | 56 ++++++++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 052e754..617b77f 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>
> *
> @@ -68,6 +68,7 @@
>  #define		MT9V032_CHIP_CONTROL_MASTER_MODE	(1 << 3)
>  #define		MT9V032_CHIP_CONTROL_DOUT_ENABLE	(1 << 7)
>  #define		MT9V032_CHIP_CONTROL_SEQUENTIAL		(1 << 8)
> +#define		MT9V024_CHIP_CONTROL_DEF_PIX_CORRECT	(1 << 9)
>  #define MT9V032_SHUTTER_WIDTH1				0x08
>  #define MT9V032_SHUTTER_WIDTH2				0x09
>  #define MT9V032_SHUTTER_WIDTH_CONTROL			0x0a
> @@ -133,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,
>  };
> @@ -160,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,
> @@ -176,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,
> @@ -188,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,
> @@ -415,7 +436,6 @@ 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;
> @@ -426,11 +446,9 @@ 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;
> -	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> -	read_mode &= ~0xff; /* bits 0x300 are reserved, on MT9V024 */
> -	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 = mt9v032_write(client, MT9V032_READ_MODE,
> +			    hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> +			    vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);

Doesn't this change completely reverse one of your previous patches ? Is it 
needed, or is it included here by mistake ?

>  	if (ret < 0)
>  		return ret;
> 
> @@ -902,6 +920,12 @@ static int mt9v032_probe(struct i2c_client *client,
>  	mt9v032->pdata = pdata;
>  	mt9v032->model = (const void *)did->driver_data;
> 
> +	/* Keep defective pixel correction enabled on MT9V024 */
> +	ret = mt9v032_read(client, MT9V032_CHIP_CONTROL);
> +	if (ret < 0)
> +		return ret;
> +	mt9v032->chip_control = ret & MT9V024_CHIP_CONTROL_DEF_PIX_CORRECT;

On the MT9V034 bit 9 is marked as reserved according to revision A of the 
datasheet, defaults to 1 after power-up and must be written to 0 for proper 
operation. This could thus break operation on the MT9V034. I don't have a copy 
of the MT9V022 register reference manual to check what happens on that chip 
though.

> +
>  	v4l2_ctrl_handler_init(&mt9v032->ctrls, 10);
> 
>  	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> @@ -1015,6 +1039,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] },
  

Hi Guennadi, Laurent,

Am Mittwoch, den 28.05.2014, 13:04 +0200 schrieb Laurent Pinchart:
> Hello,
> 
> On Wednesday 28 May 2014 12:07:57 Guennadi Liakhovetski wrote:
> > On Wed, 28 May 2014, Philipp Zabel wrote:
> > > Am Dienstag, den 27.05.2014, 21:48 +0200 schrieb Guennadi Liakhovetski:
> > > > On Mon, 26 May 2014, 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.
> > > > 
> > > > Are you aware of drivers/media/i2c/soc_camera/mt9v022.c?
> > > 
> > > Yes. Unfortunately this driver can't be used in a system without
> > > soc_camera. It uses soc_camera helpers and doesn't implement pad ops
> > > among others.
> > 
> > As I mentioned many times, this compatibility is a matter of someone just
> > needing and finally doing this. If you need this, please, extend the
> > mt9v022 driver to also work with non soc-camera hosts, if you need any
> > help - please feel free to ask, I can send you my conversion code, that
> > I've done for ov772x, but never managed to finalise testing,
> > unfortunately.
> >
> > > > With this patch you'd duplicate support for both mt9v022 and mt9v024,
> > > > which doesn't look like a good idea to me.
> > > 
> > > While this is true, given that the mt9v02x/3x sensors are so similar,
> > > the support is already duplicated in all but name.
> > > Would you suggest we should try to merge the mt9v032 and mt9v022
> > > drivers?
> > 
> > Out of 3 options:
> > 
> > 1. extend mt9v022 to work with non soc-camera hosts
> > 2. extend mt9v032 to also support mt9v022 and mt9v024
> > 3. merge both mt9v022 and mt9v032 drivers
> > 
> > option 2 seems the worst to me.

It also is the easiest to achieve and the mt9v032 driver is prettier (as
in doesn't have support for the external gpio bus shifter, which I don't
think belongs in the sensor driver).

> > I'm ok with either 1 or 3, whereas 3 is
> > more difficult than 1.
> 
> This topic has been discussed over and over. It indeed "just" requires someone 
> to do it, although it might be more complex than that sounds.
> 
> We need to fix the infrastructure to make sensor drivers completely unaware of 
> soc-camera. This isn't about extending the mt9v022 driver to work with non 
> soc-camera hosts, it's about fixing soc-camera not to require any change to 
> sensor drivers. Philipp, if you have time to work on that, we can discuss what 
> needs to be done.

I don't have a use case for soc_camera. Instead of trying to fix it to
use generic sensor drivers, I'd rather use that time to prepare
non-soc_camera capture host support.

> On the sensor side, we should have a single driver for the mt9v022, 024 and 
> 032 sensors. I would vote for merging the two drivers into 
> drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not being 
> soc-camera specific.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Laurent,

Am Mittwoch, den 28.05.2014, 13:43 +0200 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
>
> On Monday 26 May 2014 16:03:05 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>
> > ---
> >  drivers/media/i2c/mt9v032.c | 56 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 42 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> > index 052e754..617b77f 100644
> > --- a/drivers/media/i2c/mt9v032.c
> > +++ b/drivers/media/i2c/mt9v032.c
[...]
> > @@ -426,11 +446,9 @@ 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;
> > -	read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> > -	read_mode &= ~0xff; /* bits 0x300 are reserved, on MT9V024 */
> > -	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 = mt9v032_write(client, MT9V032_READ_MODE,
> > +			    hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> > +			    vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
> 
> Doesn't this change completely reverse one of your previous patches ? Is it 
> needed, or is it included here by mistake ?

That was by mistake. It answers the question whether or not the mt9v02x
sensors work without this change, though: Clearing those two bits
doesn't seem to hurt.

> >  	if (ret < 0)
> >  		return ret;
> > 
> > @@ -902,6 +920,12 @@ static int mt9v032_probe(struct i2c_client *client,
> >  	mt9v032->pdata = pdata;
> >  	mt9v032->model = (const void *)did->driver_data;
> > 
> > +	/* Keep defective pixel correction enabled on MT9V024 */
> > +	ret = mt9v032_read(client, MT9V032_CHIP_CONTROL);
> > +	if (ret < 0)
> > +		return ret;
> > +	mt9v032->chip_control = ret & MT9V024_CHIP_CONTROL_DEF_PIX_CORRECT;
> 
> On the MT9V034 bit 9 is marked as reserved according to revision A of the 
> datasheet, defaults to 1 after power-up and must be written to 0 for proper 
> operation. This could thus break operation on the MT9V034. I don't have a copy 
> of the MT9V022 register reference manual to check what happens on that chip 
> though.

I have seen this described as defect pixel correction enable bit on
mt9v022 in this document:
http://www.videologyinc.com/media/products/application%20notes/APN-24B752XA.pdf

The mt9v02x sensors still seem to work without this bit enabled.
I could split this out into a separate patch.

> > +
> >  	v4l2_ctrl_handler_init(&mt9v032->ctrls, 10);
> > 
> >  	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
> > @@ -1015,6 +1039,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
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Philipp,

On Wednesday 28 May 2014 16:36:55 Philipp Zabel wrote:
> Am Mittwoch, den 28.05.2014, 13:04 +0200 schrieb Laurent Pinchart:
> > On Wednesday 28 May 2014 12:07:57 Guennadi Liakhovetski wrote:
> > > On Wed, 28 May 2014, Philipp Zabel wrote:
> > > > Am Dienstag, den 27.05.2014, 21:48 +0200 schrieb Guennadi 
Liakhovetski:
> > > > > On Mon, 26 May 2014, 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.
> > > > > 
> > > > > Are you aware of drivers/media/i2c/soc_camera/mt9v022.c?
> > > > 
> > > > Yes. Unfortunately this driver can't be used in a system without
> > > > soc_camera. It uses soc_camera helpers and doesn't implement pad ops
> > > > among others.
> > > 
> > > As I mentioned many times, this compatibility is a matter of someone
> > > just needing and finally doing this. If you need this, please, extend
> > > the mt9v022 driver to also work with non soc-camera hosts, if you need
> > > any help - please feel free to ask, I can send you my conversion code,
> > > that I've done for ov772x, but never managed to finalise testing,
> > > unfortunately.
> > > 
> > > > > With this patch you'd duplicate support for both mt9v022 and
> > > > > mt9v024, which doesn't look like a good idea to me.
> > > > 
> > > > While this is true, given that the mt9v02x/3x sensors are so similar,
> > > > the support is already duplicated in all but name.
> > > > Would you suggest we should try to merge the mt9v032 and mt9v022
> > > > drivers?
> > > 
> > > Out of 3 options:
> > > 
> > > 1. extend mt9v022 to work with non soc-camera hosts
> > > 2. extend mt9v032 to also support mt9v022 and mt9v024
> > > 3. merge both mt9v022 and mt9v032 drivers
> > > 
> > > option 2 seems the worst to me.
> 
> It also is the easiest to achieve and the mt9v032 driver is prettier (as
> in doesn't have support for the external gpio bus shifter, which I don't
> think belongs in the sensor driver).

If you had submitted an entirely new driver for a sensor already supported by 
an soc-camera sensor driver, I would have told you to fix the problem on soc-
camera side. As you're only expanding hardware support for an existing driver, 
it's hard to nack your patch in all fairness :-) I will thus not veto option 
2, even though I would prefer if we fixed the problem once and for all. This 
doesn't mean others will accept the option though.

> > > I'm ok with either 1 or 3, whereas 3 is
> > > more difficult than 1.
> > 
> > This topic has been discussed over and over. It indeed "just" requires
> > someone to do it, although it might be more complex than that sounds.
> > 
> > We need to fix the infrastructure to make sensor drivers completely
> > unaware of soc-camera. This isn't about extending the mt9v022 driver to
> > work with non soc-camera hosts, it's about fixing soc-camera not to
> > require any change to sensor drivers. Philipp, if you have time to work
> > on that, we can discuss what needs to be done.
> 
> I don't have a use case for soc_camera. Instead of trying to fix it to
> use generic sensor drivers, I'd rather use that time to prepare
> non-soc_camera capture host support.

Which host would that be, if you can tell ?

> > On the sensor side, we should have a single driver for the mt9v022, 024
> > and 032 sensors. I would vote for merging the two drivers into
> > drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not
> > being soc-camera specific.
  

Hi Laurent, Guennadi,

Am Mittwoch, den 28.05.2014, 16:44 +0200 schrieb Laurent Pinchart:
> If you had submitted an entirely new driver for a sensor already supported by 
> an soc-camera sensor driver, I would have told you to fix the problem on soc-
> camera side. As you're only expanding hardware support for an existing driver, 
> it's hard to nack your patch in all fairness :-) I will thus not veto option 
> 2, even though I would prefer if we fixed the problem once and for all.
>
> > > > I'm ok with either 1 or 3, whereas 3 is
> > > > more difficult than 1.
> > > 
> > > This topic has been discussed over and over. It indeed "just" requires
> > > someone to do it, although it might be more complex than that sounds.
> > > 
> > > We need to fix the infrastructure to make sensor drivers completely
> > > unaware of soc-camera. This isn't about extending the mt9v022 driver to
> > > work with non soc-camera hosts, it's about fixing soc-camera not to
> > > require any change to sensor drivers. Philipp, if you have time to work
> > > on that, we can discuss what needs to be done.

What steps would need to be taken to make soc_camera work with the
non-soc_camera drivers in drivers/media/i2c?
I don't have any soc_camera platform at hand, although I could try to revive
a PXA270 board.

> > I don't have a use case for soc_camera. Instead of trying to fix it to
> > use generic sensor drivers, I'd rather use that time to prepare
> > non-soc_camera capture host support.
> 
> Which host would that be, if you can tell ?

Yes, i.MX6.

> > > On the sensor side, we should have a single driver for the mt9v022, 024
> > > and 032 sensors. I would vote for merging the two drivers into
> > > drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not
> > > being soc-camera specific.

regards
Philipp


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Philipp,

On Tuesday 03 June 2014 11:30:31 Philipp Zabel wrote:
> Am Mittwoch, den 28.05.2014, 16:44 +0200 schrieb Laurent Pinchart:
> > If you had submitted an entirely new driver for a sensor already supported
> > by an soc-camera sensor driver, I would have told you to fix the problem
> > on soc- camera side. As you're only expanding hardware support for an
> > existing driver, it's hard to nack your patch in all fairness :-) I will
> > thus not veto option 2, even though I would prefer if we fixed the
> > problem once and for all.
> >
> > > > > I'm ok with either 1 or 3, whereas 3 is
> > > > > more difficult than 1.
> > > > 
> > > > This topic has been discussed over and over. It indeed "just" requires
> > > > someone to do it, although it might be more complex than that sounds.
> > > > 
> > > > We need to fix the infrastructure to make sensor drivers completely
> > > > unaware of soc-camera. This isn't about extending the mt9v022 driver
> > > > to work with non soc-camera hosts, it's about fixing soc-camera not to
> > > > require any change to sensor drivers. Philipp, if you have time to
> > > > work on that, we can discuss what needs to be done.
> 
> What steps would need to be taken to make soc_camera work with the
> non-soc_camera drivers in drivers/media/i2c?

Guennadi, what's the status of your work on this ? What remains to be done ?

The soc-camera core provides several helper functions for subdev drivers, and 
expects the subdev drivers to implement bus configuration negotiation with the 
g_mbus_config and s_mbus_config subdev operations.

If you look at the mt9v022 driver, the helper functions used are

- soc_camera_limit_side
- soc_mbus_get_fmtdesc
- soc_camera_set_power
- soc_camera_apply_board_flags

The first two functions are general purpose helpers that could be standardized 
and moved to the v4l core, or even left in soc-camera for now as they don't 
depend on the bridge driver being compatible with soc-camera.

The last two functions are mostly self-contained as well, but depend on the 
I2C sensor device using a soc-camera structure (soc_camera_subdev_desc) for 
its platform data. That structure contains field that are common across many 
sensors, as well as a pointer to a sensor-specific platform data structure. 
The common structure is then passed to various helper functions by the sensor 
driver.

That's something I would like to see being changed, the sensor should use a 
custom structure for its platform data. If the sensor drivers wants to use the 
soc-camera helpers that don't depend on the host-side of soc-camera, it could 
then embed a soc-camera platform data structure inside its own platform data 
structure, and pass a pointer to that embedded structure to the soc-camera 
helpers.

Another point that needs to be fixed is that soc-camera performs several 
initialization steps for the sensor before probing it, such as calling 
devm_regulator_bulk_get() to retrieve the sensor regulators. Those steps 
require the sensor to use the struct soc_camera_subdev_desc as platform data. 
This should be changed as well, sensor drivers should call a soc-camera helper 
function explicitly from their probe function to perform the same task.

I don't remember the details of how soc-camera handles [gs]_mbus_config, but 
changes might be needed there as well.

That's more or less what I see needing to be fixed. Guennadi, please feel free 
to correct me.

> I don't have any soc_camera platform at hand, although I could try to revive
> a PXA270 board.
> 
> > > I don't have a use case for soc_camera. Instead of trying to fix it to
> > > use generic sensor drivers, I'd rather use that time to prepare
> > > non-soc_camera capture host support.
> > 
> > Which host would that be, if you can tell ?
> 
> Yes, i.MX6.
> 
> > > > On the sensor side, we should have a single driver for the mt9v022,
> > > > 024 and 032 sensors. I would vote for merging the two drivers into
> > > > drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not
> > > > being soc-camera specific.
  

Hi Laurent, Philipp,

Sorry for a late reply.

On Thu, 5 Jun 2014, Laurent Pinchart wrote:

> Hi Philipp,
> 
> On Tuesday 03 June 2014 11:30:31 Philipp Zabel wrote:
> > Am Mittwoch, den 28.05.2014, 16:44 +0200 schrieb Laurent Pinchart:
> > > If you had submitted an entirely new driver for a sensor already supported
> > > by an soc-camera sensor driver, I would have told you to fix the problem
> > > on soc- camera side. As you're only expanding hardware support for an
> > > existing driver, it's hard to nack your patch in all fairness :-) I will
> > > thus not veto option 2, even though I would prefer if we fixed the
> > > problem once and for all.
> > >
> > > > > > I'm ok with either 1 or 3, whereas 3 is
> > > > > > more difficult than 1.
> > > > > 
> > > > > This topic has been discussed over and over. It indeed "just" requires
> > > > > someone to do it, although it might be more complex than that sounds.
> > > > > 
> > > > > We need to fix the infrastructure to make sensor drivers completely
> > > > > unaware of soc-camera. This isn't about extending the mt9v022 driver
> > > > > to work with non soc-camera hosts, it's about fixing soc-camera not to
> > > > > require any change to sensor drivers. Philipp, if you have time to
> > > > > work on that, we can discuss what needs to be done.
> > 
> > What steps would need to be taken to make soc_camera work with the
> > non-soc_camera drivers in drivers/media/i2c?
> 
> Guennadi, what's the status of your work on this ? What remains to be done ?

I just uploaded my last - unfinished - attempt to make an OMAP3 
beagle-board work with an OV772x sensor to 
http://download.open-technology.de/testing/soc-camera-integration/ Patches 
can be pushed upstream, the .diff is, obviously, still a WiP. The work 
hasn't been finished, because O got some problems with the video, but it 
could well have been a problem with the specific set up, not with the 
conversion. Those patches were last used with a -next snapshot from 
24.12.2013, so, they might not apply directly today, but soc-camera hasn't 
changed much since then, so, conflicts shouldn't be large or difficult to 
resolve.

> The soc-camera core provides several helper functions for subdev drivers, and 
> expects the subdev drivers to implement bus configuration negotiation with the 
> g_mbus_config and s_mbus_config subdev operations.
> 
> If you look at the mt9v022 driver, the helper functions used are
> 
> - soc_camera_limit_side
> - soc_mbus_get_fmtdesc
> - soc_camera_set_power
> - soc_camera_apply_board_flags
> 
> The first two functions are general purpose helpers that could be standardized 
> and moved to the v4l core, or even left in soc-camera for now as they don't 
> depend on the bridge driver being compatible with soc-camera.
> 
> The last two functions are mostly self-contained as well, but depend on the 
> I2C sensor device using a soc-camera structure (soc_camera_subdev_desc) for 
> its platform data.

This isn't a requirement. This is just how this specific driver chooses 
to have its platform data. The omap3-isp-ov772x.diff at the above location 
shows examples, how this can be changed.

> That structure contains field that are common across many 
> sensors, as well as a pointer to a sensor-specific platform data structure. 
> The common structure is then passed to various helper functions by the sensor 
> driver.
> 
> That's something I would like to see being changed, the sensor should use a 
> custom structure for its platform data.

This is done in those patches.

> If the sensor drivers wants to use the 
> soc-camera helpers that don't depend on the host-side of soc-camera, it could 
> then embed a soc-camera platform data structure inside its own platform data 
> structure, and pass a pointer to that embedded structure to the soc-camera 
> helpers.

Exactly, this is possible now too, just respective drivers and platforms 
have to be changed.

> Another point that needs to be fixed is that soc-camera performs several 
> initialization steps for the sensor before probing it, such as calling 
> devm_regulator_bulk_get() to retrieve the sensor regulators.

This is only done in the legacy mode. In async / DT mode I2C drivers have 
to call soc_camera_power_init() themselves.

> Those steps 
> require the sensor to use the struct soc_camera_subdev_desc as platform data. 
> This should be changed as well, sensor drivers should call a soc-camera helper 
> function explicitly from their probe function to perform the same task.
> 
> I don't remember the details of how soc-camera handles [gs]_mbus_config, but 
> changes might be needed there as well.

I think that's entirely up to specific camera host drivers.

> That's more or less what I see needing to be fixed. Guennadi, please feel free 
> to correct me.

AFAICS, the current state has almost no restrictions at the soc-camera 
core level. Most of the code at the above link deals with drivers and 
platforms, just a couple of tweaks were required for soc-camera core.

Would be nice if someone could try those patches on known-to-work 
hardware.

Thanks
Guennadi

> > I don't have any soc_camera platform at hand, although I could try to revive
> > a PXA270 board.
> > 
> > > > I don't have a use case for soc_camera. Instead of trying to fix it to
> > > > use generic sensor drivers, I'd rather use that time to prepare
> > > > non-soc_camera capture host support.
> > > 
> > > Which host would that be, if you can tell ?
> > 
> > Yes, i.MX6.
> > 
> > > > > On the sensor side, we should have a single driver for the mt9v022,
> > > > > 024 and 032 sensors. I would vote for merging the two drivers into
> > > > > drivers/media/i2c/mt9v032.c, as that one is closer to the goal of not
> > > > > being soc-camera specific.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html