Re: [PATCH 3/3] iio: magnetometer: add driver for QST QMC5883L Sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning early with `-ERANGE` when the overflow flag (OVL) is set prevents the data registers from being read, leaving the sensor permanently stuck in an overflow state.
- [Medium] The device is left in continuous measurement mode on driver removal or initialization failure, causing an unnecessary power drain.
--

commit 8aa498258e2b8bddd48bf3c46ef8555a7376ea6e
Author: Siratul Islam <email@sirat.me>

iio: magnetometer: add driver for QST QMC5883L Sensor

This commit adds a new I2C driver for the QST QMC5883L 3-Axis Magnetic Sensor,
including continuous measurement mode support and scaling options.

> diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> new file mode 100644
> index 0000000000000..055e51570635c
> --- /dev/null
> +++ b/drivers/iio/magnetometer/qmc5883l.c

[ ... ]

> +static int qmc5883l_take_measurement(struct iio_dev *indio_dev, int index,
> +				     int *val)
> +{
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +	unsigned int status;
> +	__le16 buf[3];
> +	int ret;
> +
> +	scoped_guard(mutex, &data->mutex)
> +	{
> +		/* 50ms headroom over the slowest ODR (10Hz) */
> +		ret = regmap_read_poll_timeout(data->regmap,
> +					       QMC5883L_REG_STATUS1, status,
> +					       (status & QMC5883L_STATUS_DRDY),
> +					       2 * USEC_PER_MSEC,
> +					       150 * USEC_PER_MSEC);
> +		if (ret)
> +			return ret;
> +
> +		if (status & QMC5883L_STATUS_OVL)
> +			return -ERANGE;

[Severity: High]
If we return -ERANGE here when the overflow flag (OVL) is set, does the
sensor get permanently stuck in an overflow state?

In typical I2C magnetometers, the Data Ready (DRDY) and Overflow (OVL)
status bits are only cleared by reading the data registers. By returning
early without reading the data registers via regmap_bulk_read(), the DRDY
and OVL flags might remain set indefinitely. 

On subsequent measurement attempts, regmap_read_poll_timeout() will return
immediately and this check will instantly fail again, potentially locking up
the sensor until a reset.

> +
> +		ret = regmap_bulk_read(data->regmap, QMC5883L_REG_X_LSB, buf,
> +				       sizeof(buf));

[ ... ]

> +static int qmc5883l_init(struct qmc5883l_data *data)
> +{
> +	unsigned int reg;
> +	int ret;
> +

[ ... ]

> +	data->odr = QMC5883L_ODR_50HZ;
> +	data->range = QMC5883L_RNG_2G;
> +	data->osr = QMC5883L_OSR_64;
> +
> +	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL1,
> +			   (QMC5883L_MODE_CONT | data->odr | data->range |
> +			    data->osr));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[ ... ]

> +static int qmc5883l_probe(struct i2c_client *client)
> +{

[ ... ]

> +	indio_dev->name = "qmc5883l";
> +	indio_dev->info = &qmc5883l_info;
> +	indio_dev->channels = qmc5883l_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(qmc5883l_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = qmc5883l_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "qmc5883l init failed\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: Medium]
Is there a missing devm teardown callback to power down the sensor when the
driver is unbound or if registration fails?

In qmc5883l_init(), the sensor is placed into continuous sampling mode. If
devm_iio_device_register() fails, or when the driver is removed, there
doesn't appear to be a registered cleanup action (e.g., via
devm_add_action_or_reset()) to place the device back into standby mode.

Although the devm core disables the VDD/VDDIO regulators during teardown,
these regulators might be shared with other components on the board. If the
shared supply remains active, the sensor could continue continuous sampling
and unnecessarily drain power.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612124557.13750-1-email@sirat.me?part=3