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

["Rui Miguel Silva" <rmfrfs@gmail.com>]

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