[PATCH v4 0/4] Add support for AF8133J magnetometer

[PATCH v4 0/4] Add support for AF8133J magnetometer

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

This series adds support for AF8133J magnetometer sensor. It's a simple
3-axis sensor with two sensitivity options and not much else to it.

This sensor is used on both Pinephone and Pinephone Pro. DT patches
adding it will come later, once this driver is merged.

Please take a look. :)

Thank you very much,
	Ondřej Jirman

v4:
- move RPM enable in probe function before iio device registration

v3:
- collect more tags
- if (ret < 0) -> (ret) where appropriate
- scoped guard move to af8133j_set_scale()
- remove pm_runtime_disable/enable guard from af8133j_power_down_action()
- pretty much just this:
  https://megous.com/dl/tmp/0001-if-ret-0-ret-where-appropriate.patch
  https://megous.com/dl/tmp/0002-scoped-guard-move-to-af8133j_set_scale.patch
  https://megous.com/dl/tmp/0003-remove-pm_runtime_disable-enable-guard-from-af8133j_.patch

v2:
- move maintainers patch to the end of series
- bindings:
  - fix compatible definition in bindings file
  - require power supplies
  - fix descriptions
- driver:
  - sort includes
  - rework RPM, the driver should now work with RPM disabled
    among other improvements
    - I've tested RPM left and right doing device bind/unbind under
      various conditions, system suspend under various conditions,
      etc.
  - use scoped_guard for mutexes
  - use devm for power down and handle power down correctly with both
    RPM enabled/disabled without tracking power state in data->powered
  - fix issue with changing scale while RPM suspended
  - various code formatting issues resolved
- as for sign-offs, I've added co-developed-by for people I know for
  sure worked on the driver, and left other tags as they were when
  I picked up the patch 2 years ago to my Linux branch

Icenowy Zheng (3):
  dt-bindings: vendor-prefix: Add prefix for Voltafield
  dt-bindings: iio: magnetometer: Add Voltafield AF8133J
  iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

Ondrej Jirman (1):
  MAINTAINERS: Add an entry for AF8133J driver

 .../iio/magnetometer/voltafield,af8133j.yaml  |  60 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/magnetometer/Kconfig              |  12 +
 drivers/iio/magnetometer/Makefile             |   1 +
 drivers/iio/magnetometer/af8133j.c            | 524 ++++++++++++++++++
 6 files changed, 605 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
 create mode 100644 drivers/iio/magnetometer/af8133j.c

-- 
2.43.0

[PATCH v4 1/4] dt-bindings: vendor-prefix: Add prefix for Voltafield

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

Voltafile Technology Corp. is a company that produces MEMS sensors.

Add a DT vendor prefix for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Ondřej Jirman <megi@xff.cz>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 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 1a0dc04f1db4..82e9f64c90ff 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1534,6 +1534,8 @@ patternProperties:
     description: VoCore Studio
   "^voipac,.*":
     description: Voipac Technologies s.r.o.
+  "^voltafield,.*":
+    description: Voltafield Technology Corp.
   "^vot,.*":
     description: Vision Optical Technology Co., Ltd.
   "^vxt,.*":
-- 
2.43.0

[PATCH v4 2/4] dt-bindings: iio: magnetometer: Add Voltafield AF8133J

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

Voltafield AF8133J is a simple magnetometer sensor produced by Voltafield
Technology Corp, with dual power supplies (one for core and one for I/O)
and active-low reset pin.

The sensor has configurable range 1.2 - 2.2 mT and a software controlled
standby mode.

Add a device tree binding for it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Ondřej Jirman <megi@xff.cz>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../iio/magnetometer/voltafield,af8133j.yaml  | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
new file mode 100644
index 000000000000..b6ab01a6914a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/voltafield,af8133j.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Voltafield AF8133J magnetometer sensor
+
+maintainers:
+  - Ondřej Jirman <megi@xff.cz>
+
+properties:
+  compatible:
+    const: voltafield,af8133j
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      A signal for active low reset input of the sensor. (optional; if not
+      used, software reset over I2C will be used instead)
+
+  avdd-supply:
+    description:
+      A regulator that provides AVDD power (Working power, usually 3.3V) to
+      the sensor.
+
+  dvdd-supply:
+    description:
+      A regulator that provides DVDD power (Digital IO power, 1.8V - AVDD)
+      to the sensor.
+
+  mount-matrix:
+    description: An optional 3x3 mounting rotation matrix.
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - dvdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@1c {
+            compatible = "voltafield,af8133j";
+            reg = <0x1c>;
+            avdd-supply = <&reg_dldo1>;
+            dvdd-supply = <&reg_dldo1>;
+            reset-gpios = <&pio 1 1 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.43.0

[PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Ondřej Jirman]

From: Icenowy Zheng <icenowy@aosc.io>

AF8133J is a simple I2C-connected magnetometer, without interrupts.

Add a simple IIO driver for it.

Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Dalton Durst <dalton@ubports.com>
Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
---
 drivers/iio/magnetometer/Kconfig   |  12 +
 drivers/iio/magnetometer/Makefile  |   1 +
 drivers/iio/magnetometer/af8133j.c | 524 +++++++++++++++++++++++++++++
 3 files changed, 537 insertions(+)
 create mode 100644 drivers/iio/magnetometer/af8133j.c

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 38532d840f2a..cd2917d71904 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -6,6 +6,18 @@
 
 menu "Magnetometer sensors"
 
+config AF8133J
+	tristate "Voltafield AF8133J 3-Axis Magnetometer"
+	depends on I2C
+	depends on OF
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for Voltafield AF8133J I2C-based
+	  3-axis magnetometer chip.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called af8133j.
+
 config AK8974
 	tristate "Asahi Kasei AK8974 3-Axis Magnetometer"
 	depends on I2C
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index b1c784ea71c8..ec5c46fbf999 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AF8133J)	+= af8133j.o
 obj-$(CONFIG_AK8974)	+= ak8974.o
 obj-$(CONFIG_AK8975)	+= ak8975.o
 obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
new file mode 100644
index 000000000000..c75d545e152b
--- /dev/null
+++ b/drivers/iio/magnetometer/af8133j.c
@@ -0,0 +1,524 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * af8133j.c - Voltafield AF8133J magnetometer driver
+ *
+ * Copyright 2021 Icenowy Zheng <icenowy@aosc.io>
+ * Copyright 2024 Ondřej Jirman <megi@xff.cz>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AF8133J_REG_OUT		0x03
+#define AF8133J_REG_PCODE	0x00
+#define AF8133J_REG_PCODE_VAL	0x5e
+#define AF8133J_REG_STATUS	0x02
+#define AF8133J_REG_STATUS_ACQ	BIT(0)
+#define AF8133J_REG_STATE	0x0a
+#define AF8133J_REG_STATE_STBY	0x00
+#define AF8133J_REG_STATE_WORK	0x01
+#define AF8133J_REG_RANGE	0x0b
+#define AF8133J_REG_RANGE_22G	0x12
+#define AF8133J_REG_RANGE_12G	0x34
+#define AF8133J_REG_SWR		0x11
+#define AF8133J_REG_SWR_PERFORM	0x81
+
+static const char * const af8133j_supply_names[] = {
+	"avdd",
+	"dvdd",
+};
+
+struct af8133j_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct iio_mount_matrix orientation;
+
+	struct gpio_desc *reset_gpiod;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(af8133j_supply_names)];
+
+	u8 range;
+};
+
+enum af8133j_axis {
+	AXIS_X = 0,
+	AXIS_Y,
+	AXIS_Z,
+};
+
+static struct iio_mount_matrix *
+af8133j_get_mount_matrix(struct iio_dev *indio_dev,
+			 const struct iio_chan_spec *chan)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info af8133j_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, af8133j_get_mount_matrix),
+	{ }
+};
+
+#define AF8133J_CHANNEL(_si, _axis) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_ ## _axis, \
+	.address = AXIS_ ## _axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
+	.ext_info = af8133j_ext_info, \
+	.scan_index = _si, \
+	.scan_type = { \
+		.sign = 's', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+static const struct iio_chan_spec af8133j_channels[] = {
+	AF8133J_CHANNEL(0, X),
+	AF8133J_CHANNEL(1, Y),
+	AF8133J_CHANNEL(2, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static int af8133j_product_check(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
+	if (ret) {
+		dev_err(dev, "Error reading product code (%d)\n", ret);
+		return ret;
+	}
+
+	if (val != AF8133J_REG_PCODE_VAL) {
+		dev_err(dev, "Invalid product code (0x%02x)\n", val);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int af8133j_reset(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	if (data->reset_gpiod) {
+		/* If we have GPIO reset line, use it */
+		gpiod_set_value_cansleep(data->reset_gpiod, 1);
+		udelay(10);
+		gpiod_set_value_cansleep(data->reset_gpiod, 0);
+	} else {
+		/* Otherwise use software reset */
+		ret = regmap_write(data->regmap, AF8133J_REG_SWR,
+				   AF8133J_REG_SWR_PERFORM);
+		if (ret) {
+			dev_err(dev, "Failed to reset the chip\n");
+			return ret;
+		}
+	}
+
+	/* Wait for reset to finish */
+	usleep_range(1000, 1100);
+
+	/* Restore range setting */
+	if (data->range == AF8133J_REG_RANGE_22G) {
+		ret = regmap_write(data->regmap, AF8133J_REG_RANGE, data->range);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void af8133j_power_down(struct af8133j_data *data)
+{
+	gpiod_set_value_cansleep(data->reset_gpiod, 1);
+	regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+}
+
+static int af8133j_power_up(struct af8133j_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret) {
+		dev_err(dev, "Could not enable regulators\n");
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(data->reset_gpiod, 0);
+
+	/* Wait for power on reset */
+	usleep_range(15000, 16000);
+
+	ret = af8133j_reset(data);
+	if (ret) {
+		af8133j_power_down(data);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int af8133j_take_measurement(struct af8133j_data *data)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_write(data->regmap,
+			   AF8133J_REG_STATE, AF8133J_REG_STATE_WORK);
+	if (ret)
+		return ret;
+
+	/* The datasheet says "Mesaure Time <1.5ms" */
+	ret = regmap_read_poll_timeout(data->regmap, AF8133J_REG_STATUS, val,
+				       val & AF8133J_REG_STATUS_ACQ,
+				       500, 1500);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap,
+			   AF8133J_REG_STATE, AF8133J_REG_STATE_STBY);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		/*
+		 * Ignore EACCES because that happens when RPM is disabled
+		 * during system sleep, while userspace leave eg. hrtimer
+		 * trigger attached and IIO core keeps trying to do measurements.
+		 */
+		if (ret != -EACCES)
+			dev_err(dev, "Failed to power on (%d)\n", ret);
+		return ret;
+	}
+
+	scoped_guard(mutex, &data->mutex) {
+		ret = af8133j_take_measurement(data);
+		if (ret)
+			goto out_rpm_put;
+
+		ret = regmap_bulk_read(data->regmap, AF8133J_REG_OUT,
+				       buf, sizeof(__le16) * 3);
+	}
+
+out_rpm_put:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
+static const int af8133j_scales[][2] = {
+	[0] = { 0, 366210 }, // 12 gauss
+	[1] = { 0, 671386 }, // 22 gauss
+};
+
+static int af8133j_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+	__le16 buf[3];
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = af8133j_read_measurement(data, buf);
+		if (ret)
+			return ret;
+
+		*val = sign_extend32(le16_to_cpu(buf[chan->address]),
+				     chan->scan_type.realbits - 1);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+
+		if (data->range == AF8133J_REG_RANGE_12G)
+			*val2 = af8133j_scales[0][1];
+		else
+			*val2 = af8133j_scales[1][1];
+
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_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_SCALE:
+		*vals = (const int *)af8133j_scales;
+		*length = ARRAY_SIZE(af8133j_scales) * 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_set_scale(struct af8133j_data *data,
+			     unsigned int val, unsigned int val2)
+{
+	struct device *dev = &data->client->dev;
+	u8 range;
+	int ret = 0;
+
+	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
+		range = AF8133J_REG_RANGE_12G;
+	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
+		range = AF8133J_REG_RANGE_22G;
+	else
+		return -EINVAL;
+
+	pm_runtime_disable(dev);
+
+	/*
+	 * When suspended, just store the new range to data->range to be
+	 * applied later during power up.
+	 */
+	if (!pm_runtime_status_suspended(dev))
+		scoped_guard(mutex, &data->mutex)
+			ret = regmap_write(data->regmap,
+					   AF8133J_REG_RANGE, range);
+
+	pm_runtime_enable(dev);
+
+	data->range = range;
+	return ret;
+}
+
+static int af8133j_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return af8133j_set_scale(data, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int af8133j_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const struct iio_info af8133j_info = {
+	.read_raw = af8133j_read_raw,
+	.read_avail = af8133j_read_avail,
+	.write_raw = af8133j_write_raw,
+	.write_raw_get_fmt = af8133j_write_raw_get_fmt,
+};
+
+static irqreturn_t af8133j_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct af8133j_data *data = iio_priv(indio_dev);
+	s64 timestamp = iio_get_time_ns(indio_dev);
+	struct {
+		__le16 values[3];
+		s64 timestamp __aligned(8);
+	} sample;
+	int ret;
+
+	memset(&sample, 0, sizeof(sample));
+
+	ret = af8133j_read_measurement(data, sample.values);
+	if (ret)
+		goto out_done;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &sample, timestamp);
+
+out_done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct regmap_config af8133j_regmap_config = {
+	.name = "af8133j_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AF8133J_REG_SWR,
+	.cache_type = REGCACHE_NONE,
+};
+
+static void af8133j_power_down_action(void *ptr)
+{
+	struct af8133j_data *data = ptr;
+
+	if (!pm_runtime_status_suspended(&data->client->dev))
+		af8133j_power_down(data);
+}
+
+static int af8133j_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct af8133j_data *data;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int ret, i;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap initialization failed\n");
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->regmap = regmap;
+	data->range = AF8133J_REG_RANGE_12G;
+	mutex_init(&data->mutex);
+
+	data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpiod))
+		return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
+				     "Failed to get reset gpio\n");
+
+	for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
+		data->supplies[i].supply = af8133j_supply_names[i];
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret)
+		return ret;
+
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
+
+	ret = af8133j_power_up(data);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_active(dev);
+
+	ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
+	if (ret)
+		return ret;
+
+	ret = af8133j_product_check(data);
+	if (ret)
+		return ret;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 500);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_put_autosuspend(dev);
+
+	indio_dev->info = &af8133j_info;
+	indio_dev->name = "af8133j";
+	indio_dev->channels = af8133j_channels;
+	indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      &af8133j_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to setup iio triggered buffer\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register iio device");
+
+	return 0;
+}
+
+static int af8133j_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	af8133j_power_down(data);
+
+	return 0;
+}
+
+static int af8133j_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct af8133j_data *data = iio_priv(indio_dev);
+
+	return af8133j_power_up(data);
+}
+
+const struct dev_pm_ops af8133j_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+	RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
+};
+
+static const struct of_device_id af8133j_of_match[] = {
+	{ .compatible = "voltafield,af8133j", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, af8133j_of_match);
+
+static const struct i2c_device_id af8133j_id[] = {
+	{ "af8133j", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, af8133j_id);
+
+static struct i2c_driver af8133j_driver = {
+	.driver = {
+		.name = "af8133j",
+		.of_match_table = af8133j_of_match,
+		.pm = pm_ptr(&af8133j_pm_ops),
+	},
+	.probe = af8133j_probe,
+	.id_table = af8133j_id,
+};
+
+module_i2c_driver(af8133j_driver);
+
+MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
+MODULE_AUTHOR("Ondřej Jirman <megi@xff.cz>");
+MODULE_DESCRIPTION("Voltafield AF8133J magnetic sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Andrey Skvortsov]

Hi,

On 24-02-22 02:13, Ondřej Jirman wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
> 
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
> 
> Add a simple IIO driver for it.
> 
> Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Dalton Durst <dalton@ubports.com>
> Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> ---
>  drivers/iio/magnetometer/Kconfig   |  12 +
>  drivers/iio/magnetometer/Makefile  |   1 +
>  drivers/iio/magnetometer/af8133j.c | 524 +++++++++++++++++++++++++++++
>  3 files changed, 537 insertions(+)
>  create mode 100644 drivers/iio/magnetometer/af8133j.c
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 38532d840f2a..cd2917d71904 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -6,6 +6,18 @@

I've tested v4 driver again on 6.8-rc5. Works like before.

-- 
Best regards,
Andrey Skvortsov

Re: [PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Jonathan Cameron]

On Thu, 22 Feb 2024 02:13:37 +0100
Ondřej Jirman <megi@xff.cz> wrote:

> From: Icenowy Zheng <icenowy@aosc.io>
> 
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
> 
> Add a simple IIO driver for it.
> 
> Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Check patch correct moaned that Icenowy is the author (from:)
so doesn't need a co-developed.

> Signed-off-by: Dalton Durst <dalton@ubports.com>
> Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>

Hi.

A few really minor things noticed during a final review.
I'll tweak them whilst applying.  Diff is

diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/af8133j.c
index c75d545e152b..40657a08ce37 100644
--- a/drivers/iio/magnetometer/af8133j.c
+++ b/drivers/iio/magnetometer/af8133j.c
@@ -40,6 +40,10 @@ static const char * const af8133j_supply_names[] = {
 struct af8133j_data {
        struct i2c_client *client;
        struct regmap *regmap;
+       /*
+        * Protect device internal state between starting a measurement
+        * and reading the result.
+        */
        struct mutex mutex;
        struct iio_mount_matrix orientation;
 
@@ -107,8 +111,8 @@ static int af8133j_product_check(struct af8133j_data *data)
        }
 
        if (val != AF8133J_REG_PCODE_VAL) {
-               dev_err(dev, "Invalid product code (0x%02x)\n", val);
-               return -EINVAL;
+               dev_warn(dev, "Invalid product code (0x%02x)\n", val);
+               return 0; /* Allow unknown ID so fallback compatibles work */
        }
 
        return 0;
@@ -237,8 +241,8 @@ static int af8133j_read_measurement(struct af8133j_data *data, __le16 buf[3])
 }
 
 static const int af8133j_scales[][2] = {
-       [0] = { 0, 366210 }, // 12 gauss
-       [1] = { 0, 671386 }, // 22 gauss
+       [0] = { 0, 366210 }, /* 12 gauss */
+       [1] = { 0, 671386 }, /* 22 gauss */
 };


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

> +
> +struct af8133j_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct mutex mutex;

I thought checkpatch still moaned that every lock should have documentation.
I guess not.  However it's still a nice to have.

Here it seems this is about protecting device state between triggering a
measurement and getting the reading.

> +	struct iio_mount_matrix orientation;
> +
> +	struct gpio_desc *reset_gpiod;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(af8133j_supply_names)];
> +
> +	u8 range;
> +};

> +
> +static int af8133j_product_check(struct af8133j_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> +	if (ret) {
> +		dev_err(dev, "Error reading product code (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	if (val != AF8133J_REG_PCODE_VAL) {
> +		dev_err(dev, "Invalid product code (0x%02x)\n", val);
> +		return -EINVAL;

This should be warn only and we should carry on regardless.  The reason
behind this is to support fallback compatible values in DT to potentially enable
a newer device to be supported on an older kernel.

Many IIO drivers do this wrong as my understanding on what counted on
'compatible' used to be different.  Long discussions on this with the DT
maintainers led me to accept that letting ID checks fail was fine, but
that a message was appropriate.   Often a fail here actually means no device.
We have some exceptions to this rule for devices where we know the same
FW ids are in use in the wild for devices supported by different Linux
drivers - but those are thankfully rare!

> +	}
> +
> +	return 0;
> +}
> +
}

> +static const int af8133j_scales[][2] = {
> +	[0] = { 0, 366210 }, // 12 gauss
> +	[1] = { 0, 671386 }, // 22 gauss
Trivial so I'll fix it up: IIO comments are /* */
not C++ style (with exception of the SPDX stuff that needs to be).
> +};

Re: [PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Ondřej Jirman]

Hello Jonathan,

On Sun, Feb 25, 2024 at 12:07:00PM +0000, Jonathan Cameron wrote:
> On Thu, 22 Feb 2024 02:13:37 +0100
> Ondřej Jirman <megi@xff.cz> wrote:
> 
> > From: Icenowy Zheng <icenowy@aosc.io>
> > 
> > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > 
> > Add a simple IIO driver for it.
> > 
> > Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Check patch correct moaned that Icenowy is the author (from:)
> so doesn't need a co-developed.
> 
> > Signed-off-by: Dalton Durst <dalton@ubports.com>
> > Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> 
> Hi.
> 
> A few really minor things noticed during a final review.
> I'll tweak them whilst applying.  Diff is

Thank you very much for finishing touches.

> > +static int af8133j_product_check(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> > +	if (ret) {
> > +		dev_err(dev, "Error reading product code (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (val != AF8133J_REG_PCODE_VAL) {
> > +		dev_err(dev, "Invalid product code (0x%02x)\n", val);
> > +		return -EINVAL;
> 
> This should be warn only and we should carry on regardless.  The reason
> behind this is to support fallback compatible values in DT to potentially enable
> a newer device to be supported on an older kernel.
> 
> Many IIO drivers do this wrong as my understanding on what counted on
> 'compatible' used to be different.  Long discussions on this with the DT
> maintainers led me to accept that letting ID checks fail was fine, but
> that a message was appropriate.   Often a fail here actually means no device.
> We have some exceptions to this rule for devices where we know the same
> FW ids are in use in the wild for devices supported by different Linux
> drivers - but those are thankfully rare!

Makes sense. If newer device variant has the same register meanings, but just
a different ID register, this way it can be supported without driver
modifications. I'll keep it in mind when doing ID checks in other drivers.

Thanks for all your detailed notes during the review. I learned a few
new subtleties.

Kind regards,
	o.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> }
> 
> > +static const int af8133j_scales[][2] = {
> > +	[0] = { 0, 366210 }, // 12 gauss
> > +	[1] = { 0, 671386 }, // 22 gauss
> Trivial so I'll fix it up: IIO comments are /* */
> not C++ style (with exception of the SPDX stuff that needs to be).
> > +};
> 
> 
> 

Re: [PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer

[Jonathan Cameron]

On Thu, 22 Feb 2024 02:13:37 +0100
Ondřej Jirman <megi@xff.cz> wrote:

> From: Icenowy Zheng <icenowy@aosc.io>
> 
> AF8133J is a simple I2C-connected magnetometer, without interrupts.
> 
> Add a simple IIO driver for it.
> 
> Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Dalton Durst <dalton@ubports.com>
> Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>

One additional tweak from my local build tests.

static

> +const struct dev_pm_ops af8133j_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	RUNTIME_PM_OPS(af8133j_runtime_suspend, af8133j_runtime_resume, NULL)
> +};

[PATCH v4 4/4] MAINTAINERS: Add an entry for AF8133J driver

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

As I am submitting the driver and have the device to test. I'll maintain
the driver.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ed4d3868539..e027d3ec3f8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -579,6 +579,12 @@ F:	drivers/iio/accel/adxl372.c
 F:	drivers/iio/accel/adxl372_i2c.c
 F:	drivers/iio/accel/adxl372_spi.c
 
+AF8133J THREE-AXIS MAGNETOMETER DRIVER
+M:	Ondřej Jirman <megi@xff.cz>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/voltafield,af8133j.yaml
+F:	drivers/iio/magnetometer/af8133j.c
+
 AF9013 MEDIA DRIVER
 L:	linux-media@vger.kernel.org
 S:	Orphan
-- 
2.43.0