Re: [PATCH v3 3/5] iio: magnetometer: add driver for QST QMC5883P

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sun, Apr 19, 2026 at 10:32:43PM +0000, Hardik Phalet wrote:
> Add an IIO driver for the QST QMC5883P, a 3-axis anisotropic
> magneto-resistive (AMR) magnetometer with a 16-bit ADC, communicating
> over I2C. There is no existing upstream driver for this device.
> 
> The driver supports:
>  - Raw magnetic field readings on X, Y and Z axes
>  - Four full-scale ranges (+/-2 G, +/-8 G, +/-12 G, +/-30 G) selectable
>    via IIO_CHAN_INFO_SCALE
>  - Output data rate configurable via IIO_CHAN_INFO_SAMP_FREQ (10, 50,
>    100, 200 Hz)
>  - vdd-supply regulator management
> 
> Regmap with an rbtree cache is used throughout. CTRL_1 and CTRL_2 bit
> fields are accessed via regmap_field to avoid read-modify-write races.
> The STATUS register is marked precious so regmap never reads it
> speculatively and clears the DRDY/OVFL bits unexpectedly.
> 
> The probe-time init sequence is: soft reset, wait 300 us for POR
> completion, deassert reset, then drop the register cache so subsequent
> RMW writes read fresh values from the device. After reset the chip is in
> MODE_SUSPEND per datasheet §6.2.4, and is left there; the first
> userspace access will wake it via runtime PM (added in a follow-up
> patch).
> 
> Cleanup is fully devm-managed via devm_regulator_get_enable() and
> devm_iio_device_register().
> 
> Oversampling ratio and runtime PM are added in follow-up patches.

...

> +/*
> + * qmc5883p.c - QMC5883P magnetometer driver

Do not add filename inside file. It makes often become an outdated (missed to
change) if the driver is renamed.

> + * Copyright 2026 Hardik Phalet <hardik.phalet@pm.me>
> + *
> + * TODO: add triggered buffer support, PM, OSR, DSR

> + *

Drop this blank line.

> + */

...

> +#include <linux/array_size.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/unaligned.h>

Follow IWYU. Missing cleanup.h, types.h, maybe more, I haven't carefully checked.

...

> +#define QMC5883P_REG_CHIP_ID 0x00

It's better to read when it's a tab before value.

...

> +#define QMC5883P_DRDY_POLL_US 1000

(1 * USEC_PER_MSEC)

from time.h which gives better readability of what the units of the value.
Since it's used only once in the code, drop it, see below.

...

> +enum qmc5883p_channels {
> +	AXIS_X = 0,

Why? Is it Linux enum or HW-driven one? If the latter, you have to assign all
of them explicitly, otherwise it doesn't matter, no assignment is needed.

> +	AXIS_Y,
> +	AXIS_Z,
> +};

...

> +static const struct reg_field qmc5883p_rf_osr =
> +	REG_FIELD(QMC5883P_REG_CTRL_1, 4, 5);
> +static const struct reg_field qmc5883p_rf_odr =
> +	REG_FIELD(QMC5883P_REG_CTRL_1, 2, 3);
> +static const struct reg_field qmc5883p_rf_mode =
> +	REG_FIELD(QMC5883P_REG_CTRL_1, 0, 1);
> +static const struct reg_field qmc5883p_rf_rng =
> +	REG_FIELD(QMC5883P_REG_CTRL_2, 2, 3);
> +static const struct reg_field qmc5883p_rf_sftrst =
> +	REG_FIELD(QMC5883P_REG_CTRL_2, 7, 7);
> +static const struct reg_field qmc5883p_rf_chip_id =
> +	REG_FIELD(QMC5883P_REG_CHIP_ID, 0, 7);

I would make these to be one-liners.

...

> +static int qmc5883p_get_measure(struct qmc5883p_data *data, s16 *x, s16 *y,
> +				s16 *z)

Do a logical split.

static int qmc5883p_get_measure(struct qmc5883p_data *data,
				s16 *x, s16 *y, s16 *z)

> +{
> +	int ret;
> +	u8 reg_data[6];
> +	unsigned int status;
> +
> +	/*
> +	 * Poll the status register until DRDY is set or timeout.
> +	 * Read the whole register in one shot so that OVFL is captured from
> +	 * the same read: reading 0x09 clears both DRDY and OVFL, so a second
> +	 * read would always see OVFL=0.
> +	 * At ODR=10Hz one period is 100ms; use 150ms as a safe upper bound.
> +	 */
> +	ret = regmap_read_poll_timeout(data->regmap, QMC5883P_REG_STATUS,
> +				       status, status & QMC5883P_STATUS_DRDY,
> +				       QMC5883P_DRDY_POLL_US,
> +				       150 * (MICRO / MILLI));

MICRO/MILLI is used for µV to mV conversion (as we don't have it defined), but
here we have the constants in time.h (somewhere down).

				       1 * USEC_PER_MSEC, 150 * USEC_PER_MSEC);

> +	if (ret)
> +		return ret;
> +
> +	if (status & QMC5883P_STATUS_OVFL) {
> +		dev_warn_ratelimited(data->dev,
> +			"data overflow, consider reducing field range\n");
> +		ret = -ERANGE;

EOVERFLOW ? (ERANGE is fine, but the proposed might be slightly better...
or worse :-)

> +		return ret;
> +	}
> +
> +	ret = regmap_bulk_read(data->regmap, QMC5883P_REG_X_LSB, reg_data,
> +			       ARRAY_SIZE(reg_data));

They are u8, sizeof() will suffice. Note, ARRAY_SIZE() may lead to interesting
bugs in some cases.

> +	if (ret)
> +		return ret;
> +
> +	*x = (s16)get_unaligned_le16(&reg_data[0]);
> +	*y = (s16)get_unaligned_le16(&reg_data[2]);
> +	*z = (s16)get_unaligned_le16(&reg_data[4]);

Why not defining the reg_data properly from the start?

	__le16 reg_data[3];

?

> +	return ret;
> +}

...

> +static int qmc5883p_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{

> +	s16 x, y, z;

Don't we have some struct for this? In any case you can define...

struct ...3d {
	s16 x;
	s16 y;
	s16 z;
};

> +	struct qmc5883p_data *data = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int regval;
> +
> +	guard(mutex)(&data->mutex);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = qmc5883p_get_measure(data, &x, &y, &z);

...and use it here.

> +		if (ret < 0)
> +			return ret;
> +		switch (chan->address) {
> +		case AXIS_X:
> +			*val = x;
> +			break;
> +		case AXIS_Y:
> +			*val = y;
> +			break;
> +		case AXIS_Z:
> +			*val = z;
> +			break;
> +		}
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_field_read(data->rf.rng, &regval);
> +		if (ret < 0)

All these ' < 0' parts, are they needed / required?

> +			return ret;
> +		*val = qmc5883p_scale[regval][0];
> +		*val2 = qmc5883p_scale[regval][1];
> +		return IIO_VAL_INT_PLUS_NANO;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = regmap_field_read(data->rf.odr, &regval);
> +		if (ret < 0)
> +			return ret;
> +		*val = qmc5883p_odr[regval];
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +static int qmc5883p_write_scale(struct qmc5883p_data *data, int val, int val2)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(qmc5883p_scale); i++) {

	for (unsigned int i = 0; i < ARRAY_SIZE(qmc5883p_scale); i++) {


> +		if (qmc5883p_scale[i][0] == val && qmc5883p_scale[i][1] == val2)
> +			return regmap_field_write(data->rf.rng, i);
> +	}
> +
> +	return -EINVAL;
> +}

...

> +static int qmc5883p_write_odr(struct qmc5883p_data *data, int val)

As per above.

...

> +static int qmc5883p_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct qmc5883p_data *data = iio_priv(indio_dev);
> +	int ret, restore;
> +
> +	guard(mutex)(&data->mutex);
> +
> +	ret = regmap_field_write(data->rf.mode, QMC5883P_MODE_SUSPEND);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = qmc5883p_write_odr(data, val);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = qmc5883p_write_scale(data, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	restore = regmap_field_write(data->rf.mode, QMC5883P_MODE_NORMAL);

> +	if (restore && !ret)
> +		ret = restore;
> +
> +	return ret;

Why not simply do this

	if (ret)
		return ret;

	return restore;

?

> +}

...

> +static int qmc5883p_rf_init(struct qmc5883p_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = data->dev;

Don't keep duplicates, either regmap from dev or dev from regmap may be
retrieved using the respective APIs.

> +	struct qmc5883p_rf *rf = &data->rf;
> +
> +	rf->osr = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_osr);
> +	if (IS_ERR(rf->osr))
> +		return PTR_ERR(rf->osr);
> +
> +	rf->odr = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_odr);
> +	if (IS_ERR(rf->odr))
> +		return PTR_ERR(rf->odr);
> +
> +	rf->mode = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_mode);
> +	if (IS_ERR(rf->mode))
> +		return PTR_ERR(rf->mode);
> +
> +	rf->rng = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_rng);
> +	if (IS_ERR(rf->rng))
> +		return PTR_ERR(rf->rng);
> +
> +	rf->sftrst = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_sftrst);
> +	if (IS_ERR(rf->sftrst))
> +		return PTR_ERR(rf->sftrst);
> +
> +	rf->chip_id = devm_regmap_field_alloc(dev, regmap, qmc5883p_rf_chip_id);
> +	if (IS_ERR(rf->chip_id))
> +		return PTR_ERR(rf->chip_id);
> +
> +	return 0;
> +}

...

> +static int qmc5883p_read_chip_id(struct qmc5883p_data *data)
> +{
> +	int ret, regval;

Why regval is signed? regmap API uses unsigned int all over the places...

> +	ret = regmap_field_read(data->rf.chip_id, &regval);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "failed to read chip ID\n");

Single line.

> +
> +	if (regval != QMC5883P_CHIP_ID)
> +		dev_info(data->dev, "unexpected chip ID %#x, expected %#x\n",
> +			regval, QMC5883P_CHIP_ID);
> +
> +	return 0;
> +}

...

> +#define QMC5883P_CHAN(ch)                                                 \
> +	{                                                                 \
> +		.type = IIO_MAGN,                                         \
> +		.channel2 = IIO_MOD_##ch,                                 \
> +		.modified = 1,                                            \
> +		.address = AXIS_##ch,                                     \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |            \
> +				      BIT(IIO_CHAN_INFO_SCALE),           \

The style of this is different to the below, I would follow below one.

		.info_mask_separate =						\
			BIT(IIO_CHAN_INFO_RAW) |            \
			BIT(IIO_CHAN_INFO_SCALE),           \

> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +		.info_mask_shared_by_type_available =                     \
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ),                     \
> +	}

The  \ are indented with spaces! Please, use tabs.

...

> +static int qmc5883p_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct qmc5883p_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &qmc5883p_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->dev = dev;
> +	data->regmap = regmap;

> +	mutex_init(&data->mutex);

devm_mutex_init()

> +	ret = qmc5883p_rf_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to initialize regmap fields\n");
> +
> +	indio_dev->name = "qmc5883p";
> +	indio_dev->info = &qmc5883p_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = qmc5883p_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(qmc5883p_channels);
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to initialize vdd regulator\n");
> +
> +	/* Datasheet specifies up to 50 ms supply ramp + 250 us POR time. */
> +	fsleep(50 * (MICRO / MILLI) + 250);

USEC_PER_MSEC

> +
> +	ret = qmc5883p_read_chip_id(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = qmc5883p_chip_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize chip\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static const struct i2c_device_id qmc5883p_id[] = {
> +	{ "qmc5883p", 0 },

Drop ', 0' part.

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko