diff mbox series

iio: st-sensors: Update ST Sensor bindings

Message ID 20210412122331.1631643-1-linus.walleij@linaro.org
State Accepted
Commit 0cd71145803dc2b87b3afc8e2990ff0d43bf7027
Headers show
Series iio: st-sensors: Update ST Sensor bindings | expand

Commit Message

Linus Walleij April 12, 2021, 12:23 p.m. UTC
This adjusts the ST Sensor bindings with the more fine-grained
syntax checks that were proposed late in the last kernel cycle
and colliding with parallel work.

Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v1->v2:
- Use enum for the st,drdy-int-pin property.
- Drop GPIO DT include.
- Add an SPI example.
---
 .../bindings/iio/st,st-sensors.yaml           | 261 ++++++++++++------
 1 file changed, 183 insertions(+), 78 deletions(-)

-- 
2.29.2

Comments

Jonathan Cameron April 18, 2021, 10:09 a.m. UTC | #1
On Mon, 12 Apr 2021 14:23:31 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> This adjusts the ST Sensor bindings with the more fine-grained

> syntax checks that were proposed late in the last kernel cycle

> and colliding with parallel work.

> 

> Cc: devicetree@vger.kernel.org

> Reviewed-by: Rob Herring <robh@kernel.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v1->v2:

> - Use enum for the st,drdy-int-pin property.

> - Drop GPIO DT include.

> - Add an SPI example.

> ---

>  .../bindings/iio/st,st-sensors.yaml           | 261 ++++++++++++------

>  1 file changed, 183 insertions(+), 78 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml

> index 7e98f47987dc..497cb97042e0 100644

> --- a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml

> +++ b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml

> @@ -6,7 +6,9 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#

>  

>  title: STMicroelectronics MEMS sensors

>  

> -description: |

> +description: The STMicroelectronics sensor devices are pretty straight-forward

> +  I2C or SPI devices, all sharing the same device tree descriptions no matter

> +  what type of sensor it is.

>    Note that whilst this covers many STMicro MEMs sensors, some more complex

>    IMUs need their own bindings.

>    The STMicroelectronics sensor devices are pretty straight-forward I2C or

> @@ -15,90 +17,178 @@ description: |

>  

>  maintainers:

>    - Denis Ciocca <denis.ciocca@st.com>

> +  - Linus Walleij <linus.walleij@linaro.org>

>  

>  properties:

>    compatible:

> -    description: |

> -      Some values are deprecated.

> -      st,lis3lv02d (deprecated, use st,lis3lv02dl-accel)

> -      st,lis302dl-spi (deprecated, use st,lis3lv02dl-accel)

> -    enum:

> -        # Accelerometers

> -      - st,lis3lv02d

> -      - st,lis302dl-spi

> -      - st,lis3lv02dl-accel

> -      - st,lsm303dlh-accel

> -      - st,lsm303dlhc-accel

> -      - st,lis3dh-accel

> -      - st,lsm330d-accel

> -      - st,lsm330dl-accel

> -      - st,lsm330dlc-accel

> -      - st,lis331dl-accel

> -      - st,lis331dlh-accel

> -      - st,lsm303dl-accel

> -      - st,lsm303dlm-accel

> -      - st,lsm330-accel

> -      - st,lsm303agr-accel

> -      - st,lis2dh12-accel

> -      - st,h3lis331dl-accel

> -      - st,lng2dm-accel

> -      - st,lis3l02dq

> -      - st,lis2dw12

> -      - st,lis3dhh

> -      - st,lis3de

> -      - st,lis2de12

> -      - st,lis2hh12

> -        # Gyroscopes

> -      - st,l3g4200d-gyro

> -      - st,lsm330d-gyro

> -      - st,lsm330dl-gyro

> -      - st,lsm330dlc-gyro

> -      - st,l3gd20-gyro

> -      - st,l3gd20h-gyro

> -      - st,l3g4is-gyro

> -      - st,lsm330-gyro

> -      - st,lsm9ds0-gyro

> -        # Magnetometers

> -      - st,lsm303agr-magn

> -      - st,lsm303dlh-magn

> -      - st,lsm303dlhc-magn

> -      - st,lsm303dlm-magn

> -      - st,lis3mdl-magn

> -      - st,lis2mdl

> -      - st,lsm9ds1-magn

> -      - st,iis2mdc

> -        # Pressure sensors

> -      - st,lps001wp-press

> -      - st,lps25h-press

> -      - st,lps331ap-press

> -      - st,lps22hb-press

> -      - st,lps33hw

> -      - st,lps35hw

> -      - st,lps22hh

> +    oneOf:

> +      - description: STMicroelectronics Accelerometers

> +        enum:

> +          - st,h3lis331dl-accel

> +          - st,lis2de12

> +          - st,lis2dw12

> +          - st,lis2hh12

> +          - st,lis2dh12-accel

> +          - st,lis331dl-accel

> +          - st,lis331dlh-accel

> +          - st,lis3de

> +          - st,lis3dh-accel

> +          - st,lis3dhh

> +          - st,lis3l02dq

> +          - st,lis3lv02dl-accel

> +          - st,lng2dm-accel

> +          - st,lsm303agr-accel

> +          - st,lsm303dl-accel

> +          - st,lsm303dlh-accel

> +          - st,lsm303dlhc-accel

> +          - st,lsm303dlm-accel

> +          - st,lsm330-accel

> +          - st,lsm330d-accel

> +          - st,lsm330dl-accel

> +          - st,lsm330dlc-accel

> +      - description: STMicroelectronics Gyroscopes

> +        enum:

> +          - st,l3g4200d-gyro

> +          - st,l3g4is-gyro

> +          - st,l3gd20-gyro

> +          - st,l3gd20h-gyro

> +          - st,lsm330-gyro

> +          - st,lsm330d-gyro

> +          - st,lsm330dl-gyro

> +          - st,lsm330dlc-gyro

> +          - st,lsm9ds0-gyro

> +      - description: STMicroelectronics Magnetometers

> +        enum:

> +          - st,lis2mdl

> +          - st,lis3mdl-magn

> +          - st,lsm303agr-magn

> +          - st,lsm303dlh-magn

> +          - st,lsm303dlhc-magn

> +          - st,lsm303dlm-magn

> +          - st,lsm9ds1-magn

> +      - description: STMicroelectronics Pressure Sensors

> +        enum:

> +          - st,lps001wp-press

> +          - st,lps22hb-press

> +          - st,lps22hh

> +          - st,lps25h-press

> +          - st,lps331ap-press

> +          - st,lps33hw

> +          - st,lps35hw

> +      - description: Deprecated bindings

> +        enum:

> +          - st,lis302dl-spi

> +          - st,lis3lv02d

> +        deprecated: true

>  

>    reg:

>      maxItems: 1

>  

>    interrupts:

> +    description: interrupt line(s) connected to the DRDY line(s) and/or the

> +      Intertial interrupt lines INT1 and INT2 if these exist. This means up to

> +      three interrupts, and the DRDY must be the first one if it exists on

> +      the package. The trigger edge of the interrupts is sometimes software

> +      configurable in the hardware so the operating system should parse this

> +      flag and set up the trigger edge as indicated in the device tree.

>      minItems: 1

> +    maxItems: 2

>  

>    vdd-supply: true

>    vddio-supply: true

>  

>    st,drdy-int-pin:

> +    description: the pin on the package that will be used to signal

> +      "data ready" (valid values 1 or 2). This property is not configurable

> +      on all sensors.

>      $ref: /schemas/types.yaml#/definitions/uint32

> -    description:

> -      Some sensors have multiple possible pins via which they can provide

> -      a data ready interrupt.  This selects which one.

> -    enum:

> -      - 1

> -      - 2

> +    enum: [1, 2]

>  

>    drive-open-drain:

>      $ref: /schemas/types.yaml#/definitions/flag

> -    description: |

> -      The interrupt/data ready line will be configured as open drain, which

> -      is useful if several sensors share the same interrupt line.

> +    description: the interrupt/data ready line will be configured

> +      as open drain, which is useful if several sensors share the same

> +      interrupt line. (This binding is taken from pinctrl.)

> +

> +  mount-matrix:

> +    description: an optional 3x3 mounting rotation matrix.

> +

> +allOf:

> +  - if:

> +      properties:

> +        compatible:

> +          enum:

> +            # These have no interrupts

> +            - st,lps001wp

> +    then:

> +      properties:

> +        interrupts: false

> +        st,drdy-int-pin: false

> +        drive-open-drain: false

> +

> +  - if:

> +      properties:

> +        compatible:

> +          enum:

> +            # These have only DRDY

> +            - st,lis2mdl

> +            - st,lis3l02dq

> +            - st,lis3lv02dl-accel

> +            - st,lps22hb-press

> +            - st,lps22hh

> +            - st,lps25h-press

> +            - st,lps33hw

> +            - st,lps35hw

> +            - st,lsm303agr-magn

> +            - st,lsm303dlh-magn

> +            - st,lsm303dlhc-magn

> +            - st,lsm303dlm-magn

> +    then:

> +      properties:

> +        interrupts:

> +          maxItems: 1

> +        st,drdy-int-pin: false

> +

> +  - if:

> +      properties:

> +        compatible:

> +          enum:

> +            # Two intertial interrupts i.e. accelerometer/gyro interrupts

> +            - st,h3lis331dl-accel

> +            - st,l3g4200d-gyro

> +            - st,l3g4is-gyro

> +            - st,l3gd20-gyro

> +            - st,l3gd20h-gyro

> +            - st,lis2de12

> +            - st,lis2dw12

> +            - st,lis2hh12

> +            - st,lis2dh12-accel

> +            - st,lis331dl-accel

> +            - st,lis331dlh-accel

> +            - st,lis3de

> +            - st,lis3dh-accel

> +            - st,lis3dhh

> +            - st,lis3mdl-magn

> +            - st,lng2dm-accel

> +            - st,lps331ap-press

> +            - st,lsm303agr-accel

> +            - st,lsm303dlh-accel

> +            - st,lsm303dlhc-accel

> +            - st,lsm303dlm-accel

> +            - st,lsm330-accel

> +            - st,lsm330-gyro

> +            - st,lsm330d-accel

> +            - st,lsm330d-gyro

> +            - st,lsm330dl-accel

> +            - st,lsm330dl-gyro

> +            - st,lsm330dlc-accel

> +            - st,lsm330dlc-gyro

> +            - st,lsm9ds0-gyro

> +            - st,lsm9ds1-magn

> +    then:

> +      properties:

> +        interrupts:

> +          maxItems: 2

>  

>  required:

>    - compatible

> @@ -110,15 +200,30 @@ examples:

>    - |

>      #include <dt-bindings/interrupt-controller/irq.h>

>      i2c {

> -        #address-cells = <1>;

> -        #size-cells = <0>;

> -        accelerometer@1d {

> -            compatible = "st,lis3lv02dl-accel";

> -            reg = <0x1d>;

> -            interrupt-parent = <&gpio2>;

> -            interrupts = <18 IRQ_TYPE_EDGE_RISING>;

> -            pinctrl-0 = <&lis3lv02dl_nhk_mode>;

> -            pinctrl-names = "default";

> -        };

> +      #address-cells = <1>;

> +      #size-cells = <0>;

> +

> +      accelerometer@1c {

> +        compatible = "st,lis331dl-accel";

> +        reg = <0x1c>;

> +        st,drdy-int-pin = <1>;

> +        vdd-supply = <&ldo1>;

> +        vddio-supply = <&ldo2>;

> +        interrupt-parent = <&gpio>;

> +        interrupts = <18 IRQ_TYPE_EDGE_RISING>, <19 IRQ_TYPE_EDGE_RISING>;

> +      };

> +    };

> +    spi {

> +      #address-cells = <1>;

> +      #size-cells = <0>;

> +      num-cs = <1>;

> +

> +      l3g4200d: gyroscope@0 {

> +        compatible = "st,l3g4200d-gyro";

> +        st,drdy-int-pin = <2>;

> +        reg = <0>;

> +        vdd-supply = <&vcc_io>;

> +        vddio-supply = <&vcc_io>;

> +      };

>      };

> -...

> +

I tweaked this but otherwise applied to the togreg branch of iio.git and pushed
out as testing.  Thanks,

Jonathan
Maxime Ripard July 12, 2021, 1:04 p.m. UTC | #2
Hi,

On Mon, Apr 12, 2021 at 02:23:31PM +0200, Linus Walleij wrote:
> This adjusts the ST Sensor bindings with the more fine-grained

> syntax checks that were proposed late in the last kernel cycle

> and colliding with parallel work.

> 

> Cc: devicetree@vger.kernel.org

> Reviewed-by: Rob Herring <robh@kernel.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


I'm not really sure of how I supposed to fix this, but this creates an
issue on the Pinephone
(arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2.dts) that has a
LIS3MDL with only the DRDY pin routed and thus only has a single
interrupt in the DT.

One of the if condition in that patch enforces that there's two
interrupts for the LIS3MDL, but it's not really clear to me why after
looking at the datasheet?

Maxime
Jonathan Cameron July 12, 2021, 1:56 p.m. UTC | #3
On Mon, 12 Jul 2021 15:04:44 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

> Hi,

> 

> On Mon, Apr 12, 2021 at 02:23:31PM +0200, Linus Walleij wrote:

> > This adjusts the ST Sensor bindings with the more fine-grained

> > syntax checks that were proposed late in the last kernel cycle

> > and colliding with parallel work.

> > 

> > Cc: devicetree@vger.kernel.org

> > Reviewed-by: Rob Herring <robh@kernel.org>

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>  

> 

> I'm not really sure of how I supposed to fix this, but this creates an

> issue on the Pinephone

> (arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2.dts) that has a

> LIS3MDL with only the DRDY pin routed and thus only has a single

> interrupt in the DT.

> 

> One of the if condition in that patch enforces that there's two

> interrupts for the LIS3MDL, but it's not really clear to me why after

> looking at the datasheet?


It shouldn't be enforcing that 2 are specified rather that 2 'might' be
specified.  maxItems is set, but not minItems. 

Driver wise, at the moment it looks like we only handle one interrupt.
So to handle selection when two are possible and either 1 or 2 might
be wired up we need to add interrupt names (with default order so we
don't break anything before adding them to the binding).

Would that work for this device?

Jonathan
 
> 

> Maxime

>
Maxime Ripard July 12, 2021, 2:16 p.m. UTC | #4
On Mon, Jul 12, 2021 at 02:56:39PM +0100, Jonathan Cameron wrote:
> On Mon, 12 Jul 2021 15:04:44 +0200

> Maxime Ripard <maxime@cerno.tech> wrote:

> 

> > Hi,

> > 

> > On Mon, Apr 12, 2021 at 02:23:31PM +0200, Linus Walleij wrote:

> > > This adjusts the ST Sensor bindings with the more fine-grained

> > > syntax checks that were proposed late in the last kernel cycle

> > > and colliding with parallel work.

> > > 

> > > Cc: devicetree@vger.kernel.org

> > > Reviewed-by: Rob Herring <robh@kernel.org>

> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>  

> > 

> > I'm not really sure of how I supposed to fix this, but this creates an

> > issue on the Pinephone

> > (arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2.dts) that has a

> > LIS3MDL with only the DRDY pin routed and thus only has a single

> > interrupt in the DT.

> > 

> > One of the if condition in that patch enforces that there's two

> > interrupts for the LIS3MDL, but it's not really clear to me why after

> > looking at the datasheet?

> 

> It shouldn't be enforcing that 2 are specified rather that 2 'might' be

> specified.


But then you don't need that condition at all, it's already what is
being enforced by the main schema here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/st,st-sensors.yaml#n90

> maxItems is set, but not minItems. 


Yeah, and if one is missing the other is added with the value of the
other.

What the schema enforces currently is that (for the common part) the
interrupt list can be between 1 and 2 and then for a specific set of
compatibles (including the LIS3MDL) it has to be exactly 2.

Even the common part looks weird though, it says that it can handle up
to three interrupts but has maxItems: 2?

> Driver wise, at the moment it looks like we only handle one interrupt.

> So to handle selection when two are possible and either 1 or 2 might

> be wired up we need to add interrupt names (with default order so we

> don't break anything before adding them to the binding).

> 

> Would that work for this device?


I don't know the LIS3MDL to comment whether it makes sense or not, but
it looks like it's a single sensor so I'm not really sure why we'd need
more than one interrupt

Maxime
Jonathan Cameron July 12, 2021, 2:28 p.m. UTC | #5
On Mon, 12 Jul 2021 16:16:13 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

> On Mon, Jul 12, 2021 at 02:56:39PM +0100, Jonathan Cameron wrote:

> > On Mon, 12 Jul 2021 15:04:44 +0200

> > Maxime Ripard <maxime@cerno.tech> wrote:

> >   

> > > Hi,

> > > 

> > > On Mon, Apr 12, 2021 at 02:23:31PM +0200, Linus Walleij wrote:  

> > > > This adjusts the ST Sensor bindings with the more fine-grained

> > > > syntax checks that were proposed late in the last kernel cycle

> > > > and colliding with parallel work.

> > > > 

> > > > Cc: devicetree@vger.kernel.org

> > > > Reviewed-by: Rob Herring <robh@kernel.org>

> > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>    

> > > 

> > > I'm not really sure of how I supposed to fix this, but this creates an

> > > issue on the Pinephone

> > > (arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2.dts) that has a

> > > LIS3MDL with only the DRDY pin routed and thus only has a single

> > > interrupt in the DT.

> > > 

> > > One of the if condition in that patch enforces that there's two

> > > interrupts for the LIS3MDL, but it's not really clear to me why after

> > > looking at the datasheet?  

> > 

> > It shouldn't be enforcing that 2 are specified rather that 2 'might' be

> > specified.  

> 

> But then you don't need that condition at all, it's already what is

> being enforced by the main schema here:

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/st,st-sensors.yaml#n90


Good point.  I'd argue that we should drop this entry infavour or explicit match
on one of the others, but perhaps that gives an error?

> 

> > maxItems is set, but not minItems.   

> 

> Yeah, and if one is missing the other is added with the value of the

> other.


Gah.  Indeed, not good and needs fixing.

> 

> What the schema enforces currently is that (for the common part) the

> interrupt list can be between 1 and 2 and then for a specific set of

> compatibles (including the LIS3MDL) it has to be exactly 2.

> 

> Even the common part looks weird though, it says that it can handle up

> to three interrupts but has maxItems: 2?


That is indeed odd and I expect an omission on the assumption that the minItems
from the general one would not be overridden. 

@Linus?

> 

> > Driver wise, at the moment it looks like we only handle one interrupt.

> > So to handle selection when two are possible and either 1 or 2 might

> > be wired up we need to add interrupt names (with default order so we

> > don't break anything before adding them to the binding).

> > 

> > Would that work for this device?  

> 

> I don't know the LIS3MDL to comment whether it makes sense or not, but

> it looks like it's a single sensor so I'm not really sure why we'd need

> more than one interrupt


Looks like they are hard wired to specific functions.  Data ready does
what it says on the tin, but the INT line is used for threshold events.
Depending on the application a particular device is being used for, it
might well make sense to only wire either one of them.

> 

> Maxime

>
Linus Walleij July 14, 2021, 8:26 a.m. UTC | #6
On Mon, Jul 12, 2021 at 4:16 PM Maxime Ripard <maxime@cerno.tech> wrote:

> > maxItems is set, but not minItems.

>

> Yeah, and if one is missing the other is added with the value of the

> other.

>

> What the schema enforces currently is that (for the common part) the

> interrupt list can be between 1 and 2 and then for a specific set of

> compatibles (including the LIS3MDL) it has to be exactly 2.


maxItems is not an intuitive naming to what it does so it creates
bugs like this :/

Can you fix so it works with your PinePhone DTS and send a patch?
Perhaps also add as an example so it doesn't happen again?

> Even the common part looks weird though, it says that it can handle up

> to three interrupts but has maxItems: 2?


Maybe just drop maxItems for now?

Yours,
Linus Walleij
Maxime Ripard July 15, 2021, 12:21 p.m. UTC | #7
On Wed, Jul 14, 2021 at 10:26:39AM +0200, Linus Walleij wrote:
> On Mon, Jul 12, 2021 at 4:16 PM Maxime Ripard <maxime@cerno.tech> wrote:

> 

> > > maxItems is set, but not minItems.

> >

> > Yeah, and if one is missing the other is added with the value of the

> > other.

> >

> > What the schema enforces currently is that (for the common part) the

> > interrupt list can be between 1 and 2 and then for a specific set of

> > compatibles (including the LIS3MDL) it has to be exactly 2.

> 

> maxItems is not an intuitive naming to what it does so it creates

> bugs like this :/


I mean, it's complicated. Both minItems and maxItems are required for
all the items in the schema spec. In order to reduce the boilerplate,
the tooling will add the other if one is missing, which can lead to
things like this indeed.

But the alternative is not really to just optionally use minItems, it's
to have to always specify it, in all the schemas.

> Can you fix so it works with your PinePhone DTS and send a patch?

> Perhaps also add as an example so it doesn't happen again?


Yeah, I can definitely do that, I'm not really sure what makes sense for
the driver at this point though.

> > Even the common part looks weird though, it says that it can handle up

> > to three interrupts but has maxItems: 2?

> 

> Maybe just drop maxItems for now?


Dropping minItems and maxItems on interrupts will enforce that there's
only one interrupt, which isn't want we want either

Maxime
Maxime Ripard July 15, 2021, 12:29 p.m. UTC | #8
On Mon, Jul 12, 2021 at 03:28:02PM +0100, Jonathan Cameron wrote:
> On Mon, 12 Jul 2021 16:16:13 +0200

> Maxime Ripard <maxime@cerno.tech> wrote:

> 

> > On Mon, Jul 12, 2021 at 02:56:39PM +0100, Jonathan Cameron wrote:

> > > On Mon, 12 Jul 2021 15:04:44 +0200

> > > Maxime Ripard <maxime@cerno.tech> wrote:

> > >   

> > > > Hi,

> > > > 

> > > > On Mon, Apr 12, 2021 at 02:23:31PM +0200, Linus Walleij wrote:  

> > > > > This adjusts the ST Sensor bindings with the more fine-grained

> > > > > syntax checks that were proposed late in the last kernel cycle

> > > > > and colliding with parallel work.

> > > > > 

> > > > > Cc: devicetree@vger.kernel.org

> > > > > Reviewed-by: Rob Herring <robh@kernel.org>

> > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>    

> > > > 

> > > > I'm not really sure of how I supposed to fix this, but this creates an

> > > > issue on the Pinephone

> > > > (arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone-1.2.dts) that has a

> > > > LIS3MDL with only the DRDY pin routed and thus only has a single

> > > > interrupt in the DT.

> > > > 

> > > > One of the if condition in that patch enforces that there's two

> > > > interrupts for the LIS3MDL, but it's not really clear to me why after

> > > > looking at the datasheet?  

> > > 

> > > It shouldn't be enforcing that 2 are specified rather that 2 'might' be

> > > specified.  

> > 

> > But then you don't need that condition at all, it's already what is

> > being enforced by the main schema here:

> > 

> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/iio/st,st-sensors.yaml#n90

> 

> Good point.  I'd argue that we should drop this entry infavour or explicit match

> on one of the others, but perhaps that gives an error?


in an if statement, the schema under then will only be enforced if the
schema under the if is valid. In our case, if we remove the entry like
you suggested, we won't match in any if schema, so the schemas under
then will get ignored, which is what we want.

> > 

> > > maxItems is set, but not minItems.   

> > 

> > Yeah, and if one is missing the other is added with the value of the

> > other.

> 

> Gah.  Indeed, not good and needs fixing.

> 

> > What the schema enforces currently is that (for the common part) the

> > interrupt list can be between 1 and 2 and then for a specific set of

> > compatibles (including the LIS3MDL) it has to be exactly 2.

> > 

> > Even the common part looks weird though, it says that it can handle up

> > to three interrupts but has maxItems: 2?

> 

> That is indeed odd and I expect an omission on the assumption that the minItems

> from the general one would not be overridden. 

>

> @Linus?


As far as schemas work, you can't override a schema with another. They
are all validated in isolation from each other, and they all have to be
valid if you don't want any error.

So the main, global, schema will be validated, and then the smaller
schemas under then will be (if the schemas under their respective if
clause are valid). So it's really as if it was in a separate file
entirely.

> > 

> > > Driver wise, at the moment it looks like we only handle one interrupt.

> > > So to handle selection when two are possible and either 1 or 2 might

> > > be wired up we need to add interrupt names (with default order so we

> > > don't break anything before adding them to the binding).

> > > 

> > > Would that work for this device?  

> > 

> > I don't know the LIS3MDL to comment whether it makes sense or not, but

> > it looks like it's a single sensor so I'm not really sure why we'd need

> > more than one interrupt

> 

> Looks like they are hard wired to specific functions.  Data ready does

> what it says on the tin, but the INT line is used for threshold events.

> Depending on the application a particular device is being used for, it

> might well make sense to only wire either one of them.


Ok, I'll just remove the if clause then

Thanks!
Maxime
Geert Uytterhoeven Jan. 26, 2022, 2:59 p.m. UTC | #9
Hi Linus,

On Mon, Apr 12, 2021 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> This adjusts the ST Sensor bindings with the more fine-grained
> syntax checks that were proposed late in the last kernel cycle
> and colliding with parallel work.
>
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for your patch, which is now commit 0cd71145803dc2b8
("iio: st-sensors: Update ST Sensor bindings") in v5.14.

> --- a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
> +++ b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml

>    interrupts:
> +    description: interrupt line(s) connected to the DRDY line(s) and/or the
> +      Intertial interrupt lines INT1 and INT2 if these exist. This means up to
> +      three interrupts, and the DRDY must be the first one if it exists on

So this says three (the LSM9DS0 datasheet agrees)...

> +      the package. The trigger edge of the interrupts is sometimes software
> +      configurable in the hardware so the operating system should parse this
> +      flag and set up the trigger edge as indicated in the device tree.
>      minItems: 1
> +    maxItems: 2

... while this says two?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Linus Walleij Jan. 28, 2022, 3:51 p.m. UTC | #10
On Wed, Jan 26, 2022 at 3:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Apr 12, 2021 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> >    interrupts:
> > +    description: interrupt line(s) connected to the DRDY line(s) and/or the
> > +      Intertial interrupt lines INT1 and INT2 if these exist. This means up to
> > +      three interrupts, and the DRDY must be the first one if it exists on
>
> So this says three (the LSM9DS0 datasheet agrees)...
>
> > +      the package. The trigger edge of the interrupts is sometimes software
> > +      configurable in the hardware so the operating system should parse this
> > +      flag and set up the trigger edge as indicated in the device tree.
> >      minItems: 1
> > +    maxItems: 2
>
> ... while this says two?

Looks like a bug, could you send a patch? (I'm a bit preoccupied right now.)

Thanks!
Linus Walleij
Linus Walleij Jan. 28, 2022, 3:57 p.m. UTC | #11
On Fri, Jan 28, 2022 at 4:51 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jan 26, 2022 at 3:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Apr 12, 2021 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > >    interrupts:
> > > +    description: interrupt line(s) connected to the DRDY line(s) and/or the
> > > +      Intertial interrupt lines INT1 and INT2 if these exist. This means up to
> > > +      three interrupts, and the DRDY must be the first one if it exists on
> >
> > So this says three (the LSM9DS0 datasheet agrees)...
> >
> > > +      the package. The trigger edge of the interrupts is sometimes software
> > > +      configurable in the hardware so the operating system should parse this
> > > +      flag and set up the trigger edge as indicated in the device tree.
> > >      minItems: 1
> > > +    maxItems: 2
> >
> > ... while this says two?
>
> Looks like a bug, could you send a patch? (I'm a bit preoccupied right now.)

Oh wait a minute, LSM9DS0 is one of those with more than one component
inside it isn't it?

While it is a bit awkward, we do bindings per-subcomponent on these, so
for example lsm330dlc registers as "st,lsm330dlc-accel" and "st,lsm330dlc-gyro"
and it makes a bit of sense because they each have different I2C addresses
as well.

I see it as two components just sharing a physical package rather than one
component in a package.

So the IRQs are per-subcomponent, not for the entire package.

Does this influence the situation you have with LSM9DS0?

Yours,
Linus Walleij
Geert Uytterhoeven Jan. 28, 2022, 4:14 p.m. UTC | #12
Hi Linus,

On Fri, Jan 28, 2022 at 4:57 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 28, 2022 at 4:51 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Jan 26, 2022 at 3:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Apr 12, 2021 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > > >    interrupts:
> > > > +    description: interrupt line(s) connected to the DRDY line(s) and/or the
> > > > +      Intertial interrupt lines INT1 and INT2 if these exist. This means up to
> > > > +      three interrupts, and the DRDY must be the first one if it exists on
> > >
> > > So this says three (the LSM9DS0 datasheet agrees)...
> > >
> > > > +      the package. The trigger edge of the interrupts is sometimes software
> > > > +      configurable in the hardware so the operating system should parse this
> > > > +      flag and set up the trigger edge as indicated in the device tree.
> > > >      minItems: 1
> > > > +    maxItems: 2
> > >
> > > ... while this says two?
> >
> > Looks like a bug, could you send a patch? (I'm a bit preoccupied right now.)
>
> Oh wait a minute, LSM9DS0 is one of those with more than one component
> inside it isn't it?

Yes it is. And thus it needs 2 device nodes in DT.

> While it is a bit awkward, we do bindings per-subcomponent on these, so
> for example lsm330dlc registers as "st,lsm330dlc-accel" and "st,lsm330dlc-gyro"
> and it makes a bit of sense because they each have different I2C addresses
> as well.
>
> I see it as two components just sharing a physical package rather than one
> component in a package.
>
> So the IRQs are per-subcomponent, not for the entire package.

OK, that makes sense.

> Does this influence the situation you have with LSM9DS0?

Yes, it does. Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
index 7e98f47987dc..497cb97042e0 100644
--- a/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
+++ b/Documentation/devicetree/bindings/iio/st,st-sensors.yaml
@@ -6,7 +6,9 @@  $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: STMicroelectronics MEMS sensors
 
-description: |
+description: The STMicroelectronics sensor devices are pretty straight-forward
+  I2C or SPI devices, all sharing the same device tree descriptions no matter
+  what type of sensor it is.
   Note that whilst this covers many STMicro MEMs sensors, some more complex
   IMUs need their own bindings.
   The STMicroelectronics sensor devices are pretty straight-forward I2C or
@@ -15,90 +17,178 @@  description: |
 
 maintainers:
   - Denis Ciocca <denis.ciocca@st.com>
+  - Linus Walleij <linus.walleij@linaro.org>
 
 properties:
   compatible:
-    description: |
-      Some values are deprecated.
-      st,lis3lv02d (deprecated, use st,lis3lv02dl-accel)
-      st,lis302dl-spi (deprecated, use st,lis3lv02dl-accel)
-    enum:
-        # Accelerometers
-      - st,lis3lv02d
-      - st,lis302dl-spi
-      - st,lis3lv02dl-accel
-      - st,lsm303dlh-accel
-      - st,lsm303dlhc-accel
-      - st,lis3dh-accel
-      - st,lsm330d-accel
-      - st,lsm330dl-accel
-      - st,lsm330dlc-accel
-      - st,lis331dl-accel
-      - st,lis331dlh-accel
-      - st,lsm303dl-accel
-      - st,lsm303dlm-accel
-      - st,lsm330-accel
-      - st,lsm303agr-accel
-      - st,lis2dh12-accel
-      - st,h3lis331dl-accel
-      - st,lng2dm-accel
-      - st,lis3l02dq
-      - st,lis2dw12
-      - st,lis3dhh
-      - st,lis3de
-      - st,lis2de12
-      - st,lis2hh12
-        # Gyroscopes
-      - st,l3g4200d-gyro
-      - st,lsm330d-gyro
-      - st,lsm330dl-gyro
-      - st,lsm330dlc-gyro
-      - st,l3gd20-gyro
-      - st,l3gd20h-gyro
-      - st,l3g4is-gyro
-      - st,lsm330-gyro
-      - st,lsm9ds0-gyro
-        # Magnetometers
-      - st,lsm303agr-magn
-      - st,lsm303dlh-magn
-      - st,lsm303dlhc-magn
-      - st,lsm303dlm-magn
-      - st,lis3mdl-magn
-      - st,lis2mdl
-      - st,lsm9ds1-magn
-      - st,iis2mdc
-        # Pressure sensors
-      - st,lps001wp-press
-      - st,lps25h-press
-      - st,lps331ap-press
-      - st,lps22hb-press
-      - st,lps33hw
-      - st,lps35hw
-      - st,lps22hh
+    oneOf:
+      - description: STMicroelectronics Accelerometers
+        enum:
+          - st,h3lis331dl-accel
+          - st,lis2de12
+          - st,lis2dw12
+          - st,lis2hh12
+          - st,lis2dh12-accel
+          - st,lis331dl-accel
+          - st,lis331dlh-accel
+          - st,lis3de
+          - st,lis3dh-accel
+          - st,lis3dhh
+          - st,lis3l02dq
+          - st,lis3lv02dl-accel
+          - st,lng2dm-accel
+          - st,lsm303agr-accel
+          - st,lsm303dl-accel
+          - st,lsm303dlh-accel
+          - st,lsm303dlhc-accel
+          - st,lsm303dlm-accel
+          - st,lsm330-accel
+          - st,lsm330d-accel
+          - st,lsm330dl-accel
+          - st,lsm330dlc-accel
+      - description: STMicroelectronics Gyroscopes
+        enum:
+          - st,l3g4200d-gyro
+          - st,l3g4is-gyro
+          - st,l3gd20-gyro
+          - st,l3gd20h-gyro
+          - st,lsm330-gyro
+          - st,lsm330d-gyro
+          - st,lsm330dl-gyro
+          - st,lsm330dlc-gyro
+          - st,lsm9ds0-gyro
+      - description: STMicroelectronics Magnetometers
+        enum:
+          - st,lis2mdl
+          - st,lis3mdl-magn
+          - st,lsm303agr-magn
+          - st,lsm303dlh-magn
+          - st,lsm303dlhc-magn
+          - st,lsm303dlm-magn
+          - st,lsm9ds1-magn
+      - description: STMicroelectronics Pressure Sensors
+        enum:
+          - st,lps001wp-press
+          - st,lps22hb-press
+          - st,lps22hh
+          - st,lps25h-press
+          - st,lps331ap-press
+          - st,lps33hw
+          - st,lps35hw
+      - description: Deprecated bindings
+        enum:
+          - st,lis302dl-spi
+          - st,lis3lv02d
+        deprecated: true
 
   reg:
     maxItems: 1
 
   interrupts:
+    description: interrupt line(s) connected to the DRDY line(s) and/or the
+      Intertial interrupt lines INT1 and INT2 if these exist. This means up to
+      three interrupts, and the DRDY must be the first one if it exists on
+      the package. The trigger edge of the interrupts is sometimes software
+      configurable in the hardware so the operating system should parse this
+      flag and set up the trigger edge as indicated in the device tree.
     minItems: 1
+    maxItems: 2
 
   vdd-supply: true
   vddio-supply: true
 
   st,drdy-int-pin:
+    description: the pin on the package that will be used to signal
+      "data ready" (valid values 1 or 2). This property is not configurable
+      on all sensors.
     $ref: /schemas/types.yaml#/definitions/uint32
-    description:
-      Some sensors have multiple possible pins via which they can provide
-      a data ready interrupt.  This selects which one.
-    enum:
-      - 1
-      - 2
+    enum: [1, 2]
 
   drive-open-drain:
     $ref: /schemas/types.yaml#/definitions/flag
-    description: |
-      The interrupt/data ready line will be configured as open drain, which
-      is useful if several sensors share the same interrupt line.
+    description: the interrupt/data ready line will be configured
+      as open drain, which is useful if several sensors share the same
+      interrupt line. (This binding is taken from pinctrl.)
+
+  mount-matrix:
+    description: an optional 3x3 mounting rotation matrix.
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            # These have no interrupts
+            - st,lps001wp
+    then:
+      properties:
+        interrupts: false
+        st,drdy-int-pin: false
+        drive-open-drain: false
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            # These have only DRDY
+            - st,lis2mdl
+            - st,lis3l02dq
+            - st,lis3lv02dl-accel
+            - st,lps22hb-press
+            - st,lps22hh
+            - st,lps25h-press
+            - st,lps33hw
+            - st,lps35hw
+            - st,lsm303agr-magn
+            - st,lsm303dlh-magn
+            - st,lsm303dlhc-magn
+            - st,lsm303dlm-magn
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
+        st,drdy-int-pin: false
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            # Two intertial interrupts i.e. accelerometer/gyro interrupts
+            - st,h3lis331dl-accel
+            - st,l3g4200d-gyro
+            - st,l3g4is-gyro
+            - st,l3gd20-gyro
+            - st,l3gd20h-gyro
+            - st,lis2de12
+            - st,lis2dw12
+            - st,lis2hh12
+            - st,lis2dh12-accel
+            - st,lis331dl-accel
+            - st,lis331dlh-accel
+            - st,lis3de
+            - st,lis3dh-accel
+            - st,lis3dhh
+            - st,lis3mdl-magn
+            - st,lng2dm-accel
+            - st,lps331ap-press
+            - st,lsm303agr-accel
+            - st,lsm303dlh-accel
+            - st,lsm303dlhc-accel
+            - st,lsm303dlm-accel
+            - st,lsm330-accel
+            - st,lsm330-gyro
+            - st,lsm330d-accel
+            - st,lsm330d-gyro
+            - st,lsm330dl-accel
+            - st,lsm330dl-gyro
+            - st,lsm330dlc-accel
+            - st,lsm330dlc-gyro
+            - st,lsm9ds0-gyro
+            - st,lsm9ds1-magn
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
 
 required:
   - compatible
@@ -110,15 +200,30 @@  examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
     i2c {
-        #address-cells = <1>;
-        #size-cells = <0>;
-        accelerometer@1d {
-            compatible = "st,lis3lv02dl-accel";
-            reg = <0x1d>;
-            interrupt-parent = <&gpio2>;
-            interrupts = <18 IRQ_TYPE_EDGE_RISING>;
-            pinctrl-0 = <&lis3lv02dl_nhk_mode>;
-            pinctrl-names = "default";
-        };
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      accelerometer@1c {
+        compatible = "st,lis331dl-accel";
+        reg = <0x1c>;
+        st,drdy-int-pin = <1>;
+        vdd-supply = <&ldo1>;
+        vddio-supply = <&ldo2>;
+        interrupt-parent = <&gpio>;
+        interrupts = <18 IRQ_TYPE_EDGE_RISING>, <19 IRQ_TYPE_EDGE_RISING>;
+      };
+    };
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      num-cs = <1>;
+
+      l3g4200d: gyroscope@0 {
+        compatible = "st,l3g4200d-gyro";
+        st,drdy-int-pin = <2>;
+        reg = <0>;
+        vdd-supply = <&vcc_io>;
+        vddio-supply = <&vcc_io>;
+      };
     };
-...
+