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 >
