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

[PATCH v2 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..d730693f299c 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 v2 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>
---
 Changes in v2:
 -Use correct media "bus-type" dt property.
 
 Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
index bbe0e87c6188..2d4378dfd6c8 100644
--- a/Documentation/devicetree/bindings/media/atmel-isc.txt
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -21,6 +21,11 @@ Required properties for ISC:
 - pinctrl-names, pinctrl-0
 	Please refer to pinctrl-bindings.txt.
 
+Optional properties for ISC:
+- bus-type
+	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
+	is enabled.
+
 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.
-- 
2.17.1

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

[Eugen.Hristev]

On 18.01.2019 16:28, 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>
> ---
>   Changes in v2:
>   -Use correct media "bus-type" dt property.
>   
>   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> index bbe0e87c6188..2d4378dfd6c8 100644
> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -21,6 +21,11 @@ Required properties for ISC:
>   - pinctrl-names, pinctrl-0
>   	Please refer to pinctrl-bindings.txt.
>   
> +Optional properties for ISC:
> +- bus-type
> +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> +	is enabled.
> +

I don't think this patch is required at all actually, the binding 
complies to the video-interfaces bus specification which includes the 
parallel and bt.656.

Would be worth mentioning below explicitly that parallel and bt.656 are 
supported, or added above that also plain parallel bus is supported ?

>   ISC supports a single port node with parallel bus. It should contain one

here inside the previous line

>   'port' child node with child 'endpoint' node. Please refer to the bindings
>   defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> 

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

[Eugen.Hristev]

On 18.01.2019 16:28, 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>
> ---
>   Changes in v2:
>   -Use correct media "bus-type" dt property.
>   
>   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> index bbe0e87c6188..2d4378dfd6c8 100644
> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -21,6 +21,11 @@ Required properties for ISC:
>   - pinctrl-names, pinctrl-0
>   	Please refer to pinctrl-bindings.txt.
>   
> +Optional properties for ISC:
> +- bus-type
> +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> +	is enabled.
> +

I don't think this patch is required at all actually, the binding 
complies to the video-interfaces bus specification which includes the 
parallel and bt.656.

Would be worth mentioning below explicitly that parallel and bt.656 are 
supported, or added above that also plain parallel bus is supported ?

>   ISC supports a single port node with parallel bus. It should contain one

here inside the previous line

>   'port' child node with child 'endpoint' node. Please refer to the bindings
>   defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> 

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

[Ken Sloat]

> -----Original Message-----
> From: Eugen.Hristev@microchip.com <Eugen.Hristev@microchip.com>
> Sent: Friday, January 18, 2019 9:40 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; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
> documentation
> 
> 
> 
> On 18.01.2019 16:28, 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>
> > ---
> >   Changes in v2:
> >   -Use correct media "bus-type" dt property.
> >
> >   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > index bbe0e87c6188..2d4378dfd6c8 100644
> > --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > @@ -21,6 +21,11 @@ Required properties for ISC:
> >   - pinctrl-names, pinctrl-0
> >   	Please refer to pinctrl-bindings.txt.
> >
> > +Optional properties for ISC:
> > +- bus-type
> > +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> > +	is enabled.
> > +
> 
> I don't think this patch is required at all actually, the binding complies to the
> video-interfaces bus specification which includes the parallel and bt.656.
> 
> Would be worth mentioning below explicitly that parallel and bt.656 are
> supported, or added above that also plain parallel bus is supported ?
> 
> >   ISC supports a single port node with parallel bus. It should contain
> > one
> 
> here inside the previous line
Hi Eugen,

Yes it's true adding new documentation here may be overkill, but yes it should say something
(as a user I always find it helpful if the docs are more verbose than not).

So per your suggestion, how about the simplified:
"ISC supports a single port node with parallel bus and optionally Bt.656 support."

and I'll remit the other statements.

> >   'port' child node with child 'endpoint' node. Please refer to the bindings
> >   defined in Documentation/devicetree/bindings/media/video-
> interfaces.txt.
> >

Thanks,
Ken

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

[Eugen.Hristev]

On 18.01.2019 20:05, Ken Sloat wrote:
>> -----Original Message-----
>> From: Eugen.Hristev@microchip.com <Eugen.Hristev@microchip.com>
>> Sent: Friday, January 18, 2019 9:40 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; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
>> documentation
>>
>>
>>
>> On 18.01.2019 16:28, 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>
>>> ---
>>>    Changes in v2:
>>>    -Use correct media "bus-type" dt property.
>>>
>>>    Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> index bbe0e87c6188..2d4378dfd6c8 100644
>>> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> @@ -21,6 +21,11 @@ Required properties for ISC:
>>>    - pinctrl-names, pinctrl-0
>>>    	Please refer to pinctrl-bindings.txt.
>>>
>>> +Optional properties for ISC:
>>> +- bus-type
>>> +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
>>> +	is enabled.
>>> +
>>
>> I don't think this patch is required at all actually, the binding complies to the
>> video-interfaces bus specification which includes the parallel and bt.656.
>>
>> Would be worth mentioning below explicitly that parallel and bt.656 are
>> supported, or added above that also plain parallel bus is supported ?
>>
>>>    ISC supports a single port node with parallel bus. It should contain
>>> one
>>
>> here inside the previous line
> Hi Eugen,
> 
> Yes it's true adding new documentation here may be overkill, but yes it should say something
> (as a user I always find it helpful if the docs are more verbose than not).
> 
> So per your suggestion, how about the simplified:
> "ISC supports a single port node with parallel bus and optionally Bt.656 support."
> 
> and I'll remit the other statements.

That's fine with me, I will let Rob have his opinion heard as well.

Thanks again,

Eugen

> 
>>>    'port' child node with child 'endpoint' node. Please refer to the bindings
>>>    defined in Documentation/devicetree/bindings/media/video-
>> interfaces.txt.
>>>
> 
> Thanks,
> Ken
> 

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

[Eugen.Hristev]

On 18.01.2019 20:05, Ken Sloat wrote:
>> -----Original Message-----
>> From: Eugen.Hristev@microchip.com <Eugen.Hristev@microchip.com>
>> Sent: Friday, January 18, 2019 9:40 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; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
>> documentation
>>
>>
>>
>> On 18.01.2019 16:28, 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>
>>> ---
>>>    Changes in v2:
>>>    -Use correct media "bus-type" dt property.
>>>
>>>    Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> index bbe0e87c6188..2d4378dfd6c8 100644
>>> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> @@ -21,6 +21,11 @@ Required properties for ISC:
>>>    - pinctrl-names, pinctrl-0
>>>    	Please refer to pinctrl-bindings.txt.
>>>
>>> +Optional properties for ISC:
>>> +- bus-type
>>> +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
>>> +	is enabled.
>>> +
>>
>> I don't think this patch is required at all actually, the binding complies to the
>> video-interfaces bus specification which includes the parallel and bt.656.
>>
>> Would be worth mentioning below explicitly that parallel and bt.656 are
>> supported, or added above that also plain parallel bus is supported ?
>>
>>>    ISC supports a single port node with parallel bus. It should contain
>>> one
>>
>> here inside the previous line
> Hi Eugen,
> 
> Yes it's true adding new documentation here may be overkill, but yes it should say something
> (as a user I always find it helpful if the docs are more verbose than not).
> 
> So per your suggestion, how about the simplified:
> "ISC supports a single port node with parallel bus and optionally Bt.656 support."
> 
> and I'll remit the other statements.

That's fine with me, I will let Rob have his opinion heard as well.

Thanks again,

Eugen

> 
>>>    'port' child node with child 'endpoint' node. Please refer to the bindings
>>>    defined in Documentation/devicetree/bindings/media/video-
>> interfaces.txt.
>>>
> 
> Thanks,
> Ken
> 

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

[Sakari Ailus]

On Fri, Jan 18, 2019 at 06:05:23PM +0000, Ken Sloat wrote:
> > -----Original Message-----
> > From: Eugen.Hristev@microchip.com <Eugen.Hristev@microchip.com>
> > Sent: Friday, January 18, 2019 9:40 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; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
> > documentation
> > 
> > 
> > 
> > On 18.01.2019 16:28, 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>
> > > ---
> > >   Changes in v2:
> > >   -Use correct media "bus-type" dt property.
> > >
> > >   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > index bbe0e87c6188..2d4378dfd6c8 100644
> > > --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > @@ -21,6 +21,11 @@ Required properties for ISC:
> > >   - pinctrl-names, pinctrl-0
> > >   	Please refer to pinctrl-bindings.txt.
> > >
> > > +Optional properties for ISC:
> > > +- bus-type
> > > +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> > > +	is enabled.
> > > +
> > 
> > I don't think this patch is required at all actually, the binding complies to the
> > video-interfaces bus specification which includes the parallel and bt.656.
> > 
> > Would be worth mentioning below explicitly that parallel and bt.656 are
> > supported, or added above that also plain parallel bus is supported ?
> > 
> > >   ISC supports a single port node with parallel bus. It should contain
> > > one
> > 
> > here inside the previous line
> Hi Eugen,
> 
> Yes it's true adding new documentation here may be overkill, but yes it should say something
> (as a user I always find it helpful if the docs are more verbose than not).
> 
> So per your suggestion, how about the simplified:
> "ISC supports a single port node with parallel bus and optionally Bt.656 support."
> 
> and I'll remit the other statements.

Please still include the name of the property, as well as the valid values
for it (numeric as well as human-readable). The rest of the documentation
should stay in video-interfaces.txt IMO --- this is documentation for the
hardware only.

-- 
Regards,

Sakari Ailus

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

[Ken Sloat]

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: Wednesday, January 23, 2019 7:46 AM
> Cc: Eugen.Hristev@microchip.com; mchehab@kernel.org;
> Nicolas.Ferre@microchip.com; alexandre.belloni@bootlin.com;
> Ludovic.Desroches@microchip.com; linux-media@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
> documentation
> 
> On Fri, Jan 18, 2019 at 06:05:23PM +0000, Ken Sloat wrote:
> > > -----Original Message-----
> > > From: Eugen.Hristev@xxxxxxxxxxxxx <Eugen.Hristev@xxxxxxxxxxxxx>
> > > Sent: Friday, January 18, 2019 9:40 AM
> > > To: Ken Sloat <KSloat@xxxxxxxxxxxxxx>
> > > Cc: mchehab@xxxxxxxxxx; Nicolas.Ferre@xxxxxxxxxxxxx;
> > > alexandre.belloni@xxxxxxxxxxx; Ludovic.Desroches@xxxxxxxxxxxxx;
> > > linux- media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree
> > > binding documentation
> > >
> > >
> > >
> > > On 18.01.2019 16:28, Ken Sloat wrote:
> > > > From: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> > > >
> > > > Update device tree binding documentation specifying how to enable
> > > > BT656 with CRC decoding.
> > > >
> > > > Signed-off-by: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> > > > ---
> > > >   Changes in v2:
> > > >   -Use correct media "bus-type" dt property.
> > > >
> > > >   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > index bbe0e87c6188..2d4378dfd6c8 100644
> > > > --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > @@ -21,6 +21,11 @@ Required properties for ISC:
> > > >   - pinctrl-names, pinctrl-0
> > > >   	Please refer to pinctrl-bindings.txt.
> > > >
> > > > +Optional properties for ISC:
> > > > +- bus-type
> > > > +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> > > > +	is enabled.
> > > > +
> > >
> > > I don't think this patch is required at all actually, the binding
> > > complies to the video-interfaces bus specification which includes the
> parallel and bt.656.
> > >
> > > Would be worth mentioning below explicitly that parallel and bt.656
> > > are supported, or added above that also plain parallel bus is supported ?
> > >
> > > >   ISC supports a single port node with parallel bus. It should
> > > > contain one
> > >
> > > here inside the previous line
> > Hi Eugen,
> >
> > Yes it's true adding new documentation here may be overkill, but yes
> > it should say something (as a user I always find it helpful if the docs are
> more verbose than not).
> >
> > So per your suggestion, how about the simplified:
> > "ISC supports a single port node with parallel bus and optionally Bt.656
> support."
> >
> > and I'll remit the other statements.
> 
> Please still include the name of the property, as well as the valid values for it
> (numeric as well as human-readable). The rest of the documentation should
> stay in video-interfaces.txt IMO --- this is documentation for the hardware
> only.
> 
> --
> Regards,
> 
> Sakari Ailus

Thanks Sakari for the feedback. So my original patch here would be valid as is correct?

Thanks,
Ken Sloat

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

[Sakari Ailus]

Hi Ken,

On Tue, Jan 29, 2019 at 08:22:48PM +0000, Ken Sloat wrote:
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > Sent: Wednesday, January 23, 2019 7:46 AM
> > Cc: Eugen.Hristev@microchip.com; mchehab@kernel.org;
> > Nicolas.Ferre@microchip.com; alexandre.belloni@bootlin.com;
> > Ludovic.Desroches@microchip.com; linux-media@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree binding
> > documentation
> > 
> > On Fri, Jan 18, 2019 at 06:05:23PM +0000, Ken Sloat wrote:
> > > > -----Original Message-----
> > > > From: Eugen.Hristev@xxxxxxxxxxxxx <Eugen.Hristev@xxxxxxxxxxxxx>
> > > > Sent: Friday, January 18, 2019 9:40 AM
> > > > To: Ken Sloat <KSloat@xxxxxxxxxxxxxx>
> > > > Cc: mchehab@xxxxxxxxxx; Nicolas.Ferre@xxxxxxxxxxxxx;
> > > > alexandre.belloni@xxxxxxxxxxx; Ludovic.Desroches@xxxxxxxxxxxxx;
> > > > linux- media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH v2 2/2] media: atmel-isc: Update device tree
> > > > binding documentation
> > > >
> > > >
> > > >
> > > > On 18.01.2019 16:28, Ken Sloat wrote:
> > > > > From: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> > > > >
> > > > > Update device tree binding documentation specifying how to enable
> > > > > BT656 with CRC decoding.
> > > > >
> > > > > Signed-off-by: Ken Sloat <ksloat@xxxxxxxxxxxxxx>
> > > > > ---
> > > > >   Changes in v2:
> > > > >   -Use correct media "bus-type" dt property.
> > > > >
> > > > >   Documentation/devicetree/bindings/media/atmel-isc.txt | 5 +++++
> > > > >   1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > > b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > > index bbe0e87c6188..2d4378dfd6c8 100644
> > > > > --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > > > > @@ -21,6 +21,11 @@ Required properties for ISC:
> > > > >   - pinctrl-names, pinctrl-0
> > > > >   	Please refer to pinctrl-bindings.txt.
> > > > >
> > > > > +Optional properties for ISC:
> > > > > +- bus-type
> > > > > +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> > > > > +	is enabled.
> > > > > +
> > > >
> > > > I don't think this patch is required at all actually, the binding
> > > > complies to the video-interfaces bus specification which includes the
> > parallel and bt.656.
> > > >
> > > > Would be worth mentioning below explicitly that parallel and bt.656
> > > > are supported, or added above that also plain parallel bus is supported ?
> > > >
> > > > >   ISC supports a single port node with parallel bus. It should
> > > > > contain one
> > > >
> > > > here inside the previous line
> > > Hi Eugen,
> > >
> > > Yes it's true adding new documentation here may be overkill, but yes
> > > it should say something (as a user I always find it helpful if the docs are
> > more verbose than not).
> > >
> > > So per your suggestion, how about the simplified:
> > > "ISC supports a single port node with parallel bus and optionally Bt.656
> > support."
> > >
> > > and I'll remit the other statements.
> > 
> > Please still include the name of the property, as well as the valid values for it
> > (numeric as well as human-readable). The rest of the documentation should
> > stay in video-interfaces.txt IMO --- this is documentation for the hardware
> > only.
> > 
> > --
> > Regards,
> > 
> > Sakari Ailus
> 
> Thanks Sakari for the feedback. So my original patch here would be valid
> as is correct?

To the original patch --- could you add that the default is the parallel
interface, if bus-type isn't set?

Documentation for hsync-active, vsync-active and pclk-sample properties is
also missing, it'd be nice to address that at the same time. I'd assume
they're mandatory for the parallel interface as no defaults are specified.

-- 
Regards,

Sakari Ailus