1. Patchew
  2. linux
  3. View series

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

Akshay Bansod posted 1 patch 2 months, 2 weeks ago
There is a newer version of this series
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
[PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by Akshay Bansod 2 months, 2 weeks 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>
---

changes in v3:
- Use `sysfs_emit_at(buf, len - 1, "\n")` instead of directly modifying `buf[len - 1]`
  for newline termination, aligning with `sysfs_emit_at()` usage.
- Link to v2: https://lore.kernel.org/linux-iio/20250703053900.36530-1-akbansd@gmail.com/

changes in v2:
- Fixed indentation for line wrap
- Link to v1: https://lore.kernel.org/linux-iio/20250702135855.59955-1-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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 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..f0aab41f3 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2035,10 +2035,10 @@ 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 ",
-				 odr_table->odr_avl[i].milli_hz / 1000,
-				 odr_table->odr_avl[i].milli_hz % 1000);
-	buf[len - 1] = '\n';
+		len += sysfs_emit_at(buf, len, "%d.%03d ",
+				     odr_table->odr_avl[i].milli_hz / 1000,
+				     odr_table->odr_avl[i].milli_hz % 1000);
+	sysfs_emit_at(buf, len - 1, "\n");
 
 	return len;
 }
@@ -2054,9 +2054,10 @@ 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 ",
-				 fs_table->fs_avl[i].gain);
-	buf[len - 1] = '\n';
+		len += sysfs_emit_at(buf, len, "0.%09u ",
+				     fs_table->fs_avl[i].gain);
+
+	sysfs_emit_at(buf, len - 1, "\n");
 
 	return len;
 }
-- 
2.49.0
Re: [PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by Andy Shevchenko 2 months, 2 weeks ago 
On Wed, Jul 23, 2025 at 07:43:59PM +0530, 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.

...

>  	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 ",
> -				 fs_table->fs_avl[i].gain);
> -	buf[len - 1] = '\n';
> +		len += sysfs_emit_at(buf, len, "0.%09u ",
> +				     fs_table->fs_avl[i].gain);
> +
> +	sysfs_emit_at(buf, len - 1, "\n");

Still looks a bit weird (while working).

>  	return len;

I deally we should have a helper doing all this under the hood for plenty of
the (existing) users in the kernel.

In any case, I leave this change to others to comment, I don't object pushing
it in this form, either way len - 1 is simply weird.


-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by Jonathan Cameron 2 months, 2 weeks ago 
On Wed, 23 Jul 2025 17:42:28 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jul 23, 2025 at 07:43:59PM +0530, 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.  
> 
> ...
> 
> >  	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 ",
> > -				 fs_table->fs_avl[i].gain);
> > -	buf[len - 1] = '\n';
> > +		len += sysfs_emit_at(buf, len, "0.%09u ",
> > +				     fs_table->fs_avl[i].gain);
> > +
> > +	sysfs_emit_at(buf, len - 1, "\n");  
> 
> Still looks a bit weird (while working).
> 
> >  	return len;  
> 
> I deally we should have a helper doing all this under the hood for plenty of
> the (existing) users in the kernel.

hmm I'm not sure generic is terribly easy and I'd prefer this using the
read_avail callbacks that require the data in an array where ever possible.
Mind you that does the same print at len - 1 as this.  Let's play. 
Completely untested.

	for (i = 0; i < fs_table->fs_len; i++)
		len += sysfs_emit_at(buf, len, "0x%09u%c",
				     fs_table->fs_avl[i].gain,
				     ((i == fs_table->fs_len - 1) ? '\n', ' '));

better?

It's definitely not more readable than the above, but it does avoid the write
to len - 1.

> 
> In any case, I leave this change to others to comment, I don't object pushing
> it in this form, either way len - 1 is simply weird.
> 
>
Re: [PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by Andy Shevchenko 2 months, 1 week ago 
On Wed, Jul 23, 2025 at 04:41:00PM +0100, Jonathan Cameron wrote:
> On Wed, 23 Jul 2025 17:42:28 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Wed, Jul 23, 2025 at 07:43:59PM +0530, Akshay Bansod wrote:

...

> > >  	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 ",
> > > -				 fs_table->fs_avl[i].gain);
> > > -	buf[len - 1] = '\n';
> > > +		len += sysfs_emit_at(buf, len, "0.%09u ",
> > > +				     fs_table->fs_avl[i].gain);
> > > +
> > > +	sysfs_emit_at(buf, len - 1, "\n");  
> > 
> > Still looks a bit weird (while working).
> > 
> > >  	return len;  
> > 
> > I deally we should have a helper doing all this under the hood for plenty of
> > the (existing) users in the kernel.
> 
> hmm I'm not sure generic is terribly easy

I agree, I have some plans for %p specifier extension, but I was stuck with it
and it in half-basked state in some of my local Git branches.

> and I'd prefer this using the
> read_avail callbacks that require the data in an array where ever possible.
> Mind you that does the same print at len - 1 as this.  Let's play. 
> Completely untested.
> 
> 	for (i = 0; i < fs_table->fs_len; i++)
> 		len += sysfs_emit_at(buf, len, "0x%09u%c",
> 				     fs_table->fs_avl[i].gain,
> 				     ((i == fs_table->fs_len - 1) ? '\n', ' '));
> 
> better?

Without extra parentheses this makes the job.


> It's definitely not more readable than the above, but it does avoid the write
> to len - 1.
> 
> > In any case, I leave this change to others to comment, I don't object pushing
> > it in this form, either way len - 1 is simply weird.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by Jonathan Cameron 2 months ago 
On Thu, 24 Jul 2025 14:47:31 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jul 23, 2025 at 04:41:00PM +0100, Jonathan Cameron wrote:
> > On Wed, 23 Jul 2025 17:42:28 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > On Wed, Jul 23, 2025 at 07:43:59PM +0530, Akshay Bansod wrote:  
> 
> ...
> 
> > > >  	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 ",
> > > > -				 fs_table->fs_avl[i].gain);
> > > > -	buf[len - 1] = '\n';
> > > > +		len += sysfs_emit_at(buf, len, "0.%09u ",
> > > > +				     fs_table->fs_avl[i].gain);
> > > > +
> > > > +	sysfs_emit_at(buf, len - 1, "\n");    
> > > 
> > > Still looks a bit weird (while working).
> > >   
> > > >  	return len;    
> > > 
> > > I deally we should have a helper doing all this under the hood for plenty of
> > > the (existing) users in the kernel.  
> > 
> > hmm I'm not sure generic is terribly easy  
> 
> I agree, I have some plans for %p specifier extension, but I was stuck with it
> and it in half-basked state in some of my local Git branches.
> 
> > and I'd prefer this using the
> > read_avail callbacks that require the data in an array where ever possible.
> > Mind you that does the same print at len - 1 as this.  Let's play. 
> > Completely untested.
> > 
> > 	for (i = 0; i < fs_table->fs_len; i++)
> > 		len += sysfs_emit_at(buf, len, "0x%09u%c",
> > 				     fs_table->fs_avl[i].gain,
> > 				     ((i == fs_table->fs_len - 1) ? '\n', ' '));
> > 
> > better?  
> 
> Without extra parentheses this makes the job.
> 
Akshay, can you spin a new version along those lines?
> 
> > It's definitely not more readable than the above, but it does avoid the write
> > to len - 1.
> >   
> > > In any case, I leave this change to others to comment, I don't object pushing
> > > it in this form, either way len - 1 is simply weird.  
>
Re: [PATCH v3] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit
Posted by akshay bansod 1 month, 3 weeks ago 
On Saturday, 2 August 2025 10:26 pm +0530 Jonathan Cameron wrote:
> On Thu, 24 Jul 2025 14:47:31 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> > On Wed, Jul 23, 2025 at 04:41:00PM +0100, Jonathan Cameron wrote:
> > > On Wed, 23 Jul 2025 17:42:28 +0300
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > > On Wed, Jul 23, 2025 at 07:43:59PM +0530, Akshay Bansod wrote:  
> > 
> > ...
> > 
> > > > >  	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 ",
> > > > > -				 fs_table->fs_avl[i].gain);
> > > > > -	buf[len - 1] = '\n';
> > > > > +		len += sysfs_emit_at(buf, len, "0.%09u ",
> > > > > +				     fs_table->fs_avl[i].gain);
> > > > > +
> > > > > +	sysfs_emit_at(buf, len - 1, "\n");    
> > > > 
> > > > Still looks a bit weird (while working).
> > > >   
> > > > >  	return len;    
> > > > 
> > > > I deally we should have a helper doing all this under the hood for plenty of
> > > > the (existing) users in the kernel.  
> > > 
> > > hmm I'm not sure generic is terribly easy  
> > 
> > I agree, I have some plans for %p specifier extension, but I was stuck with it
> > and it in half-basked state in some of my local Git branches.
> > 
> > > and I'd prefer this using the
> > > read_avail callbacks that require the data in an array where ever possible.
> > > Mind you that does the same print at len - 1 as this.  Let's play. 
> > > Completely untested.
> > > 
> > > 	for (i = 0; i < fs_table->fs_len; i++)
> > > 		len += sysfs_emit_at(buf, len, "0x%09u%c",
> > > 				     fs_table->fs_avl[i].gain,
> > > 				     ((i == fs_table->fs_len - 1) ? '\n', ' '));
> > > 
> > > better?  
> > 
> > Without extra parentheses this makes the job.
> > 
> Akshay, can you spin a new version along those lines?

Apologies for the delay. Here's the revision
https://lore.kernel.org/linux-iio/20250811165641.1214347-1-akbansd@gmail.com/


> > 
> > > It's definitely not more readable than the above, but it does avoid the write
> > > to len - 1.
> > >   
> > > > In any case, I leave this change to others to comment, I don't object pushing
> > > > it in this form, either way len - 1 is simply weird.  
> > 
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.