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
