| Message ID | 20191129190541.30315-4-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvements to IMX290 CMOS driver | expand |
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.
Thanks
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
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);
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! Thanks, Mani > > + > > mutex_init(&imx290->lock); > > > > v4l2_ctrl_handler_init(&imx290->ctrls, 4); > > -- > Kind regards, > > Sakari Ailus
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.
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);
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(+)