Hi Jonathan,
jic23@xxxxxxxxxx wrote on Sat, 15 Jan 2022 16:56:16 +0000:
> On Wed, 15 Dec 2021 16:13:41 +0100
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> 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.
I personally prefer hiding accesses behind helpers, especially when
there is a move happening, but that's only a personal preference, I'll
drop this helper entirely and limit the use of the getter to
out-of-the-core callers.
> 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.
Yeah that is perfectly right, I didn't notice it when writing the patch.
>
> > ---
> > 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
>
Thanks,
Miquèl
