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

[Siratul Islam <siratul.islam@linux.dev>]

On Tue, 2026-07-14 at 22:28 +0200, Jorijn van der Graaf wrote:
> ...
> 
Hi! the driver looks clean overall. I see you have followed ;
> 
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +/*
> + * Support for QST QMC6308 3-Axis Magnetic Sensor on I2C bus.
> + *
> + * Copyright (C) 2026 Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>
> + *
> + * Datasheet available at
> + * <https://qstcorp.com/upload/pdf/202202/13-52-15%20QMC6308%20Datasheet%20Rev.%20F(1).pdf>
> + */
> +
> 
...
> + #define QMC6308_OSR2_MASK GENMASK(7, 6)
This is never used so it's dead code. Are you supposed to use it somewhere?
> +
> +/*
> + * Power-on completion time (datasheet Table 7), also used as a
> + * conservative bound after soft reset, for which the datasheet
> + * gives no figure.
> + */
> +#define QMC6308_POR_US 250
Describe what this value is here, and explain in call sites why you are 
using it for something else too. i.e.,

/* Power-on completion time (datasheet Table 7) */
#define QMC6308_POR_US 250

And then in call sites,
"reusing por time because..."

> +
> +#define QMC6308_AUTOSUSPEND_DELAY_MS 500
You can call it QMC6308_SLEEP_DELAY_MS to align with other values.
> +
> +struct qmc6308_data {
> + struct regmap *regmap;
> + /*
> + * Protect data->range/odr/osr.
> + * Protect poll and read during measurement (reading the status
> + * register clears DRDY).
> + */
"reading the status register clears DRDY" - This bit of information is not
describing the mutex. Explain it at a call site. i.e, where you read STATUS register
to clear DRDY.
> + struct mutex mutex;
> + struct iio_mount_matrix orientation;
> + u8 range;
> + u8 odr;
> + u8 osr;
> +};
> +
...
> +
> +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;
Use reverse x-mas tree order.
struct qmc6308_data *data = iio_priv(indio_dev);
struct device *dev = regmap_get_device(map);
struct regmap *map = data->regmap;
> +
> 
...
> +static int qmc6308_write_raw(struct iio_dev *indio_dev,
> +      const struct iio_chan_spec *chan,
> +      int val, int val2, long mask)
> +{
> + struct qmc6308_data *data = iio_priv(indio_dev);
> + unsigned int status;
> + unsigned int i;
Why not use a switch here instead of depending on 'i'. In it's current form,
we would need to keep going back to check what the scales, odr, osr arrays look like.
I would suggest following the same pattern as the QMC5883L driver.

The arrays could also be flattened like this if you follow the above suggestion.
+static const int qmc6308_odr_avail[] = { 10, 50, 100, 200 };
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE: {
> + if (val != 0)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(qmc6308_scales); i++) {
> + if (val2 == qmc6308_scales[i][1])
> + break;
> + }
> + if (i == ARRAY_SIZE(qmc6308_scales))
> + return -EINVAL;
This check won't be needed then. 
> ...
> 

> +		ret = regmap_update_bits(data->regmap, QMC6308_REG_CTRL2,
> +					 QMC6308_RNG_MASK,
> +					 FIELD_PREP(QMC6308_RNG_MASK, i));
> 
> +		data->range = i;
Another issue I noticed, what happens if the update succeeds but the "status read to clear" below fails?
You are returning an incorrect error to the userspace even though the scale change took effect. 
I think a better approach here would be a log and continue instead of failing.
> + 
> + 		return regmap_read(data->regmap, QMC6308_REG_STATUS,
> + 				&status);

...
> +}
> +
> +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);
> +
Add a comment why you are using this value here as I suggested above.
(i.e., as a conservative bound)
> + 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;
Might as well assign the default values for data->{range, odr, osr} together?
data->range = QMC6308_RNG_30G;
data->odr = QMC6308_ODR_50HZ;
data->osr = QMC6308_OSR1_8;

I find to easier to glance.
> +
> + 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));
> +}
> +
> 
...
> +
> +MODULE_DESCRIPTION("QST QMC6308 3-Axis Magnetic Sensor driver");
> +MODULE_AUTHOR("Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>");
> +MODULE_LICENSE("Dual BSD/GPL");
--
Best regards,
Sirat