Re: [PATCH 07/10] iio: core: Hide write accesses to iio_dev->currentmode

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 15 Dec 2021 16:13:41 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> In order to later move this variable within the opaque structure and now
> that there is a read-only helper for it, let's create a write helper.
> 
> The idea behind these changes is to limit the temptation of using
> ->currentmode directly, and this will soon be fully addressed by making  
> the modification to this variable impossible from a device driver by
> moving it to the opaque structure. But for now, let's just do a
> transparent change and use a new helper when ->currentmode needs to be
> accessed for modifications.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

You can probably guess from my comments on the previous but I'd prefer that
we don't do this. We don't need an accessor to do the set so let's skip
doing so.

Given next patch makes the write from drivers impossible anyway we don't
need to do this step.

One more thing inline... No need to export the symbol currently (that
might change if any of the other core modules ever use it but it's not
needed at this time.

> ---
>  drivers/iio/industrialio-buffer.c |  6 +++---
>  drivers/iio/industrialio-core.c   | 11 +++++++++++
>  include/linux/iio/iio.h           |  1 +
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index f4dbab7c44fa..190f9cc5fb2c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1065,7 +1065,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	indio_dev->active_scan_mask = config->scan_mask;
>  	indio_dev->scan_timestamp = config->scan_timestamp;
>  	indio_dev->scan_bytes = config->scan_bytes;
> -	indio_dev->currentmode = config->mode;
> +	iio_set_internal_mode(indio_dev, config->mode);
>  
>  	iio_update_demux(indio_dev);
>  
> @@ -1132,7 +1132,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	if (indio_dev->setup_ops->postdisable)
>  		indio_dev->setup_ops->postdisable(indio_dev);
>  err_undo_config:
> -	indio_dev->currentmode = INDIO_DIRECT_MODE;
> +	iio_set_internal_mode(indio_dev, INDIO_DIRECT_MODE);
>  	indio_dev->active_scan_mask = NULL;
>  
>  	return ret;
> @@ -1181,7 +1181,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>  
>  	iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
>  	indio_dev->active_scan_mask = NULL;
> -	indio_dev->currentmode = INDIO_DIRECT_MODE;
> +	iio_set_internal_mode(indio_dev, INDIO_DIRECT_MODE);
>  
>  	return ret;
>  }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a1d6e30d034a..5c7e0c9e1f59 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2068,6 +2068,17 @@ int iio_get_internal_mode(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL_GPL(iio_get_internal_mode);
>  
> +/**
> + * iio_set_internal_mode() - helper function providing write-only access to the
> + *			     internal @currentmode variable
> + * @indio_dev:		IIO device structure for device
> + **/
> +void iio_set_internal_mode(struct iio_dev *indio_dev, int mode)
> +{
> +	indio_dev->currentmode = mode;
> +}
> +EXPORT_SYMBOL_GPL(iio_set_internal_mode);

If we did do this, you don't need to export it as industrialio_buffer and industriaio_core
are both built into the same module.

> +
>  subsys_initcall(iio_init);
>  module_exit(iio_exit);
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index dcab17f44552..27551d251904 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -679,6 +679,7 @@ struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
>  					   const char *fmt, ...);
>  
>  int iio_get_internal_mode(struct iio_dev *indio_dev);
> +void iio_set_internal_mode(struct iio_dev *indio_dev, int mode);
>  
>  /**
>   * iio_buffer_enabled() - helper function to test if the buffer is enabled