[PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Sean Nyekjaer]

The correct way to set the sampling rate to 12.5 Hz before
commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
was to write 13 Hz to sampling_frequency.
Before this patch writing 13 to samplerate results in sample rate set
to 26 Hz.
Now we return EINVAL if the sampling rate is not in table.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index a3ab16615729..a14a1df8537b 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1427,7 +1427,7 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val)
 		 * ext devices can run at different odr respect to
 		 * accel sensor
 		 */
-		if (odr_table->odr_avl[i].milli_hz >= odr)
+		if (odr_table->odr_avl[i].milli_hz == odr)
 			break;
 	}
 
-- 
2.31.0

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Lorenzo Bianconi]

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

> The correct way to set the sampling rate to 12.5 Hz before
> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
> was to write 13 Hz to sampling_frequency.
> Before this patch writing 13 to samplerate results in sample rate set
> to 26 Hz.
> Now we return EINVAL if the sampling rate is not in table.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?

> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index a3ab16615729..a14a1df8537b 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1427,7 +1427,7 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val)
>  		 * ext devices can run at different odr respect to
>  		 * accel sensor
>  		 */
> -		if (odr_table->odr_avl[i].milli_hz >= odr)
> +		if (odr_table->odr_avl[i].milli_hz == odr)
>  			break;
>  	}
>  
> -- 
> 2.31.0
> 

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

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Sean Nyekjaer]

On 07/05/2021 13.02, Lorenzo Bianconi wrote:
>> The correct way to set the sampling rate to 12.5 Hz before
>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
>> was to write 13 Hz to sampling_frequency.
>> Before this patch writing 13 to samplerate results in sample rate set
>> to 26 Hz.
>> Now we return EINVAL if the sampling rate is not in table.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> 
> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
> 
I have not tested with i2c-master.
But it's not nice to round up sample rate from user space without any notice.

/Sean

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Lorenzo Bianconi]

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

> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
> >> The correct way to set the sampling rate to 12.5 Hz before
> >> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
> >> was to write 13 Hz to sampling_frequency.
> >> Before this patch writing 13 to samplerate results in sample rate set
> >> to 26 Hz.
> >> Now we return EINVAL if the sampling rate is not in table.
> >>
> >> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > 
> > I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
> > 
> I have not tested with i2c-master.
> But it's not nice to round up sample rate from user space without any notice.

the point is accel odr is used as trigger for i2c-slave sampling. Connected
i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
select the lower accel odr greather than slave device one.

Regards,
Lorenzo

> 
> /Sean
> 

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

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Sean Nyekjaer]

On 10/05/2021 08.28, Lorenzo Bianconi wrote:
>> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
>>>> The correct way to set the sampling rate to 12.5 Hz before
>>>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
>>>> was to write 13 Hz to sampling_frequency.
>>>> Before this patch writing 13 to samplerate results in sample rate set
>>>> to 26 Hz.
>>>> Now we return EINVAL if the sampling rate is not in table.
>>>>
>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>>
>>> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
>>>
>> I have not tested with i2c-master.
>> But it's not nice to round up sample rate from user space without any notice.
> 
> the point is accel odr is used as trigger for i2c-slave sampling. Connected
> i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
> select the lower accel odr greather than slave device one.
> 
> Regards,
> Lorenzo
> 

We could open up for custom sample rates if i2c-slaves are enabled?
/Sean

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Lorenzo Bianconi]

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

> On 10/05/2021 08.28, Lorenzo Bianconi wrote:
> >> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
> >>>> The correct way to set the sampling rate to 12.5 Hz before
> >>>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
> >>>> was to write 13 Hz to sampling_frequency.
> >>>> Before this patch writing 13 to samplerate results in sample rate set
> >>>> to 26 Hz.
> >>>> Now we return EINVAL if the sampling rate is not in table.
> >>>>
> >>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>>
> >>> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
> >>>
> >> I have not tested with i2c-master.
> >> But it's not nice to round up sample rate from user space without any notice.
> > 
> > the point is accel odr is used as trigger for i2c-slave sampling. Connected
> > i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
> > select the lower accel odr greather than slave device one.
> > 
> > Regards,
> > Lorenzo
> > 
> 
> We could open up for custom sample rates if i2c-slaves are enabled?

can you please explain what you mean?

> /Sean

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

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Sean Nyekjaer]

On 10/05/2021 14.07, Lorenzo Bianconi wrote:
>> On 10/05/2021 08.28, Lorenzo Bianconi wrote:
>>>> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
>>>>>> The correct way to set the sampling rate to 12.5 Hz before
>>>>>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
>>>>>> was to write 13 Hz to sampling_frequency.
>>>>>> Before this patch writing 13 to samplerate results in sample rate set
>>>>>> to 26 Hz.
>>>>>> Now we return EINVAL if the sampling rate is not in table.
>>>>>>
>>>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>>>>
>>>>> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
>>>>>
>>>> I have not tested with i2c-master.
>>>> But it's not nice to round up sample rate from user space without any notice.
>>>
>>> the point is accel odr is used as trigger for i2c-slave sampling. Connected
>>> i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
>>> select the lower accel odr greather than slave device one.
>>>
>>> Regards,
>>> Lorenzo
>>>
>>
>> We could open up for custom sample rates if i2c-slaves are enabled?
> 
> can you please explain what you mean?
> 
If i2c-slaves are enabled, it's allowed to set off table sample rates.(We roundup samplerate like now)
If i2c-slaves are disabled, we return -EINVAL if we can't find the chosen samplerate in the samplerate table?

/Sean

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Lorenzo Bianconi]

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

> On 10/05/2021 14.07, Lorenzo Bianconi wrote:
> >> On 10/05/2021 08.28, Lorenzo Bianconi wrote:
> >>>> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
> >>>>>> The correct way to set the sampling rate to 12.5 Hz before
> >>>>>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
> >>>>>> was to write 13 Hz to sampling_frequency.
> >>>>>> Before this patch writing 13 to samplerate results in sample rate set
> >>>>>> to 26 Hz.
> >>>>>> Now we return EINVAL if the sampling rate is not in table.
> >>>>>>
> >>>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>>>>
> >>>>> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
> >>>>>
> >>>> I have not tested with i2c-master.
> >>>> But it's not nice to round up sample rate from user space without any notice.
> >>>
> >>> the point is accel odr is used as trigger for i2c-slave sampling. Connected
> >>> i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
> >>> select the lower accel odr greather than slave device one.
> >>>
> >>> Regards,
> >>> Lorenzo
> >>>
> >>
> >> We could open up for custom sample rates if i2c-slaves are enabled?
> > 
> > can you please explain what you mean?
> > 
> If i2c-slaves are enabled, it's allowed to set off table sample rates.(We roundup samplerate like now)
> If i2c-slaves are disabled, we return -EINVAL if we can't find the chosen samplerate in the samplerate table?

IMO this is not a big deal because if you set the proper ODR (e.g. 26Hz) with the
i2c-master disabled, the sample rate will not be rounded-up. If you set 20Hz,
the accel ODR will be set to 26Hz. I think it does not worth to make the code
uglier and more complex to maintain.

Regards,
Lorenzo

> 
> /Sean

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

Re: [PATCH] iio: imu: st_lsm6dsx: do not roundup set samplerate

[Sean Nyekjaer]

On 10/05/2021 16.47, Lorenzo Bianconi wrote:
>> On 10/05/2021 14.07, Lorenzo Bianconi wrote:
>>>> On 10/05/2021 08.28, Lorenzo Bianconi wrote:
>>>>>> On 07/05/2021 13.02, Lorenzo Bianconi wrote:
>>>>>>>> The correct way to set the sampling rate to 12.5 Hz before
>>>>>>>> commit f8710f0357bc ("iio: imu: st_lsm6dsx: express odr in mHZ")
>>>>>>>> was to write 13 Hz to sampling_frequency.
>>>>>>>> Before this patch writing 13 to samplerate results in sample rate set
>>>>>>>> to 26 Hz.
>>>>>>>> Now we return EINVAL if the sampling rate is not in table.
>>>>>>>>
>>>>>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>>>>>>
>>>>>>> I think this patch is breaking i2c-master support available in st_lsm6dsx, have you tested it?
>>>>>>>
>>>>>> I have not tested with i2c-master.
>>>>>> But it's not nice to round up sample rate from user space without any notice.
>>>>>
>>>>> the point is accel odr is used as trigger for i2c-slave sampling. Connected
>>>>> i2c slave devices have different sample rates (e.g. LIS2MDL), so we need to
>>>>> select the lower accel odr greather than slave device one.
>>>>>
>>>>> Regards,
>>>>> Lorenzo
>>>>>
>>>>
>>>> We could open up for custom sample rates if i2c-slaves are enabled?
>>>
>>> can you please explain what you mean?
>>>
>> If i2c-slaves are enabled, it's allowed to set off table sample rates.(We roundup samplerate like now)
>> If i2c-slaves are disabled, we return -EINVAL if we can't find the chosen samplerate in the samplerate table?
> 
> IMO this is not a big deal because if you set the proper ODR (e.g. 26Hz) with the
> i2c-master disabled, the sample rate will not be rounded-up. If you set 20Hz,
> the accel ODR will be set to 26Hz. I think it does not worth to make the code
> uglier and more complex to maintain.
> 
> Regards,
> Lorenzo
> 

Sure, but we had some existing code that was setting the samplerate to 13Hz as that was the way to set 12.5Hz samplerate in the past.
Suddenly without any notice 13Hz(of cause the kernel was updated) resulted in a samplerate of 26Hz. :/

/Sean