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

-----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:

quoted

quoted

-----Original Message-----
From: Eugen.Hristev@xxxxxxxxxxxxx <redacted>
Sent: Friday, January 18, 2019 9:40 AM
To: Ken Sloat <redacted>
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:

quoted

From: Ken Sloat <redacted>

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

Signed-off-by: Ken Sloat <redacted>
---
  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.

quoted

quoted

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

quoted

  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).

quoted

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

support."

quoted

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