lkml.org 
[lkml]   [2018]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
    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@jmondi.org>
    > >>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
    > >>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
    > >>>>>>>>> ---
    > >>>>>>>>> .../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@ideasonboard.com>
    > >>>>>>>>
    > >>>>>>>>> +- 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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-03-27 12:10    [W:4.399 / U:0.552 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site