[PATCH 0/3] iio: magnetometer: add MEMSIC MMC5983MA driver

[PATCH 0/3] iio: magnetometer: add MEMSIC MMC5983MA driver

[Vladislav Kulikov]

Add an IIO driver for the MEMSIC MMC5983MA 3-axis magnetometer over
I2C. The driver provides raw magnetic field readings with per-measurement
SET/RESET offset cancellation, giving 18-bit output with a full-scale
range of +/-8 Gauss.

Tested on a Raspberry Pi 2B with the sensor on I2C-1 at 0x30.

The following chip features are intentionally left out of the initial
driver because the public datasheet does not provide enough detail to
expose them confidently through stable IIO ABI, or because they still
need more validation:

- SPI transport: deferred because SET/RESET polarity behavior has been
  reported to differ between I2C and SPI, especially around SET/RESET
  timing and/or SPI mode.
- Temperature channel: deferred until the temperature output behavior is
  better validated.
- Continuous measurement mode and Auto SET/RESET: deferred because the
  datasheet does not clearly define the interaction between CMM, TM_M,
  Meas_M_Done, and SET/RESET sequencing.
- Saturation/self-test bits: deferred because the applied test field
  strength and bit lifetime are not specified.
- BW/decimation filter tuning: only the documented measurement timing is
  used; no filter response is exposed because the filter topology and
  coefficients are not documented.

The driver uses a conservative 500 us post-SET/RESET delay before
starting the following measurement. The datasheet describes a 500 ns
SET/RESET coil pulse, but existing sample code and practical testing
indicate that a longer software delay is needed before taking the next
measurement.

Vladislav Kulikov (3):
  dt-bindings: iio: magnetometer: add MEMSIC MMC5983MA
  iio: magnetometer: add driver for MEMSIC MMC5983MA
  MAINTAINERS: add entry for MEMSIC MMC5983MA magnetometer driver

 .../iio/magnetometer/memsic,mmc5983.yaml      |  38 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/magnetometer/Kconfig              |  11 +
 drivers/iio/magnetometer/Makefile             |   1 +
 drivers/iio/magnetometer/mmc5983.c            | 330 ++++++++++++++++++
 5 files changed, 387 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
 create mode 100644 drivers/iio/magnetometer/mmc5983.c


base-commit: 7fd2df204f342fc17d1a0bfcd474b24232fb0f32
-- 
2.43.0

[PATCH 1/3] dt-bindings: iio: magnetometer: add MEMSIC MMC5983MA

[Vladislav Kulikov]

Add device tree binding documentation for the MEMSIC MMC5983MA
3-axis magnetometer connected via I2C.

Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
---
 .../iio/magnetometer/memsic,mmc5983.yaml      | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml b/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
new file mode 100644
index 000000000000..bbe2aa597f75
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/memsic,mmc5983.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MEMSIC MMC5983MA 3-axis magnetic sensor
+
+maintainers:
+  - Vladislav Kulikov <vlad.kulikov.c@gmail.com>
+
+properties:
+  compatible:
+    const: memsic,mmc5983
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        magnetometer@30 {
+            compatible = "memsic,mmc5983";
+            reg = <0x30>;
+            vdd-supply = <&vdd_3v3_reg>;
+        };
+    };
-- 
2.43.0

[PATCH 2/3] iio: magnetometer: add driver for MEMSIC MMC5983MA

[Vladislav Kulikov]

Add support for the MEMSIC MMC5983MA 3-axis magnetometer. The driver
provides raw magnetic field readings via IIO sysfs with SET/RESET
offset cancellation for each measurement.

Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
---
 drivers/iio/magnetometer/Kconfig   |  11 +
 drivers/iio/magnetometer/Makefile  |   1 +
 drivers/iio/magnetometer/mmc5983.c | 330 +++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/iio/magnetometer/mmc5983.c

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index fb313e591e85..ea2697fb5ab6 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -151,6 +151,17 @@ config MMC5633
 	  To compile this driver as a module, choose M here: the module
 	  will be called mmc5633
 
+config MMC5983
+	tristate "MEMSIC MMC5983 3-axis magnetic sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for the MEMSIC MMC5983 3-axis
+	  magnetic sensor.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mmc5983
+
 config IIO_ST_MAGN_3AXIS
 	tristate "STMicroelectronics magnetometers 3-Axis Driver"
 	depends on (I2C || SPI_MASTER) && SYSFS
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 5bd227f8c120..7fd9b3fd914e 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MAG3110)	+= mag3110.o
 obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
 obj-$(CONFIG_MMC35240)	+= mmc35240.o
 obj-$(CONFIG_MMC5633)	+= mmc5633.o
+obj-$(CONFIG_MMC5983)	+= mmc5983.o
 
 obj-$(CONFIG_IIO_ST_MAGN_3AXIS) += st_magn.o
 st_magn-y := st_magn_core.o
diff --git a/drivers/iio/magnetometer/mmc5983.c b/drivers/iio/magnetometer/mmc5983.c
new file mode 100644
index 000000000000..7f2f5133a472
--- /dev/null
+++ b/drivers/iio/magnetometer/mmc5983.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MMC5983 - MEMSIC 3-axis Magnetic Sensor
+ *
+ * Copyright (c) 2026, Vlad Kulikov <vlad.kulikov.c@gmail.com>
+ *
+ * IIO driver for MMC5983
+ */
+
+#include <linux/bits.h>
+#include <linux/cleanup.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/types.h>
+
+#define MMC5983_REG_XOUT0	0x00
+#define MMC5983_REG_XOUT1	0x01
+#define MMC5983_REG_YOUT0	0x02
+#define MMC5983_REG_YOUT1	0x03
+#define MMC5983_REG_ZOUT0	0x04
+#define MMC5983_REG_ZOUT1	0x05
+#define MMC5983_REG_XYZOUT2	0x06
+
+#define MMC5983_REG_STATUS	0x08
+
+#define MMC5983_REG_CTRL0	0x09
+#define MMC5983_REG_CTRL1	0x0A
+#define MMC5983_REG_CTRL2	0x0B
+#define MMC5983_REG_CTRL3	0x0C
+
+#define MMC5983_REG_ID		0x2F
+
+#define MMC5983_PRODUCT_ID	0x30
+
+#define MMC5983_STATUS_MEAS_M_DONE_BIT	BIT(0)
+#define MMC5983_STATUS_OTP_RD_DONE_BIT	BIT(4)
+
+#define MMC5983_CTRL0_TM_M_BIT		BIT(0)
+#define MMC5983_CTRL0_SET_BIT		BIT(3)
+#define MMC5983_CTRL0_RESET_BIT		BIT(4)
+#define MMC5983_CTRL0_OTP_RD_BIT	BIT(6)
+
+#define MMC5983_CTRL1_SW_RST_BIT	BIT(7)
+
+enum mmc5983_axis {
+	MMC5983_AXIS_X,
+	MMC5983_AXIS_Y,
+	MMC5983_AXIS_Z
+};
+
+struct mmc5983_data {
+	struct regmap *regmap;
+	/* Protects chip access during SET/RESET measurement sequence */
+	struct mutex mutex;
+};
+
+#define MMC5983_CHANNEL(_axis) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_##_axis, \
+	.address = MMC5983_AXIS_##_axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+static const struct iio_chan_spec mmc5983_channels[] = {
+	MMC5983_CHANNEL(X),
+	MMC5983_CHANNEL(Y),
+	MMC5983_CHANNEL(Z)
+};
+
+static int mmc5983_take_measurement(struct mmc5983_data *data, int m[3])
+{
+	unsigned int status;
+	u8 buf[7];
+	int ret;
+
+	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+			   MMC5983_CTRL0_TM_M_BIT);
+	if (ret)
+		return ret;
+
+	ret = regmap_read_poll_timeout(data->regmap, MMC5983_REG_STATUS,
+				       status,
+				       status & MMC5983_STATUS_MEAS_M_DONE_BIT,
+				       5000, 50000);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, MMC5983_REG_XOUT0, buf,
+			       sizeof(buf));
+	if (ret)
+		return ret;
+
+	m[0] = (buf[0] << 10) | (buf[1] << 2) | ((buf[6] >> 6) & 0x3);
+	m[1] = (buf[2] << 10) | (buf[3] << 2) | ((buf[6] >> 4) & 0x3);
+	m[2] = (buf[4] << 10) | (buf[5] << 2) | ((buf[6] >> 2) & 0x3);
+
+	return 0;
+}
+
+static int mmc5983_read_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan, int *val,
+			     int *val2, long mask)
+{
+	struct mmc5983_data *data = iio_priv(indio_dev);
+	int m1[3], m2[3];
+	int ret = -EBUSY;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		scoped_guard(mutex, &data->mutex) {
+			ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+					   MMC5983_CTRL0_SET_BIT);
+			if (ret)
+				break;
+
+			fsleep(500);
+
+			ret = mmc5983_take_measurement(data, m1);
+			if (ret)
+				break;
+
+			ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+					   MMC5983_CTRL0_RESET_BIT);
+			if (ret)
+				break;
+
+			fsleep(500);
+
+			ret = mmc5983_take_measurement(data, m2);
+			if (ret)
+				break;
+
+			*val = (m1[chan->address] - m2[chan->address]) / 2;
+			ret = IIO_VAL_INT;
+		}
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = 61035;
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info mmc5983_info = {
+	.read_raw = mmc5983_read_raw
+};
+
+static bool mmc5983_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5983_REG_CTRL0:
+	case MMC5983_REG_CTRL1:
+	case MMC5983_REG_CTRL2:
+	case MMC5983_REG_CTRL3:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mmc5983_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5983_REG_XOUT0:
+	case MMC5983_REG_XOUT1:
+	case MMC5983_REG_YOUT0:
+	case MMC5983_REG_YOUT1:
+	case MMC5983_REG_ZOUT0:
+	case MMC5983_REG_ZOUT1:
+	case MMC5983_REG_XYZOUT2:
+	case MMC5983_REG_STATUS:
+	case MMC5983_REG_CTRL0:
+	case MMC5983_REG_CTRL1:
+	case MMC5983_REG_CTRL2:
+	case MMC5983_REG_CTRL3:
+	case MMC5983_REG_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mmc5983_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5983_REG_XOUT0:
+	case MMC5983_REG_XOUT1:
+	case MMC5983_REG_YOUT0:
+	case MMC5983_REG_YOUT1:
+	case MMC5983_REG_ZOUT0:
+	case MMC5983_REG_ZOUT1:
+	case MMC5983_REG_XYZOUT2:
+	case MMC5983_REG_STATUS:
+	case MMC5983_REG_CTRL0:
+	case MMC5983_REG_CTRL1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config mmc5983_regmap_config = {
+	.name = "mmc5983_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MMC5983_REG_ID,
+	.writeable_reg = mmc5983_is_writeable_reg,
+	.readable_reg = mmc5983_is_readable_reg,
+	.volatile_reg = mmc5983_is_volatile_reg,
+};
+
+static int mmc5983_init(struct mmc5983_data *data)
+{
+	unsigned int reg_id, status;
+	int ret;
+
+	ret = regmap_read(data->regmap, MMC5983_REG_ID, &reg_id);
+	if (ret)
+		return dev_err_probe(regmap_get_device(data->regmap), ret,
+				     "Error reading product id\n");
+
+	if (reg_id != MMC5983_PRODUCT_ID)
+		return dev_err_probe(regmap_get_device(data->regmap), -ENODEV,
+				     "wrong product id 0x%02x\n", reg_id);
+
+	ret = regmap_write(data->regmap, MMC5983_REG_CTRL1,
+			   MMC5983_CTRL1_SW_RST_BIT);
+	if (ret)
+		return ret;
+
+	fsleep(10000);
+
+	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+			   MMC5983_CTRL0_OTP_RD_BIT);
+	if (ret)
+		return ret;
+
+	ret = regmap_read_poll_timeout(data->regmap, MMC5983_REG_STATUS,
+				       status,
+				       status & MMC5983_STATUS_OTP_RD_DONE_BIT,
+				       1000, 10000);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+			   MMC5983_CTRL0_SET_BIT);
+	if (ret)
+		return ret;
+
+	fsleep(500);
+
+	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
+			   MMC5983_CTRL0_RESET_BIT);
+	if (ret)
+		return ret;
+
+	fsleep(500);
+
+	return 0;
+}
+
+static int mmc5983_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct mmc5983_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+
+	ret = devm_mutex_init(dev, &data->mutex);
+	if (ret)
+		return ret;
+
+	data->regmap = devm_regmap_init_i2c(i2c, &mmc5983_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "failed to allocate register map\n");
+
+	indio_dev->info = &mmc5983_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = i2c->name;
+	indio_dev->channels = mmc5983_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mmc5983_channels);
+
+	ret = mmc5983_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "mmc5983 chip init failed\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id mmc5983_of_match[] = {
+	{ .compatible = "memsic,mmc5983" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mmc5983_of_match);
+
+static const struct i2c_device_id mmc5983_id[] = {
+	{ "mmc5983" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mmc5983_id);
+
+static struct i2c_driver mmc5983_driver = {
+	.driver = {
+		.name = "mmc5983",
+		.of_match_table = mmc5983_of_match,
+	},
+	.probe = mmc5983_probe,
+	.id_table = mmc5983_id,
+};
+module_i2c_driver(mmc5983_driver);
+
+MODULE_AUTHOR("Vladislav Kulikov <vlad.kulikov.c@gmail.com>");
+MODULE_DESCRIPTION("MEMSIC MMC5983 magnetic sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

[PATCH 3/3] MAINTAINERS: add entry for MEMSIC MMC5983MA magnetometer driver

[Vladislav Kulikov]

Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 882214b0e7db..b1d9d7b586a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17170,6 +17170,13 @@ F:	drivers/mtd/
 F:	include/linux/mtd/
 F:	include/uapi/mtd/
 
+MEMSIC MMC5983 MAGNETOMETER DRIVER
+M:	Vladislav Kulikov <vlad.kulikov.c@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
+F:	drivers/iio/magnetometer/mmc5983.c
+
 MEN A21 WATCHDOG DRIVER
 M:	Johannes Thumshirn <morbidrsa@gmail.com>
 L:	linux-watchdog@vger.kernel.org
-- 
2.43.0

Re: [PATCH 1/3] dt-bindings: iio: magnetometer: add MEMSIC MMC5983MA

[Jonathan Cameron]

On Thu,  7 May 2026 12:47:22 +0000
Vladislav Kulikov <vlad.kulikov.c@gmail.com> wrote:

> Add device tree binding documentation for the MEMSIC MMC5983MA
> 3-axis magnetometer connected via I2C.

HI Vladislav,

Usual question to answer.  How is this different from existing memsic
magnetometers that are supported?  Even if the driver is different
is there a reason to have a separate binding?  Note that same question
will apply to the driver - just provide some brief notes on how it
is different enough from existing devices. 

The others are all in trivial bindings.  They maybe should not be!

A few other comments inline.

Whilst you comment you've left the SPI side out, it would be good to still
have a DT binding even if the driver doesn't support it.


> 
> Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
> ---
>  .../iio/magnetometer/memsic,mmc5983.yaml      | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml b/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
> new file mode 100644
> index 000000000000..bbe2aa597f75
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/memsic,mmc5983.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MEMSIC MMC5983MA 3-axis magnetic sensor
> +
> +maintainers:
> +  - Vladislav Kulikov <vlad.kulikov.c@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: memsic,mmc5983
> +
> +  reg:
> +    maxItems: 1
Binding should try to be as complete as possible, even if the driver
doesn't yet use some features.  Looks like we have an interrupt to 
describe.  Also vddio is missing based on the datasheet google found for me:

https://media.digikey.com/pdf/Data%20Sheets/MEMSIC%20PDFs/MMC5983MA_RevA_4-3-19.pdf

> +
> +  vdd-supply:
> +    description: Regulator that provides power to the sensor
> +
> +required:
> +  - compatible
> +  - reg
I'm guessing it doesn't work well without a power supply
   - vdd-supply 
should be here.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        magnetometer@30 {
> +            compatible = "memsic,mmc5983";
> +            reg = <0x30>;
> +            vdd-supply = <&vdd_3v3_reg>;
> +        };
> +    };

Re: [PATCH 3/3] MAINTAINERS: add entry for MEMSIC MMC5983MA magnetometer driver

[Jonathan Cameron]

On Thu,  7 May 2026 12:47:24 +0000
Vladislav Kulikov <vlad.kulikov.c@gmail.com> wrote:

> Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
Hi Vladislav

My preference is for the Maintainers entry to be added with the
first patch in the series (dt binding only at that point) and
then the other files added in the patches that add them.
That way it's always valid and their maintainers are fully
documented as part of the patches rather than at the end
like this.

Jonathan

> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 882214b0e7db..b1d9d7b586a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17170,6 +17170,13 @@ F:	drivers/mtd/
>  F:	include/linux/mtd/
>  F:	include/uapi/mtd/
>  
> +MEMSIC MMC5983 MAGNETOMETER DRIVER
> +M:	Vladislav Kulikov <vlad.kulikov.c@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/magnetometer/memsic,mmc5983.yaml
> +F:	drivers/iio/magnetometer/mmc5983.c
> +
>  MEN A21 WATCHDOG DRIVER
>  M:	Johannes Thumshirn <morbidrsa@gmail.com>
>  L:	linux-watchdog@vger.kernel.org

Re: [PATCH 2/3] iio: magnetometer: add driver for MEMSIC MMC5983MA

[Jonathan Cameron]

On Thu,  7 May 2026 12:47:23 +0000
Vladislav Kulikov <vlad.kulikov.c@gmail.com> wrote:

> Add support for the MEMSIC MMC5983MA 3-axis magnetometer. The driver
> provides raw magnetic field readings via IIO sysfs with SET/RESET
> offset cancellation for each measurement.
> 
> Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
Hi

A few comments inline but mostly a nice clean driver.

Thanks,

Jonathan

> diff --git a/drivers/iio/magnetometer/mmc5983.c b/drivers/iio/magnetometer/mmc5983.c
> new file mode 100644
> index 000000000000..7f2f5133a472
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5983.c


> +enum mmc5983_axis {
> +	MMC5983_AXIS_X,
> +	MMC5983_AXIS_Y,
> +	MMC5983_AXIS_Z
May get more entries so add trailing comma to reduce future churn

> +};
> +
> +struct mmc5983_data {
> +	struct regmap *regmap;
> +	/* Protects chip access during SET/RESET measurement sequence */
> +	struct mutex mutex;
> +};

> +
> +static const struct iio_chan_spec mmc5983_channels[] = {
> +	MMC5983_CHANNEL(X),
> +	MMC5983_CHANNEL(Y),
> +	MMC5983_CHANNEL(Z)

We may well get channels after this so add a trailing ,

> +};

> +
> +static int mmc5983_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct mmc5983_data *data = iio_priv(indio_dev);
> +	int m1[3], m2[3];
> +	int ret = -EBUSY;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		scoped_guard(mutex, &data->mutex) {
To avoid the intent and need for that return at the end (due to compilers
being slow to catch on to for loops that are always run once!)

	case IIO_CHAN_INFO_RAW: {
		guard(mutex)(&data->mutex);

		ret = ...

		...
		reutrn IIO_VAL_INT;
	}

> +			ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +					   MMC5983_CTRL0_SET_BIT);
> +			if (ret)
> +				break;
> +
> +			fsleep(500);
> +
> +			ret = mmc5983_take_measurement(data, m1);
> +			if (ret)
> +				break;
> +
> +			ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +					   MMC5983_CTRL0_RESET_BIT);
> +			if (ret)
> +				break;
> +
> +			fsleep(500);
> +
> +			ret = mmc5983_take_measurement(data, m2);
> +			if (ret)
> +				break;
> +
> +			*val = (m1[chan->address] - m2[chan->address]) / 2;
> +			ret = IIO_VAL_INT;
> +		}
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = 61035;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int mmc5983_init(struct mmc5983_data *data)
> +{
> +	unsigned int reg_id, status;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MMC5983_REG_ID, &reg_id);
> +	if (ret)
> +		return dev_err_probe(regmap_get_device(data->regmap), ret,
Have

	struct device *dev = regmap_get_device(data->regmap);

at top of function to avoid repeats of this.  Then we should near enough
to 80 chars that at least some of these error messages become one liners.
Given lots of uses of regmap might be useful to have a local variable for
that as well.  If a given line is a tiny bit over 80 chars and that
helps readability then that is fine in IIO these days. With these
local variables I suspect that applies quite a lot in here!

> +				     "Error reading product id\n");
> +
> +	if (reg_id != MMC5983_PRODUCT_ID)
This one is a bit subtle but we never fail to probe on a missmatched product ID.
The reason is DT fallback compatibles.  If in future memsic release a new device
that has a different product ID but which is a strict superset (or identical)
in interface etc, then it will have a fallback compatible listed in the dt-binding.

Those allow old kernels to still support the device based on that compatibility.
That doesn't work if the product ID failing to match is a hard failure.

So trust the firmware and just print an dev_info() to indicate you have
a part you don't recognise rather than failing.

> +		return dev_err_probe(regmap_get_device(data->regmap), -ENODEV,
> +				     "wrong product id 0x%02x\n", reg_id);
> +
> +	ret = regmap_write(data->regmap, MMC5983_REG_CTRL1,
> +			   MMC5983_CTRL1_SW_RST_BIT);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(10000);
As below. Would expect a comment on why this time.

> +
> +	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +			   MMC5983_CTRL0_OTP_RD_BIT);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(data->regmap, MMC5983_REG_STATUS,
> +				       status,
> +				       status & MMC5983_STATUS_OTP_RD_DONE_BIT,
> +				       1000, 10000);

Why these times?  Spec reference.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +			   MMC5983_CTRL0_SET_BIT);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(500);
As below.
> +
> +	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +			   MMC5983_CTRL0_RESET_BIT);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(500);

Add a spec reference ideally that justifies this value. Sometimes
we get 'slow' parts that need longer. Hence it is good to know
where to find the numbers.

> +
> +	return 0;
> +}
> +
> +static int mmc5983_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct mmc5983_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &data->mutex);
> +	if (ret)
> +		return ret;
> +
> +	data->regmap = devm_regmap_init_i2c(i2c, &mmc5983_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "failed to allocate register map\n");
> +
> +	indio_dev->info = &mmc5983_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = i2c->name;

Use a hard coded name.  This can provide annoying fragile dependent on exactly
how the driver was bound / firmware etc and for now there is only one
right answer wrt to what the part number is.

> +	indio_dev->channels = mmc5983_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mmc5983_channels);
> +
> +	ret = mmc5983_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "mmc5983 chip init failed\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Re: [PATCH 0/3] iio: magnetometer: add MEMSIC MMC5983MA driver

[Andy Shevchenko]

On Thu, May 07, 2026 at 12:47:21PM +0000, Vladislav Kulikov wrote:
> Add an IIO driver for the MEMSIC MMC5983MA 3-axis magnetometer over
> I2C. The driver provides raw magnetic field readings with per-measurement
> SET/RESET offset cancellation, giving 18-bit output with a full-scale
> range of +/-8 Gauss.
> 
> Tested on a Raspberry Pi 2B with the sensor on I2C-1 at 0x30.
> 
> The following chip features are intentionally left out of the initial
> driver because the public datasheet does not provide enough detail to
> expose them confidently through stable IIO ABI, or because they still
> need more validation:
> 
> - SPI transport: deferred because SET/RESET polarity behavior has been
>   reported to differ between I2C and SPI, especially around SET/RESET
>   timing and/or SPI mode.
> - Temperature channel: deferred until the temperature output behavior is
>   better validated.
> - Continuous measurement mode and Auto SET/RESET: deferred because the
>   datasheet does not clearly define the interaction between CMM, TM_M,
>   Meas_M_Done, and SET/RESET sequencing.
> - Saturation/self-test bits: deferred because the applied test field
>   strength and bit lifetime are not specified.
> - BW/decimation filter tuning: only the documented measurement timing is
>   used; no filter response is exposed because the filter topology and
>   coefficients are not documented.
> 
> The driver uses a conservative 500 us post-SET/RESET delay before
> starting the following measurement. The datasheet describes a 500 ns
> SET/RESET coil pulse, but existing sample code and practical testing
> indicate that a longer software delay is needed before taking the next
> measurement.

Missed section for a new driver. Id est answer the question "Why a new brand
driver? Do we have something similar in IIO  already to be expanded to cover
this HW part?"

-- 
With Best Regards,
Andy Shevchenko