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