Re: [PATCH v2 2/3] staging: iio: ad5933: use sysfs_emit() in show functions

[Jonathan Cameron]

On Fri, 20 Mar 2026 22:24:23 +0000
Gabriel Rondon <grondon@xxxxxxxxx> wrote:

> Replace sprintf() with sysfs_emit() in all sysfs attribute show
> functions. sysfs_emit() is the preferred API for sysfs callbacks as
> it is aware of the PAGE_SIZE buffer limit.
> 
> Also remove the unnecessary (int) cast in ad5933_show_frequency()
> and use the correct format specifier %llu for the unsigned long long
> freqreg variable.
> 
> Signed-off-by: Gabriel Rondon <grondon@xxxxxxxxx>

You should have replied to Andy's comments to say why you aren't
doing the more substantial refactors he asked for.

From my point of view, this driver is far enough from compliant ABI
that the changes suggested need major rewrites of the driver.
So it is very likely all this code will get ripped out before this
leaves staging.  No particular reason you should know that though
unless you've looked much more closely at what is needed.

Hence my main motivation in picking this up is we won't take other
people's time converting these. I doubt anyone is using staging
drivers as a source of information so the usual point of ensuring
best practice in code that might get copied doesn't apply.

Anyhow with all that in mind. Applied to the testing branch of iio.git.

Thanks,

Jonathan

> ---
>  .../staging/iio/impedance-analyzer/ad5933.c   | 24 +++++++++----------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 85a422329..e5a4f8d7a 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -285,7 +285,7 @@ static ssize_t ad5933_show_frequency(struct device *dev,
>  	freqreg = (u64)freqreg * (u64)(st->mclk_hz / 4);
>  	do_div(freqreg, BIT(27));
>  
> -	return sprintf(buf, "%d\n", (int)freqreg);
> +	return sysfs_emit(buf, "%llu\n", freqreg);
>  }
>  
>  static ssize_t ad5933_store_frequency(struct device *dev,
> @@ -338,27 +338,27 @@ static ssize_t ad5933_show(struct device *dev,
>  	mutex_lock(&st->lock);
>  	switch ((u32)this_attr->address) {
>  	case AD5933_OUT_RANGE:
> -		len = sprintf(buf, "%u\n",
> -			      st->range_avail[(st->ctrl_hb >> 1) & 0x3]);
> +		len = sysfs_emit(buf, "%u\n",
> +				 st->range_avail[(st->ctrl_hb >> 1) & 0x3]);
>  		break;
>  	case AD5933_OUT_RANGE_AVAIL:
> -		len = sprintf(buf, "%u %u %u %u\n", st->range_avail[0],
> -			      st->range_avail[3], st->range_avail[2],
> -			      st->range_avail[1]);
> +		len = sysfs_emit(buf, "%u %u %u %u\n", st->range_avail[0],
> +				 st->range_avail[3], st->range_avail[2],
> +				 st->range_avail[1]);
>  		break;
>  	case AD5933_OUT_SETTLING_CYCLES:
> -		len = sprintf(buf, "%d\n", st->settling_cycles);
> +		len = sysfs_emit(buf, "%d\n", st->settling_cycles);
>  		break;
>  	case AD5933_IN_PGA_GAIN:
> -		len = sprintf(buf, "%s\n",
> -			      (st->ctrl_hb & AD5933_CTRL_PGA_GAIN_1) ?
> -			      "1" : "0.2");
> +		len = sysfs_emit(buf, "%s\n",
> +				 (st->ctrl_hb & AD5933_CTRL_PGA_GAIN_1) ?
> +				 "1" : "0.2");
>  		break;
>  	case AD5933_IN_PGA_GAIN_AVAIL:
> -		len = sprintf(buf, "1 0.2\n");
> +		len = sysfs_emit(buf, "1 0.2\n");
>  		break;
>  	case AD5933_FREQ_POINTS:
> -		len = sprintf(buf, "%d\n", st->freq_points);
> +		len = sysfs_emit(buf, "%d\n", st->freq_points);
>  		break;
>  	default:
>  		ret = -EINVAL;