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

[Ondřej Jirman]

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

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.

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.

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.

> Various comments inline, but looks pretty good in general.
> 
> Thanks,
> 
> Jonathan
> 
> > diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> > new file mode 100644
> > index 000000000000..0b03417ba134
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/af8133j.c
> > @@ -0,0 +1,525 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * af8133j.c - Voltafield AF8133J magnetometer driver
> > + *
> > + * Copyright 2021 Icenowy Zheng <icenowy@xxxxxxx>
> > + * Copyright 2024 Ondřej Jirman <megi@xxxxxx>
> > + */
> > +
> > +#include <linux/module.h>
> 
> Alphabetical order for these.
> It's fine to separate out he IIO ones on their own as you have
> done.
> 
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/regmap.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> 
> Don't think you are using this as no custom attrs.
> 
> 
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define AF8133J_DRV_NAME "af8133j"
> > +
> > +#define AF8133J_REG_OUT		0x03
> > +#define AF8133J_REG_PCODE	0x00
> > +#define AF8133J_REG_PCODE_VAL	0x5e
> > +#define AF8133J_REG_STATUS	0x02
> > +#define AF8133J_REG_STATUS_ACQ	BIT(0)
> > +#define AF8133J_REG_STATE	0x0a
> > +#define AF8133J_REG_STATE_STBY	0x00
> > +#define AF8133J_REG_STATE_WORK	0x01
> > +#define AF8133J_REG_RANGE	0x0b
> > +#define AF8133J_REG_RANGE_22G	0x12
> > +#define AF8133J_REG_RANGE_12G	0x34
> > +#define AF8133J_REG_SWR		0x11
> > +#define AF8133J_REG_SWR_PERFORM	0x81
> > +
> > +static const char * const af8133j_supply_names[] = {
> > +	"avdd",
> > +	"dvdd",
> > +};
> > +
> > +#define AF8133J_NUM_SUPPLIES ARRAY_SIZE(af8133j_supply_names)
> 
> Just use this inline, or the size of the array of regulators.
> No need for this define.
> 
> 
> > +static int af8133j_product_check(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Error reading product code (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (val != AF8133J_REG_PCODE_VAL) {
> > +		dev_err(dev, "Invalid product code (0x%02x)\n", val);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int af8133j_reset(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	int ret;
> > +
> > +	if (data->reset_gpiod) {
> > +		/* If we have GPIO reset line, use it */
> > +		gpiod_set_value_cansleep(data->reset_gpiod, 1);
> > +		udelay(10);
> > +		gpiod_set_value_cansleep(data->reset_gpiod, 0);
> > +	} else {
> > +		/* Otherwise use software reset */
> > +		ret = regmap_write(data->regmap, AF8133J_REG_SWR,
> > +				   AF8133J_REG_SWR_PERFORM);
> > +		if (ret < 0) {
> > +			dev_err(dev, "Failed to reset the chip\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* Wait for reset to finish */
> > +	usleep_range(1000, 1100);
> > +
> > +	/* Restore range setting */
> > +	if (data->range == AF8133J_REG_RANGE_22G) {
> > +		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +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.

> > +	return 0;
> > +}
> > +
> > +static void af8133j_power_down(struct af8133j_data *data)
> > +{
> > +	if (!data->powered)
> > +		return;
> > +
> > +	gpiod_set_value_cansleep(data->reset_gpiod, 1);
> > +	regulator_bulk_disable(AF8133J_NUM_SUPPLIES, data->supplies);
> > +	data->powered = false;
> > +}
> 
> > +
> > +static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power on\n");
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&data->mutex);
> 	scoped_guard(mutex, &data->mutex) {
> 		ret = af8133j_take_measurement(data);
> 		if (ret)
> 			goto error_ret;
> 
> 		ret = regmap_bulk_read(...)
> 	}
> 
> error_ret:
> 
> 	pm_runtime_mark_last_busy(dev);
> 
> 
> > +
> > +	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.

> > +
> > +	return ret;
> > +}
> 
> > +
> > +static int af8133j_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	__le16 buf[3];
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = af8133j_read_measurement(data, buf);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = sign_extend32(le16_to_cpu(buf[chan->address]),
> > +				     chan->scan_type.realbits - 1);
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 0;
> > +		*val2 = af8133j_scales[data->range == AF8133J_REG_RANGE_12G ? 0 : 1][1];
> 
> Line length is a bit long and the ternary makes it less easy to read than would be ideal.
> I'd just use an if / else.
> 		if (data->Range == AF8133J_REG_RANGE_12G)
> 			*val2 = af8133j_scales[0][1];
> 		else
> 			*val2 = af8133j_scales[1][1];
> > +		return IIO_VAL_INT_PLUS_NANO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int af8133j_set_scale(struct af8133j_data *data,
> > +			     unsigned int val, unsigned int val2)
> > +{
> > +	u8 range;
> > +	int ret;
> > +
> > +	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
> > +		range = AF8133J_REG_RANGE_12G;
> > +	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
> > +		range = AF8133J_REG_RANGE_22G;
> > +	else
> > +		return -EINVAL;
> > +
> > +	ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> > +	if (ret == 0)
> 	if (ret)
> 		return ret;
> 
> 	data->range = range;
> 
> 	return 0;
> 
> A little more code, but it is easier to review if we use the same pattern
> everywhere.
> 
> > +		data->range = range;
> > +
> > +	return ret;
> > +}
> > +
> > +static int af8133j_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		mutex_lock(&data->mutex);
> 
> Consider using scoped_guard().
> 
> > +		ret = af8133j_set_scale(data, val, val2);
> > +		mutex_unlock(&data->mutex);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				     struct iio_chan_spec const *chan,
> > +				     long mask)
> > +{
> > +	return IIO_VAL_INT_PLUS_NANO;
> > +}
> > +
> > +static const struct iio_info af8133j_info = {
> > +	.read_raw = af8133j_read_raw,
> > +	.read_avail = af8133j_read_avail,
> > +	.write_raw = af8133j_write_raw,
> > +	.write_raw_get_fmt = af8133j_write_raw_get_fmt,
> > +};
> > +
> > +static irqreturn_t af8133j_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	s64 timestamp = iio_get_time_ns(indio_dev);
> > +	struct {
> > +		__le16 values[3];
> > +		s64 timestamp __aligned(8);
> > +	} sample;
> > +	int ret;
> > +
> > +	memset(&sample, 0, sizeof(sample));
> > +
> > +	ret = af8133j_read_measurement(data, sample.values);
> > +	if (ret == 0)
> > +		iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
> Prefer to keep the error path out of line
> 
> 	if (ret)
> 		goto err_ret;
> 
> 	iio_push_to_...
> 
> error_ret:
> 	iio_trigger...
> 
> It's a little more code, but the consistency of code organization makes
> for easier review etc.
> 
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const struct regmap_config af8133j_regmap_config = {
> > +	.name = "af8133j_regmap",
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = AF8133J_REG_SWR,
> > +	.cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int af8133j_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct af8133j_data *data;
> > +	struct iio_dev *indio_dev;
> > +	struct regmap *regmap;
> > +	int ret, i;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(dev, PTR_ERR(regmap),
> > +				     "regmap initialization failed\n");
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +	data->regmap = regmap;
> > +	data->range = AF8133J_REG_RANGE_12G;
> > +	mutex_init(&data->mutex);
> > +
> > +	data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(data->reset_gpiod))
> > +		return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > +				     "Failed to get reset gpio\n");
> > +
> > +	for (i = 0; i < AF8133J_NUM_SUPPLIES; i++)
> > +		data->supplies[i].supply = af8133j_supply_names[i];
> > +	ret = devm_regulator_bulk_get(dev, AF8133J_NUM_SUPPLIES, data->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_read_mount_matrix(dev, &data->orientation);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > +
> > +	ret = af8133j_power_up(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = af8133j_reset(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> Moving over to this being handled by a devm callback would remove the need
> for these manual calls.
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = af8133j_product_check(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> > +		return ret;
> > +	}
> > +
> > +	af8133j_power_down(data);
> 
> Leave this to runtime_pm autosuspend.
> Just make sure to set it as active with appropriate get and put to ensure
> the autosuspend handling deals with this.
> 
> 
> > +
> > +	indio_dev->info = &af8133j_info;
> > +	indio_dev->name = AF8133J_DRV_NAME;
> 
> As below. I'd rather see the string here.
> 
> > +	indio_dev->channels = af8133j_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      &af8133j_trigger_handler, NULL);
> > +	if (ret < 0)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "failed to setup iio triggered buffer\n");
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register iio device");
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > +	pm_runtime_use_autosuspend(dev);
> 
> See the comment on this call in the header. You need to undo it manually - or
> use devm_pm_runtime_enable()
> 
> > +	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.

> 
> 
> 
> > +}
> > +
> > +static int __maybe_unused af8133j_runtime_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +
> > +	af8133j_power_down(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused af8133j_runtime_resume(struct device *dev)
> 
> No need to do the __maybe_unused with the changes below.
> The new way of handling this is to expose them all to the compiler and
> let it do dead code removal.
> 
> That's what the pm_ptr() magic does for us.
> 
> 
> 
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct af8133j_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = af8133j_power_up(data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = af8133j_reset(data);
> > +	if (ret) {
> > +		af8133j_power_down(data);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +const struct dev_pm_ops af8133j_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> 
> This set of macros are deprecated.  
> Use SYSTEM_SLEEP_PM_OPS etc in combination with pm_ptr()
> 
> > +	SET_RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
> > +};
> 
> > +
> > +static struct i2c_driver af8133j_driver = {
> > +	.driver = {
> > +		.name = AF8133J_DRV_NAME,
> 
> I'd prefer to just see the string here and where it used above.
> The define just makes the code harder to read.  There is no
> particular reason the driver name should match the iio_dev->name
> so little advantage in enforcing that via a define.
> 
> > +		.of_match_table = af8133j_of_match,
> > +		.pm = &af8133j_pm_ops,
> 
> pm_ptr()
> 
> otherwise you are going to get unused warnings for the struct.

thanks for all the suggestions. :)

Kind regards,
	o.

> 
> > +	},
> > +	.probe = af8133j_probe,
> > +	.remove = af8133j_remove,
> > +	.id_table = af8133j_id,
> > +};
> > +
> > +module_i2c_driver(af8133j_driver);
> > +
> > +MODULE_AUTHOR("Icenowy Zheng <icenowy@xxxxxxx>");
> > +MODULE_AUTHOR("Ondřej Jirman <megi@xxxxxx>");
> > +MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
> > +MODULE_LICENSE("GPL");
>