linux-kernel - Re: [RFC] clk: vc5: Add bindings for output configurations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHCN7xLoGUSOMtMC+uHMNbA+5MGT0dzTrwbL8J0RSMjmQ7G59g@mail.gmail.com>
Date:   Sat, 4 Apr 2020 20:38:15 -0500
From:   Adam Ford <aford173@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     devicetree <devicetree@...r.kernel.org>,
        Adam Ford-BE <aford@...conembedded.com>,
        Charles Stevens <charles.stevens@...icpd.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] clk: vc5: Add bindings for output configurations

On Sat, Apr 4, 2020 at 8:28 PM Rob Herring <robh@...nel.org> wrote:
>
> On Thu, Mar 26, 2020 at 04:32:51PM -0500, Adam Ford wrote:
> > The Versaclock can be purchased in a non-programmed configuration.
> > If that is the case, the driver needs to configure the chip to
> > output the correct signal type, voltage and slew.
> >
> > This RFC is proposing an additional binding to allow non-programmed
> > chips to be configured beyond their default configuration.
> >
> > Signed-off-by: Adam Ford <aford173@...il.com>
> >
> > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt
> > index 05a245c9df08..4bc46ed9ba4a 100644
> > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.txt
> > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.txt
> > @@ -30,6 +30,25 @@ Required properties:
> >               - 5p49v5933 and
> >               - 5p49v5935: (optional) property not present or "clkin".
> >
> > +For all output ports, an option child node can be used to specify:
> > +
> > +- mode: can be one of
> > +               - LVPECL: Low-voltage positive/psuedo emitter-coupled logic
> > +               - CMOS
> > +               - HCSL
> > +               - LVDS: Low voltage differential signal
> > +
> > +- voltage-level:  can be one of the following microvolts
> > +               - 1800000
> > +               - 2500000
> > +               - 3300000
> > +-  slew: Percent of normal, can be one of
> > +               - P80
> > +               - P85
> > +               - P90
> > +               - P100
> > +
> > +
> >  ==Mapping between clock specifier and physical pins==
> >
> >  When referencing the provided clock in the DT using phandle and
> > @@ -62,6 +81,8 @@ clock specifier, the following mapping applies:
> >
> >  ==Example==
> >
> > +#include <dt-bindings/versaclock.h>
> > +
> >  /* 25MHz reference crystal */
> >  ref25: ref25m {
> >       compatible = "fixed-clock";
> > @@ -80,6 +101,13 @@ i2c-master-node {
> >               /* Connect XIN input to 25MHz reference */
> >               clocks = <&ref25m>;
> >               clock-names = "xin";
> > +
> > +             ports@1 {
>
> 'ports' is already taken as a node name.
Rob,

The clock chip can drive multiple clocks and each output is
independent of the rest.  The idea is that port@1 would represent
output 1, port@2 would represent output 2, etc.
Is there a name you'd think we should use to represent each output?
Different variations of this chip can have different number of
outputs.

>
> > +                     reg = <1>;
>
> What do the reg value signify?

I am fine if we drop we drop it. I was under the assumption that reg
=<1> had to correspond to the port@1 and that it was required since
other devices with port sub-nodes use the reg entry.

>
> > +                     mode = <CMOS>;
> > +                     pwr_sel = <1800000>;
>
> Not documented. Don't use '-' in property names.

Do you have a preference to what name or convention you want us to use?
>

Thanks for the review.


adam

> > +                     slew = <P80>;
> > +             };
> >       };
> >  };
> >
> > --
> > 2.25.1
> >