Re: [PATCH v1 6/7] iio: st_sensors: Add lsm9ds0 IMU support

[Andy Shevchenko]

On Sun, Apr 18, 2021 at 2:07 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

Thanks for review, my answers below.

> On Wed, 14 Apr 2021 22:54:53 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > We can utilize separate drivers for accelerometer and magnetometer,
> > so here is the glue driver to enable LSM9DS0 IMU support.
> >
> > The idea was suggested by Crestez Dan Leonard in [1]. The proposed change
> > was sent as RFC due to race condition concerns, which are indeed possible.
>
> If you are going to mention races, good to give some flavour in here!

I meant that the initial idea is racy due to different devices
communicating to the same i2c address.
So, any sequence of transfers are not serialized and you may end up with

drv1 -> i2c
drv2 -> i2c
drv1 <- i2c # garbage

> This driver makes me very nervous indeed.

Why?! This one is race free as far as I can see. Or maybe I interpret
this wrongly and you are talking about initial RFC?

>  I haven't 'found' any places
> where the fact we'll write the same registers from each of the drivers
> causes problems (e.g. int pin setup etc) but perhaps I'm missing something.
>
> Shall we say that makes me rather keener to get eyes (and thought) on this
> patch than normal :)

How should I amend the commit message to state:
1. First idea (RFC by the link) *is* racy AFAIU
2. This one *is not* racy.

> > In order to amend the initial change,

You see, *in order to amend*, so here is the *amended* version.

> I went further by providing a specific
> > multi-instantiate probe driver that reuses existing accelerometer and
> > magnetometer.
> >
> > [1]: https://lore.kernel.org/patchwork/patch/670353/
> >
> > Suggested-by: Crestez Dan Leonard <leonard.crestez@xxxxxxxxx>
> > Cc: mr.lahorde@xxxxxxxxxxx
> > Cc: Matija Podravec <matija_podravec@xxxxxxxxxxx>
> > Cc: Sergey Borishchenko <borischenko.sergey@xxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> A few comments in here, though mostly about stuff related to the origin code
> you are copying so perhaps not tidying them up is preferable because it would
> complicate comparison of the two cases.

...

> > +     {
> > +             .wai = 0x49,
> > +             .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
> > +             .sensors_supported = {
> > +                     [0] = LSM9DS0_IMU_DEV_NAME,
>
> What does the name attribute report for these?
>
> Previously we've had the _accel etc postfix to differentiate the devices. I don't
> suppose it matters to much though as easy enough to identify the accelerometer
> etc from what channels are present.
>
> Of course driver may get name from somewhere different anyway, I haven't checked,
> just noticed this was different and wondered what the affect might be.

Yes, it has a postfix, that's why I leave it like this.

...

> > +static int st_lsm9ds0_power_enable(struct device *dev, struct st_lsm9ds0 *lsm9ds0)
> > +{
> > +     int ret;
> > +
> > +     /* Regulators not mandatory, but if requested we should enable them. */
>
> That's a bit of a missleading comment though cut and paste from the other driver
> code.  Key is that they will be handled by stub regulators if we don't provide
> the which is not really what that comment implies.

I see. I will remove it.

> > +     lsm9ds0->vdd = devm_regulator_get(dev, "vdd");
> > +     if (IS_ERR(lsm9ds0->vdd)) {
> > +             dev_err(dev, "unable to get Vdd supply\n");
> > +             return PTR_ERR(lsm9ds0->vdd);
> > +     }
> > +     ret = regulator_enable(lsm9ds0->vdd);
> > +     if (ret) {
> > +             dev_warn(dev, "Failed to enable specified Vdd supply\n");
>
> Given we fail to probe if this is true, dev_warn seems a bit soft.

Right.  I'll move to dev_err().

> > +             return ret;
> > +     }
> > +
> > +     lsm9ds0->vdd_io = devm_regulator_get(dev, "vddio");
> > +     if (IS_ERR(lsm9ds0->vdd_io)) {
> > +             dev_err(dev, "unable to get Vdd_IO supply\n");
> > +             regulator_disable(lsm9ds0->vdd);
> > +             return PTR_ERR(lsm9ds0->vdd_io);
> > +     }
> > +     ret = regulator_enable(lsm9ds0->vdd_io);
> > +     if (ret) {
> > +             dev_warn(dev, "Failed to enable specified Vdd_IO supply\n");

Ditto.

> > +             regulator_disable(lsm9ds0->vdd);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko