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@xxxxxxxxxx> wrote: > > > > This patch adds a YAML binding for the Analog Devices ADXL345 I2C/SPI > > accelerometer. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > --- > > > > 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@xxxxxxxxxx> > > + > > +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 > >
