Hi Alexandru,
ardeleanalex@xxxxxxxxx wrote on Thu, 16 Dec 2021 09:48:36 +0200:
> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> <miquel.raynal@xxxxxxxxxxx> wrote:
> >
> > This entry should, under no situation, be modified by device
> > drivers. Now that we have limited its read access to device drivers
> > really needing it, and handled modifications through another dedicated
> > helper within the core, we can easily move this variable to the opaque
> > structure as well as the write helper in order to prevent device driver
> > from playing with it.
> >
>
> Ah, the set-helper becomes inline.
>
> The helpers make more sense now.
>
> [ regardless of the naming of the helpers]
> Reviewed-by: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
Great! Thanks for the entire review and feedback!
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
> > drivers/iio/industrialio-core.c | 15 +++------------
> > include/linux/iio/iio-opaque.h | 16 ++++++++++++++++
> > include/linux/iio/iio.h | 5 -----
> > 3 files changed, 19 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 5c7e0c9e1f59..9836a57ff151 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2064,21 +2064,12 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > **/
> > int iio_get_internal_mode(struct iio_dev *indio_dev)
> > {
> > - return indio_dev->currentmode;
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > +
> > + return iio_dev_opaque->currentmode;
> > }
> > 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);
> > -
> > subsys_initcall(iio_init);
> > module_exit(iio_exit);
> >
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> > index 2be12b7b5dc5..72f35270a387 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -7,6 +7,9 @@
> > * struct iio_dev_opaque - industrial I/O device opaque information
> > * @indio_dev: public industrial I/O device information
> > * @id: used to identify device internally
> > + * @currentmode: operating mode currently in use, may be eventually
> > + * checked by device drivers but should be considered
> > + * read-only as this is a core internal bit
> > * @driver_module: used to make it harder to undercut users
> > * @info_exist_lock: lock to prevent use during removal
> > * @trig_readonly: mark the current trigger immutable
> > @@ -36,6 +39,7 @@
> > */
> > struct iio_dev_opaque {
> > struct iio_dev indio_dev;
> > + int currentmode;
> > int id;
> > struct module *driver_module;
> > struct mutex info_exist_lock;
> > @@ -71,4 +75,16 @@ struct iio_dev_opaque {
> > #define to_iio_dev_opaque(_indio_dev) \
> > container_of((_indio_dev), struct iio_dev_opaque, indio_dev)
> >
> > +/**
> > + * iio_set_internal_mode() - helper function providing write-only access to the
> > + * internal @currentmode variable
> > + * @indio_dev: IIO device structure for device
> > + **/
> > +static inline void iio_set_internal_mode(struct iio_dev *indio_dev, int mode)
> > +{
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > +
> > + iio_dev_opaque->currentmode = mode;
> > +}
> > +
> > #endif
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 27551d251904..d04ab89fa0c2 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -492,9 +492,6 @@ struct iio_buffer_setup_ops {
> > * registering the IIO device and can be filed up by the
> > * IIO core depending on the features advertised by the
> > * driver during other steps of the registration
> > - * @currentmode: [INTERN] operating mode currently in use, may be
> > - * eventually checked by device drivers but should be
> > - * considered read-only as this is a core internal bit
> > * @dev: [DRIVER] device structure, should be assigned a parent
> > * and owner
> > * @buffer: [DRIVER] any buffer present
> > @@ -521,7 +518,6 @@ struct iio_buffer_setup_ops {
> > */
> > struct iio_dev {
> > int modes;
> > - int currentmode;
> > struct device dev;
> >
> > struct iio_buffer *buffer;
> > @@ -679,7 +675,6 @@ 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
> > --
> > 2.27.0
> >
Thanks,
Miquèl
