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

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

[Siratul Islam]

This patch series introduces the QST QMC5883L 3-Axis Magnetic Sensor
driver. It is a simple device with minimal magnetometer functionalities.
Commonly used as (software incompatible) replacement for the
Honeywell HMC5883L sensor.

This driver implements the basic functionalities of the QMC5883L sensor,
and intentionally leaves out some features like DRDY interrupt pin support
and power management for simplicity, both of which will be addressed
in future patches.

There was an attempt to introduce this device about an year ago but
the author seems to have abandoned the patch series. Since the device
is simple enough, I decided to start from scratch.

Note: I also noticed a patch for the QMC5883P variant. Despite similar
naming, the sensors are different including different register maps,
so these devices are not compatible with each other.


Siratul Islam (3):
  dt-bindings: add entry for qstcorp
  dt-bindings: iio: magnetometer: add QST QMC5883L Sensor
  iio: magnetometer: add driver for QST QMC5883L Sensor

 .../iio/magnetometer/qstcorp,qmc5883l.yaml    |  48 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/iio/magnetometer/Kconfig              |  11 +
 drivers/iio/magnetometer/Makefile             |   2 +
 drivers/iio/magnetometer/qmc5883l.c           | 512 ++++++++++++++++++
 6 files changed, 582 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
 create mode 100644 drivers/iio/magnetometer/qmc5883l.c

--
2.54.0

[PATCH 1/3] dt-bindings: add entry for qstcorp

[Siratul Islam]

Add an entry for QST Corporation Limited

Signed-off-by: Siratul Islam <email@sirat.me>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 28784d66ae7b..11aac47f90ce 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1355,6 +1355,8 @@ patternProperties:
     description: Shenzhen QiShenglong Industrialist Co., Ltd.
   "^qnap,.*":
     description: QNAP Systems, Inc.
+  "^qstcorp,.*":
+    description: QST Corporation Limited
   "^quanta,.*":
     description: Quanta Computer Inc.
   "^radxa,.*":
-- 
2.54.0

Re: [PATCH 1/3] dt-bindings: add entry for qstcorp

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On Fri, Jun 12, 2026 at 06:45:25PM +0600, Siratul Islam wrote:
> Add an entry for QST Corporation Limited
> 
> Signed-off-by: Siratul Islam <email@sirat.me>

Link: https://www.qstcorp.com/
Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable


Cheers,
Conor.
\x02

> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 28784d66ae7b..11aac47f90ce 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1355,6 +1355,8 @@ patternProperties:
>      description: Shenzhen QiShenglong Industrialist Co., Ltd.
>    "^qnap,.*":
>      description: QNAP Systems, Inc.
> +  "^qstcorp,.*":
> +    description: QST Corporation Limited
>    "^quanta,.*":
>      description: Quanta Computer Inc.
>    "^radxa,.*":
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] dt-bindings: iio: magnetometer: add QST QMC5883L Sensor

[Siratul Islam]

Add devicetree binding for the QST QMC5883L 3-Axis Magnetic Sensor
connected via i2c.
Interrupt not implemented in driver but kept in the binding for future
addition.
Used enum so that more driver could use this binding

Signed-off-by: Siratul Islam <email@sirat.me>
---
 .../iio/magnetometer/qstcorp,qmc5883l.yaml    | 48 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml b/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
new file mode 100644
index 000000000000..238cc7e22b89
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/qstcorp,qmc5883l.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: QST QMC5883L 3-Axis Magnetic Sensor
+
+maintainers:
+  - Siratul Islam <email@sirat.me>
+
+properties:
+  compatible:
+    enum:
+      - qstcorp,qmc5883l
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+  vddio-supply: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@d {
+            compatible = "qstcorp,qmc5883l";
+            reg = <0x0d>;
+            vdd-supply = <&vdd_3v3_reg>;
+            vddio-supply = <&vdd_3v3_reg>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e035a3be797c..310074b34072 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21787,6 +21787,12 @@ F:	Documentation/networking/device_drivers/ethernet/freescale/dpaa2/overview.rst
 F:	drivers/bus/fsl-mc/
 F:	include/uapi/linux/fsl_mc.h
 
+QST QMC5883L 3-Axis Magnetic Sensor
+M:	Siratul Islam <email@sirat.me>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
+
 QT1010 MEDIA DRIVER
 L:	linux-media@vger.kernel.org
 S:	Orphan
-- 
2.54.0

Re: [PATCH 2/3] dt-bindings: iio: magnetometer: add QST QMC5883L Sensor

[Joshua Crofts]

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

> +QST QMC5883L 3-Axis Magnetic Sensor
> +M:	Siratul Islam <email@sirat.me>
> +L:	linux-iio@vger.kernel.org

There's no point in having the IIO list in your MAINTAINERS
entry, get_maintainer.pl would return it automatically based
on the driver file's path.

-- 
Kind regards

CJD

Re: [PATCH 2/3] dt-bindings: iio: magnetometer: add QST QMC5883L Sensor

[Jonathan Cameron]

On Fri, 12 Jun 2026 15:13:24 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Fri, 12 Jun 2026 18:45:26 +0600
> Siratul Islam <email@sirat.me> wrote:
> 
> > +QST QMC5883L 3-Axis Magnetic Sensor
> > +M:	Siratul Islam <email@sirat.me>
> > +L:	linux-iio@vger.kernel.org  
> 
> There's no point in having the IIO list in your MAINTAINERS
> entry, get_maintainer.pl would return it automatically based
> on the driver file's path.
True, but I'm not sure there is a standard convention for whether
lists should be added in this case or not.  

One of those things where we should decide on an answer perhaps
and stick to it.  Andy, Nuno, David, Dt folk what do you think?


> 

Re: [PATCH 2/3] dt-bindings: iio: magnetometer: add QST QMC5883L Sensor

[Jonathan Cameron]

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

> Add devicetree binding for the QST QMC5883L 3-Axis Magnetic Sensor
> connected via i2c.
> Interrupt not implemented in driver but kept in the binding for future
> addition.
No need to mention that
> Used enum so that more driver could use this binding

More devices perhaps rather than drivers (binding has nothing
directly to do with drivers).

> 
> Signed-off-by: Siratul Islam <email@sirat.me>
> ---
>  .../iio/magnetometer/qstcorp,qmc5883l.yaml    | 48 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml b/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
> new file mode 100644
> index 000000000000..238cc7e22b89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/qstcorp,qmc5883l.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: QST QMC5883L 3-Axis Magnetic Sensor
> +
> +maintainers:
> +  - Siratul Islam <email@sirat.me>
It is a bit unusual to have to description section.  Maybe
just have a link to a datasheet if there is someone that seems
reasonable stable to link to.

> +
> +properties:
> +  compatible:

The rest looks good to  me.

Thanks,

Jonathan

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

[Siratul Islam]

Add driver for the QST QMC5883L 3-Axis Magnetic Sensor
connected via i2c.

Signed-off-by: Siratul Islam <email@sirat.me>
---
 MAINTAINERS                         |   1 +
 drivers/iio/magnetometer/Kconfig    |  11 +
 drivers/iio/magnetometer/Makefile   |   2 +
 drivers/iio/magnetometer/qmc5883l.c | 512 ++++++++++++++++++++++++++++
 4 files changed, 526 insertions(+)
 create mode 100644 drivers/iio/magnetometer/qmc5883l.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 310074b34072..f94da70b91af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21792,6 +21792,7 @@ M:	Siratul Islam <email@sirat.me>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/magnetometer/qstcorp,qmc5883l.yaml
+F:	drivers/iio/magnetometer/qmc5883l.c
 
 QT1010 MEDIA DRIVER
 L:	linux-media@vger.kernel.org
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index fb313e591e85..615564174086 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -198,6 +198,17 @@ config INFINEON_TLV493D
 	  To compile this driver as a module, choose M here: the module
 	  will be called tlv493d.
 
+config QMC5883L
+	tristate "QST QMC5883L 3-Axis Magnetic Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to add support driver for QST QMC5883L 3-Axis
+	  Magnetic Sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called qmc5883l.
+
 config SENSORS_HMC5843
 	tristate
 	select IIO_BUFFER
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 5bd227f8c120..552682555d86 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -26,6 +26,8 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
 
 obj-$(CONFIG_INFINEON_TLV493D)		+= tlv493d.o
 
+obj-$(CONFIG_QMC5883L)			+= qmc5883l.o
+
 obj-$(CONFIG_SENSORS_HMC5843)		+= hmc5843_core.o
 obj-$(CONFIG_SENSORS_HMC5843_I2C)	+= hmc5843_i2c.o
 obj-$(CONFIG_SENSORS_HMC5843_SPI)	+= hmc5843_spi.o
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
@@ -0,0 +1,512 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/*
+ * Support for QST QMC5883L 3-Axis Magnetic Sensor on i2c bus.
+ *
+ * Copyright (C) 2026 Siratul Islam <email@sirat.me>
+ *
+ * Datasheet available at
+ * <https://www.qstcorp.com/upload/pdf/202512/13-52-04%20QMC5883L%20Datasheet%20Rev.%20B.pdf>
+ *
+ * Default 7-bit i2c slave address 0x0D.
+ *
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/time.h>
+
+#include <asm/byteorder.h>
+
+#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)
+
+#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 */
+	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 */
+	0, QMC5883L_SCALE_8G,	/* 8G */
+};
+
+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;
+
+		ret = regmap_bulk_read(data->regmap, QMC5883L_REG_X_LSB, buf,
+				       sizeof(buf));
+		if (ret)
+			return ret;
+
+		*val = (s16)le16_to_cpu(buf[index]);
+	}
+
+	return 0;
+}
+
+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))
+			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)
+		{
+			*val = 0;
+			*val2 = data->range == QMC5883L_RNG_2G ?
+					QMC5883L_SCALE_2G :
+					QMC5883L_SCALE_8G;
+		}
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		scoped_guard(mutex, &data->mutex)
+		{
+			switch (data->odr) {
+			case QMC5883L_ODR_200HZ:
+				*val = 200;
+				break;
+			case QMC5883L_ODR_100HZ:
+				*val = 100;
+				break;
+			case QMC5883L_ODR_50HZ:
+				*val = 50;
+				break;
+			case QMC5883L_ODR_10HZ:
+				*val = 10;
+				break;
+			default:
+				return -EINVAL;
+			}
+		}
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		scoped_guard(mutex, &data->mutex)
+		{
+			switch (data->osr) {
+			case QMC5883L_OSR_64:
+				*val = 64;
+				break;
+			case QMC5883L_OSR_128:
+				*val = 128;
+				break;
+			case QMC5883L_OSR_256:
+				*val = 256;
+				break;
+			case QMC5883L_OSR_512:
+				*val = 512;
+				break;
+			default:
+				return -EINVAL;
+			}
+		}
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+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;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		switch (val2) {
+		case QMC5883L_SCALE_2G:
+			rng = QMC5883L_RNG_2G;
+			break;
+		case QMC5883L_SCALE_8G:
+			rng = QMC5883L_RNG_8G;
+			break;
+		default:
+			return -EINVAL;
+		}
+		scoped_guard(mutex, &data->mutex)
+		{
+			ret = regmap_update_bits(data->regmap,
+						 QMC5883L_REG_CTRL1,
+						 QMC5883L_RNG_MASK, rng);
+			if (ret)
+				return ret;
+			data->range = rng;
+		}
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (val) {
+		case 200:
+			odr = QMC5883L_ODR_200HZ;
+			break;
+		case 100:
+			odr = QMC5883L_ODR_100HZ;
+			break;
+		case 50:
+			odr = QMC5883L_ODR_50HZ;
+			break;
+		case 10:
+			odr = QMC5883L_ODR_10HZ;
+			break;
+		default:
+			return -EINVAL;
+		}
+		scoped_guard(mutex, &data->mutex)
+		{
+			ret = regmap_update_bits(data->regmap,
+						 QMC5883L_REG_CTRL1,
+						 QMC5883L_ODR_MASK, odr);
+			if (ret)
+				return ret;
+			data->odr = odr;
+		}
+		break;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		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)
+		{
+			ret = regmap_update_bits(data->regmap,
+						 QMC5883L_REG_CTRL1,
+						 QMC5883L_OSR_MASK, osr);
+			if (ret)
+				return ret;
+			data->osr = osr;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qmc5883l_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = qmc5883l_odr_avail;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(qmc5883l_odr_avail);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = qmc5883l_osr_avail;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(qmc5883l_osr_avail);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = qmc5883l_rng_avail;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*length = ARRAY_SIZE(qmc5883l_rng_avail);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int qmc5883l_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return IIO_VAL_INT;
+	}
+}
+
+static const struct iio_info qmc5883l_info = {
+	.read_raw = qmc5883l_read_raw,
+	.write_raw = qmc5883l_write_raw,
+	.read_avail = qmc5883l_read_avail,
+	.write_raw_get_fmt = qmc5883l_write_raw_get_fmt,
+};
+
+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);
+	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;
+
+	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL1,
+			   (QMC5883L_MODE_CONT | data->odr | data->range |
+			    data->osr));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static bool qmc5883l_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg <= QMC5883L_REG_STATUS1;
+}
+
+static bool qmc5883l_writable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case QMC5883L_REG_CTRL1:
+	case QMC5883L_REG_CTRL2:
+	case QMC5883L_REG_SET_RESET:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config qmc5883l_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = QMC5883L_REG_ID,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = qmc5883l_volatile_reg,
+	.writeable_reg = qmc5883l_writable_reg
+};
+
+#define QMC5883L_CHANNEL(_axis)                                \
+	{                                                      \
+		.type = IIO_MAGN,                              \
+		.modified = 1,                                 \
+		.channel2 = IIO_MOD_##_axis,                   \
+		.address = QMC5883L_AXIS_##_axis,              \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),  \
+		.info_mask_shared_by_type =                    \
+			BIT(IIO_CHAN_INFO_SCALE) |             \
+			BIT(IIO_CHAN_INFO_SAMP_FREQ) |         \
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+		.info_mask_shared_by_type_available =          \
+			BIT(IIO_CHAN_INFO_SCALE) |             \
+			BIT(IIO_CHAN_INFO_SAMP_FREQ) |         \
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+	}
+
+static const struct iio_chan_spec qmc5883l_channels[] = {
+	QMC5883L_CHANNEL(X),
+	QMC5883L_CHANNEL(Y),
+	QMC5883L_CHANNEL(Z),
+};
+
+static int qmc5883l_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	struct qmc5883l_data *data;
+	struct device *dev = &client->dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(client, &qmc5883l_regmap_config);
+	if (IS_ERR(regmap)) {
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap initialization failed\n");
+	}
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable VDD regulator\n");
+
+	ret = devm_regulator_get_enable(dev, "vddio");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable VDDIO regulator\n");
+
+	fsleep(QMC5883L_PORT_US);
+
+	data = iio_priv(indio_dev);
+	data->regmap = regmap;
+	ret = devm_mutex_init(dev, &data->mutex);
+	if (ret)
+		return ret;
+
+	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);
+}
+
+static const struct of_device_id qmc5883l_match[] = {
+	{ .compatible = "qstcorp,qmc5883l" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qmc5883l_match);
+
+static const struct i2c_device_id qmc5883l_id[] = {
+	{ "qmc5883l" },
+	{ },
+};
+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,
+};
+
+module_i2c_driver(qmc5883l_driver);
+
+MODULE_DESCRIPTION("QST QMC5883L 3-Axis Magnetic Sensor driver");
+MODULE_AUTHOR("Siratul Islam <email@sirat.me>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.54.0

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

[sashiko-bot]

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

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

[Joshua Crofts]

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>
> ---

Hi Siratul,

various comments inline. I've probably missed a few things
as I only took a quick look so feel free to call me out on that!

Josh

> --- /dev/null
> +++ b/drivers/iio/magnetometer/qmc5883l.c
> @@ -0,0 +1,512 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +/*
> + * Support for QST QMC5883L 3-Axis Magnetic Sensor on i2c bus.

Nitpick, but I'd write it I2C with capital letters.

> + *
> + * Copyright (C) 2026 Siratul Islam <email@sirat.me>
> + *
> + * Datasheet available at
> + * <https://www.qstcorp.com/upload/pdf/202512/13-52-04%20QMC5883L%20Datasheet%20Rev.%20B.pdf>
> + *
> + * Default 7-bit i2c slave address 0x0D.

You also have this as a macro, consider removing this comment.

> + *
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>

Move IIO specific headers below generic linux headers, i. e.

#include <linux/x.h>
#include <linux/y.h>

#include <linux/iio/z.h>

> +#include <linux/i2c.h>
> +#include <linux/kernel.h>

No, refrain from using kernel.h, include the actual headers
instead of relying on this (on second thought, I think you've
actually included all the required headers, so you definitely
don't need this).

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/time.h>
> +
> +#include <asm/byteorder.h>
> +
> +#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)
> +
> +#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

If it's POR completion, why does the macro contain PORT instead?
> +
> +struct qmc5883l_data {
> +	struct regmap *regmap;
> +	struct mutex mutex; /* update and read regmap data */
> +	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 */

These comments are redundant IMO, you're already mentioning
the value in the macro name.

> +	0, QMC5883L_SCALE_8G,	/* 8G */
> +};
> +
> +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)

Why scoped_guard? I don't see any other possible code paths in this
function etc., just regular guard() should suffice.

> +	{
> +		/* 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;

Sashiko has a remark:

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));
> +		if (ret)
> +			return ret;
> +
> +		*val = (s16)le16_to_cpu(buf[index]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,

I'd put val on the same line as val2 and mask, more logical separation.

> +			     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))
> +			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:

Again, you can probably just add guard() before the switch.

> +		scoped_guard(mutex, &data->mutex)
> +		{
> +			*val = 0;
> +			*val2 = data->range == QMC5883L_RNG_2G ?
> +					QMC5883L_SCALE_2G :
> +					QMC5883L_SCALE_8G;
> +		}
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		scoped_guard(mutex, &data->mutex)
> +		{
> +			switch (data->odr) {
> +			case QMC5883L_ODR_200HZ:
> +				*val = 200;
> +				break;
> +			case QMC5883L_ODR_100HZ:
> +				*val = 100;
> +				break;
> +			case QMC5883L_ODR_50HZ:
> +				*val = 50;
> +				break;
> +			case QMC5883L_ODR_10HZ:
> +				*val = 10;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		scoped_guard(mutex, &data->mutex)
> +		{
> +			switch (data->osr) {
> +			case QMC5883L_OSR_64:
> +				*val = 64;
> +				break;
> +			case QMC5883L_OSR_128:
> +				*val = 128;
> +				break;
> +			case QMC5883L_OSR_256:
> +				*val = 256;
> +				break;
> +			case QMC5883L_OSR_512:
> +				*val = 512;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int qmc5883l_write_raw(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan, int val,
> +			      int val2, long mask)

Same comment with val as previous function.

> +{
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +	u8 rng;
> +	u8 osr;
> +	u8 odr;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		switch (val2) {
> +		case QMC5883L_SCALE_2G:
> +			rng = QMC5883L_RNG_2G;
> +			break;
> +		case QMC5883L_SCALE_8G:
> +			rng = QMC5883L_RNG_8G;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		scoped_guard(mutex, &data->mutex)

Again, guard() is probably good enough.

> +		{
> +			ret = regmap_update_bits(data->regmap,
> +						 QMC5883L_REG_CTRL1,
> +						 QMC5883L_RNG_MASK, rng);
> +			if (ret)
> +				return ret;
> +			data->range = rng;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (val) {
> +		case 200:
> +			odr = QMC5883L_ODR_200HZ;
> +			break;
> +		case 100:
> +			odr = QMC5883L_ODR_100HZ;
> +			break;
> +		case 50:
> +			odr = QMC5883L_ODR_50HZ;
> +			break;
> +		case 10:
> +			odr = QMC5883L_ODR_10HZ;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		scoped_guard(mutex, &data->mutex)
> +		{
> +			ret = regmap_update_bits(data->regmap,
> +						 QMC5883L_REG_CTRL1,
> +						 QMC5883L_ODR_MASK, odr);
> +			if (ret)
> +				return ret;
> +			data->odr = odr;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		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)
> +		{
> +			ret = regmap_update_bits(data->regmap,
> +						 QMC5883L_REG_CTRL1,
> +						 QMC5883L_OSR_MASK, osr);
> +			if (ret)
> +				return ret;
> +			data->osr = osr;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qmc5883l_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = qmc5883l_odr_avail;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(qmc5883l_odr_avail);
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = qmc5883l_osr_avail;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(qmc5883l_osr_avail);
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = qmc5883l_rng_avail;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*length = ARRAY_SIZE(qmc5883l_rng_avail);
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int qmc5883l_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan,
> +				      long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return IIO_VAL_INT;
> +	}
> +}
> +
> +static const struct iio_info qmc5883l_info = {
> +	.read_raw = qmc5883l_read_raw,
> +	.write_raw = qmc5883l_write_raw,
> +	.read_avail = qmc5883l_read_avail,
> +	.write_raw_get_fmt = qmc5883l_write_raw_get_fmt,
> +};
> +
> +static int qmc5883l_init(struct qmc5883l_data *data)
> +{
> +	unsigned int reg;
> +	int ret;

struct regmap *regmap = data->regmap;

Makes your lines shorter.

> +	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);

I recall a conversation about using dev_warn() or dev_info() for checking
the chip ID, up to personal preference. Nevertheless, your error messages
should follow the same format, so capital letter at the beginning (nitpick).

> +
> +	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);
> +	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;
> +
> +	ret = regmap_write(data->regmap, QMC5883L_REG_CTRL1,
> +			   (QMC5883L_MODE_CONT | data->odr | data->range |
> +			    data->osr));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static bool qmc5883l_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg <= QMC5883L_REG_STATUS1;
> +}
> +
> +static bool qmc5883l_writable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case QMC5883L_REG_CTRL1:
> +	case QMC5883L_REG_CTRL2:
> +	case QMC5883L_REG_SET_RESET:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config qmc5883l_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = QMC5883L_REG_ID,
> +	.cache_type = REGCACHE_MAPLE,
> +	.volatile_reg = qmc5883l_volatile_reg,
> +	.writeable_reg = qmc5883l_writable_reg
> +};
> +
> +#define QMC5883L_CHANNEL(_axis)                                \
> +	{                                                      \
> +		.type = IIO_MAGN,                              \
> +		.modified = 1,                                 \
> +		.channel2 = IIO_MOD_##_axis,                   \
> +		.address = QMC5883L_AXIS_##_axis,              \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),  \
> +		.info_mask_shared_by_type =                    \
> +			BIT(IIO_CHAN_INFO_SCALE) |             \
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ) |         \
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +		.info_mask_shared_by_type_available =          \
> +			BIT(IIO_CHAN_INFO_SCALE) |             \
> +			BIT(IIO_CHAN_INFO_SAMP_FREQ) |         \
> +			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +	}
> +
> +static const struct iio_chan_spec qmc5883l_channels[] = {
> +	QMC5883L_CHANNEL(X),
> +	QMC5883L_CHANNEL(Y),
> +	QMC5883L_CHANNEL(Z),
> +};
> +
> +static int qmc5883l_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	struct qmc5883l_data *data;
> +	struct device *dev = &client->dev;

Reverse Christmas tree order.

> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &qmc5883l_regmap_config);
> +	if (IS_ERR(regmap)) {

No point in adding brackets if this is a single line if statement
(checkpatch.pl should warn about this IMO).

> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "regmap initialization failed\n");
> +	}
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable VDD regulator\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vddio");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable VDDIO regulator\n");
> +
> +	fsleep(QMC5883L_PORT_US);
> +
> +	data = iio_priv(indio_dev);
> +	data->regmap = regmap;

Blank line here.

> +	ret = devm_mutex_init(dev, &data->mutex);
> +	if (ret)
> +		return ret;
> +
> +	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);
> +}

You're using devm_* functions, however I don't see any callback on driver
unbinding or a potential error during probing. Consider adding a power off
callback.

-- 
Kind regards

CJD

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

[Jonathan Cameron]

> 
> > +	{
> > +		/* 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;  
> 
> Sashiko has a remark:
> 
> 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.
> 

I was curious so checked the datasheet.
https://www.qstcorp.com/upload/pdf/202512/13-52-04%20QMC5883L%20Datasheet%20Rev.%20B.pdf

Sashiko looses this time as overflow resets if the next value is in range.

I'm with Sashiko for explicit access being needed for data ready (or a write 1 to clear)
but overflow is more of an intermittent thing so both styles exist for devices
(with and without need for specific action to clear).

Jonathan
> > +
> > +		ret = regmap_bulk_read(data->regmap, QMC5883L_REG_X_LSB, buf,
> > +				       sizeof(buf));
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = (s16)le16_to_cpu(buf[index]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +

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

[Jonathan Cameron]

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);