[2/2] staging: iio: ad2s1210: Add devicetree yaml doc
diff mbox series

Message ID 20190518221558.21799-2-tallysmartins@gmail.com
State New
Headers show
Series
  • [1/2] staging: iio: ad2s1210: Destroy mutex at remove
Related show

Commit Message

Tallys Martins May 18, 2019, 10:15 p.m. UTC
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@gmail.com>
Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>
Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>
---
 .../Documentation/devicetree/ad2s1210.yaml    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml

Comments

Jonathan Cameron May 19, 2019, 11:17 a.m. UTC | #1
On Sat, 18 May 2019 19:15:58 -0300
Tallys Martins <tallysmartins@gmail.com> wrote:

> 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@gmail.com>
> Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>
> Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>

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.

A few comments inline.

> ---
>  .../Documentation/devicetree/ad2s1210.yaml    | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 drivers/staging/iio/Documentation/devicetree/ad2s1210.yaml
> 
> 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@gmail.com>
I would check that one with the Analog devices team.

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

Thanks,

Jonathan
Alexandru Ardelean May 20, 2019, 10:17 a.m. UTC | #2
On Sun, May 19, 2019 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 18 May 2019 19:15:58 -0300
> Tallys Martins <tallysmartins@gmail.com> 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@gmail.com>
> > Signed-off-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>
> > Co-developed-by: Souza Guilherme <gdsdsilva@inf.ufpel.edu.br>
>
> 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@gmail.com>
> I would check that one with the Analog devices team.

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

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

Patch
diff mbox series

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@gmail.com>
+
+description: |
+  Analog Devices AD2S1210 Resolver to Digital SPI driver
+
+  https://www.analog.com/en/products/ad2s1210.html
+
+properties:
+  compatible:
+    enum:
+      - adi,ad2s1210
+
+  reg:
+    maxItems: 1
+
+  clock-frequency:
+    minimum: 2000
+    maximum: 20000
+    default: 8192
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+  resolver@0 {
+    compatible = "adi,ad2s1210";
+    reg = <0>;
+  };
+...