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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 12 Jun 2026 18:45:27 +0600
Siratul Islam <email@sirat.me> wrote:

> Add driver for the QST QMC5883L 3-Axis Magnetic Sensor
> connected via i2c.
> 
> Signed-off-by: Siratul Islam <email@sirat.me>

Trying to avoid repeating stuff Joshua already covered.
Various comments inline.

Thanks,

Jonathan

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


> +
> +#define QMC5883L_REG_X_LSB	0x00
> +#define QMC5883L_REG_STATUS1	0x06
> +#define QMC5883L_REG_CTRL1	0x09
> +#define QMC5883L_REG_CTRL2	0x0A
> +#define QMC5883L_REG_SET_RESET	0x0B
> +#define QMC5883L_REG_ID		0x0D
> +
> +#define QMC5883L_CHIP_ID	0xFF
> +
> +#define QMC5883L_MODE_MASK	GENMASK(1, 0)
> +#define QMC5883L_ODR_MASK	GENMASK(3, 2)
> +#define QMC5883L_RNG_MASK	GENMASK(5, 4)
> +#define QMC5883L_OSR_MASK	GENMASK(7, 6)
> +
> +#define QMC5883L_MODE_STANDBY	FIELD_PREP_CONST(QMC5883L_MODE_MASK, 0x00)
> +#define QMC5883L_MODE_CONT	FIELD_PREP_CONST(QMC5883L_MODE_MASK, 0x01)
> +
> +#define QMC5883L_ODR_10HZ	FIELD_PREP_CONST(QMC5883L_ODR_MASK, 0x00)
> +#define QMC5883L_ODR_50HZ	FIELD_PREP_CONST(QMC5883L_ODR_MASK, 0x01)
> +#define QMC5883L_ODR_100HZ	FIELD_PREP_CONST(QMC5883L_ODR_MASK, 0x02)
> +#define QMC5883L_ODR_200HZ	FIELD_PREP_CONST(QMC5883L_ODR_MASK, 0x03)
> +
> +#define QMC5883L_RNG_2G		FIELD_PREP_CONST(QMC5883L_RNG_MASK, 0x00)
> +#define QMC5883L_RNG_8G		FIELD_PREP_CONST(QMC5883L_RNG_MASK, 0x01)
> +
> +#define QMC5883L_OSR_512	FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x00)
> +#define QMC5883L_OSR_256	FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x01)
> +#define QMC5883L_OSR_128	FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x02)
> +#define QMC5883L_OSR_64		FIELD_PREP_CONST(QMC5883L_OSR_MASK, 0x03)
These are used for matching - normally we'd add defines for the filed value and
then use FIELD_GET() to extract it for matching.
e.g.
#define QMC5883L_OSR_512		0x0
#define QMC5883L_OSR_256		0x1
rather these.

> +
> +#define QMC5883L_STATUS_DRDY	BIT(0)
> +#define QMC5883L_STATUS_OVL	BIT(1)
> +
> +#define QMC5883L_SET_RESET_VAL	BIT(0)
> +#define QMC5883L_INT_DISABLE	BIT(0)
> +#define QMC5883L_SOFT_RESET	BIT(7)
> +
> +#define QMC5883L_SCALE_2G	83333
> +#define QMC5883L_SCALE_8G	333333
> +
> +/* POR completion time max per datasheet */
> +#define QMC5883L_PORT_US	350
> +
> +struct qmc5883l_data {
> +	struct regmap *regmap;
> +	struct mutex mutex; /* update and read regmap data */

Need more than that.  regmap has its own locks that do this bit.
Be sure to describe exactly what data you are protecting.
Usually it is something like read / modify / write cycles or
need to serialize groupd of actions.

> +	u8 range;
> +	u8 odr;
> +	u8 osr;
> +};
> +
> +enum qmc5883l_chan {
> +	QMC5883L_AXIS_X,
> +	QMC5883L_AXIS_Y,
> +	QMC5883L_AXIS_Z,
> +};
> +
> +static const int qmc5883l_odr_avail[] = { 10, 50, 100, 200 };
> +
> +static const int qmc5883l_osr_avail[] = { 512, 256, 128, 64 };
> +
> +static const int qmc5883l_rng_avail[] = {
> +	0, QMC5883L_SCALE_2G,	/* 2G */

I'm not sure the defines really help. Perhaps push the value down here
and then look it up from this array when matching.

> +	0, QMC5883L_SCALE_8G,	/* 8G */
> +};

> +
> +static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))

What is this serializing?  In a driver that only supports direct mode it should
never be necessary to claim it.    So this code should only be added in a patch
adding buffered modes.  If you need to serialize, most likely you should be using
a local lock as it has nothing to do with state changes from direct to buffered.

> +			return -EBUSY;
> +		ret = qmc5883l_take_measurement(indio_dev, chan->address, val);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		scoped_guard(mutex, &data->mutex)

Joshua already mentioned this, but prefer guard() and defined scope
for each of these case blocks. That just ends up easier to read.

> +		{
> +			*val = 0;
> +			*val2 = data->range == QMC5883L_RNG_2G ?
> +					QMC5883L_SCALE_2G :
> +					QMC5883L_SCALE_8G;
> +		}
> +		return IIO_VAL_INT_PLUS_NANO;
...

> +
> +static int qmc5883l_write_raw(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +	u8 rng;
> +	u8 osr;
> +	u8 odr;
Can combine as
	u8 rng, osr, odr;
without loosing readability so do that to save a few lines of scrolling!

> +	int ret;

> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
Add scope by doing
case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {

Same applies for other cases.


> +		switch (val) {
> +		case 64:
> +			osr = QMC5883L_OSR_64;
> +			break;
> +		case 128:
> +			osr = QMC5883L_OSR_128;
> +			break;
> +		case 256:
> +			osr = QMC5883L_OSR_256;
> +			break;
> +		case 512:
> +			osr = QMC5883L_OSR_512;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		scoped_guard(mutex, &data->mutex)
then this can be a guard.

Note that if it were a scoped_guard() that should be treated like an if ()
so formatting wise it would be
		scoped_guard(mutex, &data->mutex) {

However with scope added as suggested this can be
		guard(mutex)(&data->mutex);
and avoid the need for greater indent.

> +		{
> +			ret = regmap_update_bits(data->regmap,
> +						 QMC5883L_REG_CTRL1,
> +						 QMC5883L_OSR_MASK, osr);
See above,
						 QMC5883L_OSR_MASK,
						 FIELD_PREP(QMC5883L_OSR_MASK, osr));

> +			if (ret)
> +				return ret;
> +			data->osr = osr;
Here store the field value not the shifted version.

> +		}
> +		break;
		return 0; (see below)

Close scope with 
	}
here.
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Probably better to return in the good paths above and save us having
to go see if there is anything else to be done.


> +}


> +static int qmc5883l_init(struct qmc5883l_data *data)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, QMC5883L_REG_ID, &reg);
> +	if (ret)
> +		return ret;
> +
> +	/* Not failing because rev 1.0 had this register reserved */
> +	if (reg != QMC5883L_CHIP_ID)
> +		dev_warn(regmap_get_device(data->regmap),
> +			 "unknown chip id: 0x%02x, continuing\n", reg);
> +
> +	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL2,
> +			   QMC5883L_SOFT_RESET);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(QMC5883L_PORT_US);
> +
> +	/* DRDY pin no used in this version of the driver */
> +	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL2,
> +			   QMC5883L_INT_DISABLE);
I don't mind if these sorts of cases go a little over 80 chars as sometimes
it helps readability.

Does it really reset with interrupts on?  That's odd.  Mind you the
INT_ENB sounds like it would be an enable but as you have named it here
it is actually a disable so all bets are off when it comes to sensible ;)


> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, QMC5883L_REG_SET_RESET,
> +			   QMC5883L_SET_RESET_VAL);
> +	if (ret)
> +		return ret;
> +
> +	data->odr = QMC5883L_ODR_50HZ;
> +	data->range = QMC5883L_RNG_2G;
> +	data->osr = QMC5883L_OSR_64;

Generally should only set these after the write succeeds otherwise on error
these are out of sync with the device. Not that important though in an init
function as any error leads to us bailing out anyway.

> +
> +	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL1,
> +			   (QMC5883L_MODE_CONT | data->odr | data->range |
> +			    data->osr));

return regmap_write();

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


> +static const struct of_device_id qmc5883l_match[] = {
> +	{ .compatible = "qstcorp,qmc5883l" },
> +	{ },

As below, no comma.

> +};
> +MODULE_DEVICE_TABLE(of, qmc5883l_match);
> +
> +static const struct i2c_device_id qmc5883l_id[] = {
> +	{ "qmc5883l" },

Named initializers for these please. Uwe is doing some work
to ensure consistency on this across there kernel and queued
up for this kernel cycle is a patch that does that for all the
i2c_device_id tables. Uwe mentioned that a checkpatch change
to check for this was on the todo list.

> +	{ },

No trailing comma on 'terminating' entries like this. 
Whatever entries are added in the future they must not come
after this so comma here is never appropriate.

> +};
> +MODULE_DEVICE_TABLE(i2c, qmc5883l_id);
> +
> +static struct i2c_driver qmc5883l_driver = {
> +	.driver = {
> +		.name = "qmc5883l",
> +		.of_match_table = qmc5883l_match,
> +	},
> +	.id_table = qmc5883l_id,
> +	.probe = qmc5883l_probe,
> +};
> +

Trivial but a common convention (that I'm trying to encourage in IIO)
is no blank line here. It is good to keep the tighter coupling between
the structure and the macro that is its only user.

> +module_i2c_driver(qmc5883l_driver);