Re: [PATCH 2/2] dt-bindings: iio: imu: st_lsm6dsx: add asm330lhhx device bindings

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8083 bytes --]

> On Mon, 4 Apr 2022 11:33:17 +0200
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> 
> > On Apr 04, Jonathan Cameron wrote:
> > > On Sun, 3 Apr 2022 16:56:51 +0200
> > > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >   
> > > > > On Sat,  2 Apr 2022 12:09:30 +0200
> > > > > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > > >     
> > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>    
> > > > > Hi Lorenzo,
> > > > > 
> > > > > This runs in to the same feedback that was recently had for
> > > > > https://lore.kernel.org/all/?q=Add+support+for+ICM-20608-D
> > > > > but in a more extreme sense as this one presents the same whoami value
> > > > > as for other sensors already supported.  Things are made more
> > > > > fun by the fact that sensors with the same WAI seem to have different
> > > > > features (presence or not of a sensor hub - is there any documented
> > > > > way to detect that?).     
> > > > 
> > > > Hi Jonathan,
> > > > 
> > > > if we consider only the features implemented in st_lsm6dsx, asm330lhhx
> > > > will be 1:1 compatible with lsm6dsr or lsm6dso, so we can just use one
> > > > of bindings in this section to support it (the only side effect is it
> > > > will be listed as "lsm6dsr" or "lsm6dso", but I guess it is ok). Agree?  
> > > 
> > > If the part has more features than the base compatible (or a different WAI)
> > > then we can definitely have a backup compatible for it (hence making that
> > > subset of features work on an old kernel).  We still want to introduce
> > > the new compatible so that we get the name right etc going forwards and
> > > are in a good position to add the extra features if we ever get around to it.  
> > 
> > ack. I did not completely get what you mean here with "backup compatible".
> > Do you mean:
> > - use "st,lsm6dsr" for asm330lhhx on older kernels and add "st,asm330lhhx" on
> >   new ones. Do you have any pointer on how to document it?
> Take a look at the mpu6050 patches. 
> 
> 
>  properties:
>    compatible:
> -    enum:
> -      - invensense,iam20680
> -      - invensense,icm20608
> -      - invensense,icm20609
> -      - invensense,icm20689
> -      - invensense,icm20602
> -      - invensense,icm20690
> -      - invensense,mpu6000
> -      - invensense,mpu6050
> -      - invensense,mpu6500
> -      - invensense,mpu6515
> -      - invensense,mpu6880
> -      - invensense,mpu9150
> -      - invensense,mpu9250
> -      - invensense,mpu9255
> +    oneOf:
> +      - enum:
> +        - invensense,iam20680
> +        - invensense,icm20608
> +        - invensense,icm20609
> +        - invensense,icm20689
> +        - invensense,icm20602
> +        - invensense,icm20690
> +        - invensense,mpu6000
> +        - invensense,mpu6050
> +        - invensense,mpu6500
> +        - invensense,mpu6515
> +        - invensense,mpu6880
> +        - invensense,mpu9150
> +        - invensense,mpu9250
> +        - invensense,mpu9255
> +      - items:
> +        - const: invensense,icm20608d
> +        - const: invensense,icm20608
> 
> Which ends up expecting
> 
> compatible = "invensense,icm20608d", "invensense,icm20608"
> and will try matching on the first. If that fails it will try
> with the second value.

ack, thx for pointing this out. I will fix it in v2.

Regards,
Lorenzo

>  
> > 
> > or 
> > 
> > - add a "wildcard" compatible string for this kind of devices. Do you have any
> >   pointers?
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > >   
> > > > 
> > > > The only difference between asm330lhhx and asm330lhh is the former supports
> > > > sensor-hub while the latter does not declare it (even if the use the same
> > > > whoami).
> > > > AFAIK there is no way to autodetect if the sensor supports sensor-hub and
> > > > we can just try to discover slave devices connected. This can have some
> > > > downside as described in the commit:  
> > > 
> > > Ah thanks. I'd forgotten this.
> > >   
> > > > 
> > > > commit 35619155d044830357f06f1d2c8188c4530b4d7a
> > > > Author: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > Date:   Sat Nov 13 16:23:14 2021 +0100
> > > > 
> > > > iio: imu: st_lsm6dsx: add dts property to disable sensor-hub
> > > > 
> > > > I would like to merge the sections in st_lsm6dsx_settings struct for
> > > > lsm6dsr, lsm6dso.. and lsm6dsop, asm330lhh since the only difference is
> > > > sensor-hub support. I guess we can have 2 option here to avoid any
> > > > sensor-hub corner cases:
> > > > - provide the "st,disable-sensor-hub" in dts to disable sensor-hub for
> > > >   asm330lhh, lsm6dsop (need user changes)
> > > > - add a bool variable st_lsm6dsx_settings[].id[] in order to specify if the
> > > >   chip supports sensor-hub.
> > > > 
> > > > Which one do you prefer?
> > > > 
> > > > Regards,
> > > > Lorenzo
> > > >   
> > > > > 
> > > > > As such, we should really be listing this as compatible with one 
> > > > > of the parts that is already supported such as the
> > > > > LSM6DSR.
> > > > > 
> > > > > For that we'll need a slightly more complex binding and it would
> > > > > have the side effect that if the match was on that compatible we
> > > > > would list the name as whatever that part is.
> > > > > 
> > > > > I'm not sure that really matters a great deal, but it could in theory
> > > > > create a userspace ABI change if we later needed to add explicit support
> > > > > for the part due to some real differences not indicated by the WAI value.
> > > > > 
> > > > > An extension is whether we should relax the need to match on WAI if
> > > > > the part is considered compatible.  I guess that depends on just how
> > > > > compatible we think they are.
> > > > > 
> > > > > So I see several steps to this process.
> > > > > 
> > > > > 1) Add fallback compatibles for existing entries to first one with same WAI and
> > > > >    same feature set.
> > > > > 2) Add fallback compatibles beyond that to first part introduced with particular
> > > > >    characteristics.  For this we'd also want to have the driver relax its
> > > > >    handling to just warn if the WAI isn't listed for any of the parts that
> > > > >    share a particular set of characteristic (so you'll have to loop over the local
> > > > >    array again to check):
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c#L1197
> > > > > Same argument applies as for the mpu6050 that, whilst we should modify that code to
> > > > > cope, it's not a prerequisit for adding the compatible fallback to the binding.
> > > > > Personally I'd like it to be the first patch in the series that modifies the
> > > > > binding though.  Note it'll be easy to add the fallbacks for this new part as
> > > > > no mainline trees presumably use it.  To 'fix' the rest we'll have to find
> > > > > and update any DTs in mainline.
> > > > > 
> > > > > Note this won't stop us needing to add compatibles to newer kernels (at very
> > > > > least to the dt-binding, but probably also the driver), but it should help a newer
> > > > > DT 'work' with an old kernel.
> > > > > 
> > > > > Jonathan
> > > > > 
> > > > >     
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/iio/imu/st,lsm6dsx.yaml | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/imu/st,lsm6dsx.yaml b/Documentation/devicetree/bindings/iio/imu/st,lsm6dsx.yaml
> > > > > > index 0750f700a143..23637c420d20 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/imu/st,lsm6dsx.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/imu/st,lsm6dsx.yaml
> > > > > > @@ -31,6 +31,7 @@ properties:
> > > > > >        - st,lsm6dsrx
> > > > > >        - st,lsm6dst
> > > > > >        - st,lsm6dsop
> > > > > > +      - st,asm330lhhx
> > > > > >  
> > > > > >    reg:
> > > > > >      maxItems: 1    
> > > > >     
> > > >   
> > >   
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]