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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Tue, 14 Jul 2026 22:28:42 +0200
Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net> wrote:

> The QST QMC6308 is a 3-axis AMR magnetometer on I2C, a single-supply
> 4-pin WLCSP part with no interrupt/DRDY pin, found e.g. in the
> Fairphone 6. Its register map differs from the QMC5883L (chip ID at
> 0x00 instead of 0x0D, data at 0x01..0x06, and the range field living
> in control register 2), so add a separate driver rather than extending
> the QMC5883L driver.

Trim this description down for brevity. Drop the package, and I don't
think we need to mention anything it doesn't have.

> 
> Support raw X/Y/Z reads, output data rates 10/50/100/200 Hz, field
> ranges +-30/12/8/2 Gauss, filter oversampling ratios (OSR1) 8/4/2/1,
> the mount matrix, and runtime PM. The second-stage decimation filter
> (OSR2) is left at its power-on default. The package has no DRDY pin,
> so there is no trigger support.

As above I'd not mention what we aren't doing. Also not sure there
is particular value in listing the values each parameter can take.

> 
> Run measurements in the chip's periodic "normal" mode paced by the
> DRDY flag rather than in its one-shot "single" mode: the datasheet
> specifies no conversion time that could bound a one-shot wait, while
> normal mode is paced by the specified output data rates, which also
> keeps the sampling_frequency ABI meaningful.

This is a little unusual.  Unless single mode is really bad we'd
normally just use that for sysfs reads.  Still I don't really mind if
continuous makes more sense here.


> 
> Runtime PM puts the chip into its suspend mode after 500 ms without a
> reading, dropping supply current from tens-to-hundreds of microamps to
> 2-3 uA (datasheet Table 2). The suspended chip retains its registers
> and keeps responding on I2C, so resuming only rewrites the mode field
> and discards one stale sample, and configuration changes can be
> applied even while suspended. VDD is left enabled across runtime
> suspend: the on-chip suspend draw is already negligible, and register
> retention is what keeps the resume path trivial.

This is a bit verbose but I suppose some useful info for reviewers in
there.  Maybe take another look at cutting it down.

I'm going to assume fable was busy here. Try asking it to be brief
in the patch intro, or just edit it after!

> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
https://sashiko.dev/#/patchset/20260714202842.340293-1-jorijnvdgraaf%40catcrafts.net

Has a few comments. I may or may not remember to call them out in
this review as well!

Nice driver in general

Just a few minor things from me.

Jonathan

> diff --git a/drivers/iio/magnetometer/qmc6308.c b/drivers/iio/magnetometer/qmc6308.c
> new file mode 100644
> index 000000000000..ceb4b98402bb
> --- /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);

Whilst not a permanent issue, why do we care about hiding the error message? I'd
just print it whatever.

> +		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;
whilst this is technically not a bug, cleanup.h has some guidance that
was written with maintainability in mind (and a few pointed comments from
Linus Torvalds - no one likes making him grumpy ;)

Upshot (and sashiko has this right) is don't mix goto and anything from
that header. 

Here, take a look at:
pm_runtime.h and in particular
PM_RUNTIME_ACQUIRE_AUTOSUSPEND()
Don't worry about keeping it powered up for a tiny bit long than strictly
necessary.

Added bonus is that then you can use guard() rather than scoped guard
for the mutex and return directly on errors.

> +
> +		ret = regmap_bulk_read(map, QMC6308_REG_X_LSB, buf,
> +				       sizeof(buf));
> +		if (ret)
> +			goto out_rpm_put;
> +
> +		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;
> +
> +	ret = regmap_read(map, QMC6308_REG_ID, &reg);
> +	if (ret)
> +		return ret;
> +
> +	/* Allow unknown IDs so that fallback compatibles work */
> +	if (reg != QMC6308_CHIP_ID)
> +		dev_warn(regmap_get_device(map),
> +			 "Unknown chip id: 0x%02x, continuing\n", reg);
> +
> +	/* The SOFT_RST bit is not auto-cleared and must be written back 0 */
> +	ret = regmap_write(map, QMC6308_REG_CTRL2, QMC6308_SOFT_RST);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(QMC6308_POR_US);
> +
> +	data->range = QMC6308_RNG_30G;
> +
> +	ret = regmap_write(map, QMC6308_REG_CTRL2,
> +			   FIELD_PREP(QMC6308_SET_RESET_MASK,
> +				      QMC6308_SET_RESET_ON) |
> +			   FIELD_PREP(QMC6308_RNG_MASK, data->range));
> +	if (ret)
> +		return 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));

I think you mentioned in your discussion with Siratul that you'll explicitly
set all the fields.  Sashiko noted that you say you are leaving OSR2 alone
(in the patch description) but then set it to 0.

> +}



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

...

> +	ret = qmc6308_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "qmc6308 init failed\n");
> +
> +	pm_runtime_set_active(dev);
> +
> +	ret = devm_add_action_or_reset(dev, qmc6308_power_down_action, data);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_get_noresume(dev);

We've had a few cases of this recently and there are plenty in tree.
However, it's unnecessary - follow through what happens in dd.c
after probe() is called.  Ultimately it checks the counter and if 0
I believe it will power it down without needing this increment / decrement.

Dropping this tends to get sashiko confused, but will resolve the compaint
it has right now about raised reference count on exit.

> +	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;
> +
> +	pm_runtime_put_autosuspend(dev);

As above, this should be unnecessary.

> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int qmc6308_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct qmc6308_data *data = iio_priv(indio_dev);
> +
> +	return qmc6308_set_mode(data, QMC6308_MODE_SUSPEND);
> +}
> +
> +static int qmc6308_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct qmc6308_data *data = iio_priv(indio_dev);
> +	unsigned int status;
> +	int ret;
> +
> +	ret = qmc6308_set_mode(data, QMC6308_MODE_NORMAL);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * DRDY may still be set for a sample converted before the last
> +	 * suspend; clear it so the next read waits for fresh data.
> +	 */
> +	return regmap_read(data->regmap, QMC6308_REG_STATUS, &status);

This feels like a solution in the wrong place.  Why not move it
to the read_raw() path after the runtime resume call?
Sure it will add a delay on repeated reads, but they are coming from
sysfs so we don't care that much.

It is fine to do it here because nothing else resumes but none
the less it does feel rather unintuitive.  If you really want it
here maybe add a note in the read_raw() code path that says it will
have been cleared if resume occurred.

Also if this fails, you should put device back into suspend mode
so as to end up in a consistent state.

> +}



> +static struct i2c_driver qmc6308_driver = {
> +	.driver = {
> +		.name = "qmc6308",
> +		.of_match_table = qmc6308_match,
> +		.pm = pm_ptr(&qmc6308_pm_ops),
> +	},
> +	.id_table = qmc6308_id,
> +	.probe = qmc6308_probe,
> +};
> +module_i2c_driver(qmc6308_driver);
> +
> +MODULE_DESCRIPTION("QST QMC6308 3-Axis Magnetic Sensor driver");
> +MODULE_AUTHOR("Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>");
> +MODULE_LICENSE("Dual BSD/GPL");

Why BSD?  Coming from somewhere else or you have particular reason to
prefer that.  It is relatively unusual for kernel drivers.

Jonathan