| Message ID | 20210412122331.1631643-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0cd71145803dc2b87b3afc8e2990ff0d43bf7027 |
| Headers | show |
| Series | iio: st-sensors: Update ST Sensor bindings | expand |
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
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
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 >
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
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 >
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
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
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
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
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
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
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 --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>; + }; }; -... +