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

[Nuno Sá]

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. 

- Nuno Sá
>