Re: [PATCH v2] iio: adc: ad9467: Fix the "don't allow reading vref if not available" case

[Jonathan Cameron]

On Fri,  6 Dec 2024 17:39:28 +0100
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:

> The commit in Fixes adds a special case when only one possible scale is
> available.
> If several scales are available, it sets the .read_avail field of the
> struct iio_info to ad9467_read_avail().
> 
> However, this field already holds this function pointer, so the code is a
> no-op.
> 
> Use another struct iio_info instead to actually reflect the intent
> described in the commit message. This way, the structure to use is selected
> at runtime and they can be kept as const.
> 
> This is safer because modifying static structs that are shared between all
> instances like this, based on the properties of a single instance, is
> asking for trouble down the road.
> 
> Fixes: b92f94f74826 ("iio: adc: ad9467: don't allow reading vref if not available")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> ---
> This patch is compile tested only and is completely speculative.
> 
> Changes in v2:
>   - use another struct iio_info to keep the structure const
Agree entirely with David that this is the way to go.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> 
> v1: https://lore.kernel.org/linux-kernel/556f87c8931d7d7cdf56ebc79f974f8bef045b0d.1733431628.git.christophe.jaillet@xxxxxxxxxx/
> ---
>  drivers/iio/adc/ad9467.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index d358958ab310..f30119b42ba0 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -895,7 +895,7 @@ static int ad9467_update_scan_mode(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> -static struct iio_info ad9467_info = {
> +static const struct iio_info ad9467_info = {
>  	.read_raw = ad9467_read_raw,
>  	.write_raw = ad9467_write_raw,
>  	.update_scan_mode = ad9467_update_scan_mode,
> @@ -903,6 +903,14 @@ static struct iio_info ad9467_info = {
>  	.read_avail = ad9467_read_avail,
>  };
>  
> +/* Same as above, but without .read_avail */
> +static const struct iio_info ad9467_info_no_read_avail = {
> +	.read_raw = ad9467_read_raw,
> +	.write_raw = ad9467_write_raw,
> +	.update_scan_mode = ad9467_update_scan_mode,
> +	.debugfs_reg_access = ad9467_reg_access,
> +};
> +
>  static int ad9467_scale_fill(struct ad9467_state *st)
>  {
>  	const struct ad9467_chip_info *info = st->info;
> @@ -1214,11 +1222,12 @@ static int ad9467_probe(struct spi_device *spi)
>  	}
>  
>  	if (st->info->num_scales > 1)
> -		ad9467_info.read_avail = ad9467_read_avail;
> +		indio_dev->info = &ad9467_info;
> +	else
> +		indio_dev->info = &ad9467_info_no_read_avail;
>  	indio_dev->name = st->info->name;
>  	indio_dev->channels = st->info->channels;
>  	indio_dev->num_channels = st->info->num_channels;
> -	indio_dev->info = &ad9467_info;
>  
>  	ret = ad9467_iio_backend_get(st);
>  	if (ret)