Re: [PATCH 4/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

From: Jonathan Cameron
Date: Wed Feb 14 2024 - 11:34:18 EST


On Mon, 12 Feb 2024 16:04:02 +0100
Ondřej Jirman <megi@xxxxxx> wrote:

> Hi Jonathan,
>
> thank you for the patch review.
>
> On Mon, Feb 12, 2024 at 01:02:32PM +0000, Jonathan Cameron wrote:
> > On Sun, 11 Feb 2024 21:52:00 +0100
> > Ondřej Jirman <megi@xxxxxx> wrote:
> >
> > > From: Icenowy Zheng <icenowy@xxxxxxx>
> > >
> > > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > >
> > > Add a simple IIO driver for it.
> > >
> > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> > > Signed-off-by: Dalton Durst <dalton@xxxxxxxxxxx>
> > > Signed-off-by: Shoji Keita <awaittrot@xxxxxxx>
> > > Signed-off-by: Ondrej Jirman <megi@xxxxxx>
> >
> > This is a lot of sign offs. If accurate it menas.
> >
> > Icenowy wrote teh driver,
> > Dalton then 'handled' it on the path to Shoji who
> > then 'handled' it on the path to Ondrej.
> >
> > That's possible if it's been in various other trees for instance, but
> > I'd like some more explanation below the --- if that is the case.
> > Otherwise, maybe Co-developed-by: is appropriate for some of
> > the above list?
>
> Icenowy wrote basic driver, initially. Here's some older version with only Icenowy sign off:
>
> https://github.com/Icenowy/linux/commit/468ceb921dae9d75064c46d13c60cab2b42362b3
Ok. So probably the author should be Icenowy as you have it.
>
> I picked the patch into my linux tree a few years back from one of the Mobile
> Linux distributions, likely Mobian:
>
> https://megous.com/git/linux/commit/?h=af8133j-5.17&id=1afd43b002a02cade051acbe7851101258e60194
>
> So I guess Dalton and/or Shoji added the orientation matrix support, because
> that and addition of some error logging is the only difference between pure Icenowy
> version and the version with other sign offs.
ok. If we can figure that out, seems like co-developed for them as well is appropriate.

>
> Then I rewrote large parts of the driver and added a few new features, like
> support for changing the scale/range, RPM, and buffered mode.
Defintely a co-developed for you then!
>
> So I don't know how to reflect this in the tags. :) It passed through all of
> these people, and all of them touched it in some way, I think.

Lots of co-developed probably most appropriate. Basically add one for each
SoB other than Iceynow's

> > > +
> > > +static int af8133j_power_up(struct af8133j_data *data)
> > > +{
> > > + struct device *dev = &data->client->dev;
> > > + int ret;
> > > +
> > > + if (data->powered)
> > > + return 0;
> > > +
> > > + ret = regulator_bulk_enable(AF8133J_NUM_SUPPLIES, data->supplies);
> > > + if (ret) {
> > > + dev_err(dev, "Could not enable regulators\n");
> > > + return ret;
> > > + }
> > > +
> > > + gpiod_set_value_cansleep(data->reset_gpiod, 0);
> > > +
> > > + /* Wait for power on reset */
> > > + usleep_range(15000, 16000);
> > > +
> > > + data->powered = true;
> >
> > Why is this needed? The RPM code is reference counted, so I don't think
> > we should need this.
>
> It's here because of code in af8133j_remove just disables RPM and then calls
> af8133j_power_down(). I guess it can be done via RPM, too, by disabling
> autosuspend and leaving it to RPM callbacks.

ah. Don't use a flag for that, add a little utility function
that takes it as an explicit parameter. Make sure you wake the device
up using runtime_pm then disable runtime pm before powering it down manually.

>
> > > + return 0;

..


> > > +
> > > + ret = af8133j_take_measurement(data);
> > > + if (ret == 0)
> > > + ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> > > + buf, sizeof(__le16) * 3);
> > > +
> > > + mutex_unlock(&data->mutex);
> > > +
> > > + pm_runtime_mark_last_busy(dev);
> > > + if (pm_runtime_put_autosuspend(dev))
> > > + dev_err(dev, "failed to power off\n");
> > I think this will only happen if suspend returns non 0 and yours
> > doesn't. What else might cause this?
>
> I don't know, there's quite a deep callflow under
> https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L470
> with a lot of error paths. I'd say it's very unlikely to get na error here.
>
> I can drop it if you like.

I would. If something odd is going on a developer can easily
add a check back in to debug it.
>
> > > +
> > > + return ret;
> > > +}

..

> >
> > > + pm_runtime_enable(dev);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void af8133j_remove(struct i2c_client *client)
> > > +{
> > > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > + struct af8133j_data *data = iio_priv(indio_dev);
> > > + struct device *dev = &data->client->dev;
> > > +
> > > + pm_runtime_disable(dev);
> > > + pm_runtime_set_suspended(dev);
> > > +
> > > + af8133j_power_down(data);
> >
> > Can normally push these into callbacks using
> > devm_add_action_or_reset()
> > That avoids need for either explicit error handling or a remove()
> >
> > You power the device down here, but there isn't a matching call to
> > power it up in probe() (as it is powered down in there - which you
> > should leave to runtime_pm())
>
> Yes, that's the reason for powered tracking in the driver.
>
ok. Try and avoid that and just let runtime pm deal with it for you.

For future reference, crop out anything you have commented on in
a review. It saves on scrolling and reduces chances of stuff being
missed in the dicussion.


Jonathan