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

[Jonathan Cameron <jic23@kernel.org>]

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?). 

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