Re: [PATCH v3] iio: accel: st_accel: add LIS2DS12

[Kaustabh Chakraborty <kauschluss@disroot.org>]

On 2024-06-06 20:05, Jonathan Cameron wrote:
> On Thu, 06 Jun 2024 10:12:11 +0000
> kauschluss <kauschluss@disroot.org> wrote:
> 
>> On 2024-06-02 08:54, Jonathan Cameron wrote:
>> > On Sat, 1 Jun 2024 20:49:25 +0100
>> > Conor Dooley <conor@kernel.org> wrote:
>> >
>> >> On Sun, Jun 02, 2024 at 12:56:41AM +0530, Kaustabh Chakraborty wrote:
>> >> > diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
>> >> > index fd3749871121..329a4d6fb2ec 100644
>> >> > --- a/drivers/iio/accel/st_accel_i2c.c
>> >> > +++ b/drivers/iio/accel/st_accel_i2c.c
>> >> > @@ -102,6 +102,10 @@ static const struct of_device_id st_accel_of_match[] = {
>> >> >  		.compatible = "st,lis2de12",
>> >> >  		.data = LIS2DE12_ACCEL_DEV_NAME,
>> >> >  	},
>> >> > +	{
>> >> > +		.compatible = "st,lis2ds12",
>> >> > +		.data = LIS2DS12_ACCEL_DEV_NAME,
>> >> > +	},
>> >> >  	{
>> >> >  		.compatible = "st,lis2hh12",
>> >> >  		.data = LIS2HH12_ACCEL_DEV_NAME,
>> >>
>> >> > diff --git a/drivers/iio/accel/st_accel_spi.c b/drivers/iio/accel/st_accel_spi.c
>> >> > index f72a24f45322..825adab37105 100644
>> >> > --- a/drivers/iio/accel/st_accel_spi.c
>> >> > +++ b/drivers/iio/accel/st_accel_spi.c
>> >> > @@ -64,6 +64,10 @@ static const struct of_device_id st_accel_of_match[] = {
>> >> >  		.compatible = "st,lis2dh12-accel",
>> >> >  		.data = LIS2DH12_ACCEL_DEV_NAME,
>> >> >  	},
>> >> > +	{
>> >> > +		.compatible = "st,lis2ds12",
>> >> > +		.data = LIS2DS12_ACCEL_DEV_NAME,
>> >> > +	},
>> >> >  	{
>> >> >  		.compatible = "st,lis3l02dq",
>> >> >  		.data = LIS3L02DQ_ACCEL_DEV_NAME,
>> >>
>> >> Any new compatibles need to be documented in st,st-sensors.yaml
>> >
>> > At the moment the st_sensors core is doing hard matching against whoami values
>> > which isn't good.  That should ideally be fixed and the binding for this
>> > device should use a fallback compatible if the statement about compatibility
>> > is accurate.
>> 
>> I apologize for not wording the description accurately. By 
>> "compatibility",
>> I mean that the sensor settings of LIS2DE12 (such as the gain values) 
>> seem
>> to be well-suited for LIS2DS12, as per my experimentation. Both 
>> devices are
>> manufactured by ST and have no correlation regarding compatibility 
>> whatsoever.
>> In that case, a fallback compatible isn't required, right?
> 
> If only the Who Am I value prevents it working if you give the 
> compatible
> as lisde12 then even though ST rarely if ever identifies it in 
> datasheets, they are
> software compatible.  In that case we should allow for a fallback 
> compatible.
> + fix the driver not to fail on the whoami mismatch.
> Note we don't care if they have totally different packages as long as
> the driver doesn't need to know that.  If they have different numbers
> of interrupts though or power supplies, then they aren't compatible and
> we shouldn't provide a fallback.
> 
> Roughly speaking you have to compare datahsheet sections for pins (not 
> which
> but what) and register maps.

I've thoroughly checked the registers, and indeed, a lot of registers 
addresses
and other settings are different. Although the sensor settings of 
LIS2DE12
"work", they are technically incorrect.

I've fixed it, and will send it with v4 in a day or two.

Thank you!

> That applies even if the current driver will fail to probe (for now)
>> 
>> I'll make sure to rewrite the description more accurately in v4.
>> 
>> > It may just be a case of relaxing the check in st_sensors_verify_id()
>> > to printing a warning not an error message and not returning an error code
>> > (reserving error returns in that function for bus error etc.
>> 
>> I agree, if you want I may send a patch for that after I'm done with 
>> this
>> one.
> Thanks,
> 
> Jonathan
> 
>> 
>> > That doesn't need to be in this patch though.  Just have the fallback
>> > stuff in the binding and for now we can rely on matching the more
>> > precise compatible.
>> >
>> > Jonathan
>> >
>> >
>> >>
>> >> Thanks,
>> >> Conor.
>> >>