Re: [PATCH 2/2] staging: iio: ad2s1210: Add devicetree yaml doc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, May 19, 2019 at 8:18 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sat, 18 May 2019 19:15:58 -0300
> Tallys Martins <tallysmartins@xxxxxxxxx> wrote:
>

CC-ing my work-email
There are some issues with it and mailing lists; I'll hopefully sort
them out in the next weeks.

> > The driver currently has no devicetree documentation. This commit adds a
> > devicetree folder and documentation for it. Documentation must be moved
> > as well when the driver gets out of staging.
> >
> > Signed-off-by: Tallys Martins <tallysmartins@xxxxxxxxx>
> > Signed-off-by: Souza Guilherme <gdsdsilva@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Souza Guilherme <gdsdsilva@xxxxxxxxxxxxxxxx>
>
> Hi,
>
> There is no need for a direct relationship between a binding and a driver
> at all. As such, we normally don't take binding documents in staging.
>
> Just put it directly in it's final destination.  The driver can catch
> up with it later!
>
> I'm still not that comfortable with yaml (haven't gotten around
> to doing any conversions myself yet) so I'll be looking for additional
> review on these from others.

Personal note: I also don't like YAML, but that may be me feeling old.
I'm not that old to prefer XML, but old enough to prefer JSON.
I am still trying to accommodate my taste/feeling for the format, even
though I've been using it sporadically for ~5 years now in various
other elements.
Travis-CI may be the biggest user of it.
Looks like Ruby users were the biggest pushers/initiators of YAML, and
Python people seem to have jumped in shortly after.

Oh well: ¯\_(ツ)_/¯

>
> A few comments inline.
>
> > ---
> >  .../Documentation/devicetree/ad2s1210.yaml    | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml

Maybe also remove the old txt binding if adding this new one.
No need to keep them both.

> >
> > diff --git a/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml
> > new file mode 100644
> > index 000000000000..733aa07b4626
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/iio/ad2s1210.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: |
> > +  Analog Devices Inc. AD2S1210 10-Bit to 16-Bit R/D Converters
> > +
> > +maintainers:
> > +  - Graff Yang <graff.yang@xxxxxxxxx>
> I would check that one with the Analog devices team.

I guess, add (here):
Michael Hennerich <michael.hennerich@xxxxxxxxxx>

>
> > +
> > +description: |
> > +  Analog Devices AD2S1210 Resolver to Digital SPI driver
> > +
> > +  https://www.analog.com/en/products/ad2s1210.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad2s1210

These 2 are required here, since this chip operates in SPI mode 3.

spi-cpha:  true

spi-cpol: true

Also, from the driver, some GPIOs should also be documented:

static const struct ad2s1210_gpio gpios[] = {
        [AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
        [AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
        [AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
        [AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW },
        [AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW },
};

So,

adi,sample-gpios:
adi,a0-gpios:
adi,a1-gpios:
adi,res0-gpios:
adi,res10-gpios:

These also look like they are required.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    minimum: 2000
> > +    maximum: 20000
> > +    default: 8192
> This doesn't look like a modern clock binding.
> If we are going to end up changing this, then we should probably delay
> having a binding doc until after that change is made.
>
> We often only do binding docs for drivers in staging just before moving
> them out so as to avoid this sort of issue.

These clock properties are not needed here.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +  resolver@0 {
> > +    compatible = "adi,ad2s1210";
> > +    reg = <0>;
> An example is probably more useful if it includes all the optional properties
> as well.

This should also be included in an spi block:
So:

spi0 {
       resolver@0 {
            compatible = "adi,ad2s1210";
            reg = <0>;
            spi-cpha;
            spi-cpol;
            // and then the adi,***-gpios I think here
       };
};

I don't think much more-else is needed for this driver.

> > +  };
> > +...
>
> Thanks,
>
> Jonathan
>




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux