[v1] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

[PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by Akshay Bansod 3 months, 1 week ago

Update the sysfs interface for sampling frequency and scale attributes.
Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
and recommended for use in sysfs.

Signed-off-by: Akshay Bansod <akbansd@gmail.com>
---

Testing:
- Built the driver (`st_lsm6dsx_i2c`) as a module.
- Tested using `i2c-stub` to mock the device.
- Verified that reading sysfs attributes like `sampling_frequency_available`
  works correctly and shows no change in functionality.

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index c65ad4982..1cef10919 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2035,7 +2035,7 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
 
 	odr_table = &sensor->hw->settings->odr_table[sensor->id];
 	for (i = 0; i < odr_table->odr_len; i++)
-		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
+		len += sysfs_emit_at(buf, len, "%d.%03d ",
 				 odr_table->odr_avl[i].milli_hz / 1000,
 				 odr_table->odr_avl[i].milli_hz % 1000);
 	buf[len - 1] = '\n';
@@ -2054,7 +2054,7 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
 
 	fs_table = &hw->settings->fs_table[sensor->id];
 	for (i = 0; i < fs_table->fs_len; i++)
-		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
+		len += sysfs_emit_at(buf, len, "0.%09u ",
 				 fs_table->fs_avl[i].gain);
 	buf[len - 1] = '\n';
 
-- 
2.49.0

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by David Lechner 3 months, 1 week ago

On 7/2/25 8:58 AM, Akshay Bansod wrote:
> Update the sysfs interface for sampling frequency and scale attributes.
> Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> and recommended for use in sysfs.
> 
> Signed-off-by: Akshay Bansod <akbansd@gmail.com>
> ---
> 
> Testing:
> - Built the driver (`st_lsm6dsx_i2c`) as a module.
> - Tested using `i2c-stub` to mock the device.
> - Verified that reading sysfs attributes like `sampling_frequency_available`
>   works correctly and shows no change in functionality.

Nice to see it was actually tested. :-)

> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index c65ad4982..1cef10919 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -2035,7 +2035,7 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
>  
>  	odr_table = &sensor->hw->settings->odr_table[sensor->id];
>  	for (i = 0; i < odr_table->odr_len; i++)
> -		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
>  				 odr_table->odr_avl[i].milli_hz / 1000,
>  				 odr_table->odr_avl[i].milli_hz % 1000);

Let's keep checkpatch happy and change the indent of the wrapped lines to
line up with ( since the ( moved.

>  	buf[len - 1] = '\n';
> @@ -2054,7 +2054,7 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
>  
>  	fs_table = &hw->settings->fs_table[sensor->id];
>  	for (i = 0; i < fs_table->fs_len; i++)
> -		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
> +		len += sysfs_emit_at(buf, len, "0.%09u ",
>  				 fs_table->fs_avl[i].gain);

ditto

>  	buf[len - 1] = '\n';
>

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by akshay bansod 3 months, 1 week ago

On Wednesday, 2 July 2025 8:25 pm +0530 David Lechner wrote:
> On 7/2/25 8:58 AM, Akshay Bansod wrote:
> > Update the sysfs interface for sampling frequency and scale attributes.
> > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> > and recommended for use in sysfs.
> > 
> > Signed-off-by: Akshay Bansod <akbansd@gmail.com>
> > ---
> > 
> > Testing:
> > - Built the driver (`st_lsm6dsx_i2c`) as a module.
> > - Tested using `i2c-stub` to mock the device.
> > - Verified that reading sysfs attributes like `sampling_frequency_available`
> >   works correctly and shows no change in functionality.
> 
> Nice to see it was actually tested. :-)
> 
:-)
> > 
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index c65ad4982..1cef10919 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -2035,7 +2035,7 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
> >  
> >  	odr_table = &sensor->hw->settings->odr_table[sensor->id];
> >  	for (i = 0; i < odr_table->odr_len; i++)
> > -		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> > +		len += sysfs_emit_at(buf, len, "%d.%03d ",
> >  				 odr_table->odr_avl[i].milli_hz / 1000,
> >  				 odr_table->odr_avl[i].milli_hz % 1000);
> 
> Let's keep checkpatch happy and change the indent of the wrapped lines to
> line up with ( since the ( moved.
> 
noted. I wasn't aware of that.
> >  	buf[len - 1] = '\n';
> > @@ -2054,7 +2054,7 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
> >  
> >  	fs_table = &hw->settings->fs_table[sensor->id];
> >  	for (i = 0; i < fs_table->fs_len; i++)
> > -		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
> > +		len += sysfs_emit_at(buf, len, "0.%09u ",
> >  				 fs_table->fs_avl[i].gain);
> 
> ditto
> 
noted.
> >  	buf[len - 1] = '\n';
> >  
> 
> 

Thanks for the review. I'll revise the patch.

Regards,
Akshay

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by Andy Shevchenko 3 months, 1 week ago

On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:
> On 7/2/25 8:58 AM, Akshay Bansod wrote:
> > Update the sysfs interface for sampling frequency and scale attributes.
> > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> > and recommended for use in sysfs.

...

> > +		len += sysfs_emit_at(buf, len, "%d.%03d ",
> >  				 odr_table->odr_avl[i].milli_hz / 1000,
> >  				 odr_table->odr_avl[i].milli_hz % 1000);
> 
> Let's keep checkpatch happy and change the indent of the wrapped lines to
> line up with ( since the ( moved.

While I see the point, wouldn't be better to have 1000 replaced with MILLI
at the same time?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by David Lechner 3 months, 1 week ago

On 7/2/25 9:55 AM, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:
>> On 7/2/25 8:58 AM, Akshay Bansod wrote:
>>> Update the sysfs interface for sampling frequency and scale attributes.
>>> Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
>>> and recommended for use in sysfs.
> 
> ...
> 
>>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
>>>  				 odr_table->odr_avl[i].milli_hz / 1000,
>>>  				 odr_table->odr_avl[i].milli_hz % 1000);
>>
>> Let's keep checkpatch happy and change the indent of the wrapped lines to
>> line up with ( since the ( moved.
> 
> While I see the point, wouldn't be better to have 1000 replaced with MILLI
> at the same time?
> 

For anything with 3 zeros, I don't consider MILLI better (or worse).
Science shows that the average human can easily see 3 or 4 things
without having to count them [1]. So it is only when we start getting
more 0s than that is when I think we should be picky about using macros
instead.

And in this particular case, we are converting milli to micro so `1000`
should be replaced by `(MICRO / MILLI)` if we are going to do that.

[1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by Jonathan Cameron 3 months, 1 week ago

On Wed, 2 Jul 2025 10:04:23 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/2/25 9:55 AM, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:  
> >> On 7/2/25 8:58 AM, Akshay Bansod wrote:  
> >>> Update the sysfs interface for sampling frequency and scale attributes.
> >>> Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> >>> and recommended for use in sysfs.  
> > 
> > ...
> >   
> >>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
> >>>  				 odr_table->odr_avl[i].milli_hz / 1000,
> >>>  				 odr_table->odr_avl[i].milli_hz % 1000);  
> >>
> >> Let's keep checkpatch happy and change the indent of the wrapped lines to
> >> line up with ( since the ( moved.  
> > 
> > While I see the point, wouldn't be better to have 1000 replaced with MILLI
> > at the same time?
> >   
> 
> For anything with 3 zeros, I don't consider MILLI better (or worse).
> Science shows that the average human can easily see 3 or 4 things
> without having to count them [1]. So it is only when we start getting
> more 0s than that is when I think we should be picky about using macros
> instead.
> 
> And in this particular case, we are converting milli to micro so `1000`
> should be replaced by `(MICRO / MILLI)` if we are going to do that.
No we aren't.

This one is converting from milli_hz to hz + sticking to milli for the decimal
part.

Lots of other IIO cases where you would have been right, but I think not here.
> 
> [1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/
> 
>

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by David Lechner 3 months, 1 week ago

On 7/2/25 10:33 AM, Jonathan Cameron wrote:
> On Wed, 2 Jul 2025 10:04:23 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 7/2/25 9:55 AM, Andy Shevchenko wrote:
>>> On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:  
>>>> On 7/2/25 8:58 AM, Akshay Bansod wrote:  
>>>>> Update the sysfs interface for sampling frequency and scale attributes.
>>>>> Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
>>>>> and recommended for use in sysfs.  
>>>
>>> ...
>>>   
>>>>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
>>>>>  				 odr_table->odr_avl[i].milli_hz / 1000,
>>>>>  				 odr_table->odr_avl[i].milli_hz % 1000);  
>>>>
>>>> Let's keep checkpatch happy and change the indent of the wrapped lines to
>>>> line up with ( since the ( moved.  
>>>
>>> While I see the point, wouldn't be better to have 1000 replaced with MILLI
>>> at the same time?
>>>   
>>
>> For anything with 3 zeros, I don't consider MILLI better (or worse).
>> Science shows that the average human can easily see 3 or 4 things
>> without having to count them [1]. So it is only when we start getting
>> more 0s than that is when I think we should be picky about using macros
>> instead.
>>
>> And in this particular case, we are converting milli to micro so `1000`
>> should be replaced by `(MICRO / MILLI)` if we are going to do that.
> No we aren't.
> 
> This one is converting from milli_hz to hz + sticking to milli for the decimal
> part.
> 
> Lots of other IIO cases where you would have been right, but I think not here.

Oops. The %03d instead of %06d should have given it away!

>>
>> [1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/
>>
>>
>

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by Andy Shevchenko 3 months ago

Wed, Jul 02, 2025 at 10:53:31AM -0500, David Lechner kirjoitti:
> On 7/2/25 10:33 AM, Jonathan Cameron wrote:
> > On Wed, 2 Jul 2025 10:04:23 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >> On 7/2/25 9:55 AM, Andy Shevchenko wrote:
> >>> On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:  
> >>>> On 7/2/25 8:58 AM, Akshay Bansod wrote:  

...

> >>>>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
> >>>>>  				 odr_table->odr_avl[i].milli_hz / 1000,
> >>>>>  				 odr_table->odr_avl[i].milli_hz % 1000);  
> >>>>
> >>>> Let's keep checkpatch happy and change the indent of the wrapped lines to
> >>>> line up with ( since the ( moved.  
> >>>
> >>> While I see the point, wouldn't be better to have 1000 replaced with MILLI
> >>> at the same time?
> >>
> >> For anything with 3 zeros, I don't consider MILLI better (or worse).
> >> Science shows that the average human can easily see 3 or 4 things
> >> without having to count them [1]. So it is only when we start getting
> >> more 0s than that is when I think we should be picky about using macros
> >> instead.
> >>
> >> And in this particular case, we are converting milli to micro so `1000`
> >> should be replaced by `(MICRO / MILLI)` if we are going to do that.
> > No we aren't.
> > 
> > This one is converting from milli_hz to hz + sticking to milli for the decimal
> > part.
> > 
> > Lots of other IIO cases where you would have been right, but I think not here.
> 
> Oops. The %03d instead of %06d should have given it away!

I'm not sure I got your comment. The '3' vs. '6' will just define
the minimum amount of printed digits, it does *not* limit the upper
numbers anyhow (it's limited by the 'd', which is (INT_MIN .. INT_MAX).


> >> [1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by David Lechner 3 months ago

On 7/3/25 4:05 AM, Andy Shevchenko wrote:
> Wed, Jul 02, 2025 at 10:53:31AM -0500, David Lechner kirjoitti:
>> On 7/2/25 10:33 AM, Jonathan Cameron wrote:
>>> On Wed, 2 Jul 2025 10:04:23 -0500
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>> On 7/2/25 9:55 AM, Andy Shevchenko wrote:
>>>>> On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:  
>>>>>> On 7/2/25 8:58 AM, Akshay Bansod wrote:  
> 
> ...
> 
>>>>>>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
>>>>>>>  				 odr_table->odr_avl[i].milli_hz / 1000,
>>>>>>>  				 odr_table->odr_avl[i].milli_hz % 1000);  
>>>>>>
>>>>>> Let's keep checkpatch happy and change the indent of the wrapped lines to
>>>>>> line up with ( since the ( moved.  
>>>>>
>>>>> While I see the point, wouldn't be better to have 1000 replaced with MILLI
>>>>> at the same time?
>>>>
>>>> For anything with 3 zeros, I don't consider MILLI better (or worse).
>>>> Science shows that the average human can easily see 3 or 4 things
>>>> without having to count them [1]. So it is only when we start getting
>>>> more 0s than that is when I think we should be picky about using macros
>>>> instead.
>>>>
>>>> And in this particular case, we are converting milli to micro so `1000`
>>>> should be replaced by `(MICRO / MILLI)` if we are going to do that.
>>> No we aren't.
>>>
>>> This one is converting from milli_hz to hz + sticking to milli for the decimal
>>> part.
>>>
>>> Lots of other IIO cases where you would have been right, but I think not here.
>>
>> Oops. The %03d instead of %06d should have given it away!
> 
> I'm not sure I got your comment. The '3' vs. '6' will just define
> the minimum amount of printed digits, it does *not* limit the upper
> numbers anyhow (it's limited by the 'd', which is (INT_MIN .. INT_MAX).

It is after the decimal point in the printed string, so 3 digits
after a decimal point is going to be MILLI units. And the % 1000
ensures that we would never get more than 3 digits there.

> 
> 
>>>> [1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/
>

Re: [PATCH] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

Posted by Andy Shevchenko 3 months, 1 week ago

On Wed, Jul 02, 2025 at 10:04:23AM -0500, David Lechner wrote:
> On 7/2/25 9:55 AM, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 09:16:51AM -0500, David Lechner wrote:
> >> On 7/2/25 8:58 AM, Akshay Bansod wrote:

...

> >>> +		len += sysfs_emit_at(buf, len, "%d.%03d ",
> >>>  				 odr_table->odr_avl[i].milli_hz / 1000,
> >>>  				 odr_table->odr_avl[i].milli_hz % 1000);
> >>
> >> Let's keep checkpatch happy and change the indent of the wrapped lines to
> >> line up with ( since the ( moved.
> > 
> > While I see the point, wouldn't be better to have 1000 replaced with MILLI
> > at the same time?
> 
> For anything with 3 zeros, I don't consider MILLI better (or worse).
> Science shows that the average human can easily see 3 or 4 things
> without having to count them [1]. So it is only when we start getting
> more 0s than that is when I think we should be picky about using macros
> instead.
> 
> And in this particular case, we are converting milli to micro so `1000`
> should be replaced by `(MICRO / MILLI)` if we are going to do that.

I see. This changes the picture drastically. Let's leave it for another day then.

> [1]: https://www.scientificamerican.com/article/your-brain-finds-it-easy-to-size-up-four-objects-but-not-five-heres-why/

-- 
With Best Regards,
Andy Shevchenko