Re: [PATCH] iio: adc: ti-ads8688: use read_avail for available attributes

[Nuno Sá]

On Tue, 2026-03-24 at 11:42 +0000, Nuno Sá wrote:
> On Mon, 2026-03-23 at 21:56 +0000, Gabriel Rondon wrote:
> > Convert the in_voltage_scale_available and in_voltage_offset_available
> > attributes from legacy IIO_DEVICE_ATTR with custom show functions to the
> > IIO framework's read_avail callback. This uses the framework's built-in
> > support for _available attributes, removing the need for manual sysfs
> > formatting.
> > 
> > Precompute the available scale values at probe time since they depend on
> > the reference voltage which does not change after initialization.
> > 
> > Signed-off-by: Gabriel Rondon <grondon@xxxxxxxxx>
> > ---
> 
> Hi Grabriel,
> 
> Thanks for your patch, just some minor nits from me. Anyways:
> 
> Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> 
> >  drivers/iio/adc/ti-ads8688.c | 73 +++++++++++++++++++-----------------
> >  1 file changed, 38 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
> > index b0bf46cae..f37098db9 100644
> > --- a/drivers/iio/adc/ti-ads8688.c
> > +++ b/drivers/iio/adc/ti-ads8688.c
> > @@ -6,7 +6,6 @@
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> > -#include <linux/sysfs.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/err.h>
> > @@ -17,7 +16,6 @@
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > -#include <linux/iio/sysfs.h>
> >  
> >  #define ADS8688_CMD_REG(x)		(x << 8)
> >  #define ADS8688_CMD_REG_NOOP		0x00
> > @@ -66,6 +64,7 @@ struct ads8688_state {
> >  	const struct ads8688_chip_info	*chip_info;
> >  	struct spi_device		*spi;
> >  	unsigned int			vref_mv;
> > +	int				scale_avail[3][2];
> >  	enum ads8688_range		range[8];
> >  	union {
> >  		__be32 d32;
> > @@ -114,37 +113,9 @@ static const struct ads8688_ranges ads8688_range_def[5] = {
> >  	}
> >  };
> >  
> > -static ssize_t ads8688_show_scales(struct device *dev,
> > -				   struct device_attribute *attr, char *buf)
> > -{
> > -	struct ads8688_state *st = iio_priv(dev_to_iio_dev(dev));
> > -
> > -	return sprintf(buf, "0.%09u 0.%09u 0.%09u\n",
> > -		       ads8688_range_def[0].scale * st->vref_mv,
> > -		       ads8688_range_def[1].scale * st->vref_mv,
> > -		       ads8688_range_def[2].scale * st->vref_mv);
> > -}
> > -
> > -static ssize_t ads8688_show_offsets(struct device *dev,
> > -				    struct device_attribute *attr, char *buf)
> > -{
> > -	return sprintf(buf, "%d %d\n", ads8688_range_def[0].offset,
> > -		       ads8688_range_def[3].offset);
> > -}
> > -
> > -static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,
> > -		       ads8688_show_scales, NULL, 0);
> > -static IIO_DEVICE_ATTR(in_voltage_offset_available, S_IRUGO,
> > -		       ads8688_show_offsets, NULL, 0);
> > -
> > -static struct attribute *ads8688_attributes[] = {
> > -	&iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
> > -	&iio_dev_attr_in_voltage_offset_available.dev_attr.attr,
> > -	NULL,
> > -};
> > -
> > -static const struct attribute_group ads8688_attribute_group = {
> > -	.attrs = ads8688_attributes,
> > +static const int ads8688_offset_avail[] = {
> > +	-(1 << (ADS8688_REALBITS - 1)),
> > +	0,
> >  };
> >  
> >  #define ADS8688_CHAN(index)					\
> > @@ -155,6 +126,9 @@ static const struct attribute_group ads8688_attribute_group = {
> >  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)		\
> >  			      | BIT(IIO_CHAN_INFO_SCALE)	\
> >  			      | BIT(IIO_CHAN_INFO_OFFSET),	\
> > +	.info_mask_shared_by_type_available =			\
> > +			      BIT(IIO_CHAN_INFO_SCALE)		\
> > +			      | BIT(IIO_CHAN_INFO_OFFSET),	\
> >  	.scan_index = index,					\
> >  	.scan_type = {						\
> >  		.sign = 'u',					\
> > @@ -369,11 +343,34 @@ static int ads8688_write_raw_get_fmt(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
> > +static int ads8688_read_avail(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      const int **vals, int *type, int *length,
> > +			      long mask)
> > +{
> > +	struct ads8688_state *st = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*vals = (const int *)st->scale_avail;
> > +		*type = IIO_VAL_INT_PLUS_NANO;
> > +		*length = ARRAY_SIZE(st->scale_avail) * 2;
> > +		return IIO_AVAIL_LIST;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		*vals = ads8688_offset_avail;
> > +		*type = IIO_VAL_INT;
> > +		*length = ARRAY_SIZE(ads8688_offset_avail);
> > +		return IIO_AVAIL_LIST;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static const struct iio_info ads8688_info = {
> >  	.read_raw = &ads8688_read_raw,
> > +	.read_avail = &ads8688_read_avail,
> >  	.write_raw = &ads8688_write_raw,
> >  	.write_raw_get_fmt = &ads8688_write_raw_get_fmt,
> > -	.attrs = &ads8688_attribute_group,
> >  };
> >  
> >  static irqreturn_t ads8688_trigger_handler(int irq, void *p)
> > @@ -412,7 +409,7 @@ static int ads8688_probe(struct spi_device *spi)
> >  {
> >  	struct ads8688_state *st;
> >  	struct iio_dev *indio_dev;
> > -	int ret;
> > +	int ret, i;
> 
> These days you could just declare i in the for() loop and make it
> unsigned int.
> 
> >  
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >  	if (indio_dev == NULL)
> > @@ -426,6 +423,12 @@ static int ads8688_probe(struct spi_device *spi)
> >  
> >  	st->vref_mv = ret == -ENODEV ? ADS8688_VREF_MV : ret / 1000;
> >  
> > +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> > +		st->scale_avail[i][0] = 0;
> > +		st->scale_avail[i][1] = ads8688_range_def[i].scale *
> > +					st->vref_mv;
> > +	}
> 
> Even though the above crosses the column preferred limit, I don't think
> the line break hurts readability. 

Naturally I meant that the line break __hurts__ (to me at least :))

- Nuno Sá
> 
> - Nuno Sá
> >