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

[Jonathan Cameron]

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