| Message ID | 20220112205205.4082026-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c705c871106e35221f486765fcc3d978a4434c38 |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | arm64: dts: renesas: ulcb-kf: add 9-asix sensor device | expand |
Hi Nikita, Thanks for your patch! On Wed, Jan 12, 2022 at 9:52 PM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > This adds nodes for lsm9ds0 sensor installed on the KF board. > > With this patch, the sensor data becomes available over iio sysfs > interface. > > Interrupt definition is not added yet, because the interrupt lines of > lsm9ds0 are pulled to VCC on the board, which implies need for > active-low configuration. But st_sensors drivers currently can't work > with active-low interrupts on this chip. That's unfortunate, as DT describes hardware, not limitations of the software stack. > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> > --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > @@ -66,6 +66,13 @@ hdmi_3v3: regulator-hdmi-3v3 { > regulator-max-microvolt = <3300000>; > }; > > + accel_3v3: regulator-acc-3v3 { > + compatible = "regulator-fixed"; > + regulator-name = "accel-3v3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + }; > + > hdmi1-out { > compatible = "hdmi-connector"; > type = "a"; > @@ -208,6 +215,22 @@ pcm3168a_endpoint_c: endpoint { > }; > }; > }; > + > + lsm9ds0_acc_mag@1d { Please use standard node names: accelerometer@1d? > + compatible = "st,lsm9ds0-imu"; > + reg = <0x1d>; > + > + vdd-supply = <&accel_3v3>; > + vddio-supply = <&accel_3v3>; According to the bindings, the supplies are not required, so you can leave them out? Or are the bindings wrong? (The bindings also say "interrupts: maxItems 2", while the "interrupts: description" says up to three interrupts, doh...) > + }; > + > + lsm9ds0_gyro@6b { gyroscope@6b? > + compatible = "st,lsm9ds0-gyro"; > + reg = <0x6b>; > + > + vdd-supply = <&accel_3v3>; > + vddio-supply = <&accel_3v3>; > + }; > }; > }; 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
Hi Nikita, On Wed, Jan 12, 2022 at 9:52 PM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > This adds nodes for lsm9ds0 sensor installed on the KF board. > > With this patch, the sensor data becomes available over iio sysfs > interface. > > Interrupt definition is not added yet, because the interrupt lines of > lsm9ds0 are pulled to VCC on the board, which implies need for > active-low configuration. But st_sensors drivers currently can't work > with active-low interrupts on this chip. > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> Forgot something... > --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi > @@ -66,6 +66,13 @@ hdmi_3v3: regulator-hdmi-3v3 { > regulator-max-microvolt = <3300000>; > }; > > + accel_3v3: regulator-acc-3v3 { Please move up, to preserve sort order. > + compatible = "regulator-fixed"; > + regulator-name = "accel-3v3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + }; > + > hdmi1-out { > compatible = "hdmi-connector"; > type = "a"; > @@ -208,6 +215,22 @@ pcm3168a_endpoint_c: endpoint { > }; > }; > }; > + > + lsm9ds0_acc_mag@1d { Please move up, to preserve sort order. > + compatible = "st,lsm9ds0-imu"; > + reg = <0x1d>; > + > + vdd-supply = <&accel_3v3>; > + vddio-supply = <&accel_3v3>; > + }; 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
>> Interrupt definition is not added yet, because the interrupt lines of >> lsm9ds0 are pulled to VCC on the board, which implies need for >> active-low configuration. But st_sensors drivers currently can't work >> with active-low interrupts on this chip. > > That's unfortunate, as DT describes hardware, not limitations of the > software stack. Unfortunately, if interrupt definition is added, driver does wrong things and causes board hang. >> + vdd-supply = <&accel_3v3>; >> + vddio-supply = <&accel_3v3>; > > According to the bindings, the supplies are not required, so you can > leave them out? Or are the bindings wrong? If supplies are not defined, warning messages about dummy regulator are logged. > (The bindings also say "interrupts: maxItems 2", while the "interrupts: > description" says up to three interrupts, doh...) Chip has 3 interrupt outputs. On KF board, all those are ANDed together and result connected to SoC's gpio that is expected to be used as a shared active-low interrupt. Driver currently claims that this chip does not support active-low interrupts. Per datasheet, this is not true. But driver's way to set up interrupt registers does not scale to the case when interrupts have to be configured by different bits in several registers, that part of the driver has to be somehow rewritten. I guess nobody has ever tried to make these drivers (st_*) to drive a compound device (accel+gyro) with interrupts. At the same time, the device is perfectly useful without interrupts, and that is how it is enabled in the vendor BSP. Nikita
Hi Nikita, On Wed, Jan 26, 2022 at 4:28 PM Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote: > >> Interrupt definition is not added yet, because the interrupt lines of > >> lsm9ds0 are pulled to VCC on the board, which implies need for > >> active-low configuration. But st_sensors drivers currently can't work > >> with active-low interrupts on this chip. > > > > That's unfortunate, as DT describes hardware, not limitations of the > > software stack. > > Unfortunately, if interrupt definition is added, driver does wrong things and causes board hang. OK. > >> + vdd-supply = <&accel_3v3>; > >> + vddio-supply = <&accel_3v3>; > > > > According to the bindings, the supplies are not required, so you can > > leave them out? Or are the bindings wrong? > > If supplies are not defined, warning messages about dummy regulator are logged. OK. > > (The bindings also say "interrupts: maxItems 2", while the "interrupts: > > description" says up to three interrupts, doh...) > > Chip has 3 interrupt outputs. On KF board, all those are ANDed together and result connected to SoC's > gpio that is expected to be used as a shared active-low interrupt. Driver currently claims that this > chip does not support active-low interrupts. Per datasheet, this is not true. But driver's way to set up > interrupt registers does not scale to the case when interrupts have to be configured by different bits > in several registers, that part of the driver has to be somehow rewritten. I guess nobody has ever tried > to make these drivers (st_*) to drive a compound device (accel+gyro) with interrupts. > > At the same time, the device is perfectly useful without interrupts, and that is how it is enabled in > the vendor BSP. OK, will queue in renesas-devel for v5.18, with the low-hanging fruits (node names, order) fixed. 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/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi index a66301a4081d..d122e645a892 100644 --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi @@ -66,6 +66,13 @@ hdmi_3v3: regulator-hdmi-3v3 { regulator-max-microvolt = <3300000>; }; + accel_3v3: regulator-acc-3v3 { + compatible = "regulator-fixed"; + regulator-name = "accel-3v3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + }; + hdmi1-out { compatible = "hdmi-connector"; type = "a"; @@ -208,6 +215,22 @@ pcm3168a_endpoint_c: endpoint { }; }; }; + + lsm9ds0_acc_mag@1d { + compatible = "st,lsm9ds0-imu"; + reg = <0x1d>; + + vdd-supply = <&accel_3v3>; + vddio-supply = <&accel_3v3>; + }; + + lsm9ds0_gyro@6b { + compatible = "st,lsm9ds0-gyro"; + reg = <0x6b>; + + vdd-supply = <&accel_3v3>; + vddio-supply = <&accel_3v3>; + }; }; };
This adds nodes for lsm9ds0 sensor installed on the KF board. With this patch, the sensor data becomes available over iio sysfs interface. Interrupt definition is not added yet, because the interrupt lines of lsm9ds0 are pulled to VCC on the board, which implies need for active-low configuration. But st_sensors drivers currently can't work with active-low interrupts on this chip. Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com> --- arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)