[PATCH v2 0/3] media: i2c: imx219: Feature enhancements

[PATCH v2 0/3] media: i2c: imx219: Feature enhancements

[Lad Prabhakar]

This patch series does the following:
1: Makes sure the sensor is LP11 state on power up
2: Adds support for RAW8
3: Adds support for 640x480 resolution

@Dave - I have tested setting RAW8/RAW10 formats using media-ctl
application, but was only able to test streaming for RAW8 (640x480)
format due to my hardware limitations.

Changes for v2:
1: Dropped setting the format in probe to coax the sensor to enter LP11
   state.
2: Fixed switching between RAW8/RAW10 modes.
3: Fixed fps setting for 640x480 and switched to auto mode.

Lad Prabhakar (3):
  media: i2c: imx219: Fix power sequence
  media: i2c: imx219: Add support for RAW8 bit bayer format
  media: i2c: imx219: Add support for cropped 640x480 resolution

 drivers/media/i2c/imx219.c | 250 +++++++++++++++++++++++++++++++------
 1 file changed, 214 insertions(+), 36 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] media: i2c: imx219: Fix power sequence

[Lad Prabhakar]

When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
some issues with rcar mipi-csi driver. The sensor never entered into LP-11
state.

The powerup sequence in the datasheet[1] shows the sensor entering into
LP-11 in streaming mode, so to fix this issue transitions are performed
from "streaming -> standby" in the probe() after power up.

With this commit the sensor is able to enter LP-11 mode during power up,
as expected by some CSI-2 controllers.

[1] https://publiclab.org/system/images/photos/000/023/294/original/
RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f1effb5a5f66..16010ca1781a 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1224,6 +1224,23 @@ static int imx219_probe(struct i2c_client *client)
 	/* Set default mode to max resolution */
 	imx219->mode = &supported_modes[0];
 
+	/* sensor doesn't enter LP-11 state upon power up until and unless
+	 * streaming is started, so upon power up switch the modes to:
+	 * streaming -> standby
+	 */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
+	if (ret < 0)
+		goto error_power_off;
+	usleep_range(100, 110);
+
+	/* put sensor back to standby mode */
+	ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
+			       IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
+	if (ret < 0)
+		goto error_power_off;
+	usleep_range(100, 110);
+
 	ret = imx219_init_controls(imx219);
 	if (ret)
 		goto error_power_off;
-- 
2.20.1

Re: [PATCH v2 1/3] media: i2c: imx219: Fix power sequence

[Dave Stevenson]

Hi Prabhakar.

Thanks for the update.

On Fri, 6 Mar 2020 at 10:32, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
> some issues with rcar mipi-csi driver. The sensor never entered into LP-11
> state.
>
> The powerup sequence in the datasheet[1] shows the sensor entering into
> LP-11 in streaming mode, so to fix this issue transitions are performed
> from "streaming -> standby" in the probe() after power up.
>
> With this commit the sensor is able to enter LP-11 mode during power up,
> as expected by some CSI-2 controllers.
>
> [1] https://publiclab.org/system/images/photos/000/023/294/original/
> RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>


> ---
>  drivers/media/i2c/imx219.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f1effb5a5f66..16010ca1781a 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1224,6 +1224,23 @@ static int imx219_probe(struct i2c_client *client)
>         /* Set default mode to max resolution */
>         imx219->mode = &supported_modes[0];
>
> +       /* sensor doesn't enter LP-11 state upon power up until and unless
> +        * streaming is started, so upon power up switch the modes to:
> +        * streaming -> standby
> +        */
> +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
> +       if (ret < 0)
> +               goto error_power_off;
> +       usleep_range(100, 110);
> +
> +       /* put sensor back to standby mode */
> +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
> +       if (ret < 0)
> +               goto error_power_off;
> +       usleep_range(100, 110);
> +
>         ret = imx219_init_controls(imx219);
>         if (ret)
>                 goto error_power_off;
> --
> 2.20.1
>

Re: [PATCH v2 1/3] media: i2c: imx219: Fix power sequence

[Lad, Prabhakar]

Hi Dave,

On Fri, Mar 6, 2020 at 1:01 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Prabhakar.
>
> Thanks for the update.
>
> On Fri, 6 Mar 2020 at 10:32, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > When supporting Rpi Camera v2 Module on the RZ/G2E, found the driver had
> > some issues with rcar mipi-csi driver. The sensor never entered into LP-11
> > state.
> >
> > The powerup sequence in the datasheet[1] shows the sensor entering into
> > LP-11 in streaming mode, so to fix this issue transitions are performed
> > from "streaming -> standby" in the probe() after power up.
> >
> > With this commit the sensor is able to enter LP-11 mode during power up,
> > as expected by some CSI-2 controllers.
> >
> > [1] https://publiclab.org/system/images/photos/000/023/294/original/
> > RASPBERRY_PI_CAMERA_V2_DATASHEET_IMX219PQH5_7.0.0_Datasheet_XXX.PDF
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
Thank you for the Ack.

Cheers,
--Prabhakar

>
> > ---
> >  drivers/media/i2c/imx219.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index f1effb5a5f66..16010ca1781a 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -1224,6 +1224,23 @@ static int imx219_probe(struct i2c_client *client)
> >         /* Set default mode to max resolution */
> >         imx219->mode = &supported_modes[0];
> >
> > +       /* sensor doesn't enter LP-11 state upon power up until and unless
> > +        * streaming is started, so upon power up switch the modes to:
> > +        * streaming -> standby
> > +        */
> > +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> > +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STREAMING);
> > +       if (ret < 0)
> > +               goto error_power_off;
> > +       usleep_range(100, 110);
> > +
> > +       /* put sensor back to standby mode */
> > +       ret = imx219_write_reg(imx219, IMX219_REG_MODE_SELECT,
> > +                              IMX219_REG_VALUE_08BIT, IMX219_MODE_STANDBY);
> > +       if (ret < 0)
> > +               goto error_power_off;
> > +       usleep_range(100, 110);
> > +
> >         ret = imx219_init_controls(imx219);
> >         if (ret)
> >                 goto error_power_off;
> > --
> > 2.20.1
> >

[PATCH v2 2/3] media: i2c: imx219: Add support for RAW8 bit bayer format

[Lad Prabhakar]

IMX219 sensor is capable for RAW8/RAW10 modes. This commit adds support
for RAW8 bayer format.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx219.c | 161 +++++++++++++++++++++++++++++--------
 1 file changed, 127 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 16010ca1781a..f96f3ce9fd85 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -90,6 +90,11 @@
 
 #define IMX219_REG_ORIENTATION		0x0172
 
+#define IMX219_CSI_DATA_FORMAT_A_0_7	0x018c
+#define IMX219_CSI_DATA_FORMAT_A_8_15	0x018d
+
+#define IMX219_OPPXCK_DIV		0x0309
+
 /* Test Pattern Control */
 #define IMX219_REG_TEST_PATTERN		0x0600
 #define IMX219_TEST_PATTERN_DISABLE	0
@@ -168,15 +173,12 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x00},
 	{0x0175, 0x00},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
 	{0x0305, 0x03},
 	{0x0306, 0x00},
 	{0x0307, 0x39},
-	{0x0309, 0x0a},
 	{0x030b, 0x01},
 	{0x030c, 0x00},
 	{0x030d, 0x72},
@@ -230,15 +232,12 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x00},
 	{0x0175, 0x00},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
 	{0x0305, 0x03},
 	{0x0306, 0x00},
 	{0x0307, 0x39},
-	{0x0309, 0x0a},
 	{0x030b, 0x01},
 	{0x030c, 0x00},
 	{0x030d, 0x72},
@@ -290,15 +289,12 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
 	{0x0171, 0x01},
 	{0x0174, 0x01},
 	{0x0175, 0x01},
-	{0x018c, 0x0a},
-	{0x018d, 0x0a},
 	{0x0301, 0x05},
 	{0x0303, 0x01},
 	{0x0304, 0x03},
 	{0x0305, 0x03},
 	{0x0306, 0x00},
 	{0x0307, 0x39},
-	{0x0309, 0x0a},
 	{0x030b, 0x01},
 	{0x030c, 0x00},
 	{0x030d, 0x72},
@@ -348,6 +344,27 @@ static const char * const imx219_supply_name[] = {
 
 #define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
 
+/*
+ * The supported formats.
+ * This table MUST contain 4 entries per format, to cover the various flip
+ * combinations in the order
+ * - no flip
+ * - h flip
+ * - v flip
+ * - h&v flips
+ */
+static const u32 codes[] = {
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGBRG10_1X10,
+	MEDIA_BUS_FMT_SBGGR10_1X10,
+
+	MEDIA_BUS_FMT_SRGGB8_1X8,
+	MEDIA_BUS_FMT_SGRBG8_1X8,
+	MEDIA_BUS_FMT_SGBRG8_1X8,
+	MEDIA_BUS_FMT_SBGGR8_1X8,
+};
+
 /*
  * Initialisation delay between XCLR low->high and the moment when the sensor
  * can start capture (i.e. can leave software stanby) must be not less than:
@@ -413,6 +430,8 @@ struct imx219 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
+	struct v4l2_mbus_framefmt fmt;
+
 	struct clk *xclk; /* system clock to IMX219 */
 	u32 xclk_freq;
 
@@ -519,34 +538,57 @@ static int imx219_write_regs(struct imx219 *imx219,
 }
 
 /* Get bayer order based on flip setting. */
-static u32 imx219_get_format_code(struct imx219 *imx219)
+static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 {
-	/*
-	 * Only one bayer order is supported.
-	 * It depends on the flip settings.
-	 */
-	static const u32 codes[2][2] = {
-		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
-		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
-	};
+	int i;
 
 	lockdep_assert_held(&imx219->mutex);
-	return codes[imx219->vflip->val][imx219->hflip->val];
+
+	for (i = 0; i < ARRAY_SIZE(codes); i++)
+		if (codes[i] == code)
+			break;
+
+	if (i >= ARRAY_SIZE(codes))
+		i = 0;
+
+	i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
+	    (imx219->hflip->val ? 1 : 0);
+
+	return codes[i];
+}
+
+static void imx219_set_default_format(struct imx219 *imx219)
+{
+	struct v4l2_mbus_framefmt *fmt;
+
+	mutex_lock(&imx219->mutex);
+
+	fmt = &imx219->fmt;
+	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  fmt->colorspace,
+							  fmt->ycbcr_enc);
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+	fmt->width = supported_modes[0].width;
+	fmt->height = supported_modes[0].height;
+	fmt->field = V4L2_FIELD_NONE;
+
+	mutex_unlock(&imx219->mutex);
 }
 
 static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_mbus_framefmt *cur_fmt = &imx219->fmt;
 	struct v4l2_mbus_framefmt *try_fmt =
 		v4l2_subdev_get_try_format(sd, fh->pad, 0);
 
 	mutex_lock(&imx219->mutex);
 
 	/* Initialize try_fmt */
-	try_fmt->width = supported_modes[0].width;
-	try_fmt->height = supported_modes[0].height;
-	try_fmt->code = imx219_get_format_code(imx219);
-	try_fmt->field = V4L2_FIELD_NONE;
+	try_fmt = cur_fmt;
 
 	mutex_unlock(&imx219->mutex);
 
@@ -648,14 +690,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	struct imx219 *imx219 = to_imx219(sd);
 
-	/*
-	 * Only one bayer order is supported (though it depends on the flip
-	 * settings)
-	 */
-	if (code->index > 0)
+	if (code->pad != 0)
+		return -EINVAL;
+	if (code->index >= (ARRAY_SIZE(codes) / 4))
 		return -EINVAL;
 
-	code->code = imx219_get_format_code(imx219);
+	code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
 
 	return 0;
 }
@@ -669,7 +709,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fse->code != imx219_get_format_code(imx219))
+	if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
 		return -EINVAL;
 
 	fse->min_width = supported_modes[fse->index].width;
@@ -696,7 +736,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = imx219_get_format_code(imx219);
 	fmt->format.field = V4L2_FIELD_NONE;
 
 	imx219_reset_colorspace(&fmt->format);
@@ -710,10 +749,12 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
 		struct v4l2_mbus_framefmt *try_fmt =
 			v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
 		/* update the code which could change due to vflip or hflip: */
-		try_fmt->code = imx219_get_format_code(imx219);
+		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
 		fmt->format = *try_fmt;
 	} else {
 		imx219_update_pad_format(imx219, imx219->mode, fmt);
+		fmt->format.code = imx219_get_format_code(imx219,
+							  imx219->fmt.code);
 	}
 
 	return 0;
@@ -741,11 +782,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx219_mode *mode;
 	struct v4l2_mbus_framefmt *framefmt;
 	int exposure_max, exposure_def, hblank;
+	int i;
 
 	mutex_lock(&imx219->mutex);
 
+	for (i = 0; i < ARRAY_SIZE(codes); i++)
+		if (codes[i] == fmt->format.code)
+			break;
+	if (i >= ARRAY_SIZE(codes))
+		i = 0;
+
 	/* Bayer order varies with flips */
-	fmt->format.code = imx219_get_format_code(imx219);
+	fmt->format.code = imx219_get_format_code(imx219, codes[i]);
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
@@ -755,7 +803,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
 		*framefmt = fmt->format;
-	} else if (imx219->mode != mode) {
+	} else if (imx219->mode != mode ||
+		   imx219->fmt.code != fmt->format.code) {
+		imx219->fmt = fmt->format;
 		imx219->mode = mode;
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -786,6 +836,40 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx219_set_framefmt(struct imx219 *imx219)
+{
+	int ret;
+
+	switch (imx219->fmt.code) {
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
+				       IMX219_REG_VALUE_08BIT, 0x08);
+		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
+				       IMX219_REG_VALUE_08BIT, 0x08);
+		ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
+					IMX219_REG_VALUE_08BIT, 0x08);
+		break;
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
+				       IMX219_REG_VALUE_08BIT, 0x0a);
+		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
+				       IMX219_REG_VALUE_08BIT, 0x0a);
+		ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
+					IMX219_REG_VALUE_08BIT, 0x0a);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int imx219_start_streaming(struct imx219 *imx219)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
@@ -800,6 +884,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
 		return ret;
 	}
 
+	ret = imx219_set_framefmt(imx219);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to set format\n", __func__);
+		return ret;
+	}
+
 	/* Apply customized values from user */
 	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
 	if (ret)
@@ -1253,6 +1343,9 @@ static int imx219_probe(struct i2c_client *client)
 	/* Initialize source pad */
 	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
 
+	/* Initialize default format */
+	imx219_set_default_format(imx219);
+
 	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
 	if (ret) {
 		dev_err(dev, "failed to init entity pads: %d\n", ret);
-- 
2.20.1

Re: [PATCH v2 2/3] media: i2c: imx219: Add support for RAW8 bit bayer format

[Dave Stevenson]

Hi Prabhakar

On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> IMX219 sensor is capable for RAW8/RAW10 modes. This commit adds support
> for RAW8 bayer format.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I assume this is merging in the bits from my hacked together patch,
hence copying the Signed-off-by.

I've run through a set of basic tests streaming in each of the
resolutions, both 8 & 10 bit modes, and with combinations of H&V
flips. They all seem to work as expected, so I'm happy.

The frame rate is preserved between the modes so there is the one
question of link frequency in 8 bit modes presumably being lower than
specified in DT.
On the last review thread you had said you'd check the frequencies -
did you manage that (I know it's not trivial), and did that confirm
that it had/had not changed?
It's not a big deal to me, but others may care more.

Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/imx219.c | 161 +++++++++++++++++++++++++++++--------
>  1 file changed, 127 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 16010ca1781a..f96f3ce9fd85 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -90,6 +90,11 @@
>
>  #define IMX219_REG_ORIENTATION         0x0172
>
> +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> +
> +#define IMX219_OPPXCK_DIV              0x0309
> +
>  /* Test Pattern Control */
>  #define IMX219_REG_TEST_PATTERN                0x0600
>  #define IMX219_TEST_PATTERN_DISABLE    0
> @@ -168,15 +173,12 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x00},
>         {0x0175, 0x00},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
>         {0x0305, 0x03},
>         {0x0306, 0x00},
>         {0x0307, 0x39},
> -       {0x0309, 0x0a},
>         {0x030b, 0x01},
>         {0x030c, 0x00},
>         {0x030d, 0x72},
> @@ -230,15 +232,12 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x00},
>         {0x0175, 0x00},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
>         {0x0305, 0x03},
>         {0x0306, 0x00},
>         {0x0307, 0x39},
> -       {0x0309, 0x0a},
>         {0x030b, 0x01},
>         {0x030c, 0x00},
>         {0x030d, 0x72},
> @@ -290,15 +289,12 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>         {0x0171, 0x01},
>         {0x0174, 0x01},
>         {0x0175, 0x01},
> -       {0x018c, 0x0a},
> -       {0x018d, 0x0a},
>         {0x0301, 0x05},
>         {0x0303, 0x01},
>         {0x0304, 0x03},
>         {0x0305, 0x03},
>         {0x0306, 0x00},
>         {0x0307, 0x39},
> -       {0x0309, 0x0a},
>         {0x030b, 0x01},
>         {0x030c, 0x00},
>         {0x030d, 0x72},
> @@ -348,6 +344,27 @@ static const char * const imx219_supply_name[] = {
>
>  #define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
>
> +/*
> + * The supported formats.
> + * This table MUST contain 4 entries per format, to cover the various flip
> + * combinations in the order
> + * - no flip
> + * - h flip
> + * - v flip
> + * - h&v flips
> + */
> +static const u32 codes[] = {
> +       MEDIA_BUS_FMT_SRGGB10_1X10,
> +       MEDIA_BUS_FMT_SGRBG10_1X10,
> +       MEDIA_BUS_FMT_SGBRG10_1X10,
> +       MEDIA_BUS_FMT_SBGGR10_1X10,
> +
> +       MEDIA_BUS_FMT_SRGGB8_1X8,
> +       MEDIA_BUS_FMT_SGRBG8_1X8,
> +       MEDIA_BUS_FMT_SGBRG8_1X8,
> +       MEDIA_BUS_FMT_SBGGR8_1X8,
> +};
> +
>  /*
>   * Initialisation delay between XCLR low->high and the moment when the sensor
>   * can start capture (i.e. can leave software stanby) must be not less than:
> @@ -413,6 +430,8 @@ struct imx219 {
>         struct v4l2_subdev sd;
>         struct media_pad pad;
>
> +       struct v4l2_mbus_framefmt fmt;
> +
>         struct clk *xclk; /* system clock to IMX219 */
>         u32 xclk_freq;
>
> @@ -519,34 +538,57 @@ static int imx219_write_regs(struct imx219 *imx219,
>  }
>
>  /* Get bayer order based on flip setting. */
> -static u32 imx219_get_format_code(struct imx219 *imx219)
> +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>  {
> -       /*
> -        * Only one bayer order is supported.
> -        * It depends on the flip settings.
> -        */
> -       static const u32 codes[2][2] = {
> -               { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> -               { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> -       };
> +       int i;
>
>         lockdep_assert_held(&imx219->mutex);
> -       return codes[imx219->vflip->val][imx219->hflip->val];
> +
> +       for (i = 0; i < ARRAY_SIZE(codes); i++)
> +               if (codes[i] == code)
> +                       break;
> +
> +       if (i >= ARRAY_SIZE(codes))
> +               i = 0;
> +
> +       i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> +           (imx219->hflip->val ? 1 : 0);
> +
> +       return codes[i];
> +}
> +
> +static void imx219_set_default_format(struct imx219 *imx219)
> +{
> +       struct v4l2_mbus_framefmt *fmt;
> +
> +       mutex_lock(&imx219->mutex);
> +
> +       fmt = &imx219->fmt;
> +       fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +                                                         fmt->colorspace,
> +                                                         fmt->ycbcr_enc);
> +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +       fmt->width = supported_modes[0].width;
> +       fmt->height = supported_modes[0].height;
> +       fmt->field = V4L2_FIELD_NONE;
> +
> +       mutex_unlock(&imx219->mutex);
>  }
>
>  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>         struct imx219 *imx219 = to_imx219(sd);
> +       struct v4l2_mbus_framefmt *cur_fmt = &imx219->fmt;
>         struct v4l2_mbus_framefmt *try_fmt =
>                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
>
>         mutex_lock(&imx219->mutex);
>
>         /* Initialize try_fmt */
> -       try_fmt->width = supported_modes[0].width;
> -       try_fmt->height = supported_modes[0].height;
> -       try_fmt->code = imx219_get_format_code(imx219);
> -       try_fmt->field = V4L2_FIELD_NONE;
> +       try_fmt = cur_fmt;
>
>         mutex_unlock(&imx219->mutex);
>
> @@ -648,14 +690,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>  {
>         struct imx219 *imx219 = to_imx219(sd);
>
> -       /*
> -        * Only one bayer order is supported (though it depends on the flip
> -        * settings)
> -        */
> -       if (code->index > 0)
> +       if (code->pad != 0)
> +               return -EINVAL;
> +       if (code->index >= (ARRAY_SIZE(codes) / 4))
>                 return -EINVAL;
>
> -       code->code = imx219_get_format_code(imx219);
> +       code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
>
>         return 0;
>  }
> @@ -669,7 +709,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>         if (fse->index >= ARRAY_SIZE(supported_modes))
>                 return -EINVAL;
>
> -       if (fse->code != imx219_get_format_code(imx219))
> +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
>                 return -EINVAL;
>
>         fse->min_width = supported_modes[fse->index].width;
> @@ -696,7 +736,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>  {
>         fmt->format.width = mode->width;
>         fmt->format.height = mode->height;
> -       fmt->format.code = imx219_get_format_code(imx219);
>         fmt->format.field = V4L2_FIELD_NONE;
>
>         imx219_reset_colorspace(&fmt->format);
> @@ -710,10 +749,12 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
>                 struct v4l2_mbus_framefmt *try_fmt =
>                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
>                 /* update the code which could change due to vflip or hflip: */
> -               try_fmt->code = imx219_get_format_code(imx219);
> +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
>                 fmt->format = *try_fmt;
>         } else {
>                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> +               fmt->format.code = imx219_get_format_code(imx219,
> +                                                         imx219->fmt.code);
>         }
>
>         return 0;
> @@ -741,11 +782,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         const struct imx219_mode *mode;
>         struct v4l2_mbus_framefmt *framefmt;
>         int exposure_max, exposure_def, hblank;
> +       int i;
>
>         mutex_lock(&imx219->mutex);
>
> +       for (i = 0; i < ARRAY_SIZE(codes); i++)
> +               if (codes[i] == fmt->format.code)
> +                       break;
> +       if (i >= ARRAY_SIZE(codes))
> +               i = 0;
> +
>         /* Bayer order varies with flips */
> -       fmt->format.code = imx219_get_format_code(imx219);
> +       fmt->format.code = imx219_get_format_code(imx219, codes[i]);
>
>         mode = v4l2_find_nearest_size(supported_modes,
>                                       ARRAY_SIZE(supported_modes),
> @@ -755,7 +803,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
>                 *framefmt = fmt->format;
> -       } else if (imx219->mode != mode) {
> +       } else if (imx219->mode != mode ||
> +                  imx219->fmt.code != fmt->format.code) {
> +               imx219->fmt = fmt->format;
>                 imx219->mode = mode;
>                 /* Update limits and set FPS to default */
>                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -786,6 +836,40 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         return 0;
>  }
>
> +static int imx219_set_framefmt(struct imx219 *imx219)
> +{
> +       int ret;
> +
> +       switch (imx219->fmt.code) {
> +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +                                      IMX219_REG_VALUE_08BIT, 0x08);
> +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +                                      IMX219_REG_VALUE_08BIT, 0x08);
> +               ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> +                                       IMX219_REG_VALUE_08BIT, 0x08);
> +               break;
> +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> +               ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> +                                       IMX219_REG_VALUE_08BIT, 0x0a);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +       }
> +
> +       return ret;
> +}
> +
>  static int imx219_start_streaming(struct imx219 *imx219)
>  {
>         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -800,6 +884,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
>                 return ret;
>         }
>
> +       ret = imx219_set_framefmt(imx219);
> +       if (ret) {
> +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> +               return ret;
> +       }
> +
>         /* Apply customized values from user */
>         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
>         if (ret)
> @@ -1253,6 +1343,9 @@ static int imx219_probe(struct i2c_client *client)
>         /* Initialize source pad */
>         imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>
> +       /* Initialize default format */
> +       imx219_set_default_format(imx219);
> +
>         ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>         if (ret) {
>                 dev_err(dev, "failed to init entity pads: %d\n", ret);
> --
> 2.20.1
>

Re: [PATCH v2 2/3] media: i2c: imx219: Add support for RAW8 bit bayer format

[Lad, Prabhakar]

Hi Dave,

Thank you for the review.

On Fri, Mar 6, 2020 at 2:04 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Prabhakar
>
> On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > IMX219 sensor is capable for RAW8/RAW10 modes. This commit adds support
> > for RAW8 bayer format.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> I assume this is merging in the bits from my hacked together patch,
> hence copying the Signed-off-by.
>
Yes.

> I've run through a set of basic tests streaming in each of the
> resolutions, both 8 & 10 bit modes, and with combinations of H&V
> flips. They all seem to work as expected, so I'm happy.
>
> The frame rate is preserved between the modes so there is the one
> question of link frequency in 8 bit modes presumably being lower than
> specified in DT.
> On the last review thread you had said you'd check the frequencies -
> did you manage that (I know it's not trivial), and did that confirm
> that it had/had not changed?
> It's not a big deal to me, but others may care more.
>
No I didnt have tools to test the link frequencies :(

> Acked-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
Thank you for the ack.

Cheers,
--Prabhakar

> > ---
> >  drivers/media/i2c/imx219.c | 161 +++++++++++++++++++++++++++++--------
> >  1 file changed, 127 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 16010ca1781a..f96f3ce9fd85 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -90,6 +90,11 @@
> >
> >  #define IMX219_REG_ORIENTATION         0x0172
> >
> > +#define IMX219_CSI_DATA_FORMAT_A_0_7   0x018c
> > +#define IMX219_CSI_DATA_FORMAT_A_8_15  0x018d
> > +
> > +#define IMX219_OPPXCK_DIV              0x0309
> > +
> >  /* Test Pattern Control */
> >  #define IMX219_REG_TEST_PATTERN                0x0600
> >  #define IMX219_TEST_PATTERN_DISABLE    0
> > @@ -168,15 +173,12 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> >         {0x0305, 0x03},
> >         {0x0306, 0x00},
> >         {0x0307, 0x39},
> > -       {0x0309, 0x0a},
> >         {0x030b, 0x01},
> >         {0x030c, 0x00},
> >         {0x030d, 0x72},
> > @@ -230,15 +232,12 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x00},
> >         {0x0175, 0x00},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> >         {0x0305, 0x03},
> >         {0x0306, 0x00},
> >         {0x0307, 0x39},
> > -       {0x0309, 0x0a},
> >         {0x030b, 0x01},
> >         {0x030c, 0x00},
> >         {0x030d, 0x72},
> > @@ -290,15 +289,12 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0171, 0x01},
> >         {0x0174, 0x01},
> >         {0x0175, 0x01},
> > -       {0x018c, 0x0a},
> > -       {0x018d, 0x0a},
> >         {0x0301, 0x05},
> >         {0x0303, 0x01},
> >         {0x0304, 0x03},
> >         {0x0305, 0x03},
> >         {0x0306, 0x00},
> >         {0x0307, 0x39},
> > -       {0x0309, 0x0a},
> >         {0x030b, 0x01},
> >         {0x030c, 0x00},
> >         {0x030d, 0x72},
> > @@ -348,6 +344,27 @@ static const char * const imx219_supply_name[] = {
> >
> >  #define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
> >
> > +/*
> > + * The supported formats.
> > + * This table MUST contain 4 entries per format, to cover the various flip
> > + * combinations in the order
> > + * - no flip
> > + * - h flip
> > + * - v flip
> > + * - h&v flips
> > + */
> > +static const u32 codes[] = {
> > +       MEDIA_BUS_FMT_SRGGB10_1X10,
> > +       MEDIA_BUS_FMT_SGRBG10_1X10,
> > +       MEDIA_BUS_FMT_SGBRG10_1X10,
> > +       MEDIA_BUS_FMT_SBGGR10_1X10,
> > +
> > +       MEDIA_BUS_FMT_SRGGB8_1X8,
> > +       MEDIA_BUS_FMT_SGRBG8_1X8,
> > +       MEDIA_BUS_FMT_SGBRG8_1X8,
> > +       MEDIA_BUS_FMT_SBGGR8_1X8,
> > +};
> > +
> >  /*
> >   * Initialisation delay between XCLR low->high and the moment when the sensor
> >   * can start capture (i.e. can leave software stanby) must be not less than:
> > @@ -413,6 +430,8 @@ struct imx219 {
> >         struct v4l2_subdev sd;
> >         struct media_pad pad;
> >
> > +       struct v4l2_mbus_framefmt fmt;
> > +
> >         struct clk *xclk; /* system clock to IMX219 */
> >         u32 xclk_freq;
> >
> > @@ -519,34 +538,57 @@ static int imx219_write_regs(struct imx219 *imx219,
> >  }
> >
> >  /* Get bayer order based on flip setting. */
> > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >  {
> > -       /*
> > -        * Only one bayer order is supported.
> > -        * It depends on the flip settings.
> > -        */
> > -       static const u32 codes[2][2] = {
> > -               { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > -               { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > -       };
> > +       int i;
> >
> >         lockdep_assert_held(&imx219->mutex);
> > -       return codes[imx219->vflip->val][imx219->hflip->val];
> > +
> > +       for (i = 0; i < ARRAY_SIZE(codes); i++)
> > +               if (codes[i] == code)
> > +                       break;
> > +
> > +       if (i >= ARRAY_SIZE(codes))
> > +               i = 0;
> > +
> > +       i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> > +           (imx219->hflip->val ? 1 : 0);
> > +
> > +       return codes[i];
> > +}
> > +
> > +static void imx219_set_default_format(struct imx219 *imx219)
> > +{
> > +       struct v4l2_mbus_framefmt *fmt;
> > +
> > +       mutex_lock(&imx219->mutex);
> > +
> > +       fmt = &imx219->fmt;
> > +       fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > +                                                         fmt->colorspace,
> > +                                                         fmt->ycbcr_enc);
> > +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +       fmt->width = supported_modes[0].width;
> > +       fmt->height = supported_modes[0].height;
> > +       fmt->field = V4L2_FIELD_NONE;
> > +
> > +       mutex_unlock(&imx219->mutex);
> >  }
> >
> >  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >  {
> >         struct imx219 *imx219 = to_imx219(sd);
> > +       struct v4l2_mbus_framefmt *cur_fmt = &imx219->fmt;
> >         struct v4l2_mbus_framefmt *try_fmt =
> >                 v4l2_subdev_get_try_format(sd, fh->pad, 0);
> >
> >         mutex_lock(&imx219->mutex);
> >
> >         /* Initialize try_fmt */
> > -       try_fmt->width = supported_modes[0].width;
> > -       try_fmt->height = supported_modes[0].height;
> > -       try_fmt->code = imx219_get_format_code(imx219);
> > -       try_fmt->field = V4L2_FIELD_NONE;
> > +       try_fmt = cur_fmt;
> >
> >         mutex_unlock(&imx219->mutex);
> >
> > @@ -648,14 +690,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >  {
> >         struct imx219 *imx219 = to_imx219(sd);
> >
> > -       /*
> > -        * Only one bayer order is supported (though it depends on the flip
> > -        * settings)
> > -        */
> > -       if (code->index > 0)
> > +       if (code->pad != 0)
> > +               return -EINVAL;
> > +       if (code->index >= (ARRAY_SIZE(codes) / 4))
> >                 return -EINVAL;
> >
> > -       code->code = imx219_get_format_code(imx219);
> > +       code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
> >
> >         return 0;
> >  }
> > @@ -669,7 +709,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >         if (fse->index >= ARRAY_SIZE(supported_modes))
> >                 return -EINVAL;
> >
> > -       if (fse->code != imx219_get_format_code(imx219))
> > +       if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> >                 return -EINVAL;
> >
> >         fse->min_width = supported_modes[fse->index].width;
> > @@ -696,7 +736,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >  {
> >         fmt->format.width = mode->width;
> >         fmt->format.height = mode->height;
> > -       fmt->format.code = imx219_get_format_code(imx219);
> >         fmt->format.field = V4L2_FIELD_NONE;
> >
> >         imx219_reset_colorspace(&fmt->format);
> > @@ -710,10 +749,12 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> >                 struct v4l2_mbus_framefmt *try_fmt =
> >                         v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> >                 /* update the code which could change due to vflip or hflip: */
> > -               try_fmt->code = imx219_get_format_code(imx219);
> > +               try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> >                 fmt->format = *try_fmt;
> >         } else {
> >                 imx219_update_pad_format(imx219, imx219->mode, fmt);
> > +               fmt->format.code = imx219_get_format_code(imx219,
> > +                                                         imx219->fmt.code);
> >         }
> >
> >         return 0;
> > @@ -741,11 +782,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         const struct imx219_mode *mode;
> >         struct v4l2_mbus_framefmt *framefmt;
> >         int exposure_max, exposure_def, hblank;
> > +       int i;
> >
> >         mutex_lock(&imx219->mutex);
> >
> > +       for (i = 0; i < ARRAY_SIZE(codes); i++)
> > +               if (codes[i] == fmt->format.code)
> > +                       break;
> > +       if (i >= ARRAY_SIZE(codes))
> > +               i = 0;
> > +
> >         /* Bayer order varies with flips */
> > -       fmt->format.code = imx219_get_format_code(imx219);
> > +       fmt->format.code = imx219_get_format_code(imx219, codes[i]);
> >
> >         mode = v4l2_find_nearest_size(supported_modes,
> >                                       ARRAY_SIZE(supported_modes),
> > @@ -755,7 +803,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >                 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >                 *framefmt = fmt->format;
> > -       } else if (imx219->mode != mode) {
> > +       } else if (imx219->mode != mode ||
> > +                  imx219->fmt.code != fmt->format.code) {
> > +               imx219->fmt = fmt->format;
> >                 imx219->mode = mode;
> >                 /* Update limits and set FPS to default */
> >                 __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -786,6 +836,40 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         return 0;
> >  }
> >
> > +static int imx219_set_framefmt(struct imx219 *imx219)
> > +{
> > +       int ret;
> > +
> > +       switch (imx219->fmt.code) {
> > +       case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +       case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +       case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +       case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x08);
> > +               ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> > +                                       IMX219_REG_VALUE_08BIT, 0x08);
> > +               break;
> > +       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +       case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +       case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +       case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +               ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                      IMX219_REG_VALUE_08BIT, 0x0a);
> > +               ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> > +                                       IMX219_REG_VALUE_08BIT, 0x0a);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  static int imx219_start_streaming(struct imx219 *imx219)
> >  {
> >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -800,6 +884,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >                 return ret;
> >         }
> >
> > +       ret = imx219_set_framefmt(imx219);
> > +       if (ret) {
> > +               dev_err(&client->dev, "%s failed to set format\n", __func__);
> > +               return ret;
> > +       }
> > +
> >         /* Apply customized values from user */
> >         ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> >         if (ret)
> > @@ -1253,6 +1343,9 @@ static int imx219_probe(struct i2c_client *client)
> >         /* Initialize source pad */
> >         imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> >
> > +       /* Initialize default format */
> > +       imx219_set_default_format(imx219);
> > +
> >         ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> >         if (ret) {
> >                 dev_err(dev, "failed to init entity pads: %d\n", ret);
> > --
> > 2.20.1
> >

Re: [PATCH v2 2/3] media: i2c: imx219: Add support for RAW8 bit bayer format

[Sakari Ailus]

Hi Prabhakar,

On Fri, Mar 06, 2020 at 10:32:45AM +0000, Lad Prabhakar wrote:
> IMX219 sensor is capable for RAW8/RAW10 modes. This commit adds support
> for RAW8 bayer format.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx219.c | 161 +++++++++++++++++++++++++++++--------
>  1 file changed, 127 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 16010ca1781a..f96f3ce9fd85 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -90,6 +90,11 @@
>  
>  #define IMX219_REG_ORIENTATION		0x0172
>  
> +#define IMX219_CSI_DATA_FORMAT_A_0_7	0x018c
> +#define IMX219_CSI_DATA_FORMAT_A_8_15	0x018d
> +
> +#define IMX219_OPPXCK_DIV		0x0309
> +
>  /* Test Pattern Control */
>  #define IMX219_REG_TEST_PATTERN		0x0600
>  #define IMX219_TEST_PATTERN_DISABLE	0
> @@ -168,15 +173,12 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
>  	{0x0171, 0x01},
>  	{0x0174, 0x00},
>  	{0x0175, 0x00},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>  	{0x0301, 0x05},
>  	{0x0303, 0x01},
>  	{0x0304, 0x03},
>  	{0x0305, 0x03},
>  	{0x0306, 0x00},
>  	{0x0307, 0x39},
> -	{0x0309, 0x0a},
>  	{0x030b, 0x01},
>  	{0x030c, 0x00},
>  	{0x030d, 0x72},
> @@ -230,15 +232,12 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
>  	{0x0171, 0x01},
>  	{0x0174, 0x00},
>  	{0x0175, 0x00},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>  	{0x0301, 0x05},
>  	{0x0303, 0x01},
>  	{0x0304, 0x03},
>  	{0x0305, 0x03},
>  	{0x0306, 0x00},
>  	{0x0307, 0x39},
> -	{0x0309, 0x0a},
>  	{0x030b, 0x01},
>  	{0x030c, 0x00},
>  	{0x030d, 0x72},
> @@ -290,15 +289,12 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>  	{0x0171, 0x01},
>  	{0x0174, 0x01},
>  	{0x0175, 0x01},
> -	{0x018c, 0x0a},
> -	{0x018d, 0x0a},
>  	{0x0301, 0x05},
>  	{0x0303, 0x01},
>  	{0x0304, 0x03},
>  	{0x0305, 0x03},
>  	{0x0306, 0x00},
>  	{0x0307, 0x39},
> -	{0x0309, 0x0a},
>  	{0x030b, 0x01},
>  	{0x030c, 0x00},
>  	{0x030d, 0x72},
> @@ -348,6 +344,27 @@ static const char * const imx219_supply_name[] = {
>  
>  #define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
>  
> +/*
> + * The supported formats.
> + * This table MUST contain 4 entries per format, to cover the various flip
> + * combinations in the order
> + * - no flip
> + * - h flip
> + * - v flip
> + * - h&v flips
> + */
> +static const u32 codes[] = {
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SBGGR8_1X8,
> +};
> +
>  /*
>   * Initialisation delay between XCLR low->high and the moment when the sensor
>   * can start capture (i.e. can leave software stanby) must be not less than:
> @@ -413,6 +430,8 @@ struct imx219 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> +	struct v4l2_mbus_framefmt fmt;
> +
>  	struct clk *xclk; /* system clock to IMX219 */
>  	u32 xclk_freq;
>  
> @@ -519,34 +538,57 @@ static int imx219_write_regs(struct imx219 *imx219,
>  }
>  
>  /* Get bayer order based on flip setting. */
> -static u32 imx219_get_format_code(struct imx219 *imx219)
> +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>  {
> -	/*
> -	 * Only one bayer order is supported.
> -	 * It depends on the flip settings.
> -	 */
> -	static const u32 codes[2][2] = {
> -		{ MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> -		{ MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> -	};
> +	int i;

unsigned int

>  
>  	lockdep_assert_held(&imx219->mutex);
> -	return codes[imx219->vflip->val][imx219->hflip->val];
> +
> +	for (i = 0; i < ARRAY_SIZE(codes); i++)
> +		if (codes[i] == code)
> +			break;
> +
> +	if (i >= ARRAY_SIZE(codes))
> +		i = 0;
> +
> +	i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> +	    (imx219->hflip->val ? 1 : 0);
> +
> +	return codes[i];
> +}
> +
> +static void imx219_set_default_format(struct imx219 *imx219)
> +{
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	mutex_lock(&imx219->mutex);

No mutex needed; all this information is static, and you're calling the
function from probe before any driver's external APIs are accessible.

> +
> +	fmt = &imx219->fmt;
> +	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> +	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +							  fmt->colorspace,
> +							  fmt->ycbcr_enc);
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->width = supported_modes[0].width;
> +	fmt->height = supported_modes[0].height;
> +	fmt->field = V4L2_FIELD_NONE;
> +
> +	mutex_unlock(&imx219->mutex);
>  }
>  
>  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
>  	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_mbus_framefmt *cur_fmt = &imx219->fmt;
>  	struct v4l2_mbus_framefmt *try_fmt =
>  		v4l2_subdev_get_try_format(sd, fh->pad, 0);
>  
>  	mutex_lock(&imx219->mutex);
>  
>  	/* Initialize try_fmt */
> -	try_fmt->width = supported_modes[0].width;
> -	try_fmt->height = supported_modes[0].height;
> -	try_fmt->code = imx219_get_format_code(imx219);
> -	try_fmt->field = V4L2_FIELD_NONE;
> +	try_fmt = cur_fmt;

The default try format should reflect the device default, not current
format. Although the pixel order needs to be taken into account, as was
already done.

>  
>  	mutex_unlock(&imx219->mutex);
>  
> @@ -648,14 +690,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>  {
>  	struct imx219 *imx219 = to_imx219(sd);
>  
> -	/*
> -	 * Only one bayer order is supported (though it depends on the flip
> -	 * settings)
> -	 */
> -	if (code->index > 0)
> +	if (code->pad != 0)
> +		return -EINVAL;

The pad is already checked by the caller, no need to repeat here.

> +	if (code->index >= (ARRAY_SIZE(codes) / 4))
>  		return -EINVAL;
>  
> -	code->code = imx219_get_format_code(imx219);
> +	code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
>  
>  	return 0;
>  }
> @@ -669,7 +709,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fse->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
>  
> -	if (fse->code != imx219_get_format_code(imx219))
> +	if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
>  		return -EINVAL;
>  
>  	fse->min_width = supported_modes[fse->index].width;
> @@ -696,7 +736,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = imx219_get_format_code(imx219);
>  	fmt->format.field = V4L2_FIELD_NONE;
>  
>  	imx219_reset_colorspace(&fmt->format);
> @@ -710,10 +749,12 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
>  		struct v4l2_mbus_framefmt *try_fmt =
>  			v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
>  		/* update the code which could change due to vflip or hflip: */
> -		try_fmt->code = imx219_get_format_code(imx219);
> +		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
>  		fmt->format = *try_fmt;
>  	} else {
>  		imx219_update_pad_format(imx219, imx219->mode, fmt);
> +		fmt->format.code = imx219_get_format_code(imx219,
> +							  imx219->fmt.code);
>  	}
>  
>  	return 0;
> @@ -741,11 +782,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	const struct imx219_mode *mode;
>  	struct v4l2_mbus_framefmt *framefmt;
>  	int exposure_max, exposure_def, hblank;
> +	int i;

unsigned int

>  
>  	mutex_lock(&imx219->mutex);
>  
> +	for (i = 0; i < ARRAY_SIZE(codes); i++)
> +		if (codes[i] == fmt->format.code)
> +			break;
> +	if (i >= ARRAY_SIZE(codes))
> +		i = 0;
> +
>  	/* Bayer order varies with flips */
> -	fmt->format.code = imx219_get_format_code(imx219);
> +	fmt->format.code = imx219_get_format_code(imx219, codes[i]);
>  
>  	mode = v4l2_find_nearest_size(supported_modes,
>  				      ARRAY_SIZE(supported_modes),
> @@ -755,7 +803,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
>  		*framefmt = fmt->format;
> -	} else if (imx219->mode != mode) {
> +	} else if (imx219->mode != mode ||
> +		   imx219->fmt.code != fmt->format.code) {
> +		imx219->fmt = fmt->format;
>  		imx219->mode = mode;
>  		/* Update limits and set FPS to default */
>  		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -786,6 +836,40 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int imx219_set_framefmt(struct imx219 *imx219)
> +{
> +	int ret;
> +
> +	switch (imx219->fmt.code) {
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +				       IMX219_REG_VALUE_08BIT, 0x08);
> +		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +				       IMX219_REG_VALUE_08BIT, 0x08);
> +		ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> +					IMX219_REG_VALUE_08BIT, 0x08);

What kind of error codes do you expect to get? :-)

Please either return the error immediately, or check ret before the
assignment.

> +		break;
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> +				       IMX219_REG_VALUE_08BIT, 0x0a);
> +		ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> +				       IMX219_REG_VALUE_08BIT, 0x0a);
> +		ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> +					IMX219_REG_VALUE_08BIT, 0x0a);
> +		break;
> +	default:
> +		ret = -EINVAL;

You could return -EINVAL here.

> +	}
> +
> +	return ret;
> +}
> +
>  static int imx219_start_streaming(struct imx219 *imx219)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> @@ -800,6 +884,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
>  		return ret;
>  	}
>  
> +	ret = imx219_set_framefmt(imx219);
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to set format\n", __func__);

The error code could be useful.

> +		return ret;
> +	}
> +
>  	/* Apply customized values from user */
>  	ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
>  	if (ret)
> @@ -1253,6 +1343,9 @@ static int imx219_probe(struct i2c_client *client)
>  	/* Initialize source pad */
>  	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>  
> +	/* Initialize default format */
> +	imx219_set_default_format(imx219);
> +
>  	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>  	if (ret) {
>  		dev_err(dev, "failed to init entity pads: %d\n", ret);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/3] media: i2c: imx219: Add support for RAW8 bit bayer format

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Tue, Mar 10, 2020 at 10:01 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 06, 2020 at 10:32:45AM +0000, Lad Prabhakar wrote:
> > IMX219 sensor is capable for RAW8/RAW10 modes. This commit adds support
> > for RAW8 bayer format.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx219.c | 161 +++++++++++++++++++++++++++++--------
> >  1 file changed, 127 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 16010ca1781a..f96f3ce9fd85 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -90,6 +90,11 @@
> >
> >  #define IMX219_REG_ORIENTATION               0x0172
> >
> > +#define IMX219_CSI_DATA_FORMAT_A_0_7 0x018c
> > +#define IMX219_CSI_DATA_FORMAT_A_8_15        0x018d
> > +
> > +#define IMX219_OPPXCK_DIV            0x0309
> > +
> >  /* Test Pattern Control */
> >  #define IMX219_REG_TEST_PATTERN              0x0600
> >  #define IMX219_TEST_PATTERN_DISABLE  0
> > @@ -168,15 +173,12 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x00},
> >       {0x0175, 0x00},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> >       {0x0305, 0x03},
> >       {0x0306, 0x00},
> >       {0x0307, 0x39},
> > -     {0x0309, 0x0a},
> >       {0x030b, 0x01},
> >       {0x030c, 0x00},
> >       {0x030d, 0x72},
> > @@ -230,15 +232,12 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x00},
> >       {0x0175, 0x00},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> >       {0x0305, 0x03},
> >       {0x0306, 0x00},
> >       {0x0307, 0x39},
> > -     {0x0309, 0x0a},
> >       {0x030b, 0x01},
> >       {0x030c, 0x00},
> >       {0x030d, 0x72},
> > @@ -290,15 +289,12 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >       {0x0171, 0x01},
> >       {0x0174, 0x01},
> >       {0x0175, 0x01},
> > -     {0x018c, 0x0a},
> > -     {0x018d, 0x0a},
> >       {0x0301, 0x05},
> >       {0x0303, 0x01},
> >       {0x0304, 0x03},
> >       {0x0305, 0x03},
> >       {0x0306, 0x00},
> >       {0x0307, 0x39},
> > -     {0x0309, 0x0a},
> >       {0x030b, 0x01},
> >       {0x030c, 0x00},
> >       {0x030d, 0x72},
> > @@ -348,6 +344,27 @@ static const char * const imx219_supply_name[] = {
> >
> >  #define IMX219_NUM_SUPPLIES ARRAY_SIZE(imx219_supply_name)
> >
> > +/*
> > + * The supported formats.
> > + * This table MUST contain 4 entries per format, to cover the various flip
> > + * combinations in the order
> > + * - no flip
> > + * - h flip
> > + * - v flip
> > + * - h&v flips
> > + */
> > +static const u32 codes[] = {
> > +     MEDIA_BUS_FMT_SRGGB10_1X10,
> > +     MEDIA_BUS_FMT_SGRBG10_1X10,
> > +     MEDIA_BUS_FMT_SGBRG10_1X10,
> > +     MEDIA_BUS_FMT_SBGGR10_1X10,
> > +
> > +     MEDIA_BUS_FMT_SRGGB8_1X8,
> > +     MEDIA_BUS_FMT_SGRBG8_1X8,
> > +     MEDIA_BUS_FMT_SGBRG8_1X8,
> > +     MEDIA_BUS_FMT_SBGGR8_1X8,
> > +};
> > +
> >  /*
> >   * Initialisation delay between XCLR low->high and the moment when the sensor
> >   * can start capture (i.e. can leave software stanby) must be not less than:
> > @@ -413,6 +430,8 @@ struct imx219 {
> >       struct v4l2_subdev sd;
> >       struct media_pad pad;
> >
> > +     struct v4l2_mbus_framefmt fmt;
> > +
> >       struct clk *xclk; /* system clock to IMX219 */
> >       u32 xclk_freq;
> >
> > @@ -519,34 +538,57 @@ static int imx219_write_regs(struct imx219 *imx219,
> >  }
> >
> >  /* Get bayer order based on flip setting. */
> > -static u32 imx219_get_format_code(struct imx219 *imx219)
> > +static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >  {
> > -     /*
> > -      * Only one bayer order is supported.
> > -      * It depends on the flip settings.
> > -      */
> > -     static const u32 codes[2][2] = {
> > -             { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > -             { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > -     };
> > +     int i;
>
> unsigned int
>
sure will change it.

> >
> >       lockdep_assert_held(&imx219->mutex);
> > -     return codes[imx219->vflip->val][imx219->hflip->val];
> > +
> > +     for (i = 0; i < ARRAY_SIZE(codes); i++)
> > +             if (codes[i] == code)
> > +                     break;
> > +
> > +     if (i >= ARRAY_SIZE(codes))
> > +             i = 0;
> > +
> > +     i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> > +         (imx219->hflip->val ? 1 : 0);
> > +
> > +     return codes[i];
> > +}
> > +
> > +static void imx219_set_default_format(struct imx219 *imx219)
> > +{
> > +     struct v4l2_mbus_framefmt *fmt;
> > +
> > +     mutex_lock(&imx219->mutex);
>
> No mutex needed; all this information is static, and you're calling the
> function from probe before any driver's external APIs are accessible.
>
Agreed will drop the mutex.

> > +
> > +     fmt = &imx219->fmt;
> > +     fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +     fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +     fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > +     fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > +                                                       fmt->colorspace,
> > +                                                       fmt->ycbcr_enc);
> > +     fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +     fmt->width = supported_modes[0].width;
> > +     fmt->height = supported_modes[0].height;
> > +     fmt->field = V4L2_FIELD_NONE;
> > +
> > +     mutex_unlock(&imx219->mutex);
> >  }
> >
> >  static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >  {
> >       struct imx219 *imx219 = to_imx219(sd);
> > +     struct v4l2_mbus_framefmt *cur_fmt = &imx219->fmt;
> >       struct v4l2_mbus_framefmt *try_fmt =
> >               v4l2_subdev_get_try_format(sd, fh->pad, 0);
> >
> >       mutex_lock(&imx219->mutex);
> >
> >       /* Initialize try_fmt */
> > -     try_fmt->width = supported_modes[0].width;
> > -     try_fmt->height = supported_modes[0].height;
> > -     try_fmt->code = imx219_get_format_code(imx219);
> > -     try_fmt->field = V4L2_FIELD_NONE;
> > +     try_fmt = cur_fmt;
>
> The default try format should reflect the device default, not current
> format. Although the pixel order needs to be taken into account, as was
> already done.
>
My bad, shall replace that with default format.

> >
> >       mutex_unlock(&imx219->mutex);
> >
> > @@ -648,14 +690,12 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >  {
> >       struct imx219 *imx219 = to_imx219(sd);
> >
> > -     /*
> > -      * Only one bayer order is supported (though it depends on the flip
> > -      * settings)
> > -      */
> > -     if (code->index > 0)
> > +     if (code->pad != 0)
> > +             return -EINVAL;
>
> The pad is already checked by the caller, no need to repeat here.
>
OK, will drop it.

> > +     if (code->index >= (ARRAY_SIZE(codes) / 4))
> >               return -EINVAL;
> >
> > -     code->code = imx219_get_format_code(imx219);
> > +     code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
> >
> >       return 0;
> >  }
> > @@ -669,7 +709,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >       if (fse->index >= ARRAY_SIZE(supported_modes))
> >               return -EINVAL;
> >
> > -     if (fse->code != imx219_get_format_code(imx219))
> > +     if (fse->code != imx219_get_format_code(imx219, imx219->fmt.code))
> >               return -EINVAL;
> >
> >       fse->min_width = supported_modes[fse->index].width;
> > @@ -696,7 +736,6 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >  {
> >       fmt->format.width = mode->width;
> >       fmt->format.height = mode->height;
> > -     fmt->format.code = imx219_get_format_code(imx219);
> >       fmt->format.field = V4L2_FIELD_NONE;
> >
> >       imx219_reset_colorspace(&fmt->format);
> > @@ -710,10 +749,12 @@ static int __imx219_get_pad_format(struct imx219 *imx219,
> >               struct v4l2_mbus_framefmt *try_fmt =
> >                       v4l2_subdev_get_try_format(&imx219->sd, cfg, fmt->pad);
> >               /* update the code which could change due to vflip or hflip: */
> > -             try_fmt->code = imx219_get_format_code(imx219);
> > +             try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> >               fmt->format = *try_fmt;
> >       } else {
> >               imx219_update_pad_format(imx219, imx219->mode, fmt);
> > +             fmt->format.code = imx219_get_format_code(imx219,
> > +                                                       imx219->fmt.code);
> >       }
> >
> >       return 0;
> > @@ -741,11 +782,18 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       const struct imx219_mode *mode;
> >       struct v4l2_mbus_framefmt *framefmt;
> >       int exposure_max, exposure_def, hblank;
> > +     int i;
>
> unsigned int
>
sure will change it.

> >
> >       mutex_lock(&imx219->mutex);
> >
> > +     for (i = 0; i < ARRAY_SIZE(codes); i++)
> > +             if (codes[i] == fmt->format.code)
> > +                     break;
> > +     if (i >= ARRAY_SIZE(codes))
> > +             i = 0;
> > +
> >       /* Bayer order varies with flips */
> > -     fmt->format.code = imx219_get_format_code(imx219);
> > +     fmt->format.code = imx219_get_format_code(imx219, codes[i]);
> >
> >       mode = v4l2_find_nearest_size(supported_modes,
> >                                     ARRAY_SIZE(supported_modes),
> > @@ -755,7 +803,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >               framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >               *framefmt = fmt->format;
> > -     } else if (imx219->mode != mode) {
> > +     } else if (imx219->mode != mode ||
> > +                imx219->fmt.code != fmt->format.code) {
> > +             imx219->fmt = fmt->format;
> >               imx219->mode = mode;
> >               /* Update limits and set FPS to default */
> >               __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -786,6 +836,40 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >       return 0;
> >  }
> >
> > +static int imx219_set_framefmt(struct imx219 *imx219)
> > +{
> > +     int ret;
> > +
> > +     switch (imx219->fmt.code) {
> > +     case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +     case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +     case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +     case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +             ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                    IMX219_REG_VALUE_08BIT, 0x08);
> > +             ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                    IMX219_REG_VALUE_08BIT, 0x08);
> > +             ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> > +                                     IMX219_REG_VALUE_08BIT, 0x08);
>
> What kind of error codes do you expect to get? :-)
>
> Please either return the error immediately, or check ret before the
> assignment.
>
sure will change it and instead make it a bulk write.

> > +             break;
> > +     case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +     case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +     case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +     case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +             ret = imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_0_7,
> > +                                    IMX219_REG_VALUE_08BIT, 0x0a);
> > +             ret |= imx219_write_reg(imx219, IMX219_CSI_DATA_FORMAT_A_8_15,
> > +                                    IMX219_REG_VALUE_08BIT, 0x0a);
> > +             ret |= imx219_write_reg(imx219, IMX219_OPPXCK_DIV,
> > +                                     IMX219_REG_VALUE_08BIT, 0x0a);
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
>
> You could return -EINVAL here.
>
OK

> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static int imx219_start_streaming(struct imx219 *imx219)
> >  {
> >       struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > @@ -800,6 +884,12 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >               return ret;
> >       }
> >
> > +     ret = imx219_set_framefmt(imx219);
> > +     if (ret) {
> > +             dev_err(&client->dev, "%s failed to set format\n", __func__);
>
> The error code could be useful.
>
OK, will include ret in the print.

Cheers,
--Prabhakar

> > +             return ret;
> > +     }
> > +
> >       /* Apply customized values from user */
> >       ret =  __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> >       if (ret)
> > @@ -1253,6 +1343,9 @@ static int imx219_probe(struct i2c_client *client)
> >       /* Initialize source pad */
> >       imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> >
> > +     /* Initialize default format */
> > +     imx219_set_default_format(imx219);
> > +
> >       ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> >       if (ret) {
> >               dev_err(dev, "failed to init entity pads: %d\n", ret);
>
> --
> Kind regards,
>
> Sakari Ailus

[PATCH v2 3/3] media: i2c: imx219: Add support for cropped 640x480 resolution

[Lad Prabhakar]

This patch adds mode table entry for capturing cropped 640x480 resolution

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/imx219.c | 72 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f96f3ce9fd85..6a86f500ec48 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -54,6 +54,7 @@
 #define IMX219_VTS_15FPS		0x0dc6
 #define IMX219_VTS_30FPS_1080P		0x06e3
 #define IMX219_VTS_30FPS_BINNED		0x06e3
+#define IMX219_VTS_30FPS_640x480	0x06e3
 #define IMX219_VTS_MAX			0xffff
 
 #define IMX219_VBLANK_MIN		4
@@ -142,8 +143,8 @@ struct imx219_mode {
 
 /*
  * Register sets lifted off the i2C interface from the Raspberry Pi firmware
- * driver.
- * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
+ * driver for resolutions 3280x2464, 1920x1080 and 1640x1232.
+ * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
  */
 static const struct imx219_reg mode_3280x2464_regs[] = {
 	{0x0100, 0x00},
@@ -318,6 +319,63 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
 	{0x0163, 0x78},
 };
 
+static const struct imx219_reg mode_640_480_regs[] = {
+	{0x0100, 0x00},
+	{0x30eb, 0x05},
+	{0x30eb, 0x0c},
+	{0x300a, 0xff},
+	{0x300b, 0xff},
+	{0x30eb, 0x05},
+	{0x30eb, 0x09},
+	{0x0114, 0x01},
+	{0x0128, 0x00},
+	{0x012a, 0x18},
+	{0x012b, 0x00},
+	{0x0162, 0x0d},
+	{0x0163, 0x78},
+	{0x0164, 0x03},
+	{0x0165, 0xe8},
+	{0x0166, 0x08},
+	{0x0167, 0xe7},
+	{0x0168, 0x02},
+	{0x0169, 0xf0},
+	{0x016a, 0x06},
+	{0x016b, 0xaf},
+	{0x016c, 0x02},
+	{0x016d, 0x80},
+	{0x016e, 0x01},
+	{0x016f, 0xe0},
+	{0x0170, 0x01},
+	{0x0171, 0x01},
+	{0x0174, 0x03},
+	{0x0175, 0x03},
+	{0x0301, 0x05},
+	{0x0303, 0x01},
+	{0x0304, 0x03},
+	{0x0305, 0x03},
+	{0x0306, 0x00},
+	{0x0307, 0x39},
+	{0x030b, 0x01},
+	{0x030c, 0x00},
+	{0x030d, 0x72},
+	{0x0624, 0x06},
+	{0x0625, 0x68},
+	{0x0626, 0x04},
+	{0x0627, 0xd0},
+	{0x455e, 0x00},
+	{0x471e, 0x4b},
+	{0x4767, 0x0f},
+	{0x4750, 0x14},
+	{0x4540, 0x00},
+	{0x47b4, 0x14},
+	{0x4713, 0x30},
+	{0x478b, 0x10},
+	{0x478f, 0x10},
+	{0x4793, 0x10},
+	{0x4797, 0x0e},
+	{0x479b, 0x0e},
+};
+
 static const char * const imx219_test_pattern_menu[] = {
 	"Disabled",
 	"Color Bars",
@@ -424,6 +482,16 @@ static const struct imx219_mode supported_modes[] = {
 			.regs = mode_1640_1232_regs,
 		},
 	},
+	{
+		/* 640x480 30fps mode */
+		.width = 640,
+		.height = 480,
+		.vts_def = IMX219_VTS_30FPS_640x480,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_640_480_regs),
+			.regs = mode_640_480_regs,
+		},
+	},
 };
 
 struct imx219 {
-- 
2.20.1

Re: [PATCH v2 3/3] media: i2c: imx219: Add support for cropped 640x480 resolution

[Dave Stevenson]

Hi Pabhakar

Thanks for the update. One very minor nit-pick.

On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> This patch adds mode table entry for capturing cropped 640x480 resolution
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/imx219.c | 72 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f96f3ce9fd85..6a86f500ec48 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -54,6 +54,7 @@
>  #define IMX219_VTS_15FPS               0x0dc6
>  #define IMX219_VTS_30FPS_1080P         0x06e3
>  #define IMX219_VTS_30FPS_BINNED                0x06e3
> +#define IMX219_VTS_30FPS_640x480       0x06e3

Thanks on updating this - I can confirm the default is now 30fps
rather than the 90 I was seeing before.
Reducing vertical blanking down to the minimum 4 lines give me
109.3fps and all still working properly :-)

>  #define IMX219_VTS_MAX                 0xffff
>
>  #define IMX219_VBLANK_MIN              4
> @@ -142,8 +143,8 @@ struct imx219_mode {
>
>  /*
>   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> - * driver.
> - * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
> + * driver for resolutions 3280x2464, 1920x1080 and 1640x1232.
> + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.

640x480 has come from mode 1 of the firmware? mode 1 is the 1080p mode.

Having checked through the register settings they are identical to
those used by the Pi firmware for mode 7, see [1]. You could quote
that rather than stating that they were derived by yourself.

One of Sony's concerns when I discussed upstreaming this driver with
them was that people might add modes with random register settings. If
the image quality was then sub-standard they'd unjustly look bad. They
validated and blessed the register sets that we were using in the Pi
firmware, so retaining that parentage will make them happy.

[1] https://github.com/6by9/raspiraw/blob/master/imx219_modes.h#L506

>   */
>  static const struct imx219_reg mode_3280x2464_regs[] = {
>         {0x0100, 0x00},
> @@ -318,6 +319,63 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
>         {0x0163, 0x78},
>  };
>
> +static const struct imx219_reg mode_640_480_regs[] = {
> +       {0x0100, 0x00},
> +       {0x30eb, 0x05},
> +       {0x30eb, 0x0c},
> +       {0x300a, 0xff},
> +       {0x300b, 0xff},
> +       {0x30eb, 0x05},
> +       {0x30eb, 0x09},
> +       {0x0114, 0x01},
> +       {0x0128, 0x00},
> +       {0x012a, 0x18},
> +       {0x012b, 0x00},
> +       {0x0162, 0x0d},
> +       {0x0163, 0x78},
> +       {0x0164, 0x03},
> +       {0x0165, 0xe8},
> +       {0x0166, 0x08},
> +       {0x0167, 0xe7},
> +       {0x0168, 0x02},
> +       {0x0169, 0xf0},
> +       {0x016a, 0x06},
> +       {0x016b, 0xaf},
> +       {0x016c, 0x02},
> +       {0x016d, 0x80},
> +       {0x016e, 0x01},
> +       {0x016f, 0xe0},
> +       {0x0170, 0x01},
> +       {0x0171, 0x01},
> +       {0x0174, 0x03},
> +       {0x0175, 0x03},
> +       {0x0301, 0x05},
> +       {0x0303, 0x01},
> +       {0x0304, 0x03},
> +       {0x0305, 0x03},
> +       {0x0306, 0x00},
> +       {0x0307, 0x39},
> +       {0x030b, 0x01},
> +       {0x030c, 0x00},
> +       {0x030d, 0x72},
> +       {0x0624, 0x06},
> +       {0x0625, 0x68},
> +       {0x0626, 0x04},
> +       {0x0627, 0xd0},
> +       {0x455e, 0x00},
> +       {0x471e, 0x4b},
> +       {0x4767, 0x0f},
> +       {0x4750, 0x14},
> +       {0x4540, 0x00},
> +       {0x47b4, 0x14},
> +       {0x4713, 0x30},
> +       {0x478b, 0x10},
> +       {0x478f, 0x10},
> +       {0x4793, 0x10},
> +       {0x4797, 0x0e},
> +       {0x479b, 0x0e},
> +};
> +
>  static const char * const imx219_test_pattern_menu[] = {
>         "Disabled",
>         "Color Bars",
> @@ -424,6 +482,16 @@ static const struct imx219_mode supported_modes[] = {
>                         .regs = mode_1640_1232_regs,
>                 },
>         },
> +       {
> +               /* 640x480 30fps mode */
> +               .width = 640,
> +               .height = 480,
> +               .vts_def = IMX219_VTS_30FPS_640x480,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> +                       .regs = mode_640_480_regs,
> +               },
> +       },
>  };
>
>  struct imx219 {
> --
> 2.20.1
>

Re: [PATCH v2 3/3] media: i2c: imx219: Add support for cropped 640x480 resolution

[Lad, Prabhakar]

Hi Dave,

Thank you for the review.

On Fri, Mar 6, 2020 at 2:47 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Pabhakar
>
> Thanks for the update. One very minor nit-pick.
>
> On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > This patch adds mode table entry for capturing cropped 640x480 resolution
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/imx219.c | 72 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index f96f3ce9fd85..6a86f500ec48 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -54,6 +54,7 @@
> >  #define IMX219_VTS_15FPS               0x0dc6
> >  #define IMX219_VTS_30FPS_1080P         0x06e3
> >  #define IMX219_VTS_30FPS_BINNED                0x06e3
> > +#define IMX219_VTS_30FPS_640x480       0x06e3
>
> Thanks on updating this - I can confirm the default is now 30fps
> rather than the 90 I was seeing before.
> Reducing vertical blanking down to the minimum 4 lines give me
> 109.3fps and all still working properly :-)
>
thank you for testing.

> >  #define IMX219_VTS_MAX                 0xffff
> >
> >  #define IMX219_VBLANK_MIN              4
> > @@ -142,8 +143,8 @@ struct imx219_mode {
> >
> >  /*
> >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> > - * driver.
> > - * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
> > + * driver for resolutions 3280x2464, 1920x1080 and 1640x1232.
> > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
>
> 640x480 has come from mode 1 of the firmware? mode 1 is the 1080p mode.
>
my bad, my context of mode was cropped/binned.

> Having checked through the register settings they are identical to
> those used by the Pi firmware for mode 7, see [1]. You could quote
> that rather than stating that they were derived by yourself.
>
> One of Sony's concerns when I discussed upstreaming this driver with
> them was that people might add modes with random register settings. If
> the image quality was then sub-standard they'd unjustly look bad. They
> validated and blessed the register sets that we were using in the Pi
> firmware, so retaining that parentage will make them happy.
>
> [1] https://github.com/6by9/raspiraw/blob/master/imx219_modes.h#L506
>
To be honest the driver which was in-house didn't have  any references
for the register settings,
I'll instead add the reference to which you pointed and set as mode = 7.

If you are Okay Ill just resend this patch as rest have been acked.

Cheers,
--Prabhakar

> >   */
> >  static const struct imx219_reg mode_3280x2464_regs[] = {
> >         {0x0100, 0x00},
> > @@ -318,6 +319,63 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> >         {0x0163, 0x78},
> >  };
> >
> > +static const struct imx219_reg mode_640_480_regs[] = {
> > +       {0x0100, 0x00},
> > +       {0x30eb, 0x05},
> > +       {0x30eb, 0x0c},
> > +       {0x300a, 0xff},
> > +       {0x300b, 0xff},
> > +       {0x30eb, 0x05},
> > +       {0x30eb, 0x09},
> > +       {0x0114, 0x01},
> > +       {0x0128, 0x00},
> > +       {0x012a, 0x18},
> > +       {0x012b, 0x00},
> > +       {0x0162, 0x0d},
> > +       {0x0163, 0x78},
> > +       {0x0164, 0x03},
> > +       {0x0165, 0xe8},
> > +       {0x0166, 0x08},
> > +       {0x0167, 0xe7},
> > +       {0x0168, 0x02},
> > +       {0x0169, 0xf0},
> > +       {0x016a, 0x06},
> > +       {0x016b, 0xaf},
> > +       {0x016c, 0x02},
> > +       {0x016d, 0x80},
> > +       {0x016e, 0x01},
> > +       {0x016f, 0xe0},
> > +       {0x0170, 0x01},
> > +       {0x0171, 0x01},
> > +       {0x0174, 0x03},
> > +       {0x0175, 0x03},
> > +       {0x0301, 0x05},
> > +       {0x0303, 0x01},
> > +       {0x0304, 0x03},
> > +       {0x0305, 0x03},
> > +       {0x0306, 0x00},
> > +       {0x0307, 0x39},
> > +       {0x030b, 0x01},
> > +       {0x030c, 0x00},
> > +       {0x030d, 0x72},
> > +       {0x0624, 0x06},
> > +       {0x0625, 0x68},
> > +       {0x0626, 0x04},
> > +       {0x0627, 0xd0},
> > +       {0x455e, 0x00},
> > +       {0x471e, 0x4b},
> > +       {0x4767, 0x0f},
> > +       {0x4750, 0x14},
> > +       {0x4540, 0x00},
> > +       {0x47b4, 0x14},
> > +       {0x4713, 0x30},
> > +       {0x478b, 0x10},
> > +       {0x478f, 0x10},
> > +       {0x4793, 0x10},
> > +       {0x4797, 0x0e},
> > +       {0x479b, 0x0e},
> > +};
> > +
> >  static const char * const imx219_test_pattern_menu[] = {
> >         "Disabled",
> >         "Color Bars",
> > @@ -424,6 +482,16 @@ static const struct imx219_mode supported_modes[] = {
> >                         .regs = mode_1640_1232_regs,
> >                 },
> >         },
> > +       {
> > +               /* 640x480 30fps mode */
> > +               .width = 640,
> > +               .height = 480,
> > +               .vts_def = IMX219_VTS_30FPS_640x480,
> > +               .reg_list = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> > +                       .regs = mode_640_480_regs,
> > +               },
> > +       },
> >  };
> >
> >  struct imx219 {
> > --
> > 2.20.1
> >

Re: [PATCH v2 3/3] media: i2c: imx219: Add support for cropped 640x480 resolution

[Dave Stevenson]

On Fri, 6 Mar 2020 at 14:59, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> Hi Dave,
>
> Thank you for the review.
>
> On Fri, Mar 6, 2020 at 2:47 PM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Pabhakar
> >
> > Thanks for the update. One very minor nit-pick.
> >
> > On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > >
> > > This patch adds mode table entry for capturing cropped 640x480 resolution
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 72 ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 70 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index f96f3ce9fd85..6a86f500ec48 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -54,6 +54,7 @@
> > >  #define IMX219_VTS_15FPS               0x0dc6
> > >  #define IMX219_VTS_30FPS_1080P         0x06e3
> > >  #define IMX219_VTS_30FPS_BINNED                0x06e3
> > > +#define IMX219_VTS_30FPS_640x480       0x06e3
> >
> > Thanks on updating this - I can confirm the default is now 30fps
> > rather than the 90 I was seeing before.
> > Reducing vertical blanking down to the minimum 4 lines give me
> > 109.3fps and all still working properly :-)
> >
> thank you for testing.
>
> > >  #define IMX219_VTS_MAX                 0xffff
> > >
> > >  #define IMX219_VBLANK_MIN              4
> > > @@ -142,8 +143,8 @@ struct imx219_mode {
> > >
> > >  /*
> > >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> > > - * driver.
> > > - * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
> > > + * driver for resolutions 3280x2464, 1920x1080 and 1640x1232.
> > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
> >
> > 640x480 has come from mode 1 of the firmware? mode 1 is the 1080p mode.
> >
> my bad, my context of mode was cropped/binned.
>
> > Having checked through the register settings they are identical to
> > those used by the Pi firmware for mode 7, see [1]. You could quote
> > that rather than stating that they were derived by yourself.
> >
> > One of Sony's concerns when I discussed upstreaming this driver with
> > them was that people might add modes with random register settings. If
> > the image quality was then sub-standard they'd unjustly look bad. They
> > validated and blessed the register sets that we were using in the Pi
> > firmware, so retaining that parentage will make them happy.
> >
> > [1] https://github.com/6by9/raspiraw/blob/master/imx219_modes.h#L506
> >
> To be honest the driver which was in-house didn't have  any references
> for the register settings,
> I'll instead add the reference to which you pointed and set as mode = 7.

Code of unknown origin being passed around is always fun!

I hadn't referenced my raspiraw repo as the source as it's not an
officially released app, and Github repos have a tendency to move or
get deleted over time. The original comment that it was as per the
Raspberry Pi firmware (of which raspiraw is an I2C dump of the
transactions, but anyone else could attach an analyser for themselves)
seemed sufficient to me. Up to you though.

FYI The complete list of modes we have register sets for are
- mode 1 = 1920x1080 cropped
- mode 2 =  3280x2464 full FOV
- mode 3 =  same as mode 2 for legacy reasons.
- mode 4 = 1640x1232 2x2 binned
- mode 5 = 1640x922 2x2 binned and centre cropped to 16:9
- mode 6 = 1280x720 2x2 binned in the fast binning mode, and centre
cropped. For higher framerate capture (up to 120fps)
- mode 7 = 640x480 2x2 binned in the fast binning mode, and centre
cropped. For higher framerate capture (up to 200fps). I'd need to
investigate why your copy of this register set only gets up to 109fps.
Quite possibly line_length being reduced.

> If you are Okay Ill just resend this patch as rest have been acked.

Fine by me if others are happy with the rest of the patches.

  Dave

> Cheers,
> --Prabhakar
>
> > >   */
> > >  static const struct imx219_reg mode_3280x2464_regs[] = {
> > >         {0x0100, 0x00},
> > > @@ -318,6 +319,63 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > >         {0x0163, 0x78},
> > >  };
> > >
> > > +static const struct imx219_reg mode_640_480_regs[] = {
> > > +       {0x0100, 0x00},
> > > +       {0x30eb, 0x05},
> > > +       {0x30eb, 0x0c},
> > > +       {0x300a, 0xff},
> > > +       {0x300b, 0xff},
> > > +       {0x30eb, 0x05},
> > > +       {0x30eb, 0x09},
> > > +       {0x0114, 0x01},
> > > +       {0x0128, 0x00},
> > > +       {0x012a, 0x18},
> > > +       {0x012b, 0x00},
> > > +       {0x0162, 0x0d},
> > > +       {0x0163, 0x78},
> > > +       {0x0164, 0x03},
> > > +       {0x0165, 0xe8},
> > > +       {0x0166, 0x08},
> > > +       {0x0167, 0xe7},
> > > +       {0x0168, 0x02},
> > > +       {0x0169, 0xf0},
> > > +       {0x016a, 0x06},
> > > +       {0x016b, 0xaf},
> > > +       {0x016c, 0x02},
> > > +       {0x016d, 0x80},
> > > +       {0x016e, 0x01},
> > > +       {0x016f, 0xe0},
> > > +       {0x0170, 0x01},
> > > +       {0x0171, 0x01},
> > > +       {0x0174, 0x03},
> > > +       {0x0175, 0x03},
> > > +       {0x0301, 0x05},
> > > +       {0x0303, 0x01},
> > > +       {0x0304, 0x03},
> > > +       {0x0305, 0x03},
> > > +       {0x0306, 0x00},
> > > +       {0x0307, 0x39},
> > > +       {0x030b, 0x01},
> > > +       {0x030c, 0x00},
> > > +       {0x030d, 0x72},
> > > +       {0x0624, 0x06},
> > > +       {0x0625, 0x68},
> > > +       {0x0626, 0x04},
> > > +       {0x0627, 0xd0},
> > > +       {0x455e, 0x00},
> > > +       {0x471e, 0x4b},
> > > +       {0x4767, 0x0f},
> > > +       {0x4750, 0x14},
> > > +       {0x4540, 0x00},
> > > +       {0x47b4, 0x14},
> > > +       {0x4713, 0x30},
> > > +       {0x478b, 0x10},
> > > +       {0x478f, 0x10},
> > > +       {0x4793, 0x10},
> > > +       {0x4797, 0x0e},
> > > +       {0x479b, 0x0e},
> > > +};
> > > +
> > >  static const char * const imx219_test_pattern_menu[] = {
> > >         "Disabled",
> > >         "Color Bars",
> > > @@ -424,6 +482,16 @@ static const struct imx219_mode supported_modes[] = {
> > >                         .regs = mode_1640_1232_regs,
> > >                 },
> > >         },
> > > +       {
> > > +               /* 640x480 30fps mode */
> > > +               .width = 640,
> > > +               .height = 480,
> > > +               .vts_def = IMX219_VTS_30FPS_640x480,
> > > +               .reg_list = {
> > > +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> > > +                       .regs = mode_640_480_regs,
> > > +               },
> > > +       },
> > >  };
> > >
> > >  struct imx219 {
> > > --
> > > 2.20.1
> > >

Re: [PATCH v2 3/3] media: i2c: imx219: Add support for cropped 640x480 resolution

[Laurent Pinchart]

On Fri, Mar 06, 2020 at 05:02:30PM +0000, Dave Stevenson wrote:
> On Fri, 6 Mar 2020 at 14:59, Lad, Prabhakar wrote:
> > On Fri, Mar 6, 2020 at 2:47 PM Dave Stevenson wrote:
> > > On Fri, 6 Mar 2020 at 10:33, Lad Prabhakar wrote:
> > > >
> > > > This patch adds mode table entry for capturing cropped 640x480 resolution
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/media/i2c/imx219.c | 72 ++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 70 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index f96f3ce9fd85..6a86f500ec48 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -54,6 +54,7 @@
> > > >  #define IMX219_VTS_15FPS               0x0dc6
> > > >  #define IMX219_VTS_30FPS_1080P         0x06e3
> > > >  #define IMX219_VTS_30FPS_BINNED                0x06e3
> > > > +#define IMX219_VTS_30FPS_640x480       0x06e3
> > >
> > > Thanks on updating this - I can confirm the default is now 30fps
> > > rather than the 90 I was seeing before.
> > > Reducing vertical blanking down to the minimum 4 lines give me
> > > 109.3fps and all still working properly :-)
> >
> > thank you for testing.
> >
> > > >  #define IMX219_VTS_MAX                 0xffff
> > > >
> > > >  #define IMX219_VBLANK_MIN              4
> > > > @@ -142,8 +143,8 @@ struct imx219_mode {
> > > >
> > > >  /*
> > > >   * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> > > > - * driver.
> > > > - * 3280x2464 = mode 2, 1920x1080 = mode 1, and 1640x1232 = mode 4.
> > > > + * driver for resolutions 3280x2464, 1920x1080 and 1640x1232.
> > > > + * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 1.
> > >
> > > 640x480 has come from mode 1 of the firmware? mode 1 is the 1080p mode.
> >
> > my bad, my context of mode was cropped/binned.
> >
> > > Having checked through the register settings they are identical to
> > > those used by the Pi firmware for mode 7, see [1]. You could quote
> > > that rather than stating that they were derived by yourself.
> > >
> > > One of Sony's concerns when I discussed upstreaming this driver with
> > > them was that people might add modes with random register settings. If
> > > the image quality was then sub-standard they'd unjustly look bad. They
> > > validated and blessed the register sets that we were using in the Pi
> > > firmware, so retaining that parentage will make them happy.
> > >
> > > [1] https://github.com/6by9/raspiraw/blob/master/imx219_modes.h#L506
> > >
> > To be honest the driver which was in-house didn't have  any references
> > for the register settings,
> > I'll instead add the reference to which you pointed and set as mode = 7.
> 
> Code of unknown origin being passed around is always fun!
> 
> I hadn't referenced my raspiraw repo as the source as it's not an
> officially released app, and Github repos have a tendency to move or
> get deleted over time. The original comment that it was as per the
> Raspberry Pi firmware (of which raspiraw is an I2C dump of the
> transactions, but anyone else could attach an analyser for themselves)
> seemed sufficient to me. Up to you though.
> 
> FYI The complete list of modes we have register sets for are
> - mode 1 = 1920x1080 cropped
> - mode 2 =  3280x2464 full FOV
> - mode 3 =  same as mode 2 for legacy reasons.
> - mode 4 = 1640x1232 2x2 binned
> - mode 5 = 1640x922 2x2 binned and centre cropped to 16:9
> - mode 6 = 1280x720 2x2 binned in the fast binning mode, and centre
> cropped. For higher framerate capture (up to 120fps)
> - mode 7 = 640x480 2x2 binned in the fast binning mode, and centre
> cropped. For higher framerate capture (up to 200fps). I'd need to
> investigate why your copy of this register set only gets up to 109fps.
> Quite possibly line_length being reduced.
> 
> > If you are Okay Ill just resend this patch as rest have been acked.
> 
> Fine by me if others are happy with the rest of the patches.
> 
> > > >   */
> > > >  static const struct imx219_reg mode_3280x2464_regs[] = {
> > > >         {0x0100, 0x00},
> > > > @@ -318,6 +319,63 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > > >         {0x0163, 0x78},
> > > >  };
> > > >
> > > > +static const struct imx219_reg mode_640_480_regs[] = {
> > > > +       {0x0100, 0x00},
> > > > +       {0x30eb, 0x05},
> > > > +       {0x30eb, 0x0c},
> > > > +       {0x300a, 0xff},
> > > > +       {0x300b, 0xff},
> > > > +       {0x30eb, 0x05},
> > > > +       {0x30eb, 0x09},
> > > > +       {0x0114, 0x01},
> > > > +       {0x0128, 0x00},
> > > > +       {0x012a, 0x18},
> > > > +       {0x012b, 0x00},
> > > > +       {0x0162, 0x0d},
> > > > +       {0x0163, 0x78},
> > > > +       {0x0164, 0x03},
> > > > +       {0x0165, 0xe8},
> > > > +       {0x0166, 0x08},
> > > > +       {0x0167, 0xe7},
> > > > +       {0x0168, 0x02},
> > > > +       {0x0169, 0xf0},
> > > > +       {0x016a, 0x06},
> > > > +       {0x016b, 0xaf},
> > > > +       {0x016c, 0x02},
> > > > +       {0x016d, 0x80},
> > > > +       {0x016e, 0x01},
> > > > +       {0x016f, 0xe0},
> > > > +       {0x0170, 0x01},
> > > > +       {0x0171, 0x01},
> > > > +       {0x0174, 0x03},
> > > > +       {0x0175, 0x03},
> > > > +       {0x0301, 0x05},
> > > > +       {0x0303, 0x01},
> > > > +       {0x0304, 0x03},
> > > > +       {0x0305, 0x03},
> > > > +       {0x0306, 0x00},
> > > > +       {0x0307, 0x39},
> > > > +       {0x030b, 0x01},
> > > > +       {0x030c, 0x00},
> > > > +       {0x030d, 0x72},
> > > > +       {0x0624, 0x06},
> > > > +       {0x0625, 0x68},
> > > > +       {0x0626, 0x04},
> > > > +       {0x0627, 0xd0},

I'm wondering where these four values come from. They set the
TP_WINDOW_WIDTH and TP_WINDOW_HEIGHT registers to 1640 and 1232
respectively, which seems to have been copied from the 1640x1232 mode.
Are they right for the 640x480 mode ? All the other modes set those two
registers to the same values as X_OUTPUT_SIZE and Y_OUTPUT_SIZE.

> > > > +       {0x455e, 0x00},
> > > > +       {0x471e, 0x4b},
> > > > +       {0x4767, 0x0f},
> > > > +       {0x4750, 0x14},
> > > > +       {0x4540, 0x00},
> > > > +       {0x47b4, 0x14},
> > > > +       {0x4713, 0x30},
> > > > +       {0x478b, 0x10},
> > > > +       {0x478f, 0x10},
> > > > +       {0x4793, 0x10},
> > > > +       {0x4797, 0x0e},
> > > > +       {0x479b, 0x0e},
> > > > +};
> > > > +
> > > >  static const char * const imx219_test_pattern_menu[] = {
> > > >         "Disabled",
> > > >         "Color Bars",
> > > > @@ -424,6 +482,16 @@ static const struct imx219_mode supported_modes[] = {
> > > >                         .regs = mode_1640_1232_regs,
> > > >                 },
> > > >         },
> > > > +       {
> > > > +               /* 640x480 30fps mode */
> > > > +               .width = 640,
> > > > +               .height = 480,
> > > > +               .vts_def = IMX219_VTS_30FPS_640x480,
> > > > +               .reg_list = {
> > > > +                       .num_of_regs = ARRAY_SIZE(mode_640_480_regs),
> > > > +                       .regs = mode_640_480_regs,
> > > > +               },
> > > > +       },
> > > >  };
> > > >
> > > >  struct imx219 {

-- 
Regards,

Laurent Pinchart