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

[Alexandru Ardelean <ardeleanalex@gmail.com>]

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
>