diff mbox series

[3/5] media: i2c: imx290: Add RAW12 mode support

Message ID 20191129190541.30315-4-manivannan.sadhasivam@linaro.org
State New
Headers show
Series None | expand

Commit Message

Manivannan Sadhasivam Nov. 29, 2019, 7:05 p.m. UTC 
IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
12 bit formats. Since the driver already supports RAW10 mode, let's add
the missing RAW12 mode as well.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

---
 drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

-- 
2.17.1

Comments

Manivannan Sadhasivam Nov. 30, 2019, 2:09 p.m. UTC | #1 
Hi Fabio,

On Fri, Nov 29, 2019 at 04:49:25PM -0300, Fabio Estevam wrote:
> Hi Manivannan,

> 

> On Fri, Nov 29, 2019 at 4:07 PM Manivannan Sadhasivam

> <manivannan.sadhasivam@linaro.org> wrote:

>                }

> > +

> > +               imx290->bpp = 10;

> > +

> > +               break;

> > +       case MEDIA_BUS_FMT_SRGGB12_1X12:

> > +               ret = imx290_set_register_array(imx290, imx290_12bit_settings,

> > +                                               ARRAY_SIZE(

> > +                                                       imx290_12bit_settings));

> 

> Could you please write the ARRAY_SIZE and its parameter in the same line?

> 

> It would improve readability.

> 


I don't favor this change but Sakari did this to supress the checkpatch
warning while applying my initial patch, so now I did this here itself
to maintain the uniformity.

Thanks,
Mani

> Thanks
Sakari Ailus Dec. 3, 2019, 8:54 a.m. UTC | #2 
Hi Manivannan,

On Sat, Nov 30, 2019 at 12:35:39AM +0530, Manivannan Sadhasivam wrote:
> IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and

> 12 bit formats. Since the driver already supports RAW10 mode, let's add

> the missing RAW12 mode as well.

> 

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---

>  drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++

>  1 file changed, 32 insertions(+)

> 

> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c

> index e218c959a729..d5bb3a59ac46 100644

> --- a/drivers/media/i2c/imx290.c

> +++ b/drivers/media/i2c/imx290.c

> @@ -75,6 +75,7 @@ struct imx290 {

>  	struct clk *xclk;

>  	struct regmap *regmap;

>  	int nlanes;

> +	u8 bpp;

>  

>  	struct v4l2_subdev sd;

>  	struct v4l2_fwnode_endpoint ep;

> @@ -98,6 +99,7 @@ struct imx290_pixfmt {

>  

>  static const struct imx290_pixfmt imx290_formats[] = {

>  	{ MEDIA_BUS_FMT_SRGGB10_1X10 },

> +	{ MEDIA_BUS_FMT_SRGGB12_1X12 },

>  };

>  

>  static const struct regmap_config imx290_regmap_config = {

> @@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {

>  	{ 0x300b, 0x00},

>  };

>  

> +static const struct imx290_regval imx290_12bit_settings[] = {

> +	{ 0x3005, 0x01 },

> +	{ 0x3046, 0x01 },

> +	{ 0x3129, 0x00 },

> +	{ 0x317c, 0x00 },

> +	{ 0x31ec, 0x0e },

> +	{ 0x3441, 0x0c },

> +	{ 0x3442, 0x0c },

> +	{ 0x300a, 0xf0 },

> +	{ 0x300b, 0x00 },

> +};

> +

>  /* supported link frequencies */

>  static const s64 imx290_link_freq[] = {

>  	IMX290_DEFAULT_LINK_FREQ,

> @@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,

>  			dev_err(imx290->dev, "Could not set format registers\n");

>  			return ret;

>  		}

> +

> +		imx290->bpp = 10;

> +

> +		break;

> +	case MEDIA_BUS_FMT_SRGGB12_1X12:

> +		ret = imx290_set_register_array(imx290, imx290_12bit_settings,

> +						ARRAY_SIZE(

> +							imx290_12bit_settings));

> +		if (ret < 0) {

> +			dev_err(imx290->dev, "Could not set format registers\n");

> +			return ret;

> +		}

> +

> +		imx290->bpp = 12;

> +

>  		break;

>  	default:

>  		dev_err(imx290->dev, "Unknown pixel format\n");

> @@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)

>  		goto free_err;

>  	}

>  

> +	/* Default bits per pixel value */

> +	imx290->bpp = 10;


Where is the format being initialised at the moment? Nowhere?

If that is the case, I think it should be fixed before this patch.

> +

>  	mutex_init(&imx290->lock);

>  

>  	v4l2_ctrl_handler_init(&imx290->ctrls, 4);


-- 
Kind regards,

Sakari Ailus
Sakari Ailus Dec. 27, 2019, 3:28 p.m. UTC | #3 
Hi Manivannan,

On Sun, Dec 15, 2019 at 11:16:06PM +0530, Manivannan Sadhasivam wrote:
> Hi Sakari,

> 

> On Tue, Dec 03, 2019 at 10:54:17AM +0200, Sakari Ailus wrote:

> > Hi Manivannan,

> > 

> > On Sat, Nov 30, 2019 at 12:35:39AM +0530, Manivannan Sadhasivam wrote:

> > > IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and

> > > 12 bit formats. Since the driver already supports RAW10 mode, let's add

> > > the missing RAW12 mode as well.

> > > 

> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > > ---

> > >  drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++

> > >  1 file changed, 32 insertions(+)

> > > 

> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c

> > > index e218c959a729..d5bb3a59ac46 100644

> > > --- a/drivers/media/i2c/imx290.c

> > > +++ b/drivers/media/i2c/imx290.c

> > > @@ -75,6 +75,7 @@ struct imx290 {

> > >  	struct clk *xclk;

> > >  	struct regmap *regmap;

> > >  	int nlanes;

> > > +	u8 bpp;

> > >  

> > >  	struct v4l2_subdev sd;

> > >  	struct v4l2_fwnode_endpoint ep;

> > > @@ -98,6 +99,7 @@ struct imx290_pixfmt {

> > >  

> > >  static const struct imx290_pixfmt imx290_formats[] = {

> > >  	{ MEDIA_BUS_FMT_SRGGB10_1X10 },

> > > +	{ MEDIA_BUS_FMT_SRGGB12_1X12 },

> > >  };

> > >  

> > >  static const struct regmap_config imx290_regmap_config = {

> > > @@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {

> > >  	{ 0x300b, 0x00},

> > >  };

> > >  

> > > +static const struct imx290_regval imx290_12bit_settings[] = {

> > > +	{ 0x3005, 0x01 },

> > > +	{ 0x3046, 0x01 },

> > > +	{ 0x3129, 0x00 },

> > > +	{ 0x317c, 0x00 },

> > > +	{ 0x31ec, 0x0e },

> > > +	{ 0x3441, 0x0c },

> > > +	{ 0x3442, 0x0c },

> > > +	{ 0x300a, 0xf0 },

> > > +	{ 0x300b, 0x00 },

> > > +};

> > > +

> > >  /* supported link frequencies */

> > >  static const s64 imx290_link_freq[] = {

> > >  	IMX290_DEFAULT_LINK_FREQ,

> > > @@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,

> > >  			dev_err(imx290->dev, "Could not set format registers\n");

> > >  			return ret;

> > >  		}

> > > +

> > > +		imx290->bpp = 10;

> > > +

> > > +		break;

> > > +	case MEDIA_BUS_FMT_SRGGB12_1X12:

> > > +		ret = imx290_set_register_array(imx290, imx290_12bit_settings,

> > > +						ARRAY_SIZE(

> > > +							imx290_12bit_settings));

> > > +		if (ret < 0) {

> > > +			dev_err(imx290->dev, "Could not set format registers\n");

> > > +			return ret;

> > > +		}

> > > +

> > > +		imx290->bpp = 12;

> > > +

> > >  		break;

> > >  	default:

> > >  		dev_err(imx290->dev, "Unknown pixel format\n");

> > > @@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)

> > >  		goto free_err;

> > >  	}

> > >  

> > > +	/* Default bits per pixel value */

> > > +	imx290->bpp = 10;

> > 

> > Where is the format being initialised at the moment? Nowhere?

> > 

> > If that is the case, I think it should be fixed before this patch.

> > 

> 

> Sorry, I don't quite understand what you're suggesting here. The bpp

> is initialised because that's the default bit value at power up and

> this value is being used below in imx290_calc_pixel_rate(). I'm not sure

> why we need to initialise the format before set_fmt!


Alternatively set_fmt needs to be called then.

It looks like you can call VIDIOC_SUBDEV_G_FMT without the format being
initialised first, and if that's the case, then it's a driver bug. I don't
have the sensor so I can't test it.

-- 
Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e218c959a729..d5bb3a59ac46 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -75,6 +75,7 @@  struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	int nlanes;
+	u8 bpp;
 
 	struct v4l2_subdev sd;
 	struct v4l2_fwnode_endpoint ep;
@@ -98,6 +99,7 @@  struct imx290_pixfmt {
 
 static const struct imx290_pixfmt imx290_formats[] = {
 	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
+	{ MEDIA_BUS_FMT_SRGGB12_1X12 },
 };
 
 static const struct regmap_config imx290_regmap_config = {
@@ -265,6 +267,18 @@  static const struct imx290_regval imx290_10bit_settings[] = {
 	{ 0x300b, 0x00},
 };
 
+static const struct imx290_regval imx290_12bit_settings[] = {
+	{ 0x3005, 0x01 },
+	{ 0x3046, 0x01 },
+	{ 0x3129, 0x00 },
+	{ 0x317c, 0x00 },
+	{ 0x31ec, 0x0e },
+	{ 0x3441, 0x0c },
+	{ 0x3442, 0x0c },
+	{ 0x300a, 0xf0 },
+	{ 0x300b, 0x00 },
+};
+
 /* supported link frequencies */
 static const s64 imx290_link_freq[] = {
 	IMX290_DEFAULT_LINK_FREQ,
@@ -550,6 +564,21 @@  static int imx290_write_current_format(struct imx290 *imx290,
 			dev_err(imx290->dev, "Could not set format registers\n");
 			return ret;
 		}
+
+		imx290->bpp = 10;
+
+		break;
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+						ARRAY_SIZE(
+							imx290_12bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+
+		imx290->bpp = 12;
+
 		break;
 	default:
 		dev_err(imx290->dev, "Unknown pixel format\n");
@@ -910,6 +939,9 @@  static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
+	/* Default bits per pixel value */
+	imx290->bpp = 10;
+
 	mutex_init(&imx290->lock);
 
 	v4l2_ctrl_handler_init(&imx290->ctrls, 4);