linux-kernel - Re: [PATCH v2 3/4] media: i2c: ov5640: Add support for BT656 mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ac104d73-eaaf-1199-d7a3-d1c6ccbc68c0@st.com>
Date:   Thu, 10 Sep 2020 10:09:31 +0000
From:   Hugues FRUCHET <hugues.fruchet@...com>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Biju Das <biju.das.jz@...renesas.com>,
        Prabhakar <prabhakar.csengg@...il.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] media: i2c: ov5640: Add support for BT656 mode


Hi Prabhakar,

I'm currently testing the OV5640 CCIR656 embedded synchronisation mode 
on STM32MP1 running STM32 DCMI camera interface.
Tests not yet fully completed but sounds good, more details below.

On 8/3/20 4:31 PM, Lad Prabhakar wrote:
> Enable support for BT656 mode.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
> ---
>   drivers/media/i2c/ov5640.c | 40 +++++++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index ec444bee2ce9..08c67250042f 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -82,6 +82,7 @@
>   #define OV5640_REG_VFIFO_HSIZE		0x4602
>   #define OV5640_REG_VFIFO_VSIZE		0x4604
>   #define OV5640_REG_JPG_MODE_SELECT	0x4713
> +#define OV5640_REG_CCIR656_CTRL00	0x4730
>   #define OV5640_REG_POLARITY_CTRL00	0x4740
>   #define OV5640_REG_MIPI_CTRL00		0x4800
>   #define OV5640_REG_DEBUG_MODE		0x4814
> @@ -1216,6 +1217,18 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>   			      BIT(1), on ? 0 : BIT(1));
>   }
>   
> +static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
> +{
> +	int ret;
> +
> +	ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00, on ? 0x1 : 0x00);
> +	if (ret)
> +		return ret;

Please add a comment explaining bit fields from datasheet:
Bit[7]: SYNC code selection
0: Automatically generate SYNC code
1: SYNC code from register setting 0x4732~4735
Bit[4:3]: Blank toggle data options
00: Toggle data is 1'h040/1'h200
Bit[1]: Clip data disable (so clip is enabled)
Bit[0]: CCIR656 mode enable

So 0x1 stands for CCIR656 with automatic SYNC codes: SAV/EAV with 
default values ie SAV=0xFF000080 & EAV=0xFF00009d.

On STM32 platform, this correspond to DCMI configuration ESCR=0xff9d80ff 
and ESUR=0xffffffff.
On Renesas platform, I have not seen any configuration in code, this 
SAV/EAV mode seems to be handled by default by hardware, do you confirm ?

Note that another CCIR656 embedded synchro mode could be used with 
custom synchro codes FS/FE/LS/LE in registers 0x4732-0x4735, this
mode is enabled with CCIR656_CTRL00(0x4730) set to 0x81:
Bit[7]: SYNC code selection
1: SYNC code from register setting 0x4732~4735

> +
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> +				OV5640_SOFTWARE_WAKEUP : OV5640_SOFTWARE_PWDN);
> +}
> +
>   static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>   {
>   	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> @@ -2022,18 +2035,20 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
>   	 *		datasheet and hardware, 0 is active high
>   	 *		and 1 is active low...)
>   	 */
> -	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -		pclk_pol = 1;
> -	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -		hsync_pol = 1;
> -	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -		vsync_pol = 1;
> +	if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
> +		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +			pclk_pol = 1;
> +		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +			hsync_pol = 1;
> +		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +			vsync_pol = 1;
>   
> -	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> -			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> +		ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> +				       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
>   
> -	if (ret)
> -		return ret;
> +		if (ret)
> +			return ret;
> +	}
>   
>   	/*
>   	 * powerdown MIPI TX/RX PHY & disable MIPI
> @@ -2057,7 +2072,8 @@ static int ov5640_set_dvp(struct ov5640_dev *sensor, bool on)
>   	 * - 4:		PCLK output enable
>   	 * - [3:0]:	D[9:6] output enable
>   	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
> +			       sensor->ep.bus_type == V4L2_MBUS_PARALLEL ? 0x7f : 0x1f);
>   	if (ret)
>   		return ret;
>   
> @@ -2911,6 +2927,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>   
>   		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
>   			ret = ov5640_set_stream_mipi(sensor, enable);
> +		else if (sensor->ep.bus_type == V4L2_MBUS_BT656)
> +			ret = ov5640_set_stream_bt656(sensor, enable);
>   		else
>   			ret = ov5640_set_stream_dvp(sensor, enable);
>   
> 

BR, Hugues.