Hi Alexandru,
ardeleanalex@xxxxxxxxx wrote on Thu, 16 Dec 2021 08:40:12 +0200:
> 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).
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
