[RFC PATCH 0/3] media: imx: imx7-media-csi: i.MX8MM support

[RFC PATCH 0/3] media: imx: imx7-media-csi: i.MX8MM support

[Laurent Pinchart]

Hello,

This small patch series updates the imx7-media-csi driver to work on the
i.MX8MM with an OV5640 sensor.

Patch 1/3 extends the nxp,imx7-csi DT bindings with a compatible string
for the i.MX8MM. While the CSI bridge in that SoC doesn't seem to differ
from the one in the i.MX7 according to the reference manual, experience
shows that NXP reference manuals are not always reliable. To be on the
safe side, a new fsl,imx8mm-csi compatible string, with a fallback on
fsl,imx7-csi, will avoid future backward-compatibility problems.

Patches 2/3 and 3/3 fix issues with RAW8 and RAW10 capture from an
OV5640 sensor. The fixes are the result of experimentation and study of
NXP BSP drivers, as the reference manual doesn't provide much
information in this area. I'm not very happy with this, as understanding
the exact effect of the register fields modified by those two patches
would be better. Still, without support from NXP (which I would really,
really appreciate - anyone from NXP reading this ?), I can't do better.

Given those concerns, I would also appreciate if this series could be
tested widely for possible regressions. There should be no change for
YUV formats, so only raw formats (RAW8, RAW10, RAW12 and RAW14) need to
be tested.

Laurent Pinchart (3):
  dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support
  media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
  media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1

 .../bindings/media/nxp,imx7-csi.yaml          | 12 +++++++----
 drivers/staging/media/imx/imx7-media-csi.c    | 21 +++++--------------
 2 files changed, 13 insertions(+), 20 deletions(-)

-- 
Regards,

Laurent Pinchart

[RFC PATCH 1/3] dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support

[Laurent Pinchart]

The i.MX8MM integrates a CSI bridge IP core, as the i.MX7. There seems
to be no difference between the two SoCs according to the reference
manual, but as documentation may not be accurate, add a compatible
string for the i.MX8MM, with a fallback on the compatible i.MX7.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../devicetree/bindings/media/nxp,imx7-csi.yaml      | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
index d91575b8ebb9..5922a2795167 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/media/nxp,imx7-csi.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: i.MX7 CMOS Sensor Interface
+title: i.MX7 and i.MX8 CSI bridge (CMOS Sensor Interface)
 
 maintainers:
   - Rui Miguel Silva <rmfrfs@gmail.com>
@@ -15,9 +15,13 @@ description: |
 
 properties:
   compatible:
-    enum:
-      - fsl,imx7-csi
-      - fsl,imx6ul-csi
+    oneOf:
+      - enum:
+          - fsl,imx7-csi
+          - fsl,imx6ul-csi
+      - items:
+          - const: fsl,imx8mm-csi
+          - const: fsl,imx7-csi
 
   reg:
     maxItems: 1
-- 
Regards,

Laurent Pinchart

[RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Laurent Pinchart]

Sample code from NXP, as well as experiments on i.MX8MM with RAW10
capture with an OV5640 sensor connected over CSI-2, showed that the
TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
larger than 8 bits. Do so, even if the reference manual doesn't clearly
describe the effect of the field.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index f85a2f5f1413..256b9aa978f0 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 	int width = out_pix->width;
 	u32 stride = 0;
 	u32 cr1, cr18;
+	u32 cr3 = 0;
 
 	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
 
@@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG10_1X10:
 		case MEDIA_BUS_FMT_SGRBG10_1X10:
 		case MEDIA_BUS_FMT_SRGGB10_1X10:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
 			break;
 		case MEDIA_BUS_FMT_Y12_1X12:
@@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG12_1X12:
 		case MEDIA_BUS_FMT_SGRBG12_1X12:
 		case MEDIA_BUS_FMT_SRGGB12_1X12:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
 			break;
 		case MEDIA_BUS_FMT_Y14_1X14:
@@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG14_1X14:
 		case MEDIA_BUS_FMT_SGRBG14_1X14:
 		case MEDIA_BUS_FMT_SRGGB14_1X14:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
 			break;
 		/*
@@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 
 	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
 	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
-	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
+	imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
 	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
 
 	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
-- 
Regards,

Laurent Pinchart

[RFC PATCH 3/3] media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1

[Laurent Pinchart]

The PIXEL_BIT field of the CSICR1 register is documented as setting the
Bayer data width to 10 bits, and is set by the driver for all non-YUV
pixel formats. Test code from NXP showed that the bit shouldn't be set
for Bayer formats, and this was confirmed by experimentation with RAW8
capture (which doesn't work when setting the field) and RAW10 capture
(for which setting the field doesn't seem to make a difference) on
i.MX8MM with an OV5640 sensor connected over CSI-2. Don't set it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 256b9aa978f0..94ee8d9838ee 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -495,21 +495,6 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
 			break;
 		}
-
-		switch (out_pix->pixelformat) {
-		case V4L2_PIX_FMT_Y10:
-		case V4L2_PIX_FMT_Y12:
-		case V4L2_PIX_FMT_SBGGR8:
-		case V4L2_PIX_FMT_SGBRG8:
-		case V4L2_PIX_FMT_SGRBG8:
-		case V4L2_PIX_FMT_SRGGB8:
-		case V4L2_PIX_FMT_SBGGR16:
-		case V4L2_PIX_FMT_SGBRG16:
-		case V4L2_PIX_FMT_SGRBG16:
-		case V4L2_PIX_FMT_SRGGB16:
-			cr1 |= BIT_PIXEL_BIT;
-			break;
-		}
 	}
 
 	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/3] media: imx: imx7-media-csi: i.MX8MM support

[Rui Miguel Silva]

Hi Laurent,
Thanks for extending support for imx8.

On Sun May 16, 2021 at 3:42 AM WEST, Laurent Pinchart wrote:
> Hello,
>
> This small patch series updates the imx7-media-csi driver to work on the
> i.MX8MM with an OV5640 sensor.
>
> Patch 1/3 extends the nxp,imx7-csi DT bindings with a compatible string
> for the i.MX8MM. While the CSI bridge in that SoC doesn't seem to differ
> from the one in the i.MX7 according to the reference manual, experience
> shows that NXP reference manuals are not always reliable. To be on the
> safe side, a new fsl,imx8mm-csi compatible string, with a fallback on
> fsl,imx7-csi, will avoid future backward-compatibility problems.
>
> Patches 2/3 and 3/3 fix issues with RAW8 and RAW10 capture from an
> OV5640 sensor. The fixes are the result of experimentation and study of
> NXP BSP drivers, as the reference manual doesn't provide much
> information in this area. I'm not very happy with this, as understanding
> the exact effect of the register fields modified by those two patches
> would be better. Still, without support from NXP (which I would really,
> really appreciate - anyone from NXP reading this ?), I can't do better.
>
> Given those concerns, I would also appreciate if this series could be
> tested widely for possible regressions. There should be no change for
> YUV formats, so only raw formats (RAW8, RAW10, RAW12 and RAW14) need to
> be tested.

I've tested with my setup in imx7 which is RAW10 only, and everything
looks fine.

I only have a small suggestion in 2/3.

------
Cheers,
     Rui

>
> Laurent Pinchart (3):
>   dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support
>   media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
>   media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1
>
>  .../bindings/media/nxp,imx7-csi.yaml          | 12 +++++++----
>  drivers/staging/media/imx/imx7-media-csi.c    | 21 +++++--------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
>
> -- 
> Regards,
>
> Laurent Pinchart

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Rui Miguel Silva]

Hi Laurent,
On Sun May 16, 2021 at 3:42 AM WEST, Laurent Pinchart wrote:

> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  	int width = out_pix->width;
>  	u32 stride = 0;
>  	u32 cr1, cr18;
> +	u32 cr3 = 0;
>  
>  	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>  
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG10_1X10:
>  		case MEDIA_BUS_FMT_SGRBG10_1X10:
>  		case MEDIA_BUS_FMT_SRGGB10_1X10:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
>  			break;
>  		case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG12_1X12:
>  		case MEDIA_BUS_FMT_SGRBG12_1X12:
>  		case MEDIA_BUS_FMT_SRGGB12_1X12:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
>  			break;
>  		case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG14_1X14:
>  		case MEDIA_BUS_FMT_SGRBG14_1X14:
>  		case MEDIA_BUS_FMT_SRGGB14_1X14:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
>  			break;
>  		/*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  
>  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
>  	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> -	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> +	imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);

I would prefer if you initialize cr3 with BIT_FRMCNT_RST above at
declaration or even better bellow after cr18 read or something like
that, would make it more coherent with the rest of the cr's handling.

other than that LGTM.

------
Cheers,
     Rui


>  	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>  
>  	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> -- 
> Regards,
>
> Laurent Pinchart

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Frieder Schrempf]

On 16.05.21 04:42, Laurent Pinchart wrote:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

For the ADV7280-M I also have the diffs below applied. Do you think setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8? In the RM it mentions YUV422 in the description of BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all wrong.

I know this is not really related to this patch. I'm just wondering how to properly support my setup.

--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -346,6 +346,11 @@ struct csis_pix_format {
 
 static const struct csis_pix_format mipi_csis_formats[] = {
        /* YUV formats. */
+       {
+               .code = MEDIA_BUS_FMT_UYVY8_2X8,
+               .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
+               .width = 8,
+       },
        {
                .code = MEDIA_BUS_FMT_UYVY8_1X16,
                .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,

--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
        /* Color format */
        val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
        val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
-       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
+       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
        mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
 
        /* Pixel resolution */

--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
                case MEDIA_BUS_FMT_UYVY8_1X16:
                case MEDIA_BUS_FMT_YUYV8_2X8:
                case MEDIA_BUS_FMT_YUYV8_1X16:
-                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
+                       cr3 |= BIT_TWO_8BIT_SENSOR;
+                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
                        break;
                }
        }

> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  	int width = out_pix->width;
>  	u32 stride = 0;
>  	u32 cr1, cr18;
> +	u32 cr3 = 0;
>  
>  	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>  
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG10_1X10:
>  		case MEDIA_BUS_FMT_SGRBG10_1X10:
>  		case MEDIA_BUS_FMT_SRGGB10_1X10:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
>  			break;
>  		case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG12_1X12:
>  		case MEDIA_BUS_FMT_SGRBG12_1X12:
>  		case MEDIA_BUS_FMT_SRGGB12_1X12:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
>  			break;
>  		case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG14_1X14:
>  		case MEDIA_BUS_FMT_SGRBG14_1X14:
>  		case MEDIA_BUS_FMT_SRGGB14_1X14:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
>  			break;
>  		/*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  
>  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
>  	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> -	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> +	imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
>  	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>  
>  	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> 

Re: [RFC PATCH 3/3] media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1

[Rui Miguel Silva]

Hi Laurent,
On Sun May 16, 2021 at 3:42 AM WEST, Laurent Pinchart wrote:

> The PIXEL_BIT field of the CSICR1 register is documented as setting the
> Bayer data width to 10 bits, and is set by the driver for all non-YUV
> pixel formats. Test code from NXP showed that the bit shouldn't be set
> for Bayer formats, and this was confirmed by experimentation with RAW8
> capture (which doesn't work when setting the field) and RAW10 capture
> (for which setting the field doesn't seem to make a difference) on
> i.MX8MM with an OV5640 sensor connected over CSI-2. Don't set it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 15 ---------------
>  1 file changed, 15 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 256b9aa978f0..94ee8d9838ee 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -495,21 +495,6 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  			cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>  			break;
>  		}
> -
> -		switch (out_pix->pixelformat) {
> -		case V4L2_PIX_FMT_Y10:
> -		case V4L2_PIX_FMT_Y12:
> -		case V4L2_PIX_FMT_SBGGR8:
> -		case V4L2_PIX_FMT_SGBRG8:
> -		case V4L2_PIX_FMT_SGRBG8:
> -		case V4L2_PIX_FMT_SRGGB8:
> -		case V4L2_PIX_FMT_SBGGR16:
> -		case V4L2_PIX_FMT_SGBRG16:
> -		case V4L2_PIX_FMT_SGRBG16:
> -		case V4L2_PIX_FMT_SRGGB16:
> -			cr1 |= BIT_PIXEL_BIT;
> -			break;
> -		}

LGTM

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui

>  	}
>  
>  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> -- 
> Regards,
>
> Laurent Pinchart

Re: [RFC PATCH 1/3] dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support

[Martin Kepplinger]

Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> The i.MX8MM integrates a CSI bridge IP core, as the i.MX7. There
> seems
> to be no difference between the two SoCs according to the reference
> manual, but as documentation may not be accurate, add a compatible
> string for the i.MX8MM, with a fallback on the compatible i.MX7.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/media/nxp,imx7-csi.yaml      | 12 ++++++++--
> --
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-
> csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> index d91575b8ebb9..5922a2795167 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/media/nxp,imx7-csi.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: i.MX7 CMOS Sensor Interface
> +title: i.MX7 and i.MX8 CSI bridge (CMOS Sensor Interface)
>  
>  maintainers:
>    - Rui Miguel Silva <rmfrfs@gmail.com>
> @@ -15,9 +15,13 @@ description: |
>  
>  properties:
>    compatible:
> -    enum:
> -      - fsl,imx7-csi
> -      - fsl,imx6ul-csi
> +    oneOf:
> +      - enum:
> +          - fsl,imx7-csi
> +          - fsl,imx6ul-csi
> +      - items:
> +          - const: fsl,imx8mm-csi
> +          - const: fsl,imx7-csi
>  
>    reg:
>      maxItems: 1

isn't the fsl,imx8mm-csi compatible missing in the driver then?

thanks!
                     martin

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Martin Kepplinger]

Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> formats
> larger than 8 bits. Do so, even if the reference manual doesn't
> clearly
> describe the effect of the field.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>         int width = out_pix->width;
>         u32 stride = 0;
>         u32 cr1, cr18;
> +       u32 cr3 = 0;
>  
>         cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>  
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG10_1X10:
>                 case MEDIA_BUS_FMT_SGRBG10_1X10:
>                 case MEDIA_BUS_FMT_SRGGB10_1X10:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
>                         break;
>                 case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG12_1X12:
>                 case MEDIA_BUS_FMT_SGRBG12_1X12:
>                 case MEDIA_BUS_FMT_SRGGB12_1X12:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
>                         break;
>                 case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG14_1X14:
>                 case MEDIA_BUS_FMT_SGRBG14_1X14:
>                 case MEDIA_BUS_FMT_SRGGB14_1X14:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
>                         break;
>                 /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>  
>         imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
>         imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> CSI_CSICR2);
> -       imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> +       imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
>         imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>  
>         imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> CSI_CSIRXCNT);

I can confirm that BIT_TWO_8BIT_SENSOR is needed for raw10 bayer
formats for me here.

Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>

thanks,
                          martin

Re: [RFC PATCH 3/3] media: imx: imx7-media-csi: Don't set PIXEL_BIT in CSICR1

[Martin Kepplinger]

Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> The PIXEL_BIT field of the CSICR1 register is documented as setting
> the
> Bayer data width to 10 bits, and is set by the driver for all non-YUV
> pixel formats. Test code from NXP showed that the bit shouldn't be
> set
> for Bayer formats, and this was confirmed by experimentation with
> RAW8
> capture (which doesn't work when setting the field) and RAW10 capture
> (for which setting the field doesn't seem to make a difference) on
> i.MX8MM with an OV5640 sensor connected over CSI-2. Don't set it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index 256b9aa978f0..94ee8d9838ee 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -495,21 +495,6 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                         cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>                         break;
>                 }
> -
> -               switch (out_pix->pixelformat) {
> -               case V4L2_PIX_FMT_Y10:
> -               case V4L2_PIX_FMT_Y12:
> -               case V4L2_PIX_FMT_SBGGR8:
> -               case V4L2_PIX_FMT_SGBRG8:
> -               case V4L2_PIX_FMT_SGRBG8:
> -               case V4L2_PIX_FMT_SRGGB8:
> -               case V4L2_PIX_FMT_SBGGR16:
> -               case V4L2_PIX_FMT_SGBRG16:
> -               case V4L2_PIX_FMT_SGRBG16:
> -               case V4L2_PIX_FMT_SRGGB16:
> -                       cr1 |= BIT_PIXEL_BIT;
> -                       break;
> -               }
>         }
>  
>         imx7_csi_reg_write(csi, cr1, CSI_CSICR1);

I can confirm I never needed to set that bit despite said documentation
for 10bit bayer (on imx8mq).

Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>

thanks,
                              martin

Re: [RFC PATCH 1/3] dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support

[Laurent Pinchart]

Hi Martin,

On Tue, May 18, 2021 at 01:26:21PM +0200, Martin Kepplinger wrote:
> Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> > The i.MX8MM integrates a CSI bridge IP core, as the i.MX7. There seems
> > to be no difference between the two SoCs according to the reference
> > manual, but as documentation may not be accurate, add a compatible
> > string for the i.MX8MM, with a fallback on the compatible i.MX7.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../devicetree/bindings/media/nxp,imx7-csi.yaml      | 12 ++++++++--
> > --
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-
> > csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > index d91575b8ebb9..5922a2795167 100644
> > --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> > @@ -4,7 +4,7 @@
> >  $id: http://devicetree.org/schemas/media/nxp,imx7-csi.yaml#
> >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >  
> > -title: i.MX7 CMOS Sensor Interface
> > +title: i.MX7 and i.MX8 CSI bridge (CMOS Sensor Interface)
> >  
> >  maintainers:
> >    - Rui Miguel Silva <rmfrfs@gmail.com>
> > @@ -15,9 +15,13 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - fsl,imx7-csi
> > -      - fsl,imx6ul-csi
> > +    oneOf:
> > +      - enum:
> > +          - fsl,imx7-csi
> > +          - fsl,imx6ul-csi
> > +      - items:
> > +          - const: fsl,imx8mm-csi
> > +          - const: fsl,imx7-csi
> >  
> >    reg:
> >      maxItems: 1
> 
> isn't the fsl,imx8mm-csi compatible missing in the driver then?

The driver will match on the "fsl,imx7-csi" compatible string, which the
bindings makes mandatory as a fallback for i.MX8MM. If we later find
differences between the CSI bridge in the i.MX7 and i.MX8MM, we can add
the compatible string to the driver to enable device-specific code
without any backward-compatibility issue.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/3] dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support

[Rob Herring]

On Sun, 16 May 2021 05:42:14 +0300, Laurent Pinchart wrote:
> The i.MX8MM integrates a CSI bridge IP core, as the i.MX7. There seems
> to be no difference between the two SoCs according to the reference
> manual, but as documentation may not be accurate, add a compatible
> string for the i.MX8MM, with a fallback on the compatible i.MX7.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/media/nxp,imx7-csi.yaml      | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Laurent Pinchart]

Hi Frieder,

On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
> On 16.05.21 04:42, Laurent Pinchart wrote:
> > Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> > capture with an OV5640 sensor connected over CSI-2, showed that the
> > TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> > larger than 8 bits. Do so, even if the reference manual doesn't clearly
> > describe the effect of the field.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> For the ADV7280-M I also have the diffs below applied. Do you think
> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?

Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
convention.

> In the RM it mentions YUV422 in the description of
> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
> wrong.

That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
retest.

> I know this is not really related to this patch. I'm just wondering
> how to properly support my setup.

It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
fields are not well documented, and neither is the CSI_CR18
MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
documented, how they're integrated isn't described. So far, I can only
guess.

> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -346,6 +346,11 @@ struct csis_pix_format {
>  
>  static const struct csis_pix_format mipi_csis_formats[] = {
>         /* YUV formats. */
> +       {
> +               .code = MEDIA_BUS_FMT_UYVY8_2X8,
> +               .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> +               .width = 8,
> +       },
>         {
>                 .code = MEDIA_BUS_FMT_UYVY8_1X16,
>                 .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> 
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
>         /* Color format */
>         val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>         val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> -       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
> +       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
>         mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>  
>         /* Pixel resolution */
> 
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>                 case MEDIA_BUS_FMT_UYVY8_1X16:
>                 case MEDIA_BUS_FMT_YUYV8_2X8:
>                 case MEDIA_BUS_FMT_YUYV8_1X16:
> -                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> +                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;

I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
you tried setting neither ?

Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
driver is the width value passed to v4l2_get_link_freq(). With
MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
equal to half of the actual value, and thus a wrong Ths_settle value. It
shuold have no other influence though.

>                         break;
>                 }
>         }
> 
> > ---
> >  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index f85a2f5f1413..256b9aa978f0 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  	int width = out_pix->width;
> >  	u32 stride = 0;
> >  	u32 cr1, cr18;
> > +	u32 cr3 = 0;
> >  
> >  	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> >  
> > @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG10_1X10:
> >  		case MEDIA_BUS_FMT_SGRBG10_1X10:
> >  		case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> >  			break;
> >  		case MEDIA_BUS_FMT_Y12_1X12:
> > @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG12_1X12:
> >  		case MEDIA_BUS_FMT_SGRBG12_1X12:
> >  		case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> >  			break;
> >  		case MEDIA_BUS_FMT_Y14_1X14:
> > @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  		case MEDIA_BUS_FMT_SGBRG14_1X14:
> >  		case MEDIA_BUS_FMT_SGRBG14_1X14:
> >  		case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +			cr3 |= BIT_TWO_8BIT_SENSOR;
> >  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> >  			break;
> >  		/*
> > @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >  
> >  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> >  	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> > -	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> > +	imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> >  	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
> >  
> >  	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> > 

-- 
Regards,

Laurent Pinchart

[RFC PATCH 2/3 v1.1] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Laurent Pinchart]

Sample code from NXP, as well as experiments on i.MX8MM with RAW10
capture with an OV5640 sensor connected over CSI-2, showed that the
TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
larger than 8 bits. Do so, even if the reference manual doesn't clearly
describe the effect of the field.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Initialize cr3 to BIT_FRMCNT_RST
---
 drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index f85a2f5f1413..5ae9ed1849e9 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -421,6 +421,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 	struct v4l2_pix_format *out_pix = &vdev->fmt;
 	int width = out_pix->width;
 	u32 stride = 0;
+	u32 cr3 = BIT_FRMCNT_RST;
 	u32 cr1, cr18;
 
 	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
@@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG10_1X10:
 		case MEDIA_BUS_FMT_SGRBG10_1X10:
 		case MEDIA_BUS_FMT_SRGGB10_1X10:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
 			break;
 		case MEDIA_BUS_FMT_Y12_1X12:
@@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG12_1X12:
 		case MEDIA_BUS_FMT_SGRBG12_1X12:
 		case MEDIA_BUS_FMT_SRGGB12_1X12:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
 			break;
 		case MEDIA_BUS_FMT_Y14_1X14:
@@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 		case MEDIA_BUS_FMT_SGBRG14_1X14:
 		case MEDIA_BUS_FMT_SGRBG14_1X14:
 		case MEDIA_BUS_FMT_SRGGB14_1X14:
+			cr3 |= BIT_TWO_8BIT_SENSOR;
 			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
 			break;
 		/*
@@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
 
 	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
 	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
-	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
+	imx7_csi_reg_write(csi, cr3, CSI_CSICR3);
 	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
 
 	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/3 v1.1] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Rui Miguel Silva]

Thanks Laurent,
On Wed May 19, 2021 at 1:23 AM WEST, Laurent Pinchart wrote:

> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

------
Cheers,
     Rui
> ---
> Changes since v1:
>
> - Initialize cr3 to BIT_FRMCNT_RST
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..5ae9ed1849e9 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -421,6 +421,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  	struct v4l2_pix_format *out_pix = &vdev->fmt;
>  	int width = out_pix->width;
>  	u32 stride = 0;
> +	u32 cr3 = BIT_FRMCNT_RST;
>  	u32 cr1, cr18;
>  
>  	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG10_1X10:
>  		case MEDIA_BUS_FMT_SGRBG10_1X10:
>  		case MEDIA_BUS_FMT_SRGGB10_1X10:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
>  			break;
>  		case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG12_1X12:
>  		case MEDIA_BUS_FMT_SGRBG12_1X12:
>  		case MEDIA_BUS_FMT_SRGGB12_1X12:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
>  			break;
>  		case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  		case MEDIA_BUS_FMT_SGBRG14_1X14:
>  		case MEDIA_BUS_FMT_SGRBG14_1X14:
>  		case MEDIA_BUS_FMT_SRGGB14_1X14:
> +			cr3 |= BIT_TWO_8BIT_SENSOR;
>  			cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
>  			break;
>  		/*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>  
>  	imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
>  	imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> -	imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> +	imx7_csi_reg_write(csi, cr3, CSI_CSICR3);
>  	imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>  
>  	imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> -- 
> Regards,
>
> Laurent Pinchart

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Frieder Schrempf]

Hi Laurent,

On 19.05.21 02:16, Laurent Pinchart wrote:
> Hi Frieder,
> 
> On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
>> On 16.05.21 04:42, Laurent Pinchart wrote:
>>> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
>>> capture with an OV5640 sensor connected over CSI-2, showed that the
>>> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
>>> larger than 8 bits. Do so, even if the reference manual doesn't clearly
>>> describe the effect of the field.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> For the ADV7280-M I also have the diffs below applied. Do you think
>> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
> 
> Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
> nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
> transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
> convention.

I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then maybe the ADV driver needs to be fixed.

> 
>> In the RM it mentions YUV422 in the description of
>> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
>> wrong.
> 
> That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
> don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
> retest.
> 
>> I know this is not really related to this patch. I'm just wondering
>> how to properly support my setup.
> 
> It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
> fields are not well documented, and neither is the CSI_CR18
> MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
> documented, how they're integrated isn't described. So far, I can only
> guess.
> 
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -346,6 +346,11 @@ struct csis_pix_format {
>>  
>>  static const struct csis_pix_format mipi_csis_formats[] = {
>>         /* YUV formats. */
>> +       {
>> +               .code = MEDIA_BUS_FMT_UYVY8_2X8,
>> +               .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>> +               .width = 8,
>> +       },
>>         {
>>                 .code = MEDIA_BUS_FMT_UYVY8_1X16,
>>                 .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>>
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
>>         /* Color format */
>>         val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>>         val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
>> -       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
>> +       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
>>         mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>>  
>>         /* Pixel resolution */
>>
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>>                 case MEDIA_BUS_FMT_UYVY8_1X16:
>>                 case MEDIA_BUS_FMT_YUYV8_2X8:
>>                 case MEDIA_BUS_FMT_YUYV8_1X16:
>> -                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>> +                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
> 
> I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
> you tried setting neither ?

Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors from the MIPI CSI2 controller and it doesn't work at all.

> 
> Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
> between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
> driver is the width value passed to v4l2_get_link_freq(). With
> MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
> equal to half of the actual value, and thus a wrong Ths_settle value. It
> shuold have no other influence though.

The link frequency calculation doesn't work for me at the moment, as the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle value.

Thanks for your efforts!
Frieder

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Laurent Pinchart]

Hi Frieder,

On Tue, May 25, 2021 at 11:59:40AM +0200, Frieder Schrempf wrote:
> On 19.05.21 02:16, Laurent Pinchart wrote:
> > On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
> >> On 16.05.21 04:42, Laurent Pinchart wrote:
> >>> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> >>> capture with an OV5640 sensor connected over CSI-2, showed that the
> >>> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> >>> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> >>> describe the effect of the field.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> For the ADV7280-M I also have the diffs below applied. Do you think
> >> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
> > 
> > Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
> > nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
> > transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
> > convention.
> 
> I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But
> if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then
> maybe the ADV driver needs to be fixed.

That would be the ideal situation. We need to be careful about not
breaking backward compatibility, but apart from that,
MEDIA_BUS_FMT_UYVY8_1X16 is the way to go.

I started writing a documentation patch, and found it it's already
documented :-) Documentation/userspace-api/media/v4l/subdev-formats.rst
states:

    The media bus pixel codes document parallel formats. Should the
    pixel data be transported over a serial bus, the media bus pixel
    code that describes a parallel format that transfers a sample on a
    single clock cycle is used. For instance, both
    MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used on
    parallel busses for transferring an 8 bits per sample BGR data,
    whereas on serial busses the data in this format is only referred to
    using MEDIA_BUS_FMT_BGR888_1X24. This is because there is
    effectively only a single way to transport that format on the serial
    busses.

> >> In the RM it mentions YUV422 in the description of
> >> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
> >> wrong.
> > 
> > That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
> > don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
> > retest.
> > 
> >> I know this is not really related to this patch. I'm just wondering
> >> how to properly support my setup.
> > 
> > It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
> > fields are not well documented, and neither is the CSI_CR18
> > MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
> > documented, how they're integrated isn't described. So far, I can only
> > guess.
> > 
> >> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> >> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> >> @@ -346,6 +346,11 @@ struct csis_pix_format {
> >>  
> >>  static const struct csis_pix_format mipi_csis_formats[] = {
> >>         /* YUV formats. */
> >> +       {
> >> +               .code = MEDIA_BUS_FMT_UYVY8_2X8,
> >> +               .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> >> +               .width = 8,
> >> +       },
> >>         {
> >>                 .code = MEDIA_BUS_FMT_UYVY8_1X16,
> >>                 .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> >>
> >> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> >> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> >> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
> >>         /* Color format */
> >>         val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
> >>         val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> >> -       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
> >> +       val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
> >>         mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
> >>  
> >>         /* Pixel resolution */
> >>
> >> --- a/drivers/staging/media/imx/imx7-media-csi.c
> >> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> >> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >>                 case MEDIA_BUS_FMT_UYVY8_1X16:
> >>                 case MEDIA_BUS_FMT_YUYV8_2X8:
> >>                 case MEDIA_BUS_FMT_YUYV8_1X16:
> >> -                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> >> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> >> +                       cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
> > 
> > I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
> > you tried setting neither ?
> 
> Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors
> from the MIPI CSI2 controller and it doesn't work at all.

What PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT do exactly is not totally
clear, we'll have to iron it out to ensure drivers can pick the right
settings automatically.

> > Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
> > between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
> > driver is the width value passed to v4l2_get_link_freq(). With
> > MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
> > equal to half of the actual value, and thus a wrong Ths_settle value. It
> > shuold have no other influence though.
> 
> The link frequency calculation doesn't work for me at the moment, as
> the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ
> or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle
> value.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Martin Kepplinger]

Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> formats
> larger than 8 bits. Do so, even if the reference manual doesn't
> clearly
> describe the effect of the field.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>         int width = out_pix->width;
>         u32 stride = 0;
>         u32 cr1, cr18;
> +       u32 cr3 = 0;
>  
>         cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>  
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG10_1X10:
>                 case MEDIA_BUS_FMT_SGRBG10_1X10:
>                 case MEDIA_BUS_FMT_SRGGB10_1X10:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
>                         break;
>                 case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG12_1X12:
>                 case MEDIA_BUS_FMT_SGRBG12_1X12:
>                 case MEDIA_BUS_FMT_SRGGB12_1X12:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
>                         break;
>                 case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>                 case MEDIA_BUS_FMT_SGBRG14_1X14:
>                 case MEDIA_BUS_FMT_SGRBG14_1X14:
>                 case MEDIA_BUS_FMT_SRGGB14_1X14:
> +                       cr3 |= BIT_TWO_8BIT_SENSOR;
>                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
>                         break;
>                 /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>  
>         imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
>         imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> CSI_CSICR2);
> -       imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> +       imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
>         imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>  
>         imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> CSI_CSIRXCNT);

this patch series looks good to me - I need it for the driver to run on
imx8mq.

Reviewed-by: Martin Kepplinger <martin.kepplinger@puri.sm>

Could you either rebase and resend as non-RFC or queue them up more
directly?

thank you,

                     martin

Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats

[Laurent Pinchart]

Hi Martin,

On Wed, Jul 28, 2021 at 10:54:23AM +0200, Martin Kepplinger wrote:
> Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> > Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> > capture with an OV5640 sensor connected over CSI-2, showed that the
> > TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> > formats
> > larger than 8 bits. Do so, even if the reference manual doesn't
> > clearly
> > describe the effect of the field.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> > b/drivers/staging/media/imx/imx7-media-csi.c
> > index f85a2f5f1413..256b9aa978f0 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >         int width = out_pix->width;
> >         u32 stride = 0;
> >         u32 cr1, cr18;
> > +       u32 cr3 = 0;
> >  
> >         cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> >  
> > @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >                 case MEDIA_BUS_FMT_SGBRG10_1X10:
> >                 case MEDIA_BUS_FMT_SGRBG10_1X10:
> >                 case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> >                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> >                         break;
> >                 case MEDIA_BUS_FMT_Y12_1X12:
> > @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >                 case MEDIA_BUS_FMT_SGBRG12_1X12:
> >                 case MEDIA_BUS_FMT_SGRBG12_1X12:
> >                 case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> >                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> >                         break;
> >                 case MEDIA_BUS_FMT_Y14_1X14:
> > @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >                 case MEDIA_BUS_FMT_SGBRG14_1X14:
> >                 case MEDIA_BUS_FMT_SGRBG14_1X14:
> >                 case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +                       cr3 |= BIT_TWO_8BIT_SENSOR;
> >                         cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> >                         break;
> >                 /*
> > @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >  
> >         imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> >         imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> > CSI_CSICR2);
> > -       imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> > +       imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> >         imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
> >  
> >         imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> > CSI_CSIRXCNT);
> 
> this patch series looks good to me - I need it for the driver to run on
> imx8mq.
> 
> Reviewed-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> 
> Could you either rebase and resend as non-RFC or queue them up more
> directly?

I've just sent a pull request that includes v1.1 of this patch. I'm
sorry I missed your Reviewed-by tag :-S

https://lore.kernel.org/linux-media/YQFY5FS8v3Y3KkEA@pendragon.ideasonboard.com/T/#u

-- 
Regards,

Laurent Pinchart