| Message ID | 20211215151344.163036-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Light core IIO enhancements | expand |
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
Hi Jonathan, jic23@kernel.org wrote on Sat, 15 Jan 2022 16:56:16 +0000: > 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. 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
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); + 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
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> --- 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(-)