Re: [RFC][PATCH] dt-bindings: iio: accel: add adi,adxl345.yaml binding

["Ardelean, Alexandru" <alexandru.Ardelean@analog.com>]

On Mon, 2019-05-06 at 09:17 -0500, Rob Herring wrote:
> [External]
> 
> 
> On Mon, May 6, 2019 at 6:46 AM Alexandru Ardelean
> <alexandru.ardelean@analog.com> wrote:
> > 
> > This patch adds a YAML binding for the Analog Devices ADXL345 I2C/SPI
> > accelerometer.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > 
> > And now for the RFC part.
> > Normally, I would dig into source-code to try to figure this out, but
> > at
> > this point-in-time, I am low on time/energy to do this.
> > And maybe this helps trigger a discussion about this.
> > 
> > Apologies if this has been coverted on the devicetree mailing list, but
> > at least we'd get some coverage on the IIO list (with this).
> > 
> > The ADXL345 device (as others) supports both I2C & SPI interfaces.
> > 
> > Question1: do we write 2 YAML files, or 1 ? I was looking at Zephyr
> > (for
> > some ideas/reference) but it seems to me that the YAML DT binding
> > format is
> > different than this one ? They write 2 files for ADXL372 (1 for SPI, 1
> > for
> > I2C).
> > 
> > Question1-a: one thing is that SPI requires some props to be `required`
> > that would not be required for the I2C binding. This could be solved by
> > doing 2 files, but if doing 1 YAML file, is there a way to do
> > conditional
> > `required` ? i.e. property is required if `SPI` ? not sure how to check
> > for
> > SPI vs I2C, it would be interesting (at some point) to somehow enforce
> > SPI/I2C bindings correctness.
> 
> The challenge here is there's not really any way for the schema to
> know which bus it is. The only ways to know this are knowing all
> possible spi or i2c controller compatibles or using the parent node
> name (which hasn't been strictly enforced). In order to get this
> information available to the schema, we'd need to add the information
> to the node. We do this with '$nodename'. We could add a '$bus'
> property for example. The tools would have to understand different
> buses and things like I2C muxes complicate doing that.
> 
> Once you have something like $bus available, you could either have 2
> files with a custom 'select' that checks compatible and $bus or we
> could have 1 file using if/then/else keywords. However, we don't yet
> support if/then/else json-schema that was added in draft7, but that's
> being worked on by Maxime Ripard.
> 
> However, for this case, I'd just not worry about the issue. Really,
> spi-cpha and spi-cpol should not be required. If only 1 mode is
> supported, the driver can know that. IOW, it is implied by the
> compatible strings.
> 
> 
> > Question2: `make dt_binding_check` seems to generate only the first
> > example. Is this known behavior, or do I need to take something else
> > into
> > consideration ?
> 
> That's correct. I haven't figured out how to do a variable number of
> examples in kbuild.

Then, would it be fine to have multiple examples, and wait for this to pop-
in the YAML dt stuff at a later point in time ?
Or, just 1 example ?

> 
> > Question3: one idea that was neat in Zephyr, is that there is a
> > 'inherits'
> > + `!include` mechanism for including common SPI & I2C device stuff. It
> > would be neat to have this. Is there a way to do this now, or maybe
> > this
> > would come later ? Maybe, just having a way to include a YAML file into
> > another would be interesting.
> 
> This is done with the 'allOf' keyword at the top-level. See
> i2c-gpio.yaml for an example.
> 
> I don't think that helps you here. If you had a long list of custom
> properties, then it might be useful. But that's orthogonal to your
> issue of needing conditional logic.
> 
> > 
> >  .../bindings/iio/accel/adi,adxl345.yaml       | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > new file mode 100644
> > index 000000000000..246b90c07aaa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: 
> > http://devicetree.org/schemas/iio/accelerometers/adi,adxl345.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADXL345/ADXL375 3-Axis Digital Accelerometers
> > +
> > +maintainers:
> > +  - Michael Hennerich <michael.hennerich@analog.com>
> > +
> > +description: |
> > +  Driver for Analog Devices ADXL345/ADXL375 3-Axis Digital
> > Accelerometers
> 
> This is not a driver.
> 
> > +    http://www.analog.com/en/products/mems/accelerometers/adxl345.html
> > +    
> > http://www.analog.com/en/products/sensors-mems/accelerometers/adxl375.html
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - adi,adxl345
> > +              - adi,adxl375
> 
> This can be simplified to just:
> 
> compatible:
>   enum:
>     - adi,adxl345
>     - adi,adxl375
> 
> 'oneOf' is only needed if you have different combinations of
> compatibles (a variable number or multiple fallbacks). 'items' is only
> needed if compatible is more than 1 string.
> 
> > +
> > +  reg:
> > +    description:
> > +      The I2C address or SPI chip select number of the sensor
> 
> The top-level description should probably say the device supports I2C
> and SPI. You don't need a description here for a common property
> unless you have something device specific to add.
> 
> > +    maxItems: 1
> > +
> > +  spi-cpha:
> > +    description:
> > +      SPI clock phase must be set, to select SPI mode 3
> 
> Again, standard property, so just 'spi-cpha: true' is sufficient.
> 
> > +
> > +  spi-cpol:
> > +    description:
> > +      SPI clock polarity must be set, to select SPI mode 3
> > +
> > +  interrupts:
> > +    description:
> > +      A variable number of interrupts warrants a description of what
> > conditions
> > +      affect the number of interrupts. Otherwise, descriptions on
> > standard
> > +      properties are not necessary.
> 
> This description from the example is explaining when you need a
> description. For a single interrupt, just need 'maxItems: 1'.

Yep, I noticed right after sending.
Sorry about that.
I think I started this file a few weeks ago, and came back from vacation a
bit of an airhead.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    /* Example for a I2C device node */
> > +    accelerometer@2a {
> 
> This should give you an error because the examples default to assuming
> a simple-bus. So you need an 'i2c' node around this.
> 
> > +         compatible = "adi,adxl345";
> > +         reg = <0x53>;
> > +    };
> > +  - |
> > +    /* Example for a SPI device node */
> > +    accelerometer@0 {
> > +         compatible = "adi,adxl345";
> > +         reg = <0>;
> > +         spi-max-frequency = <5000000>;
> > +         spi-cpol;
> > +         spi-cpha;
> > +    };
> > --
> > 2.17.1
> >