Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[jacopo mondi]

Hi Vladimir,

On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 03/27/2018 11:57 AM, jacopo mondi wrote:
> > Hi Vladimir,
> >
> > On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
> >> Hi Sergei,
> >>
> >> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
> >>> Hello!
> >>>
> >>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
> >>> [...]
> >>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Niklas SÃderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++
> >>>>>>>>>   1 file changed, 66 insertions(+)
> >>>>>>>>>   create mode 100644
> >>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >>>>>>>>>
> >>>>>>>>> diff --git
> >>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 0000000..8225c6a
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++
> >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >>>>>>>>> @@ -0,0 +1,66 @@
> >>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder
> >>>>>>>>> +-------------------------------------------
> >>>>>>>>> +
> >>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> >>>>>>>>> streams
> >>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes,
> >>>>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
> >>>>>>>>> outputs.
> >>>>>>>>> +
> >>>>>>>>> +Single or dual operation modes, output data mapping and DDR output modes
> >>>>>>>>> are
> >>>>>>>>> +configured through input signals and the chip does not expose any control
> >>>>>>>>> bus.
> >>>>>>>>> +
> >>>>>>>>> +Required properties:
> >>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024"
> >>>>>>>>> +
> >>>>>>>>> +Optional properties:
> >>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
> >>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> >>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs
> >>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry
> >>>>>>>> As explained in a comment to one of the previous versions of this series, I'm
> >>>>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies
> >>>>>>>> for now, as I believe there's very little chance they will be connected to
> >>>>>>>> separately controllable regulators (all supplies use the same voltage). In the
> >>>>>>>> very unlikely event that this occurs in design we need to support in the
> >>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional
> >>>>>>>> without breaking backward compatibility.
> >>>>>>> I'm okay with that.
> >>>>>>>
> >>>>>>>> Apart from that,
> >>>>>>>>
> >>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>>>>>>>
> >>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low
> >>>>>>> powerdown-gpios is the semi-standard name.
> >>>>>>>
> >>>>>> right, I've also noticed it. If possible please avoid shortenings in
> >>>>>> property names.
> >>>>>
> >>>>> It is not shortening, it just follow pin name from decoder's datasheet.
> >>>>>
> >>>>>>
> >>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high
> >>>>>>>>> +
> >>>>>> And this one is also a not ever met property name, please consider to
> >>>>>> rename it to 'enable-gpios', for instance display panels define it.
> >>>>>
> >>>>>
> >>>>> Again, it follows datasheet naming scheme. Has something changed in DT
> >>>>> conventions?
> >>>>
> >>>> Seconded. My understanding is that the property name should reflect
> >>>> what reported in the the chip manual. For THC63LVD1024 the enable and
> >>>> power down pins are named 'OE' and 'PDWN' respectively.
> >>>
> >>>     But don't we need the vendor prefix in the prop names then, like
> >>> "renesas,oe-gpios" then?
> >>>
> >>
> >> Seconded, with a correction to "thine,oe-gpios".
> >>
> >
> > mmm, okay then...
> >
> > A grep for that semi-standard properties names in Documentation/
> > returns only usage examples and no actual definitions, so I assume this
> > is why they are semi-standard.
>
> Here we have to be specific about a particular property, let it be 'oe-gpios'
> vs. 'enable-gpios' and let's collect some statistics:
>
> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l
> 0
>
> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l
> 86
>
> While 'thine,oe-gpios' would be correct, I see no reason to introduce a vendor
> specific property to define a pin with a common and well understood purpose.
>
> If you go forward with the vendor specific prefix, apparently you can set the name
> to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the datasheet names
> the pin as "OE GPIO" or "OE connected to a GPIO"? I guess no.
>

Let me clarify I don't want to push for a vendor specific name or
similar, I'm fine with using 'semi-standard' names, I'm just confused
by the 'semi-standard' definition. I guess from your examples, the
usage count makes a difference here.

> Standards do not define '-gpios' suffix, but partially the description is found
> in Documentation/bindings/gpio/gpio.txt, still it is not a section in any
> standard as far as I know.

>
> > Seems like there is some tribal knowledge involved in defining what
> > is semi-standard and what's not, or are those properties documented somewhere?
> >
>
> The point is that there is no formal standard which describes every IP,
> every IC and every single their property, some device node names and property
> names are recommended in ePAPR and Devicetree Specification though.
>
> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST pin?) and
> 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. 'powerdown-gpios'.

I see all your points and I agree with most of them. Anyway, if the
chip manual describes a pin as 'RST' I would not find it confusing to
have a 'rst-gpio' defined in bindings :)

Let me be a bit pesky here: what if a chip defines a reset GPIO, which
is definitely a reset, but names it, say "XYZ" ? Would you prefer to
see it defined as "reset-gpios" for consistency with other bindings,
or "xyz-gpios" for consistency with documentation?

Thanks
   j
>
> >> If vendor agnostic properties are supposed to be used, then please follow
> >> the referenced by Rob semi-standard notations.
>
> --
> With best wishes,
> Vladimir

Attachment: signature.asc
Description: PGP signature