[PATCH 00/10] Light core IIO enhancements

[PATCH 00/10] Light core IIO enhancements

[Miquel Raynal]

Hello,

As part of a previous discussion with Jonathan (I know, it was three
months ago, sorry for the delay :-$), it appeared necessary to do a
little bit of cleanup in the IIO core and at least provide a little bit
more documentation for developers diving into the IIO subsystem for the
first time. My first move was to write a blog post trying to explain
(from my point of view but with the review of Jonathan) what is
necessary to understand when writing and IIO kernel driver:
https://bootlin.com/blog/the-backbone-of-a-linux-industrial-i-o-driver/

Here is now the second halve of this work, with few things that I tried
to address.

* The currentmode indio_dev entry was misused by several device drivers,
  I tried to understand which ones were wrong (there are two still
  accessing this variable left, I decided not to touch them because I
  think it is legitimate) and I tried to find an alternative.

* Then, I tried to make new accesses to this variable harder in order to
  avoid future mistakes, by creating a couple of helpers and moving it
  to the opaque structure.

* I continued with the IIO modes which are completely
  undocumented. Following a few exchanges with Jonathan, I tried to take
  most of this discussion and put it into a kernel doc header.

* When doing the above task, I realized that there was a particular case
  handled for kfifo buffers which I tried to simplify, but if this is a
  mistake, I'll just drop the patch.

Hope this will help!

Cheers,
Miquèl

Miquel Raynal (10):
  iio: core: Fix the kernel doc regarding the currentmode iio_dev entry
  iio: core: Enhance the kernel doc of modes and currentmodes iio_dev
    entries
  iio: magnetometer: rm3100: Stop abusing the ->currentmode
  iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode
  iio: st_sensors: Use iio_device_claim/release_direct_mode() when
    relevant
  iio: core: Hide read accesses to iio_dev->currentmode
  iio: core: Hide write accesses to iio_dev->currentmode
  iio: core: Move iio_dev->currentmode to the opaque structure
  iio: core: Simplify the registration of kfifo buffers
  iio: core: Clarify the modes

 drivers/iio/accel/bmc150-accel-core.c         |  4 +-
 drivers/iio/accel/fxls8962af-core.c           |  1 -
 drivers/iio/accel/sca3000.c                   |  1 -
 drivers/iio/accel/ssp_accel_sensor.c          |  1 -
 drivers/iio/adc/at91-sama5d2_adc.c            |  4 +-
 drivers/iio/adc/ina2xx-adc.c                  |  1 -
 drivers/iio/adc/stm32-dfsdm-adc.c             |  5 +-
 drivers/iio/adc/ti_am335x_adc.c               |  4 +-
 drivers/iio/buffer/kfifo_buf.c                | 12 +---
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  3 +-
 drivers/iio/common/scmi_sensors/scmi_iio.c    |  1 -
 .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++----
 drivers/iio/gyro/ssp_gyro_sensor.c            |  1 -
 drivers/iio/health/max30100.c                 |  1 -
 drivers/iio/health/max30102.c                 |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_accel.c |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  |  1 -
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  1 -
 drivers/iio/industrialio-buffer.c             | 12 ++--
 drivers/iio/industrialio-core.c               | 13 +++++
 drivers/iio/industrialio-trigger.c            |  2 +-
 drivers/iio/light/apds9960.c                  |  1 -
 drivers/iio/magnetometer/rm3100-core.c        | 15 +----
 .../staging/iio/impedance-analyzer/ad5933.c   |  1 -
 include/linux/iio/iio-opaque.h                | 16 ++++++
 include/linux/iio/iio.h                       | 57 ++++++++++++++++---
 include/linux/iio/kfifo_buf.h                 |  5 +-
 27 files changed, 114 insertions(+), 79 deletions(-)

-- 
2.27.0

[PATCH 01/10] iio: core: Fix the kernel doc regarding the currentmode iio_dev entry

[Miquel Raynal]

This is an internal variable, which should be accessed in a very
sporadic way and in no case changed by any device driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 324561b7a5e8..06433c2c2968 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -488,7 +488,7 @@ struct iio_buffer_setup_ops {
 /**
  * struct iio_dev - industrial I/O device
  * @modes:		[DRIVER] operating modes supported by device
- * @currentmode:	[DRIVER] current operating mode
+ * @currentmode:	[INTERN] current operating mode
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @buffer:		[DRIVER] any buffer present
-- 
2.27.0

Re: [PATCH 01/10] iio: core: Fix the kernel doc regarding the currentmode iio_dev entry

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> This is an internal variable, which should be accessed in a very
> sporadic way and in no case changed by any device driver.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/iio/iio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 324561b7a5e8..06433c2c2968 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -488,7 +488,7 @@ struct iio_buffer_setup_ops {
>  /**
>   * struct iio_dev - industrial I/O device
>   * @modes:             [DRIVER] operating modes supported by device
> - * @currentmode:       [DRIVER] current operating mode
> + * @currentmode:       [INTERN] current operating mode
>   * @dev:               [DRIVER] device structure, should be assigned a parent
>   *                     and owner
>   * @buffer:            [DRIVER] any buffer present
> --
> 2.27.0
>

Re: [PATCH 01/10] iio: core: Fix the kernel doc regarding the currentmode iio_dev entry

[Jonathan Cameron]

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

Sorry for taking so long to get to these. I knew some of them were
likely to take some thinking so they kept falling at the end of
my review list and I kept running out of time / energy!

Anyhow, time to make some progress.

> This is an internal variable, which should be accessed in a very
> sporadic way and in no case changed by any device driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Hohum.  Plenty of history around this one but I think you are right
that it now makes sense to mark it as INTERN (and when we can move it
over to the opaque structure).

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  include/linux/iio/iio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 324561b7a5e8..06433c2c2968 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -488,7 +488,7 @@ struct iio_buffer_setup_ops {
>  /**
>   * struct iio_dev - industrial I/O device
>   * @modes:		[DRIVER] operating modes supported by device
> - * @currentmode:	[DRIVER] current operating mode
> + * @currentmode:	[INTERN] current operating mode
>   * @dev:		[DRIVER] device structure, should be assigned a parent
>   *			and owner
>   * @buffer:		[DRIVER] any buffer present

[PATCH 02/10] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Miquel Raynal]

Let's provide more details about these two variables because their
understanding may not be straightforward for someone not used to the IIO
subsystem internal logic. The different modes will soon be also be more
documented for the same reason.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 06433c2c2968..0312da2e83f8 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -487,8 +487,14 @@ struct iio_buffer_setup_ops {
 
 /**
  * struct iio_dev - industrial I/O device
- * @modes:		[DRIVER] operating modes supported by device
- * @currentmode:	[INTERN] current operating mode
+ * @modes:		[DRIVER] list of operating modes supported by the IIO
+ *			device, this list should be initialized before
+ *			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
-- 
2.27.0

Re: [PATCH 02/10] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Let's provide more details about these two variables because their
> understanding may not be straightforward for someone not used to the IIO
> subsystem internal logic. The different modes will soon be also be more
> documented for the same reason.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/iio/iio.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 06433c2c2968..0312da2e83f8 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops {
>
>  /**
>   * struct iio_dev - industrial I/O device
> - * @modes:             [DRIVER] operating modes supported by device
> - * @currentmode:       [INTERN] current operating mode
> + * @modes:             [DRIVER] list of operating modes supported by the IIO

I'd argue that it may make sense to highlight that this list of modes
is represented as a bitmask.
When reading docs, it may not be obvious at first (I guess).

> + *                     device, this list should be initialized before
> + *                     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
> --
> 2.27.0
>

Re: [PATCH 02/10] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Miquel Raynal]

Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Let's provide more details about these two variables because their
> > understanding may not be straightforward for someone not used to the IIO
> > subsystem internal logic. The different modes will soon be also be more
> > documented for the same reason.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/linux/iio/iio.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 06433c2c2968..0312da2e83f8 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops {
> >
> >  /**
> >   * struct iio_dev - industrial I/O device
> > - * @modes:             [DRIVER] operating modes supported by device
> > - * @currentmode:       [INTERN] current operating mode
> > + * @modes:             [DRIVER] list of operating modes supported by the IIO  
> 
> I'd argue that it may make sense to highlight that this list of modes
> is represented as a bitmask.
> When reading docs, it may not be obvious at first (I guess).

That is a good idea. I'll add this to the next iteration.

> > + *                     device, this list should be initialized before
> > + *                     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
> > --
> > 2.27.0
> >  

Thanks,
Miquèl

Re: [PATCH 02/10] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:15:52 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > Let's provide more details about these two variables because their
> > > understanding may not be straightforward for someone not used to the IIO
> > > subsystem internal logic. The different modes will soon be also be more
> > > documented for the same reason.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/linux/iio/iio.h | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index 06433c2c2968..0312da2e83f8 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops {
> > >
> > >  /**
> > >   * struct iio_dev - industrial I/O device
> > > - * @modes:             [DRIVER] operating modes supported by device
> > > - * @currentmode:       [INTERN] current operating mode
> > > + * @modes:             [DRIVER] list of operating modes supported by the IIO    
> > 
> > I'd argue that it may make sense to highlight that this list of modes
> > is represented as a bitmask.
> > When reading docs, it may not be obvious at first (I guess).  
> 
> That is a good idea. I'll add this to the next iteration.

Good.

> 
> > > + *                     device, this list should be initialized before
> > > + *                     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

Perhaps make it a little clearer that some bits are set as a result of
enabling particular features.  Maybe even call out which functions do it.
From what I recall, that's a very short list.

> > > + * @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

We should add an accessor function to read it.  (maybe later in your series? :)

> > >   * @dev:               [DRIVER] device structure, should be assigned a parent
> > >   *                     and owner
> > >   * @buffer:            [DRIVER] any buffer present
> > > --
> > > 2.27.0
> > >    
> 
> Thanks,
> Miquèl

Re: [PATCH 02/10] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 15:51:45 +0000:

> On Thu, 16 Dec 2021 09:15:52 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Alexandru,
> > 
> > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:24:51 +0200:
> >   
> > > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:    
> > > >
> > > > Let's provide more details about these two variables because their
> > > > understanding may not be straightforward for someone not used to the IIO
> > > > subsystem internal logic. The different modes will soon be also be more
> > > > documented for the same reason.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/linux/iio/iio.h | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > index 06433c2c2968..0312da2e83f8 100644
> > > > --- a/include/linux/iio/iio.h
> > > > +++ b/include/linux/iio/iio.h
> > > > @@ -487,8 +487,14 @@ struct iio_buffer_setup_ops {
> > > >
> > > >  /**
> > > >   * struct iio_dev - industrial I/O device
> > > > - * @modes:             [DRIVER] operating modes supported by device
> > > > - * @currentmode:       [INTERN] current operating mode
> > > > + * @modes:             [DRIVER] list of operating modes supported by the IIO      
> > > 
> > > I'd argue that it may make sense to highlight that this list of modes
> > > is represented as a bitmask.
> > > When reading docs, it may not be obvious at first (I guess).    
> > 
> > That is a good idea. I'll add this to the next iteration.  
> 
> Good.

Done.

> 
> >   
> > > > + *                     device, this list should be initialized before
> > > > + *                     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  
> 
> Perhaps make it a little clearer that some bits are set as a result of
> enabling particular features.  Maybe even call out which functions do it.
> From what I recall, that's a very short list.

Done.

> 
> > > > + * @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  
> 
> We should add an accessor function to read it.  (maybe later in your series? :)

Yup!

> 
> > > >   * @dev:               [DRIVER] device structure, should be assigned a parent
> > > >   *                     and owner
> > > >   * @buffer:            [DRIVER] any buffer present
> > > > --
> > > > 2.27.0
> > > >      
> > 
> > Thanks,
> > Miquèl  
> 


Thanks,
Miquèl

[PATCH 03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Miquel Raynal]

This is an internal variable for the core, here it is set to a "default"
value by the driver in order to later be able to perform checks against
it. None of this is needed because this check actually cares about the
buffers being enabled or not. So it is an unproper side-channel access
to the information "are the buffers enabled?", returned officially by
the iio_buffer_enabled() helper. Use this helper instead.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 13914273c999..be0057f82218 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
 	struct iio_dev *indio_dev = d;
 	struct rm3100_data *data = iio_priv(indio_dev);
 
-	switch (indio_dev->currentmode) {
-	case INDIO_DIRECT_MODE:
+	if (!iio_buffer_enabled(indio_dev))
 		complete(&data->measuring_done);
-		break;
-	case INDIO_BUFFER_TRIGGERED:
+	else
 		iio_trigger_poll(data->drdy_trig);
-		break;
-	default:
-		dev_err(indio_dev->dev.parent,
-			"device mode out of control, current mode: %d",
-			indio_dev->currentmode);
-	}
 
 	return IRQ_WAKE_THREAD;
 }
@@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 			goto unlock_return;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
@@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
 	indio_dev->channels = rm3100_channels;
 	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
 
 	if (!irq)
 		data->use_interrupt = false;
-- 
2.27.0

Re: [PATCH 03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> This is an internal variable for the core, here it is set to a "default"
> value by the driver in order to later be able to perform checks against
> it. None of this is needed because this check actually cares about the
> buffers being enabled or not. So it is an unproper side-channel access
> to the information "are the buffers enabled?", returned officially by
> the iio_buffer_enabled() helper. Use this helper instead.

A few comments inline.

>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> index 13914273c999..be0057f82218 100644
> --- a/drivers/iio/magnetometer/rm3100-core.c
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
>         struct iio_dev *indio_dev = d;
>         struct rm3100_data *data = iio_priv(indio_dev);
>
> -       switch (indio_dev->currentmode) {
> -       case INDIO_DIRECT_MODE:
> +       if (!iio_buffer_enabled(indio_dev))
>                 complete(&data->measuring_done);
> -               break;
> -       case INDIO_BUFFER_TRIGGERED:

This is one of those semantic changes that looks correct, but may end
up getting comments that it should be validated by someone with
hardware [and for good reason].
Especially in places like Ref1 (below).

But I guess the iio_get_internal_mode() helper could still be used.
I guess I'd wait for more opinions on this.

> +       else
>                 iio_trigger_poll(data->drdy_trig);
> -               break;
> -       default:
> -               dev_err(indio_dev->dev.parent,
> -                       "device mode out of control, current mode: %d",
> -                       indio_dev->currentmode);
> -       }
>
>         return IRQ_WAKE_THREAD;
>  }
> @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
>                         goto unlock_return;
>         }
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {

Ref1

> +       if (iio_buffer_enabled(indio_dev)) {
>                 /* Writing TMRC registers requires CMM reset. */
>                 ret = regmap_write(regmap, RM3100_REG_CMM, 0);
>                 if (ret < 0)
> @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
>         indio_dev->channels = rm3100_channels;
>         indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
>         indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> -       indio_dev->currentmode = INDIO_DIRECT_MODE;

This is fine :)

>
>         if (!irq)
>                 data->use_interrupt = false;
> --
> 2.27.0
>

Re: [PATCH 03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Miquel Raynal]

Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:40:12 +0200:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > This is an internal variable for the core, here it is set to a "default"
> > value by the driver in order to later be able to perform checks against
> > it. None of this is needed because this check actually cares about the
> > buffers being enabled or not. So it is an unproper side-channel access
> > to the information "are the buffers enabled?", returned officially by
> > the iio_buffer_enabled() helper. Use this helper instead.  
> 
> A few comments inline.
> 
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
> >  1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> > index 13914273c999..be0057f82218 100644
> > --- a/drivers/iio/magnetometer/rm3100-core.c
> > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
> >         struct iio_dev *indio_dev = d;
> >         struct rm3100_data *data = iio_priv(indio_dev);
> >
> > -       switch (indio_dev->currentmode) {
> > -       case INDIO_DIRECT_MODE:
> > +       if (!iio_buffer_enabled(indio_dev))
> >                 complete(&data->measuring_done);
> > -               break;
> > -       case INDIO_BUFFER_TRIGGERED:  
> 
> This is one of those semantic changes that looks correct, but may end
> up getting comments that it should be validated by someone with
> hardware [and for good reason].
> Especially in places like Ref1 (below).

I think here we are pretty safe assuming that the change will not break
the driver because this is a construction that is very common in IIO
drivers.

Below, as stated in the cover letter, this is just my own
understanding and I'll happily drop the change if someone
thinks/observes this is unsafe.

> But I guess the iio_get_internal_mode() helper could still be used.
> I guess I'd wait for more opinions on this.
> 
> > +       else
> >                 iio_trigger_poll(data->drdy_trig);
> > -               break;
> > -       default:
> > -               dev_err(indio_dev->dev.parent,
> > -                       "device mode out of control, current mode: %d",
> > -                       indio_dev->currentmode);
> > -       }
> >
> >         return IRQ_WAKE_THREAD;
> >  }
> > @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
> >                         goto unlock_return;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {  
> 
> Ref1
> 
> > +       if (iio_buffer_enabled(indio_dev)) {
> >                 /* Writing TMRC registers requires CMM reset. */
> >                 ret = regmap_write(regmap, RM3100_REG_CMM, 0);
> >                 if (ret < 0)
> > @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
> >         indio_dev->channels = rm3100_channels;
> >         indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
> >         indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> > -       indio_dev->currentmode = INDIO_DIRECT_MODE;  
> 
> This is fine :)
> 
> >
> >         if (!irq)
> >                 data->use_interrupt = false;
> > --
> > 2.27.0
> >  


Thanks,
Miquèl

Re: [PATCH 03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Jonathan Cameron]

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

> This is an internal variable for the core, here it is set to a "default"
> value by the driver in order to later be able to perform checks against
> it. None of this is needed because this check actually cares about the
> buffers being enabled or not. So it is an unproper side-channel access
> to the information "are the buffers enabled?", returned officially by
> the iio_buffer_enabled() helper. Use this helper instead.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Hi Miquel,

Make sure to +cc driver authors on v2.

Whilst I think this is safe we should definitely give them visibility.

Obviously some IIO drivers probably have authors who have long moved on
but this one is only 2018 vintage so Song Qiang might still have
access to hardware or be willing to do a review!

Jonathan


> ---
>  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> index 13914273c999..be0057f82218 100644
> --- a/drivers/iio/magnetometer/rm3100-core.c
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
>  	struct iio_dev *indio_dev = d;
>  	struct rm3100_data *data = iio_priv(indio_dev);
>  
> -	switch (indio_dev->currentmode) {
> -	case INDIO_DIRECT_MODE:
> +	if (!iio_buffer_enabled(indio_dev))
>  		complete(&data->measuring_done);
> -		break;
> -	case INDIO_BUFFER_TRIGGERED:
> +	else
>  		iio_trigger_poll(data->drdy_trig);
> -		break;
> -	default:
> -		dev_err(indio_dev->dev.parent,
> -			"device mode out of control, current mode: %d",
> -			indio_dev->currentmode);
> -	}
>  
>  	return IRQ_WAKE_THREAD;
>  }
> @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
>  			goto unlock_return;
>  	}
>  
> -	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +	if (iio_buffer_enabled(indio_dev)) {
>  		/* Writing TMRC registers requires CMM reset. */
>  		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
>  		if (ret < 0)
> @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
>  	indio_dev->channels = rm3100_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
>  	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> -	indio_dev->currentmode = INDIO_DIRECT_MODE;
>  
>  	if (!irq)
>  		data->use_interrupt = false;

Re: [PATCH 03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 15:58:32 +0000:

> On Wed, 15 Dec 2021 16:13:37 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > This is an internal variable for the core, here it is set to a "default"
> > value by the driver in order to later be able to perform checks against
> > it. None of this is needed because this check actually cares about the
> > buffers being enabled or not. So it is an unproper side-channel access
> > to the information "are the buffers enabled?", returned officially by
> > the iio_buffer_enabled() helper. Use this helper instead.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> Hi Miquel,
> 
> Make sure to +cc driver authors on v2.
> 
> Whilst I think this is safe we should definitely give them visibility.
> 
> Obviously some IIO drivers probably have authors who have long moved on
> but this one is only 2018 vintage so Song Qiang might still have
> access to hardware or be willing to do a review!

Right, I've added Song Qiang into a Cc: tag for this patch for the next
iteration.

Thanks,
Miquèl

[PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

This is an internal variable of the core, let's use the
iio_buffer_enabled() helper which is exported for the following purpose:
telling if the current mode is a buffered mode, which is precisely what
this driver looks for.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 1cfefb3b5e56..a3b8827d3bbf 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
 	 * In continuous mode, use fast mode configuration,
 	 * if it provides a better resolution.
 	 */
-	if (adc->nconv == 1 && !trig &&
-	    (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
+	if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {
 		if (fl->flo[1].res >= fl->flo[0].res) {
 			fl->fast = 1;
 			flo = &fl->flo[1];
@@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
 		cr1 = DFSDM_CR1_RCH(chan->channel);
 
 		/* Continuous conversions triggered by SPI clk in buffer mode */
-		if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
+		if (iio_buffer_enabled(indio_dev))
 			cr1 |= DFSDM_CR1_RCONT(1);
 
 		cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
-- 
2.27.0

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> This is an internal variable of the core, let's use the
> iio_buffer_enabled() helper which is exported for the following purpose:
> telling if the current mode is a buffered mode, which is precisely what
> this driver looks for.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 1cfefb3b5e56..a3b8827d3bbf 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
>          * In continuous mode, use fast mode configuration,
>          * if it provides a better resolution.
>          */
> -       if (adc->nconv == 1 && !trig &&
> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {

This may become tricky if other modes get added later.
STM does a relatively good job in updating and re-using their drivers
(even if some of them do look quirky sometimes).

So, the question here would be: is "iio_buffer_enabled(indio_dev)"
going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
INDIO_BUFFER_HARDWARE get added?

I'd also ping some STM people for some feedback, acks or testing.

>                 if (fl->flo[1].res >= fl->flo[0].res) {
>                         fl->fast = 1;
>                         flo = &fl->flo[1];
> @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
>                 cr1 = DFSDM_CR1_RCH(chan->channel);
>
>                 /* Continuous conversions triggered by SPI clk in buffer mode */
> -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> +               if (iio_buffer_enabled(indio_dev))
>                         cr1 |= DFSDM_CR1_RCONT(1);
>
>                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> --
> 2.27.0
>

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:

> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > This is an internal variable of the core, let's use the
> > iio_buffer_enabled() helper which is exported for the following purpose:
> > telling if the current mode is a buffered mode, which is precisely what
> > this driver looks for.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> >          * In continuous mode, use fast mode configuration,
> >          * if it provides a better resolution.
> >          */
> > -       if (adc->nconv == 1 && !trig &&
> > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {  
> 
> This may become tricky if other modes get added later.
> STM does a relatively good job in updating and re-using their drivers
> (even if some of them do look quirky sometimes).
> 
> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> INDIO_BUFFER_HARDWARE get added?

I would argue, is this a real problem? Today iio_buffer_enabled() seem
to handle well what this driver is expecting. If tomorrow someone adds
another mode, that is his/her responsibility to state "okay, this
section is not common to all buffer styles *anymore*, so we need to do
a more fine grained check against ->currentmodes than
iio_buffer_enabled() does". In that case using the ->currentmodes
getter would be the right way to go, but only at that particular
moment, not today.

> 
> I'd also ping some STM people for some feedback, acks or testing.
> 
> >                 if (fl->flo[1].res >= fl->flo[0].res) {
> >                         fl->fast = 1;
> >                         flo = &fl->flo[1];
> > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> >
> >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > +               if (iio_buffer_enabled(indio_dev))
> >                         cr1 |= DFSDM_CR1_RCONT(1);
> >
> >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > --
> > 2.27.0
> >  


Thanks,
Miquèl

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:22:35 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > This is an internal variable of the core, let's use the
> > > iio_buffer_enabled() helper which is exported for the following purpose:
> > > telling if the current mode is a buffered mode, which is precisely what
> > > this driver looks for.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> > >          * In continuous mode, use fast mode configuration,
> > >          * if it provides a better resolution.
> > >          */
> > > -       if (adc->nconv == 1 && !trig &&
> > > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {    
> > 
> > This may become tricky if other modes get added later.
> > STM does a relatively good job in updating and re-using their drivers
> > (even if some of them do look quirky sometimes).

Their hardware is crazy/complicated so tends to push the limits!

> > 
> > So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> > going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> > INDIO_BUFFER_HARDWARE get added?  
> 
> I would argue, is this a real problem? Today iio_buffer_enabled() seem
> to handle well what this driver is expecting. If tomorrow someone adds
> another mode, that is his/her responsibility to state "okay, this
> section is not common to all buffer styles *anymore*, so we need to do
> a more fine grained check against ->currentmodes than
> iio_buffer_enabled() does". In that case using the ->currentmodes
> getter would be the right way to go, but only at that particular
> moment, not today.

It should be isolated to this driver, so I think it is fine to use
the broader check today, but I'll leave this to the st folks as
it's their driver and I don't feel that strongly about it.

> 
> > 
> > I'd also ping some STM people for some feedback, acks or testing.

Definitely on this - they are an active bunch who do a great job of looking
after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
others are on v2 cc list).


> >   
> > >                 if (fl->flo[1].res >= fl->flo[0].res) {
> > >                         fl->fast = 1;
> > >                         flo = &fl->flo[1];
> > > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> > >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> > >
> > >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > > +               if (iio_buffer_enabled(indio_dev))
> > >                         cr1 |= DFSDM_CR1_RCONT(1);
> > >
> > >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > > --
> > > 2.27.0
> > >    
> 
> 
> Thanks,
> Miquèl

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:

> On Thu, 16 Dec 2021 09:22:35 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Alexandru,
> > 
> > ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> >   
> > > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:    
> > > >
> > > > This is an internal variable of the core, let's use the
> > > > iio_buffer_enabled() helper which is exported for the following purpose:
> > > > telling if the current mode is a buffered mode, which is precisely what
> > > > this driver looks for.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > index 1cfefb3b5e56..a3b8827d3bbf 100644
> > > > --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> > > > @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> > > >          * In continuous mode, use fast mode configuration,
> > > >          * if it provides a better resolution.
> > > >          */
> > > > -       if (adc->nconv == 1 && !trig &&
> > > > -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> > > > +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {      
> > > 
> > > This may become tricky if other modes get added later.
> > > STM does a relatively good job in updating and re-using their drivers
> > > (even if some of them do look quirky sometimes).  
> 
> Their hardware is crazy/complicated so tends to push the limits!
> 
> > > 
> > > So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> > > going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> > > INDIO_BUFFER_HARDWARE get added?    
> > 
> > I would argue, is this a real problem? Today iio_buffer_enabled() seem
> > to handle well what this driver is expecting. If tomorrow someone adds
> > another mode, that is his/her responsibility to state "okay, this
> > section is not common to all buffer styles *anymore*, so we need to do
> > a more fine grained check against ->currentmodes than
> > iio_buffer_enabled() does". In that case using the ->currentmodes
> > getter would be the right way to go, but only at that particular
> > moment, not today.  
> 
> It should be isolated to this driver, so I think it is fine to use
> the broader check today, but I'll leave this to the st folks as
> it's their driver and I don't feel that strongly about it.
> 
> >   
> > > 
> > > I'd also ping some STM people for some feedback, acks or testing.  
> 
> Definitely on this - they are an active bunch who do a great job of looking
> after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
> others are on v2 cc list).
> 

I'll add Olivier Moysan as well in the next version who has been quite
active on this driver as well according to git log.

> 
> > >     
> > > >                 if (fl->flo[1].res >= fl->flo[0].res) {
> > > >                         fl->fast = 1;
> > > >                         flo = &fl->flo[1];
> > > > @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
> > > >                 cr1 = DFSDM_CR1_RCH(chan->channel);
> > > >
> > > >                 /* Continuous conversions triggered by SPI clk in buffer mode */
> > > > -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> > > > +               if (iio_buffer_enabled(indio_dev))
> > > >                         cr1 |= DFSDM_CR1_RCONT(1);
> > > >
> > > >                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> > > > --
> > > > 2.27.0
> > > >      
> > 
> > 
> > Thanks,
> > Miquèl  
> 


Thanks,
Miquèl

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Fabrice Gasnier]

On 1/28/22 4:04 PM, Miquel Raynal wrote:
> Hi Jonathan,
> 
> jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:
> 
>> On Thu, 16 Dec 2021 09:22:35 +0100
>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>>> Hi Alexandru,
>>>
>>> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
>>>   
>>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
>>>> <miquel.raynal@bootlin.com> wrote:    
>>>>>
>>>>> This is an internal variable of the core, let's use the
>>>>> iio_buffer_enabled() helper which is exported for the following purpose:
>>>>> telling if the current mode is a buffered mode, which is precisely what
>>>>> this driver looks for.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
>>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> index 1cfefb3b5e56..a3b8827d3bbf 100644
>>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
>>>>> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
>>>>>          * In continuous mode, use fast mode configuration,
>>>>>          * if it provides a better resolution.
>>>>>          */
>>>>> -       if (adc->nconv == 1 && !trig &&
>>>>> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
>>>>> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {      
>>>>
>>>> This may become tricky if other modes get added later.
>>>> STM does a relatively good job in updating and re-using their drivers
>>>> (even if some of them do look quirky sometimes).  
>>
>> Their hardware is crazy/complicated so tends to push the limits!
>>
>>>>
>>>> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
>>>> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
>>>> INDIO_BUFFER_HARDWARE get added?    
>>>
>>> I would argue, is this a real problem? Today iio_buffer_enabled() seem
>>> to handle well what this driver is expecting. If tomorrow someone adds
>>> another mode, that is his/her responsibility to state "okay, this
>>> section is not common to all buffer styles *anymore*, so we need to do
>>> a more fine grained check against ->currentmodes than
>>> iio_buffer_enabled() does". In that case using the ->currentmodes
>>> getter would be the right way to go, but only at that particular
>>> moment, not today.  
>>
>> It should be isolated to this driver, so I think it is fine to use
>> the broader check today, but I'll leave this to the st folks as
>> it's their driver and I don't feel that strongly about it.

Hi Miquel, Alexandru, Jonathan, all,

First, sorry for the delay.

Indeed, I don't expect any functional changes here by using
iio_buffer_enabled(indio_dev).
So it should be fine to use it. You're right, the driver looks for
buffer mode in both places where this gets used.

Just an additional statement is: the driver also checks for no trigger,
and single channel in both places (to select desired mode in the dfsdm).
As I see, only INDIO_BUFFER_SOFTWARE is expected then.

For my own understanding (I'm just asking), why not using the
currentmodes getter routine ?

I've had a look at the whole series [1], It seems used elsewhere. I may
miss something... It would be 100% equivalent to current code to use:
iio_get_internal_mode(indio_dev) & INDIO_BUFFER_SOFTWARE ?

This would be safe in case new modes gets introduced later ?
(another note: unless these new modes gets set by default in the 'modes'
field, this should have no impact here as well anyway ?)

[1]
https://lore.kernel.org/linux-iio/CA+U=DsoMLD1EpK7yDXaQ_HwTQgm_TeZvM_eykE6jWYgg6P3Y7w@mail.gmail.com/T/


>>
>>>   
>>>>
>>>> I'd also ping some STM people for some feedback, acks or testing.  
>>
>> Definitely on this - they are an active bunch who do a great job of looking
>> after these drivers.  I've cc'd Fabrice. Make sure he (and possibly some
>> others are on v2 cc list).
>>
> 
> I'll add Olivier Moysan as well in the next version who has been quite
> active on this driver as well according to git log.

Indeed, please add both Olivier and I next time.

Best Regards,
Thanks,
Fabrice

> 
>>
>>>>     
>>>>>                 if (fl->flo[1].res >= fl->flo[0].res) {
>>>>>                         fl->fast = 1;
>>>>>                         flo = &fl->flo[1];
>>>>> @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
>>>>>                 cr1 = DFSDM_CR1_RCH(chan->channel);
>>>>>
>>>>>                 /* Continuous conversions triggered by SPI clk in buffer mode */
>>>>> -               if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
>>>>> +               if (iio_buffer_enabled(indio_dev))
>>>>>                         cr1 |= DFSDM_CR1_RCONT(1);
>>>>>
>>>>>                 cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
>>>>> --
>>>>> 2.27.0
>>>>>      
>>>
>>>
>>> Thanks,
>>> Miquèl  
>>
> 
> 
> Thanks,
> Miquèl
> 

Re: [PATCH 04/10] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

Hi Fabrice,

fabrice.gasnier@foss.st.com wrote on Tue, 1 Feb 2022 09:41:03 +0100:

> On 1/28/22 4:04 PM, Miquel Raynal wrote:
> > Hi Jonathan,
> > 
> > jic23@kernel.org wrote on Sat, 15 Jan 2022 16:06:19 +0000:
> >   
> >> On Thu, 16 Dec 2021 09:22:35 +0100
> >> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>  
> >>> Hi Alexandru,
> >>>
> >>> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:47:02 +0200:
> >>>     
> >>>> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> >>>> <miquel.raynal@bootlin.com> wrote:      
> >>>>>
> >>>>> This is an internal variable of the core, let's use the
> >>>>> iio_buffer_enabled() helper which is exported for the following purpose:
> >>>>> telling if the current mode is a buffered mode, which is precisely what
> >>>>> this driver looks for.
> >>>>>
> >>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> ---
> >>>>>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
> >>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> index 1cfefb3b5e56..a3b8827d3bbf 100644
> >>>>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> >>>>> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
> >>>>>          * In continuous mode, use fast mode configuration,
> >>>>>          * if it provides a better resolution.
> >>>>>          */
> >>>>> -       if (adc->nconv == 1 && !trig &&
> >>>>> -           (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> >>>>> +       if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {        
> >>>>
> >>>> This may become tricky if other modes get added later.
> >>>> STM does a relatively good job in updating and re-using their drivers
> >>>> (even if some of them do look quirky sometimes).    
> >>
> >> Their hardware is crazy/complicated so tends to push the limits!
> >>  
> >>>>
> >>>> So, the question here would be: is "iio_buffer_enabled(indio_dev)"
> >>>> going to be valid [in this place] once INDIO_BUFFER_TRIGGERED or
> >>>> INDIO_BUFFER_HARDWARE get added?      
> >>>
> >>> I would argue, is this a real problem? Today iio_buffer_enabled() seem
> >>> to handle well what this driver is expecting. If tomorrow someone adds
> >>> another mode, that is his/her responsibility to state "okay, this
> >>> section is not common to all buffer styles *anymore*, so we need to do
> >>> a more fine grained check against ->currentmodes than
> >>> iio_buffer_enabled() does". In that case using the ->currentmodes
> >>> getter would be the right way to go, but only at that particular
> >>> moment, not today.    
> >>
> >> It should be isolated to this driver, so I think it is fine to use
> >> the broader check today, but I'll leave this to the st folks as
> >> it's their driver and I don't feel that strongly about it.  
> 
> Hi Miquel, Alexandru, Jonathan, all,
> 
> First, sorry for the delay.
> 
> Indeed, I don't expect any functional changes here by using
> iio_buffer_enabled(indio_dev).
> So it should be fine to use it. You're right, the driver looks for
> buffer mode in both places where this gets used.
> 
> Just an additional statement is: the driver also checks for no trigger,
> and single channel in both places (to select desired mode in the dfsdm).
> As I see, only INDIO_BUFFER_SOFTWARE is expected then.

Ok, thanks for the validation, do not hesitate to drop a Reviewed-by to
the next version of this series if you agree with the changes made here.

> For my own understanding (I'm just asking), why not using the
> currentmodes getter routine ?
> 
> I've had a look at the whole series [1], It seems used elsewhere. I may
> miss something... It would be 100% equivalent to current code to use:
> iio_get_internal_mode(indio_dev) & INDIO_BUFFER_SOFTWARE ?
> 
> This would be safe in case new modes gets introduced later ?
> (another note: unless these new modes gets set by default in the 'modes'
> field, this should have no impact here as well anyway ?)

I would argue that this is more a conceptual change. IMHO:
- currentmode is a variable that should have been kept internal
- checking against its value directly is kind of a hack and should be
  avoided when possible because we want the core to have full freedom
  over the way it manages these flags
- if you want to verify if buffers are enabled, then the core offers
  you a dedicated helper that does exactly this, and will do it better
  than if hardcoded by individual writers, generally

And it's not "used elsewhere" anymore thanks to this series :) only two
drivers _really_ need to check the actual current mode to do specific
actions, but that's all.

I hope it clarifies a bit.

Thanks,
Miquèl

[PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Miquel Raynal]

The st_sensors_core driver hardcodes the content of the
iio_device_claim_direct_mode() and iio_device_release_direct_mode()
helpers. Let's get rid of this handcrafted implementation and use the
proper core helpers instead. Additionally, this lowers the tab level
(which is always good) and prevents the use of the ->currentmode
variable which is not supposed to be used like this anyway.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++----------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 1de395bda03e..e57e85c06f4b 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
 	int err;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
-		err = -EBUSY;
+	err = iio_device_claim_direct_mode(indio_dev);
+	if (err)
+		return err;
+
+	err = st_sensors_set_enable(indio_dev, true);
+	if (err < 0)
 		goto out;
-	} else {
-		err = st_sensors_set_enable(indio_dev, true);
-		if (err < 0)
-			goto out;
 
-		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
-		err = st_sensors_read_axis_data(indio_dev, ch, val);
-		if (err < 0)
-			goto out;
+	msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
+	err = st_sensors_read_axis_data(indio_dev, ch, val);
+	if (err < 0)
+		goto out;
 
-		*val = *val >> ch->scan_type.shift;
+	*val = *val >> ch->scan_type.shift;
 
-		err = st_sensors_set_enable(indio_dev, false);
-	}
+	err = st_sensors_set_enable(indio_dev, false);
 out:
-	mutex_unlock(&indio_dev->mlock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return err;
 }
-- 
2.27.0

Re: [PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> The st_sensors_core driver hardcodes the content of the
> iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> helpers. Let's get rid of this handcrafted implementation and use the
> proper core helpers instead. Additionally, this lowers the tab level
> (which is always good) and prevents the use of the ->currentmode
> variable which is not supposed to be used like this anyway.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++----------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 1de395bda03e..e57e85c06f4b 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
>         int err;
>         struct st_sensor_data *sdata = iio_priv(indio_dev);
>
> -       mutex_lock(&indio_dev->mlock);
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> -               err = -EBUSY;
> +       err = iio_device_claim_direct_mode(indio_dev);

I'm afraid, for this driver, we would first need a cleanup of
indio_dev->mlock usage.
Or at least that's how I would start it.
i.e. remove the indio_dev->mlock and replace it with it's own
mutex/lock in all places (except this one).

The whole story about mlock is a bit old.
As I was told, it was initially defined in the iio_dev object, but not
very strictly controlled during review [of drivers].
Drivers kept using it (as a convenience lock).
It was later defined to be an IIO framework lock.
Now, there's a (slow) ongoing work to move mlock inside the
iio_dev_opaque struct, and make each driver use it's own lock, OR use
iio_device_{claim,release}_direct_mode() where appropriate.

FWIW: this change could go in as-is.
But there's still the point of implementing another lock on the
st_sensor_data type.
I would try to split this work into another [parallel] series, because
otherwise [if fitted into this series] it would just grow and be
slow-to-review series.
But ¯\_(ツ)_/¯

> +       if (err)
> +               return err;
> +
> +       err = st_sensors_set_enable(indio_dev, true);
> +       if (err < 0)
>                 goto out;
> -       } else {
> -               err = st_sensors_set_enable(indio_dev, true);
> -               if (err < 0)
> -                       goto out;
>
> -               msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> -               err = st_sensors_read_axis_data(indio_dev, ch, val);
> -               if (err < 0)
> -                       goto out;
> +       msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> +       err = st_sensors_read_axis_data(indio_dev, ch, val);
> +       if (err < 0)
> +               goto out;
>
> -               *val = *val >> ch->scan_type.shift;
> +       *val = *val >> ch->scan_type.shift;
>
> -               err = st_sensors_set_enable(indio_dev, false);
> -       }
> +       err = st_sensors_set_enable(indio_dev, false);
>  out:
> -       mutex_unlock(&indio_dev->mlock);
> +       iio_device_release_direct_mode(indio_dev);
>
>         return err;
>  }
> --
> 2.27.0
>

Re: [PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Miquel Raynal]

Hello Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:16:34 +0200:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > The st_sensors_core driver hardcodes the content of the
> > iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> > helpers. Let's get rid of this handcrafted implementation and use the
> > proper core helpers instead. Additionally, this lowers the tab level
> > (which is always good) and prevents the use of the ->currentmode
> > variable which is not supposed to be used like this anyway.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++----------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> > index 1de395bda03e..e57e85c06f4b 100644
> > --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> > +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> > @@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> >         int err;
> >         struct st_sensor_data *sdata = iio_priv(indio_dev);
> >
> > -       mutex_lock(&indio_dev->mlock);
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > -               err = -EBUSY;
> > +       err = iio_device_claim_direct_mode(indio_dev);  
> 
> I'm afraid, for this driver, we would first need a cleanup of
> indio_dev->mlock usage.
> Or at least that's how I would start it.
> i.e. remove the indio_dev->mlock and replace it with it's own
> mutex/lock in all places (except this one).
> 
> The whole story about mlock is a bit old.
> As I was told, it was initially defined in the iio_dev object, but not
> very strictly controlled during review [of drivers].
> Drivers kept using it (as a convenience lock).
> It was later defined to be an IIO framework lock.

I see, thanks for the explanation!

> Now, there's a (slow) ongoing work to move mlock inside the
> iio_dev_opaque struct, and make each driver use it's own lock, OR use
> iio_device_{claim,release}_direct_mode() where appropriate.
> 
> FWIW: this change could go in as-is.

/me breathes :-)

> But there's still the point of implementing another lock on the
> st_sensor_data type.
> I would try to split this work into another [parallel] series, because
> otherwise [if fitted into this series] it would just grow and be
> slow-to-review series.
> But ¯\_(ツ)_/¯

To be honest, my first goal was to document the modes enumeration.
Then, I realized currentmodes was also needing a bit of explanations as
well. Jonathan added that there were misuses with this variable. I then
tried to reduce it's overall use (when not particularly needed) and I
ended up doing this much bigger series, because every commit prepares
the field for the next one.

The situation in this driver is (as you truly report):
- the mlock is correctly used in the read_raw hook but everything is
  hardcoded
- the mlock is abused everywhere else

I am very sorry but I am not willing to entirely rework this driver
because it's not the point of my series, I don't have the hardware and
I know this would led to yet another series of changes, which I will
have no time to handle >.<

My goal here is to reduce the usage count of currentmodes across all
the device driver. This addresses the former point and I think it's
completely valid because a series doing exactly what you request would
definitely do this in two distinct steps as well O:-)

Thanks,
Miquèl

Re: [PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:32:43 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:16:34 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > The st_sensors_core driver hardcodes the content of the
> > > iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> > > helpers. Let's get rid of this handcrafted implementation and use the
> > > proper core helpers instead. Additionally, this lowers the tab level
> > > (which is always good) and prevents the use of the ->currentmode
> > > variable which is not supposed to be used like this anyway.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++----------
> > >  1 file changed, 13 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> > > index 1de395bda03e..e57e85c06f4b 100644
> > > --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> > > +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> > > @@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> > >         int err;
> > >         struct st_sensor_data *sdata = iio_priv(indio_dev);
> > >
> > > -       mutex_lock(&indio_dev->mlock);
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > -               err = -EBUSY;
> > > +       err = iio_device_claim_direct_mode(indio_dev);    
> > 
> > I'm afraid, for this driver, we would first need a cleanup of
> > indio_dev->mlock usage.
> > Or at least that's how I would start it.
> > i.e. remove the indio_dev->mlock and replace it with it's own
> > mutex/lock in all places (except this one).
> > 
> > The whole story about mlock is a bit old.
> > As I was told, it was initially defined in the iio_dev object, but not
> > very strictly controlled during review [of drivers].
> > Drivers kept using it (as a convenience lock).
> > It was later defined to be an IIO framework lock.  
> 
> I see, thanks for the explanation!

That's accurate.  Historical mistakes and all :)
We've been unwinding this for at least 5 years now so most of the simple
cases are now gone, though it seems not all of them!

> 
> > Now, there's a (slow) ongoing work to move mlock inside the
> > iio_dev_opaque struct, and make each driver use it's own lock, OR use
> > iio_device_{claim,release}_direct_mode() where appropriate.
> > 
> > FWIW: this change could go in as-is.  
> 
> /me breathes :-)
> 
> > But there's still the point of implementing another lock on the
> > st_sensor_data type.
> > I would try to split this work into another [parallel] series, because
> > otherwise [if fitted into this series] it would just grow and be
> > slow-to-review series.
> > But ¯\_(ツ)_/¯  
> 
> To be honest, my first goal was to document the modes enumeration.
> Then, I realized currentmodes was also needing a bit of explanations as
> well. Jonathan added that there were misuses with this variable. I then
> tried to reduce it's overall use (when not particularly needed) and I
> ended up doing this much bigger series, because every commit prepares
> the field for the next one.
> 
> The situation in this driver is (as you truly report):
> - the mlock is correctly used in the read_raw hook but everything is
>   hardcoded
> - the mlock is abused everywhere else
> 
> I am very sorry but I am not willing to entirely rework this driver
> because it's not the point of my series, I don't have the hardware and
> I know this would led to yet another series of changes, which I will
> have no time to handle >.<
> 
> My goal here is to reduce the usage count of currentmodes across all
> the device driver. This addresses the former point and I think it's
> completely valid because a series doing exactly what you request would
> definitely do this in two distinct steps as well O:-)

+CC some folks who are active on this driver (there are lots of them
as it covers some very common devices). For now I've +cc Denis but there
are others you should add to a v2.

Hmm. Question is whether doing this change in isolation from the more
general cleanup of mlock is a good move. It won't be obvious what
can race if someone comes along later trying to remove mlock usage...

One perhaps non obvious thing is at that a driver should not rely in
any way on the implementation of iio_device_claim/release_direct_mode().
It's only documented characteristics is it will fail to claim if we
are in buffered mode and that it prevents races with a transition to
buffered modes.

This particular read_raw path doesn't seem to use any shared
state (buffer is allocated in st_sensors_read_axis_data) but it
is turning the power on and off which is potentially fairly nasty
(and interestingly does rely on shared state - see later).

So it should use a local lock as well as mlock to prevent multiple
calls of st_sensors_read_info_raw racing with each other.
(claim_direct doesn't guarantee that because it's an implementation
detail that a driver should rely on - right now as it's open coded
we can know it is safe).

I took a quick look at what other mlock usage we do have...

The other cases in st_sensors_core.c should at most have been
iio_device_claim_direct_mode() etc.  Right now they'll block indefinitely
on a read of relevant _avail which is not good!

However, not clear why they even need to do that as they are simply
listing sdata->sensor_settings->odr.odr_avl etc which is const
data for a give device type.  So I'm fairly sure we could just drop those
two cases.

The mlock use in st_accel_core.c is again dubious as it will block
indefinitely if the buffer is enabled... 

So why is it here?  Probably
1) Avoid changing sampling frequency whilst buffer is running which
should be an iio_device_claim_direct_mode()
2) There is some state that might potentially want keeping in sync - which
would benefit from a local lock.  In particular sdata->odr which is
a local cache of the output data rate is used in st_sensors_set_enable()
which it could race with (if we can't rely on claim_direct())...

This last one is why I don't think we can do this function in isolation.

Sorry :(

On the plus side it's not that hard to fix as:
1) Drop the protection on _avail functions - it seems to be pointless.
2) Add a new lock that actually only ensures device setting for ODR is
   atomic wrt to the cached value.

Plenty of folks to test.   I'm happy to roll the patch if you want me
to, but as it's a precursor to your series perhaps better that you do.

I'm sure one of the people who have made changes to this driver recently
will help with testing.

Jonathan

> 
> Thanks,
> Miquèl
> 

[PATCH 06/10] iio: core: Hide read accesses to iio_dev->currentmode

[Miquel Raynal]

In order to later move this variable within the opaque structure, let's
create a helper for accessing it in read-only mode. This helper will be
exposed and kept accessible for the few drivers that could need it. The
write access to this variable however should be fully reserved to the
core so in a second step we will add another helper, not exported to the
device drivers.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/bmc150-accel-core.c |  4 ++--
 drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
 drivers/iio/industrialio-buffer.c     |  6 +++---
 drivers/iio/industrialio-core.c       | 11 +++++++++++
 drivers/iio/industrialio-trigger.c    |  2 +-
 include/linux/iio/iio.h               |  9 ++++++---
 6 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index b0678c351e82..0a191463d462 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret = 0;
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4c922ef634f8..2b7f6371950e 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, true);
 
 	/* if we are not in triggered mode, we cannot enable the buffer. */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/* we continue with the triggered buffer */
@@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, false);
 
 	/* if we are not in triggered mode, nothing to do here */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/*
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e180728914c0..f4dbab7c44fa 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 			goto err_disable_buffers;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
 		ret = iio_trigger_attach_poll_func(indio_dev->trig,
 						   indio_dev->pollfunc);
 		if (ret)
@@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	return 0;
 
 err_detach_pollfunc:
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
@@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 			ret = ret2;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 463a63d5bf56..a1d6e30d034a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2057,6 +2057,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
+/**
+ * iio_get_internal_mode() - helper function providing read-only access to the
+ *			     opaque @currentmode variable
+ * @indio_dev:		IIO device structure for device
+ **/
+int iio_get_internal_mode(struct iio_dev *indio_dev)
+{
+	return indio_dev->currentmode;
+}
+EXPORT_SYMBOL_GPL(iio_get_internal_mode);
+
 subsys_initcall(iio_init);
 module_exit(iio_exit);
 
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index b23caa2f2aa1..71b07d6111d6 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -411,7 +411,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 	int ret;
 
 	mutex_lock(&indio_dev->mlock);
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
 		mutex_unlock(&indio_dev->mlock);
 		return -EBUSY;
 	}
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 0312da2e83f8..dcab17f44552 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -677,15 +677,18 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
 __printf(2, 3)
 struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
 					   const char *fmt, ...);
+
+int iio_get_internal_mode(struct iio_dev *indio_dev);
+
 /**
  * iio_buffer_enabled() - helper function to test if the buffer is enabled
  * @indio_dev:		IIO device structure for device
  **/
 static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {
-	return indio_dev->currentmode
-		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
-		   INDIO_BUFFER_SOFTWARE);
+	return iio_get_internal_mode(indio_dev) &
+		(INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
+		 INDIO_BUFFER_SOFTWARE);
 }
 
 /**
-- 
2.27.0

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

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> In order to later move this variable within the opaque structure, let's
> create a helper for accessing it in read-only mode. This helper will be
> exposed and kept accessible for the few drivers that could need it. The
> write access to this variable however should be fully reserved to the
> core so in a second step we will add another helper, not exported to the
> device drivers.

The naming needs a bit of discussion.
I would have gone for iio_dev_get_current_mode() or something like that.

And I would probably not use this helper inside the IIO core stuff
(i.e. drivers/iio/industrialio-*.c files)
Mostly because [if now used only in IIO core] it makes the
"indio_dev->currentmode" assignment and access easier to trace.

There's also the change that accessing "indio_dev->currentmode"
becomes a function-symbol which has rules at link-time and may add
some new jmp/ret instructions.
But It doesn't look like this is used in any fast-paths, so it's not
an issue as much.

>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/accel/bmc150-accel-core.c |  4 ++--
>  drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
>  drivers/iio/industrialio-buffer.c     |  6 +++---
>  drivers/iio/industrialio-core.c       | 11 +++++++++++
>  drivers/iio/industrialio-trigger.c    |  2 +-
>  include/linux/iio/iio.h               |  9 ++++++---
>  6 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index b0678c351e82..0a191463d462 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
>         struct bmc150_accel_data *data = iio_priv(indio_dev);
>         int ret = 0;
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
>                 return 0;
>
>         mutex_lock(&data->mutex);
> @@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
>  {
>         struct bmc150_accel_data *data = iio_priv(indio_dev);
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
>                 return 0;
>
>         mutex_lock(&data->mutex);
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4c922ef634f8..2b7f6371950e 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
>                 return at91_adc_configure_touch(st, true);
>
>         /* if we are not in triggered mode, we cannot enable the buffer. */
> -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>                 return -EINVAL;
>
>         /* we continue with the triggered buffer */
> @@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
>                 return at91_adc_configure_touch(st, false);
>
>         /* if we are not in triggered mode, nothing to do here */
> -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
>                 return -EINVAL;
>
>         /*
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e180728914c0..f4dbab7c44fa 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>                         goto err_disable_buffers;
>         }
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
>                 ret = iio_trigger_attach_poll_func(indio_dev->trig,
>                                                    indio_dev->pollfunc);
>                 if (ret)
> @@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>         return 0;
>
>  err_detach_pollfunc:
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
>                 iio_trigger_detach_poll_func(indio_dev->trig,
>                                              indio_dev->pollfunc);
>         }
> @@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>                         ret = ret2;
>         }
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
>                 iio_trigger_detach_poll_func(indio_dev->trig,
>                                              indio_dev->pollfunc);
>         }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 463a63d5bf56..a1d6e30d034a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2057,6 +2057,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
>
> +/**
> + * iio_get_internal_mode() - helper function providing read-only access to the
> + *                          opaque @currentmode variable
> + * @indio_dev:         IIO device structure for device
> + **/
> +int iio_get_internal_mode(struct iio_dev *indio_dev)
> +{
> +       return indio_dev->currentmode;
> +}
> +EXPORT_SYMBOL_GPL(iio_get_internal_mode);
> +
>  subsys_initcall(iio_init);
>  module_exit(iio_exit);
>
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index b23caa2f2aa1..71b07d6111d6 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -411,7 +411,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
>         int ret;
>
>         mutex_lock(&indio_dev->mlock);
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
>                 mutex_unlock(&indio_dev->mlock);
>                 return -EBUSY;
>         }
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 0312da2e83f8..dcab17f44552 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -677,15 +677,18 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
>  __printf(2, 3)
>  struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
>                                            const char *fmt, ...);
> +
> +int iio_get_internal_mode(struct iio_dev *indio_dev);
> +
>  /**
>   * iio_buffer_enabled() - helper function to test if the buffer is enabled
>   * @indio_dev:         IIO device structure for device
>   **/
>  static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
>  {
> -       return indio_dev->currentmode
> -               & (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> -                  INDIO_BUFFER_SOFTWARE);
> +       return iio_get_internal_mode(indio_dev) &
> +               (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> +                INDIO_BUFFER_SOFTWARE);
>  }
>
>  /**
> --
> 2.27.0
>

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

[Alexandru Ardelean]

On Thu, Dec 16, 2021 at 9:38 AM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > In order to later move this variable within the opaque structure, let's
> > create a helper for accessing it in read-only mode. This helper will be
> > exposed and kept accessible for the few drivers that could need it. The
> > write access to this variable however should be fully reserved to the
> > core so in a second step we will add another helper, not exported to the
> > device drivers.
>
> The naming needs a bit of discussion.
> I would have gone for iio_dev_get_current_mode() or something like that.
>
> And I would probably not use this helper inside the IIO core stuff
> (i.e. drivers/iio/industrialio-*.c files)
> Mostly because [if now used only in IIO core] it makes the
> "indio_dev->currentmode" assignment and access easier to trace.

Oh, I just saw the next patch with iio_set_internal_mode().
I guess having a set helper (like this) resolves the discussion about tracing.
Still not sure whether it makes sense to use these helpers inside IIO
core, but ¯\_(ツ)_/¯

>
> There's also the change that accessing "indio_dev->currentmode"
> becomes a function-symbol which has rules at link-time and may add
> some new jmp/ret instructions.
> But It doesn't look like this is used in any fast-paths, so it's not
> an issue as much.
>
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/accel/bmc150-accel-core.c |  4 ++--
> >  drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
> >  drivers/iio/industrialio-buffer.c     |  6 +++---
> >  drivers/iio/industrialio-core.c       | 11 +++++++++++
> >  drivers/iio/industrialio-trigger.c    |  2 +-
> >  include/linux/iio/iio.h               |  9 ++++++---
> >  6 files changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > index b0678c351e82..0a191463d462 100644
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
> >         struct bmc150_accel_data *data = iio_priv(indio_dev);
> >         int ret = 0;
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
> >                 return 0;
> >
> >         mutex_lock(&data->mutex);
> > @@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
> >  {
> >         struct bmc150_accel_data *data = iio_priv(indio_dev);
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
> >                 return 0;
> >
> >         mutex_lock(&data->mutex);
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index 4c922ef634f8..2b7f6371950e 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >                 return at91_adc_configure_touch(st, true);
> >
> >         /* if we are not in triggered mode, we cannot enable the buffer. */
> > -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
> >                 return -EINVAL;
> >
> >         /* we continue with the triggered buffer */
> > @@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> >                 return at91_adc_configure_touch(st, false);
> >
> >         /* if we are not in triggered mode, nothing to do here */
> > -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
> >                 return -EINVAL;
> >
> >         /*
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index e180728914c0..f4dbab7c44fa 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >                         goto err_disable_buffers;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 ret = iio_trigger_attach_poll_func(indio_dev->trig,
> >                                                    indio_dev->pollfunc);
> >                 if (ret)
> > @@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >         return 0;
> >
> >  err_detach_pollfunc:
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 iio_trigger_detach_poll_func(indio_dev->trig,
> >                                              indio_dev->pollfunc);
> >         }
> > @@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
> >                         ret = ret2;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 iio_trigger_detach_poll_func(indio_dev->trig,
> >                                              indio_dev->pollfunc);
> >         }
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 463a63d5bf56..a1d6e30d034a 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2057,6 +2057,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> >
> > +/**
> > + * iio_get_internal_mode() - helper function providing read-only access to the
> > + *                          opaque @currentmode variable
> > + * @indio_dev:         IIO device structure for device
> > + **/
> > +int iio_get_internal_mode(struct iio_dev *indio_dev)
> > +{
> > +       return indio_dev->currentmode;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_get_internal_mode);
> > +
> >  subsys_initcall(iio_init);
> >  module_exit(iio_exit);
> >
> > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> > index b23caa2f2aa1..71b07d6111d6 100644
> > --- a/drivers/iio/industrialio-trigger.c
> > +++ b/drivers/iio/industrialio-trigger.c
> > @@ -411,7 +411,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
> >         int ret;
> >
> >         mutex_lock(&indio_dev->mlock);
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 mutex_unlock(&indio_dev->mlock);
> >                 return -EBUSY;
> >         }
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 0312da2e83f8..dcab17f44552 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -677,15 +677,18 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
> >  __printf(2, 3)
> >  struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
> >                                            const char *fmt, ...);
> > +
> > +int iio_get_internal_mode(struct iio_dev *indio_dev);
> > +
> >  /**
> >   * iio_buffer_enabled() - helper function to test if the buffer is enabled
> >   * @indio_dev:         IIO device structure for device
> >   **/
> >  static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
> >  {
> > -       return indio_dev->currentmode
> > -               & (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > -                  INDIO_BUFFER_SOFTWARE);
> > +       return iio_get_internal_mode(indio_dev) &
> > +               (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > +                INDIO_BUFFER_SOFTWARE);
> >  }
> >
> >  /**
> > --
> > 2.27.0
> >

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

[Miquel Raynal]

Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:38:17 +0200:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > In order to later move this variable within the opaque structure, let's
> > create a helper for accessing it in read-only mode. This helper will be
> > exposed and kept accessible for the few drivers that could need it. The
> > write access to this variable however should be fully reserved to the
> > core so in a second step we will add another helper, not exported to the
> > device drivers.  
> 
> The naming needs a bit of discussion.
> I would have gone for iio_dev_get_current_mode() or something like that.

I honestly tried both, but it appeared important to me to name it
"internal" so that people are not too tempted to use it in the first
place. Other advises are welcome, I an definitely switch to
iio_dev_get/set_current_mode() if it is preferred.

> And I would probably not use this helper inside the IIO core stuff
> (i.e. drivers/iio/industrialio-*.c files)
> Mostly because [if now used only in IIO core] it makes the
> "indio_dev->currentmode" assignment and access easier to trace.

I think you meant in a later review that this was fine given the fact
that a setter helper was also introduced. The goal behind using a
helper literally everywhere was to avoid introducing an indirect access
to the opaque structure everywhere, and do this final change almost
transparently for the users.

> There's also the change that accessing "indio_dev->currentmode"
> becomes a function-symbol which has rules at link-time and may add
> some new jmp/ret instructions.
> But It doesn't look like this is used in any fast-paths, so it's not
> an issue as much.

I was also tempted to do that in the first place but this does not work
anymore as soon as the variable is moved to the opaque structure (only
the setter, which is internal to the core, may end up in the
iio-opaque.h header). Otherwise we would end up exporting the opaque
header from iio.h which is truly not what we want.

> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/accel/bmc150-accel-core.c |  4 ++--
> >  drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
> >  drivers/iio/industrialio-buffer.c     |  6 +++---
> >  drivers/iio/industrialio-core.c       | 11 +++++++++++
> >  drivers/iio/industrialio-trigger.c    |  2 +-
> >  include/linux/iio/iio.h               |  9 ++++++---
> >  6 files changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> > index b0678c351e82..0a191463d462 100644
> > --- a/drivers/iio/accel/bmc150-accel-core.c
> > +++ b/drivers/iio/accel/bmc150-accel-core.c
> > @@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
> >         struct bmc150_accel_data *data = iio_priv(indio_dev);
> >         int ret = 0;
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
> >                 return 0;
> >
> >         mutex_lock(&data->mutex);
> > @@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
> >  {
> >         struct bmc150_accel_data *data = iio_priv(indio_dev);
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
> >                 return 0;
> >
> >         mutex_lock(&data->mutex);
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index 4c922ef634f8..2b7f6371950e 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
> >                 return at91_adc_configure_touch(st, true);
> >
> >         /* if we are not in triggered mode, we cannot enable the buffer. */
> > -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
> >                 return -EINVAL;
> >
> >         /* we continue with the triggered buffer */
> > @@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
> >                 return at91_adc_configure_touch(st, false);
> >
> >         /* if we are not in triggered mode, nothing to do here */
> > -       if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> > +       if (!(iio_get_internal_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
> >                 return -EINVAL;
> >
> >         /*
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index e180728914c0..f4dbab7c44fa 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >                         goto err_disable_buffers;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 ret = iio_trigger_attach_poll_func(indio_dev->trig,
> >                                                    indio_dev->pollfunc);
> >                 if (ret)
> > @@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> >         return 0;
> >
> >  err_detach_pollfunc:
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 iio_trigger_detach_poll_func(indio_dev->trig,
> >                                              indio_dev->pollfunc);
> >         }
> > @@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
> >                         ret = ret2;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 iio_trigger_detach_poll_func(indio_dev->trig,
> >                                              indio_dev->pollfunc);
> >         }
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 463a63d5bf56..a1d6e30d034a 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2057,6 +2057,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> >
> > +/**
> > + * iio_get_internal_mode() - helper function providing read-only access to the
> > + *                          opaque @currentmode variable
> > + * @indio_dev:         IIO device structure for device
> > + **/
> > +int iio_get_internal_mode(struct iio_dev *indio_dev)
> > +{
> > +       return indio_dev->currentmode;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_get_internal_mode);
> > +
> >  subsys_initcall(iio_init);
> >  module_exit(iio_exit);
> >
> > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> > index b23caa2f2aa1..71b07d6111d6 100644
> > --- a/drivers/iio/industrialio-trigger.c
> > +++ b/drivers/iio/industrialio-trigger.c
> > @@ -411,7 +411,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
> >         int ret;
> >
> >         mutex_lock(&indio_dev->mlock);
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> >                 mutex_unlock(&indio_dev->mlock);
> >                 return -EBUSY;
> >         }
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 0312da2e83f8..dcab17f44552 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -677,15 +677,18 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
> >  __printf(2, 3)
> >  struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
> >                                            const char *fmt, ...);
> > +
> > +int iio_get_internal_mode(struct iio_dev *indio_dev);
> > +
> >  /**
> >   * iio_buffer_enabled() - helper function to test if the buffer is enabled
> >   * @indio_dev:         IIO device structure for device
> >   **/
> >  static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
> >  {
> > -       return indio_dev->currentmode
> > -               & (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > -                  INDIO_BUFFER_SOFTWARE);
> > +       return iio_get_internal_mode(indio_dev) &
> > +               (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > +                INDIO_BUFFER_SOFTWARE);
> >  }
> >
> >  /**
> > --
> > 2.27.0
> >  


Thanks,
Miquèl

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

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:37:57 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:38:17 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > In order to later move this variable within the opaque structure, let's
> > > create a helper for accessing it in read-only mode. This helper will be
> > > exposed and kept accessible for the few drivers that could need it. The
> > > write access to this variable however should be fully reserved to the
> > > core so in a second step we will add another helper, not exported to the
> > > device drivers.    
> > 
> > The naming needs a bit of discussion.
> > I would have gone for iio_dev_get_current_mode() or something like that.  
> 
> I honestly tried both, but it appeared important to me to name it
> "internal" so that people are not too tempted to use it in the first
> place. Other advises are welcome, I an definitely switch to
> iio_dev_get/set_current_mode() if it is preferred.

There are valid reasons to use it (such as the cases you have below)
so I think the iio_device_get_current_mode() naming is probably best.
Note long form of dev as more consistent with existing functions.
They could all have been iio_dev_* but given they aren't let us
keep to full device for consistency.

> 
> > And I would probably not use this helper inside the IIO core stuff
> > (i.e. drivers/iio/industrialio-*.c files)
> > Mostly because [if now used only in IIO core] it makes the
> > "indio_dev->currentmode" assignment and access easier to trace.  
> 
> I think you meant in a later review that this was fine given the fact
> that a setter helper was also introduced. The goal behind using a
> helper literally everywhere was to avoid introducing an indirect access
> to the opaque structure everywhere, and do this final change almost
> transparently for the users.
> 
> > There's also the change that accessing "indio_dev->currentmode"
> > becomes a function-symbol which has rules at link-time and may add
> > some new jmp/ret instructions.
> > But It doesn't look like this is used in any fast-paths, so it's not
> > an issue as much.  
> 
> I was also tempted to do that in the first place but this does not work
> anymore as soon as the variable is moved to the opaque structure (only
> the setter, which is internal to the core, may end up in the
> iio-opaque.h header). Otherwise we would end up exporting the opaque
> header from iio.h which is truly not what we want.

My reading of Alex's comment was he was only referring to the core code
changes (which already have visibility of the opaque structure).

Also, the only case where I can see this being an issue is the

iio_buffer_enabled call.  Given you are adding a function call to that
anyway, might as well just move that implementation down into a non
inline function and export the symbol.  We pay the minor cost either way.


> 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/accel/bmc150-accel-core.c |  4 ++--
> > >  drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
> > >  drivers/iio/industrialio-buffer.c     |  6 +++---
> > >  drivers/iio/industrialio-core.c       | 11 +++++++++++
> > >  drivers/iio/industrialio-trigger.c    |  2 +-
> > >  include/linux/iio/iio.h               |  9 ++++++---
> > >  6 files changed, 25 insertions(+), 11 deletions(-)
> > >


> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index e180728914c0..f4dbab7c44fa 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> > >                         goto err_disable_buffers;
> > >         }
> > >
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {

This function can already see both iio_dev and iio_dev_opaque structures so
I think I'd prefer to keep the direct access.

> > >                 ret = iio_trigger_attach_poll_func(indio_dev->trig,
> > >                                                    indio_dev->pollfunc);
> > >                 if (ret)
> > > @@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
> > >         return 0;
> > >
> > >  err_detach_pollfunc:
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> > >                 iio_trigger_detach_poll_func(indio_dev->trig,
> > >                                              indio_dev->pollfunc);
> > >         }
> > > @@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
> > >                         ret = ret2;

Same for this function

> > >         }
> > >
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> > >                 iio_trigger_detach_poll_func(indio_dev->trig,
> > >                                              indio_dev->pollfunc);
> > >         }
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index 463a63d5bf56..a1d6e30d034a 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -2057,6 +2057,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > >
> > > +/**
> > > + * iio_get_internal_mode() - helper function providing read-only access to the
> > > + *                          opaque @currentmode variable
> > > + * @indio_dev:         IIO device structure for device
> > > + **/

I was going to complain and say this should be */ but
then checked and see we have a random mixture in this file... Hohum. one for the
list of things to tidy up when someone is really bored...

> > > +int iio_get_internal_mode(struct iio_dev *indio_dev)
> > > +{
> > > +       return indio_dev->currentmode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_get_internal_mode);
> > > +
> > >  subsys_initcall(iio_init);
> > >  module_exit(iio_exit);
> > >
> > > diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> > > index b23caa2f2aa1..71b07d6111d6 100644
> > > --- a/drivers/iio/industrialio-trigger.c
> > > +++ b/drivers/iio/industrialio-trigger.c
> > > @@ -411,7 +411,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
> > >         int ret;
> > >
> > >         mutex_lock(&indio_dev->mlock);
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
Also already has visibility of the opaque structure.

> > > +       if (iio_get_internal_mode(indio_dev) == INDIO_BUFFER_TRIGGERED) {
> > >                 mutex_unlock(&indio_dev->mlock);
> > >                 return -EBUSY;
> > >         }
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index 0312da2e83f8..dcab17f44552 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -677,15 +677,18 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
> > >  __printf(2, 3)
> > >  struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
> > >                                            const char *fmt, ...);
> > > +
> > > +int iio_get_internal_mode(struct iio_dev *indio_dev);
> > > +
> > >  /**
> > >   * iio_buffer_enabled() - helper function to test if the buffer is enabled
> > >   * @indio_dev:         IIO device structure for device
> > >   **/
> > >  static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
> > >  {
> > > -       return indio_dev->currentmode
> > > -               & (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > > -                  INDIO_BUFFER_SOFTWARE);
> > > +       return iio_get_internal_mode(indio_dev) &
So this is the one place we have to use it given the function is inline.

However, the fact we then end up with an function that can't be inlined anyway
suggests we should move this implementation out of the header as there is
no benefit in having it here.  Hopefully no one will mind the perf impact
of this (seems unlikely but you never know...)

> > > +               (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> > > +                INDIO_BUFFER_SOFTWARE);
> > >  }
> > >
> > >  /**
> > > --
> > > 2.27.0
> > >    
> 
> 
> Thanks,
> Miquèl

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

[Miquel Raynal]

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(-)

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
-- 
2.27.0

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

[Jonathan Cameron]

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

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

[Miquel Raynal]

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

[PATCH 08/10] iio: core: Move iio_dev->currentmode to the opaque structure

[Miquel Raynal]

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.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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

Re: [PATCH 08/10] iio: core: Move iio_dev->currentmode to the opaque structure

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
<miquel.raynal@bootlin.com> 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@gmail.com>

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  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
>

Re: [PATCH 08/10] iio: core: Move iio_dev->currentmode to the opaque structure

[Miquel Raynal]

Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:48:36 +0200:

> On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> <miquel.raynal@bootlin.com> 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@gmail.com>

Great! Thanks for the entire review and feedback!

> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  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

Re: [PATCH 08/10] iio: core: Move iio_dev->currentmode to the opaque structure

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:38:48 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:48:36 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:03 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> 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.

Ah.  I didn't read ahead either.   I'm still unconvinced that it serves
any useful purpose though.

I'm entirely in favour of moving the variable but direct access is
fine form the core code.  We have similar cases for iio_dev_opaque->id
for example where we provide a read only accessor for drivers and directly
use it in the core.

> > 
> > [ regardless of the naming of the helpers]
> > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>  
> 
> Great! Thanks for the entire review and feedback!
> 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  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

[PATCH 09/10] iio: core: Simplify the registration of kfifo buffers

[Miquel Raynal]

Among all the users of the kfifo buffers, no one uses the
INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
simplify a little bit the internals - overall the documentation - by
eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
default with kfifo buffers, which will basically mimic what all the "non
direct" modes do.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/fxls8962af-core.c                  |  1 -
 drivers/iio/accel/sca3000.c                          |  1 -
 drivers/iio/accel/ssp_accel_sensor.c                 |  1 -
 drivers/iio/adc/ina2xx-adc.c                         |  1 -
 drivers/iio/adc/ti_am335x_adc.c                      |  4 +---
 drivers/iio/buffer/kfifo_buf.c                       | 12 ++----------
 .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
 drivers/iio/common/scmi_sensors/scmi_iio.c           |  1 -
 drivers/iio/gyro/ssp_gyro_sensor.c                   |  1 -
 drivers/iio/health/max30100.c                        |  1 -
 drivers/iio/health/max30102.c                        |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c    |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c     |  1 -
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c       |  1 -
 drivers/iio/light/apds9960.c                         |  1 -
 drivers/staging/iio/impedance-analyzer/ad5933.c      |  1 -
 include/linux/iio/kfifo_buf.h                        |  5 ++---
 17 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 32989d91b982..cf494937a346 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -1209,7 +1209,6 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 			return ret;
 
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &fxls8962af_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index c6b75308148a..a65a05f1208b 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1465,7 +1465,6 @@ static int sca3000_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &sca3000_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
index 04dcb2b657ee..d20f8d299c11 100644
--- a/drivers/iio/accel/ssp_accel_sensor.c
+++ b/drivers/iio/accel/ssp_accel_sensor.c
@@ -113,7 +113,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_accel_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_accel_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index a4b2ff9e0dd5..013d2d586df1 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1030,7 +1030,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	indio_dev->name = id->name;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ina2xx_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index dbdc1ef48566..567d43a30955 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
 {
 	int ret;
 
-	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
-					  setup_ops);
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 416d35a61ae2..f17473e06ee8 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -197,7 +197,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
 	.set_length = &iio_set_length_kfifo,
 	.release = &iio_kfifo_buffer_release,
 
-	.modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
+	.modes = INDIO_BUFFER_SOFTWARE,
 };
 
 struct iio_buffer *iio_kfifo_allocate(void)
@@ -259,8 +259,6 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  * devm_iio_kfifo_buffer_setup_ext - Allocate a kfifo buffer & attach it to an IIO device
  * @dev: Device object to which to attach the life-time of this kfifo buffer
  * @indio_dev: The device the buffer should be attached to
- * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
- *		INDIO_BUFFER_TRIGGERED).
  * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
  * @buffer_attrs: Extra sysfs buffer attributes for this IIO buffer
  *
@@ -271,22 +269,16 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  */
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs)
 {
 	struct iio_buffer *buffer;
 
-	if (!mode_flags)
-		return -EINVAL;
-
 	buffer = devm_iio_kfifo_allocate(dev);
 	if (!buffer)
 		return -ENOMEM;
 
-	mode_flags &= kfifo_access_funcs.modes;
-
-	indio_dev->modes |= mode_flags;
+	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
 	indio_dev->setup_ops = setup_ops;
 
 	buffer->attrs = buffer_attrs;
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index b2725c6adc7f..a4cf1d9a8a49 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -333,8 +333,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 			 * We can not use trigger here, as events are generated
 			 * as soon as sample_frequency is set.
 			 */
-			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
-							      INDIO_BUFFER_SOFTWARE, NULL,
+			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL,
 							      cros_ec_sensor_fifo_attributes);
 			if (ret)
 				return ret;
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 7cf2bf282cef..792807b2894f 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -631,7 +631,6 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 
 		err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
 						  scmi_iio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &scmi_iio_buffer_ops);
 		if (err < 0) {
 			dev_err(dev,
diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
index 46ed12771d2f..eebc036717af 100644
--- a/drivers/iio/gyro/ssp_gyro_sensor.c
+++ b/drivers/iio/gyro/ssp_gyro_sensor.c
@@ -113,7 +113,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_gyro_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_gyro_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 36ba7611d9ce..ad5717965223 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -433,7 +433,6 @@ static int max30100_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30100_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 2292876c55e2..abbcef563807 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -542,7 +542,6 @@ static int max30102_probe(struct i2c_client *client,
 	}
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30102_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 383cc3250342..c3f433ad3af6 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -731,7 +731,6 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
 	indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index cec1dd0e0464..9d94a8518e3c 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -743,7 +743,6 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
 	indio_dev->setup_ops = &inv_icm42600_buffer_ops;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 16730a780964..f80c62849d30 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -746,7 +746,6 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 			continue;
 
 		ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
-						  INDIO_BUFFER_SOFTWARE,
 						  &st_lsm6dsx_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
index 4141c0fa7bc4..09b831f9f40b 100644
--- a/drivers/iio/light/apds9960.c
+++ b/drivers/iio/light/apds9960.c
@@ -1003,7 +1003,6 @@ static int apds9960_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &apds9960_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 793918e1c45f..f177b20f0f2d 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -749,7 +749,6 @@ static int ad5933_probe(struct i2c_client *client,
 	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ad5933_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
index ccd2ceae7b25..8a83fb58232d 100644
--- a/include/linux/iio/kfifo_buf.h
+++ b/include/linux/iio/kfifo_buf.h
@@ -12,11 +12,10 @@ void iio_kfifo_free(struct iio_buffer *r);
 
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs);
 
-#define devm_iio_kfifo_buffer_setup(dev, indio_dev, mode_flags, setup_ops)	\
-	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (mode_flags), (setup_ops), NULL)
+#define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops)	\
+	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
 
 #endif
-- 
2.27.0

Re: [PATCH 09/10] iio: core: Simplify the registration of kfifo buffers

[Alexandru Ardelean]

On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Among all the users of the kfifo buffers, no one uses the
> INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
> simplify a little bit the internals - overall the documentation - by
> eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
> default with kfifo buffers, which will basically mimic what all the "non
> direct" modes do.
>

I think this patch here (and 10/10) can be part of a separate series.
This discussion is important enough to have it's own series.
It's a bit of an API discussion.

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/accel/fxls8962af-core.c                  |  1 -
>  drivers/iio/accel/sca3000.c                          |  1 -
>  drivers/iio/accel/ssp_accel_sensor.c                 |  1 -
>  drivers/iio/adc/ina2xx-adc.c                         |  1 -
>  drivers/iio/adc/ti_am335x_adc.c                      |  4 +---
>  drivers/iio/buffer/kfifo_buf.c                       | 12 ++----------
>  .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
>  drivers/iio/common/scmi_sensors/scmi_iio.c           |  1 -
>  drivers/iio/gyro/ssp_gyro_sensor.c                   |  1 -
>  drivers/iio/health/max30100.c                        |  1 -
>  drivers/iio/health/max30102.c                        |  1 -
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c    |  1 -
>  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c     |  1 -
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c       |  1 -
>  drivers/iio/light/apds9960.c                         |  1 -
>  drivers/staging/iio/impedance-analyzer/ad5933.c      |  1 -
>  include/linux/iio/kfifo_buf.h                        |  5 ++---
>  17 files changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index 32989d91b982..cf494937a346 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -1209,7 +1209,6 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>                         return ret;
>
>                 ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> -                                                 INDIO_BUFFER_SOFTWARE,
>                                                   &fxls8962af_buffer_ops);
>                 if (ret)
>                         return ret;
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index c6b75308148a..a65a05f1208b 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1465,7 +1465,6 @@ static int sca3000_probe(struct spi_device *spi)
>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &sca3000_ring_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
> index 04dcb2b657ee..d20f8d299c11 100644
> --- a/drivers/iio/accel/ssp_accel_sensor.c
> +++ b/drivers/iio/accel/ssp_accel_sensor.c
> @@ -113,7 +113,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
>         indio_dev->available_scan_masks = ssp_accel_scan_mask;
>
>         ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &ssp_accel_buffer_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index a4b2ff9e0dd5..013d2d586df1 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -1030,7 +1030,6 @@ static int ina2xx_probe(struct i2c_client *client,
>         indio_dev->name = id->name;
>
>         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &ina2xx_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index dbdc1ef48566..567d43a30955 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
>  {
>         int ret;
>
> -       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
> -                                         setup_ops);
> +       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> index 416d35a61ae2..f17473e06ee8 100644
> --- a/drivers/iio/buffer/kfifo_buf.c
> +++ b/drivers/iio/buffer/kfifo_buf.c
> @@ -197,7 +197,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
>         .set_length = &iio_set_length_kfifo,
>         .release = &iio_kfifo_buffer_release,
>
> -       .modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
> +       .modes = INDIO_BUFFER_SOFTWARE,
>  };
>
>  struct iio_buffer *iio_kfifo_allocate(void)
> @@ -259,8 +259,6 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
>   * devm_iio_kfifo_buffer_setup_ext - Allocate a kfifo buffer & attach it to an IIO device
>   * @dev: Device object to which to attach the life-time of this kfifo buffer
>   * @indio_dev: The device the buffer should be attached to
> - * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
> - *             INDIO_BUFFER_TRIGGERED).
>   * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
>   * @buffer_attrs: Extra sysfs buffer attributes for this IIO buffer
>   *
> @@ -271,22 +269,16 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
>   */
>  int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
>                                     struct iio_dev *indio_dev,
> -                                   int mode_flags,
>                                     const struct iio_buffer_setup_ops *setup_ops,
>                                     const struct attribute **buffer_attrs)
>  {
>         struct iio_buffer *buffer;
>
> -       if (!mode_flags)
> -               return -EINVAL;
> -
>         buffer = devm_iio_kfifo_allocate(dev);
>         if (!buffer)
>                 return -ENOMEM;
>
> -       mode_flags &= kfifo_access_funcs.modes;
> -
> -       indio_dev->modes |= mode_flags;
> +       indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
>         indio_dev->setup_ops = setup_ops;
>
>         buffer->attrs = buffer_attrs;
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index b2725c6adc7f..a4cf1d9a8a49 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -333,8 +333,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>                          * We can not use trigger here, as events are generated
>                          * as soon as sample_frequency is set.
>                          */
> -                       ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
> -                                                             INDIO_BUFFER_SOFTWARE, NULL,
> +                       ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL,
>                                                               cros_ec_sensor_fifo_attributes);
>                         if (ret)
>                                 return ret;
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> index 7cf2bf282cef..792807b2894f 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -631,7 +631,6 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
>
>                 err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
>                                                   scmi_iio_dev,
> -                                                 INDIO_BUFFER_SOFTWARE,
>                                                   &scmi_iio_buffer_ops);
>                 if (err < 0) {
>                         dev_err(dev,
> diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
> index 46ed12771d2f..eebc036717af 100644
> --- a/drivers/iio/gyro/ssp_gyro_sensor.c
> +++ b/drivers/iio/gyro/ssp_gyro_sensor.c
> @@ -113,7 +113,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
>         indio_dev->available_scan_masks = ssp_gyro_scan_mask;
>
>         ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &ssp_gyro_buffer_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> index 36ba7611d9ce..ad5717965223 100644
> --- a/drivers/iio/health/max30100.c
> +++ b/drivers/iio/health/max30100.c
> @@ -433,7 +433,6 @@ static int max30100_probe(struct i2c_client *client,
>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &max30100_buffer_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index 2292876c55e2..abbcef563807 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -542,7 +542,6 @@ static int max30102_probe(struct i2c_client *client,
>         }
>
>         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &max30102_buffer_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 383cc3250342..c3f433ad3af6 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -731,7 +731,6 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
>         indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
>
>         ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &inv_icm42600_buffer_ops);
>         if (ret)
>                 return ERR_PTR(ret);
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index cec1dd0e0464..9d94a8518e3c 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -743,7 +743,6 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
>         indio_dev->setup_ops = &inv_icm42600_buffer_ops;
>
>         ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &inv_icm42600_buffer_ops);
>         if (ret)
>                 return ERR_PTR(ret);
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 16730a780964..f80c62849d30 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -746,7 +746,6 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
>                         continue;
>
>                 ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
> -                                                 INDIO_BUFFER_SOFTWARE,
>                                                   &st_lsm6dsx_buffer_ops);
>                 if (ret)
>                         return ret;
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 4141c0fa7bc4..09b831f9f40b 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -1003,7 +1003,6 @@ static int apds9960_probe(struct i2c_client *client,
>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &apds9960_buffer_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 793918e1c45f..f177b20f0f2d 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -749,7 +749,6 @@ static int ad5933_probe(struct i2c_client *client,
>         indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
>
>         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> -                                         INDIO_BUFFER_SOFTWARE,
>                                           &ad5933_ring_setup_ops);
>         if (ret)
>                 return ret;
> diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> index ccd2ceae7b25..8a83fb58232d 100644
> --- a/include/linux/iio/kfifo_buf.h
> +++ b/include/linux/iio/kfifo_buf.h
> @@ -12,11 +12,10 @@ void iio_kfifo_free(struct iio_buffer *r);
>
>  int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
>                                     struct iio_dev *indio_dev,
> -                                   int mode_flags,
>                                     const struct iio_buffer_setup_ops *setup_ops,
>                                     const struct attribute **buffer_attrs);
>
> -#define devm_iio_kfifo_buffer_setup(dev, indio_dev, mode_flags, setup_ops)     \
> -       devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (mode_flags), (setup_ops), NULL)
> +#define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops) \
> +       devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
>
>  #endif
> --
> 2.27.0
>

Re: [PATCH 09/10] iio: core: Simplify the registration of kfifo buffers

[Jonathan Cameron]

On Thu, 16 Dec 2021 09:52:38 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Among all the users of the kfifo buffers, no one uses the
> > INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
> > simplify a little bit the internals - overall the documentation - by
> > eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
> > default with kfifo buffers, which will basically mimic what all the "non
> > direct" modes do.
> >  
> 
> I think this patch here (and 10/10) can be part of a separate series.
> This discussion is important enough to have it's own series.
> It's a bit of an API discussion.
>
One question inline.
 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/accel/fxls8962af-core.c                  |  1 -
> >  drivers/iio/accel/sca3000.c                          |  1 -
> >  drivers/iio/accel/ssp_accel_sensor.c                 |  1 -
> >  drivers/iio/adc/ina2xx-adc.c                         |  1 -
> >  drivers/iio/adc/ti_am335x_adc.c                      |  4 +---
> >  drivers/iio/buffer/kfifo_buf.c                       | 12 ++----------
> >  .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
> >  drivers/iio/common/scmi_sensors/scmi_iio.c           |  1 -
> >  drivers/iio/gyro/ssp_gyro_sensor.c                   |  1 -
> >  drivers/iio/health/max30100.c                        |  1 -
> >  drivers/iio/health/max30102.c                        |  1 -
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c    |  1 -
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c     |  1 -
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c       |  1 -
> >  drivers/iio/light/apds9960.c                         |  1 -
> >  drivers/staging/iio/impedance-analyzer/ad5933.c      |  1 -
> >  include/linux/iio/kfifo_buf.h                        |  5 ++---
> >  17 files changed, 6 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> > index 32989d91b982..cf494937a346 100644
> > --- a/drivers/iio/accel/fxls8962af-core.c
> > +++ b/drivers/iio/accel/fxls8962af-core.c
> > @@ -1209,7 +1209,6 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
> >                         return ret;
> >
> >                 ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > -                                                 INDIO_BUFFER_SOFTWARE,
> >                                                   &fxls8962af_buffer_ops);
> >                 if (ret)
> >                         return ret;
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index c6b75308148a..a65a05f1208b 100644
> > --- a/drivers/iio/accel/sca3000.c
> > +++ b/drivers/iio/accel/sca3000.c
> > @@ -1465,7 +1465,6 @@ static int sca3000_probe(struct spi_device *spi)
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &sca3000_ring_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
> > index 04dcb2b657ee..d20f8d299c11 100644
> > --- a/drivers/iio/accel/ssp_accel_sensor.c
> > +++ b/drivers/iio/accel/ssp_accel_sensor.c
> > @@ -113,7 +113,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
> >         indio_dev->available_scan_masks = ssp_accel_scan_mask;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &ssp_accel_buffer_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index a4b2ff9e0dd5..013d2d586df1 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -1030,7 +1030,6 @@ static int ina2xx_probe(struct i2c_client *client,
> >         indio_dev->name = id->name;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &ina2xx_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> > index dbdc1ef48566..567d43a30955 100644
> > --- a/drivers/iio/adc/ti_am335x_adc.c
> > +++ b/drivers/iio/adc/ti_am335x_adc.c
> > @@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
> >  {
> >         int ret;
> >
> > -       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> > -                                         setup_ops);
> > +       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
> >         if (ret)
> >                 return ret;
> >
> > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > index 416d35a61ae2..f17473e06ee8 100644
> > --- a/drivers/iio/buffer/kfifo_buf.c
> > +++ b/drivers/iio/buffer/kfifo_buf.c
> > @@ -197,7 +197,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
> >         .set_length = &iio_set_length_kfifo,
> >         .release = &iio_kfifo_buffer_release,
> >
> > -       .modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
> > +       .modes = INDIO_BUFFER_SOFTWARE,

Why is this safe to do?  Don't we need triggered buffers to match mode
when used with a kfifo?

If I'm remembering / reading how the mode matching works, this breaks the
industrialio-triggered-buffer.c case where it sets the mode to
INDIO_BUFFER_TRIGGERED (mostly I think to make sure we get a warning in
iio_verify_update() if no trigger is supplied - I can't remember what other
difference it makes (or if it does).



> >  };
> >
> >  struct iio_buffer *iio_kfifo_allocate(void)
> > @@ -259,8 +259,6 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
> >   * devm_iio_kfifo_buffer_setup_ext - Allocate a kfifo buffer & attach it to an IIO device
> >   * @dev: Device object to which to attach the life-time of this kfifo buffer
> >   * @indio_dev: The device the buffer should be attached to
> > - * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
> > - *             INDIO_BUFFER_TRIGGERED).
> >   * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
> >   * @buffer_attrs: Extra sysfs buffer attributes for this IIO buffer
> >   *
> > @@ -271,22 +269,16 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
> >   */
> >  int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
> >                                     struct iio_dev *indio_dev,
> > -                                   int mode_flags,
> >                                     const struct iio_buffer_setup_ops *setup_ops,
> >                                     const struct attribute **buffer_attrs)
> >  {
> >         struct iio_buffer *buffer;
> >
> > -       if (!mode_flags)
> > -               return -EINVAL;
> > -
> >         buffer = devm_iio_kfifo_allocate(dev);
> >         if (!buffer)
> >                 return -ENOMEM;
> >
> > -       mode_flags &= kfifo_access_funcs.modes;
> > -
> > -       indio_dev->modes |= mode_flags;
> > +       indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> >         indio_dev->setup_ops = setup_ops;
> >
> >         buffer->attrs = buffer_attrs;
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index b2725c6adc7f..a4cf1d9a8a49 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -333,8 +333,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >                          * We can not use trigger here, as events are generated
> >                          * as soon as sample_frequency is set.
> >                          */
> > -                       ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
> > -                                                             INDIO_BUFFER_SOFTWARE, NULL,
> > +                       ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL,
> >                                                               cros_ec_sensor_fifo_attributes);
> >                         if (ret)
> >                                 return ret;
> > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > index 7cf2bf282cef..792807b2894f 100644
> > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> > @@ -631,7 +631,6 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
> >
> >                 err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
> >                                                   scmi_iio_dev,
> > -                                                 INDIO_BUFFER_SOFTWARE,
> >                                                   &scmi_iio_buffer_ops);
> >                 if (err < 0) {
> >                         dev_err(dev,
> > diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
> > index 46ed12771d2f..eebc036717af 100644
> > --- a/drivers/iio/gyro/ssp_gyro_sensor.c
> > +++ b/drivers/iio/gyro/ssp_gyro_sensor.c
> > @@ -113,7 +113,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
> >         indio_dev->available_scan_masks = ssp_gyro_scan_mask;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &ssp_gyro_buffer_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
> > index 36ba7611d9ce..ad5717965223 100644
> > --- a/drivers/iio/health/max30100.c
> > +++ b/drivers/iio/health/max30100.c
> > @@ -433,7 +433,6 @@ static int max30100_probe(struct i2c_client *client,
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &max30100_buffer_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> > index 2292876c55e2..abbcef563807 100644
> > --- a/drivers/iio/health/max30102.c
> > +++ b/drivers/iio/health/max30102.c
> > @@ -542,7 +542,6 @@ static int max30102_probe(struct i2c_client *client,
> >         }
> >
> >         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &max30102_buffer_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > index 383cc3250342..c3f433ad3af6 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > @@ -731,7 +731,6 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> >         indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
> >
> >         ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &inv_icm42600_buffer_ops);
> >         if (ret)
> >                 return ERR_PTR(ret);
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > index cec1dd0e0464..9d94a8518e3c 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > @@ -743,7 +743,6 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> >         indio_dev->setup_ops = &inv_icm42600_buffer_ops;
> >
> >         ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &inv_icm42600_buffer_ops);
> >         if (ret)
> >                 return ERR_PTR(ret);
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index 16730a780964..f80c62849d30 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -746,7 +746,6 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> >                         continue;
> >
> >                 ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
> > -                                                 INDIO_BUFFER_SOFTWARE,
> >                                                   &st_lsm6dsx_buffer_ops);
> >                 if (ret)
> >                         return ret;
> > diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> > index 4141c0fa7bc4..09b831f9f40b 100644
> > --- a/drivers/iio/light/apds9960.c
> > +++ b/drivers/iio/light/apds9960.c
> > @@ -1003,7 +1003,6 @@ static int apds9960_probe(struct i2c_client *client,
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &apds9960_buffer_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index 793918e1c45f..f177b20f0f2d 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -749,7 +749,6 @@ static int ad5933_probe(struct i2c_client *client,
> >         indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >
> >         ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > -                                         INDIO_BUFFER_SOFTWARE,
> >                                           &ad5933_ring_setup_ops);
> >         if (ret)
> >                 return ret;
> > diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
> > index ccd2ceae7b25..8a83fb58232d 100644
> > --- a/include/linux/iio/kfifo_buf.h
> > +++ b/include/linux/iio/kfifo_buf.h
> > @@ -12,11 +12,10 @@ void iio_kfifo_free(struct iio_buffer *r);
> >
> >  int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
> >                                     struct iio_dev *indio_dev,
> > -                                   int mode_flags,
> >                                     const struct iio_buffer_setup_ops *setup_ops,
> >                                     const struct attribute **buffer_attrs);
> >
> > -#define devm_iio_kfifo_buffer_setup(dev, indio_dev, mode_flags, setup_ops)     \
> > -       devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (mode_flags), (setup_ops), NULL)
> > +#define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops) \
> > +       devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
> >
> >  #endif
> > --
> > 2.27.0
> >  

Re: [PATCH 09/10] iio: core: Simplify the registration of kfifo buffers

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 17:12:46 +0000:

> On Thu, 16 Dec 2021 09:52:38 +0200
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > Among all the users of the kfifo buffers, no one uses the
> > > INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
> > > simplify a little bit the internals - overall the documentation - by
> > > eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
> > > default with kfifo buffers, which will basically mimic what all the "non
> > > direct" modes do.
> > >    
> > 
> > I think this patch here (and 10/10) can be part of a separate series.
> > This discussion is important enough to have it's own series.
> > It's a bit of an API discussion.
> >  
> One question inline.
>  
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/accel/fxls8962af-core.c                  |  1 -
> > >  drivers/iio/accel/sca3000.c                          |  1 -
> > >  drivers/iio/accel/ssp_accel_sensor.c                 |  1 -
> > >  drivers/iio/adc/ina2xx-adc.c                         |  1 -
> > >  drivers/iio/adc/ti_am335x_adc.c                      |  4 +---
> > >  drivers/iio/buffer/kfifo_buf.c                       | 12 ++----------
> > >  .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
> > >  drivers/iio/common/scmi_sensors/scmi_iio.c           |  1 -
> > >  drivers/iio/gyro/ssp_gyro_sensor.c                   |  1 -
> > >  drivers/iio/health/max30100.c                        |  1 -
> > >  drivers/iio/health/max30102.c                        |  1 -
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c    |  1 -
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c     |  1 -
> > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c       |  1 -
> > >  drivers/iio/light/apds9960.c                         |  1 -
> > >  drivers/staging/iio/impedance-analyzer/ad5933.c      |  1 -
> > >  include/linux/iio/kfifo_buf.h                        |  5 ++---
> > >  17 files changed, 6 insertions(+), 31 deletions(-)

[...]

> > > --- a/drivers/iio/adc/ti_am335x_adc.c
> > > +++ b/drivers/iio/adc/ti_am335x_adc.c
> > > @@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
> > >  {
> > >         int ret;
> > >
> > > -       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > > -                                         INDIO_BUFFER_SOFTWARE,
> > > -                                         setup_ops);
> > > +       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
> > >         if (ret)
> > >                 return ret;
> > >
> > > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > > index 416d35a61ae2..f17473e06ee8 100644
> > > --- a/drivers/iio/buffer/kfifo_buf.c
> > > +++ b/drivers/iio/buffer/kfifo_buf.c
> > > @@ -197,7 +197,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
> > >         .set_length = &iio_set_length_kfifo,
> > >         .release = &iio_kfifo_buffer_release,
> > >
> > > -       .modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
> > > +       .modes = INDIO_BUFFER_SOFTWARE,  
> 
> Why is this safe to do?  Don't we need triggered buffers to match mode
> when used with a kfifo?
> 
> If I'm remembering / reading how the mode matching works, this breaks the
> industrialio-triggered-buffer.c case where it sets the mode to
> INDIO_BUFFER_TRIGGERED (mostly I think to make sure we get a warning in
> iio_verify_update() if no trigger is supplied - I can't remember what other
> difference it makes (or if it does).

Mmmh that's right, I overlooked that part. Indeed we still need to
support the INDIO_BUFFER_TRIGGERED mode in the kfifo_access_funcs
modes.

Do you think the rest of the patch is still legit? I believe it stills
simplifies the logic for the user but if you disagree I'll drop it off
entirely.

Thanks,
Miquèl

Re: [PATCH 09/10] iio: core: Simplify the registration of kfifo buffers

[Jonathan Cameron]

On Wed, 2 Feb 2022 09:09:36 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> jic23@kernel.org wrote on Sat, 15 Jan 2022 17:12:46 +0000:
> 
> > On Thu, 16 Dec 2021 09:52:38 +0200
> > Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> >   
> > > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:    
> > > >
> > > > Among all the users of the kfifo buffers, no one uses the
> > > > INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
> > > > simplify a little bit the internals - overall the documentation - by
> > > > eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
> > > > default with kfifo buffers, which will basically mimic what all the "non
> > > > direct" modes do.
> > > >      
> > > 
> > > I think this patch here (and 10/10) can be part of a separate series.
> > > This discussion is important enough to have it's own series.
> > > It's a bit of an API discussion.
> > >    
> > One question inline.
> >    
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/iio/accel/fxls8962af-core.c                  |  1 -
> > > >  drivers/iio/accel/sca3000.c                          |  1 -
> > > >  drivers/iio/accel/ssp_accel_sensor.c                 |  1 -
> > > >  drivers/iio/adc/ina2xx-adc.c                         |  1 -
> > > >  drivers/iio/adc/ti_am335x_adc.c                      |  4 +---
> > > >  drivers/iio/buffer/kfifo_buf.c                       | 12 ++----------
> > > >  .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
> > > >  drivers/iio/common/scmi_sensors/scmi_iio.c           |  1 -
> > > >  drivers/iio/gyro/ssp_gyro_sensor.c                   |  1 -
> > > >  drivers/iio/health/max30100.c                        |  1 -
> > > >  drivers/iio/health/max30102.c                        |  1 -
> > > >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c    |  1 -
> > > >  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c     |  1 -
> > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c       |  1 -
> > > >  drivers/iio/light/apds9960.c                         |  1 -
> > > >  drivers/staging/iio/impedance-analyzer/ad5933.c      |  1 -
> > > >  include/linux/iio/kfifo_buf.h                        |  5 ++---
> > > >  17 files changed, 6 insertions(+), 31 deletions(-)  
> 
> [...]
> 
> > > > --- a/drivers/iio/adc/ti_am335x_adc.c
> > > > +++ b/drivers/iio/adc/ti_am335x_adc.c
> > > > @@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
> > > >  {
> > > >         int ret;
> > > >
> > > > -       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> > > > -                                         INDIO_BUFFER_SOFTWARE,
> > > > -                                         setup_ops);
> > > > +       ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > > > index 416d35a61ae2..f17473e06ee8 100644
> > > > --- a/drivers/iio/buffer/kfifo_buf.c
> > > > +++ b/drivers/iio/buffer/kfifo_buf.c
> > > > @@ -197,7 +197,7 @@ static const struct iio_buffer_access_funcs kfifo_access_funcs = {
> > > >         .set_length = &iio_set_length_kfifo,
> > > >         .release = &iio_kfifo_buffer_release,
> > > >
> > > > -       .modes = INDIO_BUFFER_SOFTWARE | INDIO_BUFFER_TRIGGERED,
> > > > +       .modes = INDIO_BUFFER_SOFTWARE,    
> > 
> > Why is this safe to do?  Don't we need triggered buffers to match mode
> > when used with a kfifo?
> > 
> > If I'm remembering / reading how the mode matching works, this breaks the
> > industrialio-triggered-buffer.c case where it sets the mode to
> > INDIO_BUFFER_TRIGGERED (mostly I think to make sure we get a warning in
> > iio_verify_update() if no trigger is supplied - I can't remember what other
> > difference it makes (or if it does).  
> 
> Mmmh that's right, I overlooked that part. Indeed we still need to
> support the INDIO_BUFFER_TRIGGERED mode in the kfifo_access_funcs
> modes.
> 
> Do you think the rest of the patch is still legit? I believe it stills
> simplifies the logic for the user but if you disagree I'll drop it off
> entirely.
> 
yup. The rest was fine from what I recall.

J
> Thanks,
> Miquèl

[PATCH 10/10] iio: core: Clarify the modes

[Miquel Raynal]

As part of a previous discussion with Jonathan Cameron [1], it appeared
necessary to clarify the meaning of each mode so that new developers
could understand better what they should use or not use and when.

The idea of renaming these modes as been let aside because naming is a
big deal and requires a lot of thinking. So for now let's focus on
correctly explaining what each mode implies.

[1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index d04ab89fa0c2..75b561fd63d0 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -314,7 +314,45 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
 s64 iio_get_time_ns(const struct iio_dev *indio_dev);
 unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
 
-/* Device operating modes */
+/**
+ * Device operating modes
+ * @INDIO_DIRECT_MODE: There is an access to the last single value available.
+ * On most devices, this is a single-shot read. On some devices with data
+ * streams without an 'on-demand' function, this might also be the 'last value'
+ * feature. Above all, this mode internally means that we are not in any of the
+ * other modes, and sysfs reads will definitely work.
+ * Device drivers are pleased to inquire the core about this mode.
+ * @INDIO_BUFFER_TRIGGERED: Most common mode when dealing with kfifo buffers.
+ * It indicates that there is an explicit trigger that must be used. This
+ * requests the core to attach a poll function when enabling the buffer, which
+ * is indicated by the _TRIGGERED suffix.
+ * The core will ensure this mode is set when registering a triggered buffer.
+ * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
+ * No poll function can be attached because there is no triggered infrastructure
+ * we can use to cause capture. There is a kfifo that the hardware will fill,
+ * but not "one scan at a time", just like in a continuous stream. This means
+ * the core will not attach any poll function when enabling the buffer.
+ * The core will ensure this mode is set when registering a simple kfifo buffer.
+ * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
+ * Same as above but this time the buffer is not a kfifo where we have direct
+ * access to the data. Instead, the consumer driver must access the data through
+ * side-channels (or DMA when there is no demux possible in software).
+ * The core will ensure this mode is set when registering a dmaengine buffer.
+ * @INDIO_EVENT_TRIGGERED: Very specific, do not use this mode.
+ * Triggers usually refer to an external event which will start data capture.
+ * Here it is kind of the opposite as, a particular state of the data might
+ * produce an event which can be considered as an event. We don't necessarily
+ * have access to the data itself, but to the event produced. For example, this
+ * can be a threshold detector. The internal path of this mode is very close to
+ * the INDIO_BUFFER_TRIGGERED mode.
+ * The core will ensure this mode is set when registering a triggered event.
+ * @INDIO_HARDWARE_TRIGGERED: STM32 specific mode, do not use it.
+ * Here, triggers can result in data capture and can be routed to multiple
+ * hardware components, which make them close to regular triggers in the way
+ * they must be managed by the core, but without the entire interrupts/poll
+ * functions burden. All of this is irrelevant as it is all hardware mediated
+ * and distributed.
+ */
 #define INDIO_DIRECT_MODE		0x01
 #define INDIO_BUFFER_TRIGGERED		0x02
 #define INDIO_BUFFER_SOFTWARE		0x04
-- 
2.27.0

Re: [PATCH 10/10] iio: core: Clarify the modes

[Jonathan Cameron]

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

> As part of a previous discussion with Jonathan Cameron [1], it appeared
> necessary to clarify the meaning of each mode so that new developers
> could understand better what they should use or not use and when.
> 
> The idea of renaming these modes as been let aside because naming is a
> big deal and requires a lot of thinking. So for now let's focus on
> correctly explaining what each mode implies.
> 
> [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/iio/iio.h | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index d04ab89fa0c2..75b561fd63d0 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -314,7 +314,45 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
>  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
>  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
>  
> -/* Device operating modes */
> +/**
> + * Device operating modes
> + * @INDIO_DIRECT_MODE: There is an access to the last single value available.

I'd avoid 'last' as not obvious wrt to what time point.  Perhaps use something
horrible like "timely"?

> + * On most devices, this is a single-shot read. On some devices with data
> + * streams without an 'on-demand' function, this might also be the 'last value'
> + * feature. Above all, this mode internally means that we are not in any of the
> + * other modes, and sysfs reads will definitely work.

Should work ;)  They might fail for a wide variety of other reasons.

> + * Device drivers are pleased to inquire the core about this mode.
Not totally sure what you mean here.  Perhaps
Device drivers should inform the core if they support this mode.

> + * @INDIO_BUFFER_TRIGGERED: Most common mode when dealing with kfifo buffers.

Avoid "common". That may well change in future as fifos are become increasingly
common on devices over time.  Perhaps just drop this first sentence.

> + * It indicates that there is an explicit trigger that must be used. This

Indicates that an explicit trigger is required. (subtle difference from what you
wrote in that you kind of imply there is only one possible choice)

> + * requests the core to attach a poll function when enabling the buffer, which
> + * is indicated by the _TRIGGERED suffix.
> + * The core will ensure this mode is set when registering a triggered buffer.

I'd call out the function name (mostly to be inline with below where you need
to because there isn't a particularly good way to describe what it is doing).

> + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> + * No poll function can be attached because there is no triggered infrastructure
> + * we can use to cause capture. There is a kfifo that the hardware will fill,
> + * but not "one scan at a time", just like in a continuous stream.

No real relationship to a continuous stream that I can see.  Perhaps something like
"Typically hardware will have a buffer that can hold multiple scans. Software may
 read one or more scans at a single time and push the available data to a Kfifo."

> This means
> + * the core will not attach any poll function when enabling the buffer.
> + * The core will ensure this mode is set when registering a simple kfifo buffer.

I'd call out the function name here.  The above registers a kfifo as well which is
pretty simple...

> + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> + * Same as above but this time the buffer is not a kfifo where we have direct
> + * access to the data. Instead, the consumer driver must access the data through
> + * side-channels 
What do you mean by side-channels here?  That term gets over used - perhaps
"non software visible channels"

 + (or DMA when there is no demux possible in software).
> + * The core will ensure this mode is set when registering a dmaengine buffer.

> + * @INDIO_EVENT_TRIGGERED: Very specific, do not use this mode.

:) That's harsh.. If you happen to be supporting hardware that works this way
it's a valid setting.  Perhaps we'd be safe to say:
"Very unusual."

> + * Triggers usually refer to an external event which will start data capture.
> + * Here it is kind of the opposite as, a particular state of the data might
> + * produce an event which can be considered as an event. We don't necessarily
> + * have access to the data itself, but to the event produced. For example, this
> + * can be a threshold detector. The internal path of this mode is very close to
> + * the INDIO_BUFFER_TRIGGERED mode.
> + * The core will ensure this mode is set when registering a triggered event.
> + * @INDIO_HARDWARE_TRIGGERED: STM32 specific mode, do not use it.

I'd avoid that comment because it'll rot when some other hardware needs something
like this.  Again, perhaps "Very rare / unusual." will be enough to put people
off using it.

> + * Here, triggers can result in data capture and can be routed to multiple
> + * hardware components, which make them close to regular triggers in the way
> + * they must be managed by the core, but without the entire interrupts/poll
> + * functions burden. All of this is irrelevant as it is all hardware mediated
> + * and distributed.

"All this" is not totally clear.  Interrupts are irrelevant as the data flow
is hardware mediated and distributed.

Nice descriptions in general.  Nature of these things is without a straw man
to poke holes in I'd never get around to documenting this very much
appreciated that you took the time to figure all the weird corners out and
write this up.

Jonathan


> + */
>  #define INDIO_DIRECT_MODE		0x01
>  #define INDIO_BUFFER_TRIGGERED		0x02
>  #define INDIO_BUFFER_SOFTWARE		0x04

Re: [PATCH 10/10] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 17:30:50 +0000:

> On Wed, 15 Dec 2021 16:13:44 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > necessary to clarify the meaning of each mode so that new developers
> > could understand better what they should use or not use and when.
> > 
> > The idea of renaming these modes as been let aside because naming is a
> > big deal and requires a lot of thinking. So for now let's focus on
> > correctly explaining what each mode implies.
> > 
> > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/linux/iio/iio.h | 40 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index d04ab89fa0c2..75b561fd63d0 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -314,7 +314,45 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
> >  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
> >  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
> >  
> > -/* Device operating modes */
> > +/**
> > + * Device operating modes
> > + * @INDIO_DIRECT_MODE: There is an access to the last single value available.  
> 
> I'd avoid 'last' as not obvious wrt to what time point.  Perhaps use something
> horrible like "timely"?

I don't feel a big difference between the two, besides timely being far
from easy to understand IMHO, but I'll use it if you think it's best.

> > + * On most devices, this is a single-shot read. On some devices with data
> > + * streams without an 'on-demand' function, this might also be the 'last value'
> > + * feature. Above all, this mode internally means that we are not in any of the
> > + * other modes, and sysfs reads will definitely work.  
> 
> Should work ;)  They might fail for a wide variety of other reasons.

Right.

> > + * Device drivers are pleased to inquire the core about this mode.  
> Not totally sure what you mean here.  Perhaps
> Device drivers should inform the core if they support this mode.

Ok.

> > + * @INDIO_BUFFER_TRIGGERED: Most common mode when dealing with kfifo buffers.  
> 
> Avoid "common". That may well change in future as fifos are become increasingly
> common on devices over time.  Perhaps just drop this first sentence.

I don't think dropping this sentence is a good idea. My first goal here
is to make it easier for newcomers to understand these modes. Here it
clearly states "if you're dealing with a kfifo, keep reading, otherwise
just check out the next mode". Of course this might evolve over time
and if it is the case we can later update the documentation.

I've dropped the "Most" instead, to still indicate this is fairly
common but should not be read like something almost automatic.

> > + * It indicates that there is an explicit trigger that must be used. This  
> 
> Indicates that an explicit trigger is required. (subtle difference from what you
> wrote in that you kind of imply there is only one possible choice)

Fair enough.

> > + * requests the core to attach a poll function when enabling the buffer, which
> > + * is indicated by the _TRIGGERED suffix.
> > + * The core will ensure this mode is set when registering a triggered buffer.  
> 
> I'd call out the function name (mostly to be inline with below where you need
> to because there isn't a particularly good way to describe what it is doing).

Done.

> 
> > + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> > + * No poll function can be attached because there is no triggered infrastructure
> > + * we can use to cause capture. There is a kfifo that the hardware will fill,
> > + * but not "one scan at a time", just like in a continuous stream.  
> 
> No real relationship to a continuous stream that I can see.  Perhaps something like
> "Typically hardware will have a buffer that can hold multiple scans. Software may
>  read one or more scans at a single time and push the available data to a Kfifo."

Added.

> 
> > This means
> > + * the core will not attach any poll function when enabling the buffer.
> > + * The core will ensure this mode is set when registering a simple kfifo buffer.  
> 
> I'd call out the function name here.  The above registers a kfifo as well which is
> pretty simple...

Sure.

> 
> > + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> > + * Same as above but this time the buffer is not a kfifo where we have direct
> > + * access to the data. Instead, the consumer driver must access the data through
> > + * side-channels   
> What do you mean by side-channels here?  That term gets over used - perhaps
> "non software visible channels"

Clear.

> 
>  + (or DMA when there is no demux possible in software).
> > + * The core will ensure this mode is set when registering a dmaengine buffer.  
> 
> > + * @INDIO_EVENT_TRIGGERED: Very specific, do not use this mode.  
> 
> :) That's harsh..

Looks like you changed your mind, that's almost what you proposed back
in September ;)

> If you happen to be supporting hardware that works this way
> it's a valid setting.  Perhaps we'd be safe to say:
> "Very unusual."
> 
> > + * Triggers usually refer to an external event which will start data capture.
> > + * Here it is kind of the opposite as, a particular state of the data might
> > + * produce an event which can be considered as an event. We don't necessarily
> > + * have access to the data itself, but to the event produced. For example, this
> > + * can be a threshold detector. The internal path of this mode is very close to
> > + * the INDIO_BUFFER_TRIGGERED mode.
> > + * The core will ensure this mode is set when registering a triggered event.
> > + * @INDIO_HARDWARE_TRIGGERED: STM32 specific mode, do not use it.  
> 
> I'd avoid that comment because it'll rot when some other hardware needs something
> like this.  Again, perhaps "Very rare / unusual." will be enough to put people
> off using it.

As you prefer.

> > + * Here, triggers can result in data capture and can be routed to multiple
> > + * hardware components, which make them close to regular triggers in the way
> > + * they must be managed by the core, but without the entire interrupts/poll
> > + * functions burden. All of this is irrelevant as it is all hardware mediated
> > + * and distributed.  
> 
> "All this" is not totally clear.  Interrupts are irrelevant as the data flow
> is hardware mediated and distributed.

Thanks for the alternative.

> 
> Nice descriptions in general.  Nature of these things is without a straw man
> to poke holes in I'd never get around to documenting this very much
> appreciated that you took the time to figure all the weird corners out and
> write this up.
> 

I'm happy if this can be useful!

V2 finally coming soon.

Thanks,
Miquèl

Re: [PATCH 10/10] iio: core: Clarify the modes

[Jonathan Cameron]

On Wed, 2 Feb 2022 14:46:35 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> jic23@kernel.org wrote on Sat, 15 Jan 2022 17:30:50 +0000:
> 
> > On Wed, 15 Dec 2021 16:13:44 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > necessary to clarify the meaning of each mode so that new developers
> > > could understand better what they should use or not use and when.
> > > 
> > > The idea of renaming these modes as been let aside because naming is a
> > > big deal and requires a lot of thinking. So for now let's focus on
> > > correctly explaining what each mode implies.
> > > 
> > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > 
> > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/linux/iio/iio.h | 40 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 39 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index d04ab89fa0c2..75b561fd63d0 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -314,7 +314,45 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
> > >  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
> > >  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
> > >  
> > > -/* Device operating modes */
> > > +/**
> > > + * Device operating modes
> > > + * @INDIO_DIRECT_MODE: There is an access to the last single value available.    
> > 
> > I'd avoid 'last' as not obvious wrt to what time point.  Perhaps use something
> > horrible like "timely"?  
> 
> I don't feel a big difference between the two, besides timely being far
> from easy to understand IMHO, but I'll use it if you think it's best.
timely is deliberately slightly vague.  An alternative would be to lay it out
in detail

There is an access to either:
a) The last single value available for devices that do not provide on demand
   reads.
b) A read of a new value is performed on demand.

> 
> > > + * On most devices, this is a single-shot read. On some devices with data
> > > + * streams without an 'on-demand' function, this might also be the 'last value'
> > > + * feature. Above all, this mode internally means that we are not in any of the
> > > + * other modes, and sysfs reads will definitely work.    
> > 
> > Should work ;)  They might fail for a wide variety of other reasons.  
> 
> Right.
> 
> > > + * Device drivers are pleased to inquire the core about this mode.    
> > Not totally sure what you mean here.  Perhaps
> > Device drivers should inform the core if they support this mode.  
> 
> Ok.
> 
> > > + * @INDIO_BUFFER_TRIGGERED: Most common mode when dealing with kfifo buffers.    
> > 
> > Avoid "common". That may well change in future as fifos are become increasingly
> > common on devices over time.  Perhaps just drop this first sentence.  
> 
> I don't think dropping this sentence is a good idea. My first goal here
> is to make it easier for newcomers to understand these modes. Here it
> clearly states "if you're dealing with a kfifo, keep reading, otherwise
> just check out the next mode". Of course this might evolve over time
> and if it is the case we can later update the documentation.
> 
> I've dropped the "Most" instead, to still indicate this is fairly
> common but should not be read like something almost automatic.
That works.

> 
> > > + * It indicates that there is an explicit trigger that must be used. This    
> > 
> > Indicates that an explicit trigger is required. (subtle difference from what you
> > wrote in that you kind of imply there is only one possible choice)  
> 
> Fair enough.
> 
> > > + * requests the core to attach a poll function when enabling the buffer, which
> > > + * is indicated by the _TRIGGERED suffix.
> > > + * The core will ensure this mode is set when registering a triggered buffer.    
> > 
> > I'd call out the function name (mostly to be inline with below where you need
> > to because there isn't a particularly good way to describe what it is doing).  
> 
> Done.
> 
> >   
> > > + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> > > + * No poll function can be attached because there is no triggered infrastructure
> > > + * we can use to cause capture. There is a kfifo that the hardware will fill,
> > > + * but not "one scan at a time", just like in a continuous stream.    
> > 
> > No real relationship to a continuous stream that I can see.  Perhaps something like
> > "Typically hardware will have a buffer that can hold multiple scans. Software may
> >  read one or more scans at a single time and push the available data to a Kfifo."  
> 
> Added.
> 
> >   
> > > This means
> > > + * the core will not attach any poll function when enabling the buffer.
> > > + * The core will ensure this mode is set when registering a simple kfifo buffer.    
> > 
> > I'd call out the function name here.  The above registers a kfifo as well which is
> > pretty simple...  
> 
> Sure.
> 
> >   
> > > + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> > > + * Same as above but this time the buffer is not a kfifo where we have direct
> > > + * access to the data. Instead, the consumer driver must access the data through
> > > + * side-channels     
> > What do you mean by side-channels here?  That term gets over used - perhaps
> > "non software visible channels"  
> 
> Clear.
> 
> > 
> >  + (or DMA when there is no demux possible in software).  
> > > + * The core will ensure this mode is set when registering a dmaengine buffer.    
> >   
> > > + * @INDIO_EVENT_TRIGGERED: Very specific, do not use this mode.    
> > 
> > :) That's harsh..  
> 
> Looks like you changed your mind, that's almost what you proposed back
> in September ;)

I'm inconsistent :)

> 
> > If you happen to be supporting hardware that works this way
> > it's a valid setting.  Perhaps we'd be safe to say:
> > "Very unusual."
> >   
> > > + * Triggers usually refer to an external event which will start data capture.
> > > + * Here it is kind of the opposite as, a particular state of the data might
> > > + * produce an event which can be considered as an event. We don't necessarily
> > > + * have access to the data itself, but to the event produced. For example, this
> > > + * can be a threshold detector. The internal path of this mode is very close to
> > > + * the INDIO_BUFFER_TRIGGERED mode.
> > > + * The core will ensure this mode is set when registering a triggered event.
> > > + * @INDIO_HARDWARE_TRIGGERED: STM32 specific mode, do not use it.    
> > 
> > I'd avoid that comment because it'll rot when some other hardware needs something
> > like this.  Again, perhaps "Very rare / unusual." will be enough to put people
> > off using it.  
> 
> As you prefer.
> 
> > > + * Here, triggers can result in data capture and can be routed to multiple
> > > + * hardware components, which make them close to regular triggers in the way
> > > + * they must be managed by the core, but without the entire interrupts/poll
> > > + * functions burden. All of this is irrelevant as it is all hardware mediated
> > > + * and distributed.    
> > 
> > "All this" is not totally clear.  Interrupts are irrelevant as the data flow
> > is hardware mediated and distributed.  
> 
> Thanks for the alternative.
> 
> > 
> > Nice descriptions in general.  Nature of these things is without a straw man
> > to poke holes in I'd never get around to documenting this very much
> > appreciated that you took the time to figure all the weird corners out and
> > write this up.
> >   
> 
> I'm happy if this can be useful!
> 
> V2 finally coming soon.
You beat me replying but I don't think any of the above replies will greatly influence things.

This wordy stuff always takes more thought that code so yet again you end
up at the end of my review queue with these on the basis of too hard - I'll
do it later :)

Jonathan

Re: [PATCH 10/10] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 5 Feb 2022 18:56:00 +0000:

> On Wed, 2 Feb 2022 14:46:35 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Jonathan,
> > 
> > jic23@kernel.org wrote on Sat, 15 Jan 2022 17:30:50 +0000:
> >   
> > > On Wed, 15 Dec 2021 16:13:44 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > > > necessary to clarify the meaning of each mode so that new developers
> > > > could understand better what they should use or not use and when.
> > > > 
> > > > The idea of renaming these modes as been let aside because naming is a
> > > > big deal and requires a lot of thinking. So for now let's focus on
> > > > correctly explaining what each mode implies.
> > > > 
> > > > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > > > 
> > > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/linux/iio/iio.h | 40 +++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 39 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > index d04ab89fa0c2..75b561fd63d0 100644
> > > > --- a/include/linux/iio/iio.h
> > > > +++ b/include/linux/iio/iio.h
> > > > @@ -314,7 +314,45 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
> > > >  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
> > > >  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
> > > >  
> > > > -/* Device operating modes */
> > > > +/**
> > > > + * Device operating modes
> > > > + * @INDIO_DIRECT_MODE: There is an access to the last single value available.      
> > > 
> > > I'd avoid 'last' as not obvious wrt to what time point.  Perhaps use something
> > > horrible like "timely"?    
> > 
> > I don't feel a big difference between the two, besides timely being far
> > from easy to understand IMHO, but I'll use it if you think it's best.  
> timely is deliberately slightly vague.  An alternative would be to lay it out
> in detail
> 
> There is an access to either:
> a) The last single value available for devices that do not provide on demand
>    reads.
> b) A read of a new value is performed on demand.

I just get now why you refused the "last" wording.

That feels infinitely clearer. I'll wait for feedback on the second
version, and include these additional details.

[...]

> > V2 finally coming soon.  
> You beat me replying but I don't think any of the above replies will greatly influence things.
> 
> This wordy stuff always takes more thought that code so yet again you end
> up at the end of my review queue with these on the basis of too hard - I'll
> do it later :)

Take your time, you're not the only reviewer either.

Thanks,
Miquèl