Re: [PATCH 2/2] iio: magnetometer: add support for QST QMC6308

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with scope-based resource management (scoped_guard).
- [Medium] The driver unconditionally clears the OSR2 bits during initialization, which contradicts the commit message claim.
- [Medium] PM runtime usage counter leak on probe error path if devm_pm_runtime_enable() fails.
--

--- Patch [2]: [PATCH 2/2] iio: magnetometer: add support for QST QMC6308 ---
commit 853c93f15847bd14844e93c8fdb6cfc5b96c3f4e
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

iio: magnetometer: add support for QST QMC6308

Adds an I2C driver for the QST QMC6308 3-axis magnetometer. The driver
supports raw reads, multiple ranges, output data rates, filter oversampling
ratios, and runtime PM, taking measurements in normal mode paced by DRDY.

> diff --git a/drivers/iio/magnetometer/qmc6308.c b/drivers/iio/magnetometer/qmc6308.c
> new file mode 100644
> index 0000000000000..ceb4b98402bb0
> --- /dev/null
> +++ b/drivers/iio/magnetometer/qmc6308.c
[ ... ]
> +static int qmc6308_take_measurement(struct iio_dev *indio_dev, int index,
> +				    int *val)
> +{
> +	struct qmc6308_data *data = iio_priv(indio_dev);
> +	struct regmap *map = data->regmap;
> +	struct device *dev = regmap_get_device(map);
> +	unsigned int status;
> +	__le16 buf[3];
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		/* EACCES means a read raced runtime PM disable on suspend */
> +		if (ret != -EACCES)
> +			dev_err(dev, "Failed to power on (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	scoped_guard(mutex, &data->mutex) {
> +		/* 50ms headroom over the slowest ODR (10Hz) */
> +		ret = regmap_read_poll_timeout(map, QMC6308_REG_STATUS,
> +					       status,
> +					       (status & QMC6308_STATUS_DRDY),
> +					       2 * USEC_PER_MSEC,
> +					       150 * USEC_PER_MSEC);
> +		if (ret)
> +			goto out_rpm_put;
> +
> +		ret = regmap_bulk_read(map, QMC6308_REG_X_LSB, buf,
> +				       sizeof(buf));
> +		if (ret)
> +			goto out_rpm_put;

[Severity: Medium]
Is it safe to mix goto-based error handling with scoped_guard() in the same
function? The cleanup subsystem documentation indicates that functions should
either use scope-based cleanup for all resources or convert none of them, to
avoid confusing ownership semantics.

> +
> +		if (status & QMC6308_STATUS_OVFL)
> +			ret = -ERANGE;
> +	}
> +
> +out_rpm_put:
> +	pm_runtime_put_autosuspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	*val = (s16)le16_to_cpu(buf[index]);
> +
> +	return 0;
> +}
[ ... ]
> +static int qmc6308_init(struct qmc6308_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	unsigned int reg;
> +	int ret;
[ ... ]
> +	data->odr = QMC6308_ODR_50HZ;
> +	data->osr = QMC6308_OSR1_8;
> +
> +	return regmap_write(map, QMC6308_REG_CTRL1,
> +			    FIELD_PREP(QMC6308_MODE_MASK,
> +				       QMC6308_MODE_NORMAL) |
> +			    FIELD_PREP(QMC6308_ODR_MASK, data->odr) |
> +			    FIELD_PREP(QMC6308_OSR1_MASK, data->osr));

[Severity: Medium]
Does this regmap_write() clear the OSR2 bits, contradicting the commit
message which states OSR2 is left at its power-on default?

Since regmap_write() overwrites the entire 8-bit register, the OSR2 field
(bits 7:6) will be implicitly zeroed here. Would using regmap_update_bits()
prevent overwriting the power-on default?

> +}
[ ... ]
> +static int qmc6308_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct qmc6308_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *map;
> +	int ret;
[ ... ]
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, QMC6308_AUTOSUSPEND_DELAY_MS);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does returning directly here leak the PM runtime usage counter if
devm_pm_runtime_enable() fails?

Since pm_runtime_get_noresume() increments the counter just above, it appears
we might need to balance it with pm_runtime_put_noidle() on this error path
to prevent the parent or I2C device from being kept artificially active.

> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714202842.340293-1-jorijnvdgraaf@catcrafts.net?part=2