[PATCH v1 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[PATCH v1 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[Ken Sloat]

From: Ken Sloat <ksloat@aampglobal.com>

The ISC driver currently supports ITU-R 601 encoding which
utilizes the external hysync and vsync signals. ITU-R 656
format removes the need for these pins by embedding the
sync pulses within the data packet.

To support this feature, enable necessary register bits
when this feature is enabled via device tree.

Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
---
 drivers/media/platform/atmel/atmel-isc-regs.h | 2 ++
 drivers/media/platform/atmel/atmel-isc.c      | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
index 2aadc19235ea..8b6f4db15bdc 100644
--- a/drivers/media/platform/atmel/atmel-isc-regs.h
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -24,6 +24,8 @@
 #define ISC_PFE_CFG0_HPOL_LOW   BIT(0)
 #define ISC_PFE_CFG0_VPOL_LOW   BIT(1)
 #define ISC_PFE_CFG0_PPOL_LOW   BIT(2)
+#define ISC_PFE_CFG0_CCIR656    BIT(9)
+#define ISC_PFE_CFG0_CCIR_CRC   BIT(10)
 
 #define ISC_PFE_CFG0_MODE_PROGRESSIVE   (0x0 << 4)
 #define ISC_PFE_CFG0_MODE_MASK          GENMASK(6, 4)
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 50178968b8a6..9a399aa7ca92 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1095,7 +1095,8 @@ static int isc_configure(struct isc_device *isc)
 	pfe_cfg0  |= subdev->pfe_cfg0 | ISC_PFE_CFG0_MODE_PROGRESSIVE;
 	mask = ISC_PFE_CFG0_BPS_MASK | ISC_PFE_CFG0_HPOL_LOW |
 	       ISC_PFE_CFG0_VPOL_LOW | ISC_PFE_CFG0_PPOL_LOW |
-	       ISC_PFE_CFG0_MODE_MASK;
+	       ISC_PFE_CFG0_MODE_MASK | ISC_PFE_CFG0_CCIR_CRC |
+		   ISC_PFE_CFG0_CCIR656;
 
 	regmap_update_bits(regmap, ISC_PFE_CFG0, mask, pfe_cfg0);
 
@@ -2084,6 +2085,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 		if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
 
+		if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
+			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
+					ISC_PFE_CFG0_CCIR656;
+
 		subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
 		subdev_entity->asd->match.fwnode =
 			of_fwnode_handle(rem);
-- 
2.17.1

[PATCH v1 2/2] media: atmel-isc: Update device tree binding documentation

[Ken Sloat]

From: Ken Sloat <ksloat@aampglobal.com>

Update device tree binding documentation specifying how to
enable BT656 with CRC decoding.

Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
---
 Documentation/devicetree/bindings/media/atmel-isc.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
index bbe0e87c6188..e787edeea7da 100644
--- a/Documentation/devicetree/bindings/media/atmel-isc.txt
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -25,6 +25,9 @@ ISC supports a single port node with parallel bus. It should contain one
 'port' child node with child 'endpoint' node. Please refer to the bindings
 defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
 
+If all endpoint bus flags (i.e. hsync-active) are omitted, then CCIR656
+decoding (embedded sync) with CRC decoding is enabled.
+
 Example:
 isc: isc@f0008000 {
 	compatible = "atmel,sama5d2-isc";
-- 
2.17.1

Re: [PATCH v1 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[Eugen.Hristev]

On 28.12.2018 18:59, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> The ISC driver currently supports ITU-R 601 encoding which
> utilizes the external hysync and vsync signals. ITU-R 656
> format removes the need for these pins by embedding the
> sync pulses within the data packet.
> 
> To support this feature, enable necessary register bits
> when this feature is enabled via device tree.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
Acked-by: Eugen Hristev <eugen.hristev@microchip.com>

Also for my reference, which board and which sensor did you test this with ?

Thanks

> ---
>   drivers/media/platform/atmel/atmel-isc-regs.h | 2 ++
>   drivers/media/platform/atmel/atmel-isc.c      | 7 ++++++-
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 

Re: [PATCH v1 2/2] media: atmel-isc: Update device tree binding documentation

[Eugen.Hristev]

On 28.12.2018 18:59, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Update device tree binding documentation specifying how to
> enable BT656 with CRC decoding.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
Acked-by: Eugen Hristev <eugen.hristev@microchip.com>

> ---
>   Documentation/devicetree/bindings/media/atmel-isc.txt | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> index bbe0e87c6188..e787edeea7da 100644
> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -25,6 +25,9 @@ ISC supports a single port node with parallel bus. It should contain one
>   'port' child node with child 'endpoint' node. Please refer to the bindings
>   defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>   
> +If all endpoint bus flags (i.e. hsync-active) are omitted, then CCIR656
> +decoding (embedded sync) with CRC decoding is enabled.
> +
>   Example:
>   isc: isc@f0008000 {
>   	compatible = "atmel,sama5d2-isc";
> 

Re: [PATCH v1 2/2] media: atmel-isc: Update device tree binding documentation

[Hans Verkuil]

On 12/28/18 17:59, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Update device tree binding documentation specifying how to
> enable BT656 with CRC decoding.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> ---
>  Documentation/devicetree/bindings/media/atmel-isc.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> index bbe0e87c6188..e787edeea7da 100644
> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -25,6 +25,9 @@ ISC supports a single port node with parallel bus. It should contain one
>  'port' child node with child 'endpoint' node. Please refer to the bindings
>  defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>  
> +If all endpoint bus flags (i.e. hsync-active) are omitted, then CCIR656
> +decoding (embedded sync) with CRC decoding is enabled.

Sorry, this is wrong. There is a bus-type property defined in video-interfaces.txt
that you should use to determine whether this is a parallel or a Bt.656 bus.

BTW, for v2 also CC this to devicetree@vger.kernel.org, since it has to be reviewed
by the DT maintainers.

Regards,

	Hans

> +
>  Example:
>  isc: isc@f0008000 {
>  	compatible = "atmel,sama5d2-isc";
> 

Re: [PATCH v1 2/2] media: atmel-isc: Update device tree binding documentation

[Hans Verkuil]

On 01/08/19 14:44, Hans Verkuil wrote:
> On 12/28/18 17:59, Ken Sloat wrote:
>> From: Ken Sloat <ksloat@aampglobal.com>
>>
>> Update device tree binding documentation specifying how to
>> enable BT656 with CRC decoding.
>>
>> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
>> ---
>>  Documentation/devicetree/bindings/media/atmel-isc.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> index bbe0e87c6188..e787edeea7da 100644
>> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -25,6 +25,9 @@ ISC supports a single port node with parallel bus. It should contain one
>>  'port' child node with child 'endpoint' node. Please refer to the bindings
>>  defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>>  
>> +If all endpoint bus flags (i.e. hsync-active) are omitted, then CCIR656
>> +decoding (embedded sync) with CRC decoding is enabled.
> 
> Sorry, this is wrong. There is a bus-type property defined in video-interfaces.txt
> that you should use to determine whether this is a parallel or a Bt.656 bus.

Actually, that's what your code already does, so it seems this text in the bindings doc
is just plain wrong.

	Hans

> 
> BTW, for v2 also CC this to devicetree@vger.kernel.org, since it has to be reviewed
> by the DT maintainers.
> 
> Regards,
> 
> 	Hans
> 
>> +
>>  Example:
>>  isc: isc@f0008000 {
>>  	compatible = "atmel,sama5d2-isc";
>>
> 

RE: [PATCH v1 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[Ken Sloat]

> From: Eugen.Hristev@microchip.com <Eugen.Hristev@microchip.com>
> Sent: Monday, January 7, 2019 6:10 AM
> To: Ken Sloat <KSloat@aampglobal.com>
> Cc: mchehab@kernel.org; Nicolas.Ferre@microchip.com;
> alexandre.belloni@bootlin.com; Ludovic.Desroches@microchip.com; linux-
> media@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] media: atmel-isc: Add support for BT656 with CRC
> decoding
> 
> 
> 
> On 28.12.2018 18:59, Ken Sloat wrote:
> > From: Ken Sloat <ksloat@aampglobal.com>
> >
> > The ISC driver currently supports ITU-R 601 encoding which utilizes
> > the external hysync and vsync signals. ITU-R 656 format removes the
> > need for these pins by embedding the sync pulses within the data
> > packet.
> >
> > To support this feature, enable necessary register bits when this
> > feature is enabled via device tree.
> >
> > Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> Acked-by: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Also for my reference, which board and which sensor did you test this with ?
> 
> Thanks
> 
> > ---
> >   drivers/media/platform/atmel/atmel-isc-regs.h | 2 ++
> >   drivers/media/platform/atmel/atmel-isc.c      | 7 ++++++-
> >   2 files changed, 8 insertions(+), 1 deletion(-)
> >

Hi Eugen,

Sorry for my delayed reply. I tested this with a tw9990 sensor connected to a custom board based on the SAMA5D27-SOM1-EK1 board.

Thanks,
Ken

RE: [PATCH v1 2/2] media: atmel-isc: Update device tree binding documentation

[Ken Sloat]

> From: Hans Verkuil <hverkuil@xs4all.nl>
> Sent: Tuesday, January 8, 2019 8:46 AM
> To: Ken Sloat <KSloat@aampglobal.com>; eugen.hristev@microchip.com
> Cc: mchehab@kernel.org; nicolas.ferre@microchip.com;
> alexandre.belloni@bootlin.com; ludovic.desroches@microchip.com; linux-
> media@vger.kernel.org
> Subject: Re: [PATCH v1 2/2] media: atmel-isc: Update device tree binding
> documentation
> 
> On 01/08/19 14:44, Hans Verkuil wrote:
> > On 12/28/18 17:59, Ken Sloat wrote:
> >> From: Ken Sloat <ksloat@aampglobal.com>
> >>
> >> Update device tree binding documentation specifying how to enable
> >> BT656 with CRC decoding.
> >>
> >> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> >> ---
> >>  Documentation/devicetree/bindings/media/atmel-isc.txt | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> index bbe0e87c6188..e787edeea7da 100644
> >> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> @@ -25,6 +25,9 @@ ISC supports a single port node with parallel bus.
> >> It should contain one  'port' child node with child 'endpoint' node.
> >> Please refer to the bindings  defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>
> >> +If all endpoint bus flags (i.e. hsync-active) are omitted, then
> >> +CCIR656 decoding (embedded sync) with CRC decoding is enabled.
> >
> > Sorry, this is wrong. There is a bus-type property defined in
> > video-interfaces.txt that you should use to determine whether this is a
> parallel or a Bt.656 bus.
> 
> Actually, that's what your code already does, so it seems this text in the
> bindings doc is just plain wrong.
> 
> 	Hans
> 
> >
> > BTW, for v2 also CC this to devicetree@vger.kernel.org, since it has
> > to be reviewed by the DT maintainers.
> >
> > Regards,
> >
> > 	Hans
> >
> >> +
> >>  Example:
> >>  isc: isc@f0008000 {
> >>  	compatible = "atmel,sama5d2-isc";
> >>
> >

Hi Hans,

My apologies you are correct. The way I documented it here was the old way of doing it in the kernel but still worked for my setup as it appears the v4l2 subsystem still makes the assumption of 656 mode if these flags are all omitted. I will update with the proper "bus-type" property and resubmit here copying the dt list as well.

Thanks,
Ken