Re: [PATCH RESEND v3 15/17] iio: adc: ad7768-1: replace manual attribute declaration

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 12 Feb 2025 15:18:48 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> Use read_avail callback from struct iio_info to replace the manual
> declaration of sampling_frequency_available attribute.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
> v3 Changes:
> * New patch in v3.
> ---
>  drivers/iio/adc/ad7768-1.c | 58 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 716cf3582577..8aea38c154fe 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -187,6 +187,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.indexed = 1,
>  		.channel = 0,
>  		.scan_index = 0,
> @@ -207,6 +208,7 @@ struct ad7768_state {
>  	unsigned int mclk_freq;
>  	unsigned int dec_rate;
>  	unsigned int samp_freq;
> +	unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_clk_config)];
>  	struct completion completion;
>  	struct iio_trigger *trig;
>  	struct gpio_desc *gpio_sync_in;
> @@ -564,28 +566,6 @@ static int ad7768_set_freq(struct ad7768_state *st,
>  	return 0;
>  }
>  
> -static ssize_t ad7768_sampling_freq_avail(struct device *dev,
> -					  struct device_attribute *attr,
> -					  char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7768_state *st = iio_priv(indio_dev);
> -	unsigned int freq;
> -	int i, len = 0;
> -
> -	for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++) {
> -		freq = DIV_ROUND_CLOSEST(st->mclk_freq,
> -					 ad7768_clk_config[i].clk_div);
> -		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ", freq);
> -	}
> -
> -	buf[len - 1] = '\n';
> -
> -	return len;
> -}
> -
> -static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(ad7768_sampling_freq_avail);
> -
>  static int ad7768_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long info)
> @@ -633,6 +613,29 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int ad7768_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	int i;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++)
> +			st->samp_freq_avail[i] = DIV_ROUND_CLOSEST(st->mclk_freq,
> +								   ad7768_clk_config[i].clk_div);
There are some complex race conditions around these available arrays, so in
general it is better to make it obvious when they are static after
init vs actually dynamic.  In this case I think we can fill this
in the moment we know mclk_freq?  If so please move the calculation into
probe() and just reference it here.

How to close the race condition is an ongoing topic but I don't think
that problem applies here anyway!

Jonathan

> +
> +		*vals = (int *)st->samp_freq_avail;
> +		*length = ARRAY_SIZE(ad7768_clk_config);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int ad7768_write_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long info)
> @@ -655,15 +658,6 @@ static int ad7768_read_label(struct iio_dev *indio_dev,
>  	return sprintf(label, "%s\n", st->labels[chan->channel]);
>  }
>  
> -static struct attribute *ad7768_attributes[] = {
> -	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group ad7768_group = {
> -	.attrs = ad7768_attributes,
> -};
> -
>  static int ad7768_get_current_scan_type(const struct iio_dev *indio_dev,
>  					const struct iio_chan_spec *chan)
>  {
> @@ -674,8 +668,8 @@ static int ad7768_get_current_scan_type(const struct iio_dev *indio_dev,
>  }
>  
>  static const struct iio_info ad7768_info = {
> -	.attrs = &ad7768_group,
>  	.read_raw = &ad7768_read_raw,
> +	.read_avail = &ad7768_read_avail,
>  	.write_raw = &ad7768_write_raw,
>  	.read_label = ad7768_read_label,
>  	.get_current_scan_type = &ad7768_get_current_scan_type,