> On Mon, 4 Apr 2022 11:33:17 +0200 > Lorenzo Bianconi <lorenzo.bianconi@xxxxxxxxxx> wrote: > > > On Apr 04, Jonathan Cameron wrote: > > > On Sun, 3 Apr 2022 16:56:51 +0200 > > > Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote: > > > > > > > > On Sat, 2 Apr 2022 12:09:30 +0200 > > > > > Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote: > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> > > > > > 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@xxxxxxxxxx> > > > > 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:
signature.asc
Description: PGP signature
