Message-ID: <20240214170136.00003a22@Huawei.com>
Date: Wed, 14 Feb 2024 17:01:36 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ondřej Jirman <megi@....cz>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Andrey Skvortsov <andrej.skvortzov@...il.com>, Icenowy Zheng
	<icenowy@...c.io>, Dalton Durst <dalton@...orts.com>, Shoji Keita
	<awaittrot@...k.jp>, <linux-iio@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] iio: magnetometer: add a driver for Voltafield
 AF8133J magnetometer

On Mon, 12 Feb 2024 18:53:55 +0100
Ondřej Jirman <megi@....cz> wrote:

> From: Icenowy Zheng <icenowy@...c.io>
> 
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
> 
> Add a simple IIO driver for it.
> 
> Co-developed-by: Icenowy Zheng <icenowy@...c.io>
> Signed-off-by: Icenowy Zheng <icenowy@...c.io>
> Signed-off-by: Dalton Durst <dalton@...orts.com>
> Signed-off-by: Shoji Keita <awaittrot@...k.jp>
> Co-developed-by: Ondrej Jirman <megi@....cz>
> Signed-off-by: Ondrej Jirman <megi@....cz>


Hi a few comments (mostly on changes)

The runtime_pm handling can be simplified somewhat if you
rearrange probe a little.

> diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
> new file mode 100644
> index 000000000000..1f64a2337f6e
> --- /dev/null
> +++ b/drivers/iio/magnetometer/af8133j.c
> @@ -0,0 +1,528 @@


> +static int af8133j_take_measurement(struct af8133j_data *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_write(data->regmap,
> +			   AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The datasheet says "Mesaure Time <1.5ms" */
> +	ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
> +				       val & AF8133J_REG_STATUS_ACQ,
> +				       500, 1500);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap,
> +			   AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);

return regmap_write()

regmap accesses return 0 or a negative error code enabling little code
reductions like this.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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) {
> +		/*
> +		 * Ignore EACCES because that happens when RPM is disabled
> +		 * during system sleep, while userspace leave eg. hrtimer
> +		 * trigger attached and IIO core keeps trying to do measurements.

Yeah. We still need to fix that more elegantly :(

> +		 */
> +		if (ret != -EACCES)
> +			dev_err(dev, "Failed to power on (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	scoped_guard(mutex, &data->mutex) {
> +		ret = af8133j_take_measurement(data);
> +		if (ret)
> +			goto out_rpm_put;
> +
> +		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
> +				       buf, sizeof(__le16) * 3);
> +	}
> +
> +out_rpm_put:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +


> +
> +static int af8133j_set_scale(struct af8133j_data *data,
> +			     unsigned int val, unsigned int val2)
> +{
> +	struct device *dev = &data->client->dev;
> +	u8 range;
> +	int ret = 0;
> +
> +	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;
> +
> +	pm_runtime_disable(dev);
> +
> +	/*
> +	 * When suspended, just store the new range to data->range to be
> +	 * applied later during power up.
Better to just do
	pm_runtime_resume_and_get() here

> +	 */
> +	if (!pm_runtime_status_suspended(dev))
> +		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, range);
> +
> +	pm_runtime_enable(dev);
and
	pm_runtime_mark_last_busy(dev);
	pm_runtime_put_autosuspend(dev);
here.

The userspace interface is only way this function is called so rearrange
probe a little so that you don't need extra complexity in these functions.


> +
> +	data->range = range;

If the write failed, generally don't update the cached value.

> +	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:
> +		scoped_guard(mutex, &data->mutex)
> +			ret = af8133j_set_scale(data, val, val2);

Look more closely at what scoped_guard() does.
			return af8133j_set_scale(data, val, val2);
is fine and simpler as no local variable needed.

> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static void af8133j_power_down_action(void *ptr)
> +{
> +	struct af8133j_data *data = ptr;
> +	struct device *dev = &data->client->dev;
> +
> +	pm_runtime_disable(dev);
You group together unwinding of calls that occur in very
different places in probe.  Don't do that as it leas
to disabling runtime pm having never enabled it
in some error paths.  That may be safe but if fails the
obviously correct test.

Instead, have multiple callbacks registered.
Disable will happen anyway due to 
> +	if (!pm_runtime_status_suspended(dev))
This works as the stub for no runtime pm support returns
false.

So this is a good solution to the normal dance of turning power on
just to turn it off shortly afterwards.

> +		af8133j_power_down(data);
> +	pm_runtime_enable(dev);
Why?

> +}
> +
> +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 < ARRAY_SIZE(af8133j_supply_names); i++)
> +		data->supplies[i].supply = af8133j_supply_names[i];
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->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;
> +
> +	pm_runtime_set_active(dev);
> +
> +	ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);

As mentioned above, this should only undo things done before this point.
So just the af8133j_power_down() I think.

> +	if (ret)
> +		return ret;
> +
> +	ret = af8133j_product_check(data);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &af8133j_info;
> +	indio_dev->name = "af8133j";
> +	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_get_noresume(dev);

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	ret = devm_pm_runtime_enable(dev);

This already deals with pm_runtime_disable() so you shouldn't need do it manually.
Also you want to enable that before the devm_iio_device_register() to avoid
problems with it not being available as the userspace interfaces are used.

So just move this up a few lines.


> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +}

Powered by blists - more mailing lists