[PATCH 0/2] New driver for ams AS5600 Position Sensor

[PATCH 0/2] New driver for ams AS5600 Position Sensor

[Aditya Dutt]

This patch series adds support for the ams AS5600 Position Sensor.

The AS5600 is a Hall-based rotary magnetic position sensor using
planar sensors that convert the magnetic field component perpendicular
to the surface of the chip into a voltage, or a numerical value
available through i2c.

The driver registers the chip as an IIO_ANGL device.
It also exposes the raw registers through debugfs for further configuration.

Datasheet: https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
Co-developed-by: Frank Zago <frank@zago.net> 
Signed-off-by: Frank Zago <frank@zago.net>
Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
---

This patch is based on the work done by Frank Zago:
v2: https://lore.kernel.org/all/20211225175353.4254-1-frank@zago.net/
v1: https://lore.kernel.org/all/20211216202651.120172-1-frank@zago.net/

I have done the changes suggested by Jonathan Cameron in the follow-ups.

I picked this up because there has been no progress on this since 2021 and
Frank Zago has previously stated he isn't trying to upstream his drivers:
https://lore.kernel.org/all/e052d872-6de2-42f4-8b36-d1e2f8359624@zago.net/

Currently, I have not added support for:
- OUT (PWM)
- PGO (GPIO used for OTP)
- DIR (GPIO used for clockwise/anti-clockwise detection)

I have tested this on a Beaglebone Black with as5600 support compiled as a
kernel module (m) as well as in-kernel (y).

changes since Frank Zago's v2:
- direct register access in debugfs is now raw register access without any
  mappings as suggested by Jonathan Cameron
- added device tree support and bindings
- in as5600_probe(), reading ZPOS and MPOS should be a word not a byte
- removed "Read then write" behavior in as5600_reg_access_write() since
  register access is now raw, reading and manipulating the correct bits and
  writing is now the duty of userspace.
- changed "Datasheet" links to the product page instead of the direct PDF
  since the PDF links are not stable


Aditya Dutt (2):
  dt-bindings: iio: position: Add ams AS5600 Position Sensor
  iio: position: Add support for ams AS5600 angle sensor

 .../bindings/iio/position/ams,as5600.yaml     |  42 ++
 Documentation/iio/as5600.rst                  |  84 ++++
 Documentation/iio/index.rst                   |   1 +
 MAINTAINERS                                   |   8 +
 drivers/iio/position/Kconfig                  |  10 +
 drivers/iio/position/Makefile                 |   1 +
 drivers/iio/position/as5600.c                 | 373 ++++++++++++++++++
 7 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
 create mode 100644 Documentation/iio/as5600.rst
 create mode 100644 drivers/iio/position/as5600.c

-- 
2.34.1

[PATCH 1/2] dt-bindings: iio: position: Add ams AS5600 Position Sensor

[Aditya Dutt]

The AS5600 is a Hall-based rotary magnetic position sensor using
planar sensors that convert the magnetic field component perpendicular
to the surface of the chip into a voltage, or a numerical value
available through i2c.

Add dt-bindings for the sensor.

Datasheet: https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
---
 .../bindings/iio/position/ams,as5600.yaml     | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/position/ams,as5600.yaml

diff --git a/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml b/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
new file mode 100644
index 000000000000..d4c92dd41dd6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/position/ams,as5600.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ams AS5600 Position Sensor
+
+maintainers:
+  - Aditya Dutt <duttaditya18@gmail.com>
+
+description: |
+  12-Bit Programmable Contactless Potentiometer
+
+properties:
+  compatible:
+    enum:
+      - ams,as5600
+  reg:
+    maxItems: 1
+    description: |
+      The I2C register address of the device. Typical address for AS5600: 0x36.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ams5600@36 {
+        compatible = "ams,as5600";
+        reg = <0x36>;
+      };
+    };
+
+...
-- 
2.34.1

[PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[Aditya Dutt]

The AS5600 is a Hall-based rotary magnetic position sensor using
planar sensors that convert the magnetic field component perpendicular
to the surface of the chip into a voltage, or a numerical value
available through i2c.

The driver registers the chip as an IIO_ANGL device.
It also exposes the raw registers through debugfs for further configuration.

Datasheet: https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
Co-developed-by: Frank Zago <frank@zago.net>
Signed-off-by: Frank Zago <frank@zago.net>
Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
---
 Documentation/iio/as5600.rst  |  84 ++++++++
 Documentation/iio/index.rst   |   1 +
 MAINTAINERS                   |   8 +
 drivers/iio/position/Kconfig  |  10 +
 drivers/iio/position/Makefile |   1 +
 drivers/iio/position/as5600.c | 373 ++++++++++++++++++++++++++++++++++
 6 files changed, 477 insertions(+)
 create mode 100644 Documentation/iio/as5600.rst
 create mode 100644 drivers/iio/position/as5600.c

diff --git a/Documentation/iio/as5600.rst b/Documentation/iio/as5600.rst
new file mode 100644
index 000000000000..d74c4052e590
--- /dev/null
+++ b/Documentation/iio/as5600.rst
@@ -0,0 +1,84 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+=================
+ams AS5600 driver
+=================
+
+
+Overview
+========
+
+The ams AS5600 is a 12-Bit Programmable Contactless Potentiometer. Its
+i2c address is 0x36.
+
+For more information, see the datasheet at
+
+  https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
+
+
+Channels
+========
+
+The driver provides **two channels**:
+
+- **Channel 0**: raw, unscaled angle measurement
+- **Channel 1**: scaled angle measurement according to the configured
+  ``ZPOS`` / ``MPOS`` range
+
+``ZPOS`` and ``MPOS`` let a user restrict the angle returned, which improves
+the precision returned, since the angle returned is still in the 0 to
+4095 range. The minimal angle recommended is 18 degrees.
+
+The following files are exposed under ``/sys/bus/iio/devices/iio:deviceX``
+where X is the IIO index of the device.
+
++----------------+-------------------------------------------------+
+| File           | Description                                     |
++================+=================================================+
+| in_angl0_raw   | Raw angle measurement                           |
++----------------+-------------------------------------------------+
+| in_angl0_scale | Scale for channel 0                             |
++----------------+-------------------------------------------------+
+| in_angl1_raw   | Adjusted angle measurement, scaled by ZPOS/MPOS |
++----------------+-------------------------------------------------+
+| in_angl1_scale | Scale for channel 1                             |
++----------------+-------------------------------------------------+
+
+
+Accessing the device registers
+==============================
+
+The driver exposes direct register access via debugfs. This allows reading and
+writing I2C registers for debugging or configuration.
+
+Assuming the device is iio:deviceX, its debugfs path will be:
+
+.. code-block:: sh
+
+  $ AS5600=/sys/kernel/debug/iio/iio:deviceX/direct_reg_access
+
+Locate the index of a register to access in the datasheet, then use
+the following commands to read a value:
+
+.. code-block:: sh
+
+  $ echo <reg> > $AS5600/direct_reg_access
+  $ cat $AS5600/direct_reg_access
+
+or this to write a value:
+
+.. code-block:: sh
+
+  $ echo <reg> <value> > $AS5600/direct_reg_access
+
+For instance, this would return the lower byte of RAW ANGLE.
+
+.. code-block:: sh
+
+  $ echo 0x0D > $AS5600/direct_reg_access
+  $ cat $AS5600/direct_reg_access
+
+.. warning::
+
+   Register ``BURN`` (0xFF) permanently modifies device behavior.
+   Use with caution after reading the datasheet carefully.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 315ae37d6fd4..14d097f753f1 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -35,6 +35,7 @@ Industrial I/O Kernel Drivers
    adxl313
    adxl380
    adxl345
+   as5600
    bno055
    ep93xx_adc
    opt4060
diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..ffef001ea7c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1368,6 +1368,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/amphion,vpu.yaml
 F:	drivers/media/platform/amphion/
 
+AMS AS5600 DRIVER
+M:	Aditya Dutt <duttaditya18@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
+F:	Documentation/iio/as5600.rst
+F:	drivers/iio/position/as5600.c
+
 AMS AS73211 DRIVER
 M:	Christian Eggers <ceggers@arri.de>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
index 1576a6380b53..111ed551ae79 100644
--- a/drivers/iio/position/Kconfig
+++ b/drivers/iio/position/Kconfig
@@ -6,6 +6,16 @@
 
 menu "Linear and angular position sensors"
 
+config AS5600
+	tristate "ams AS5600 angular position sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build support for the ams 5600
+	  12-Bit Programmable Contactless Potentiometer.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called as5600.
+
 config IQS624_POS
 	tristate "Azoteq IQS624/625 angular position sensors"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
index d70902f2979d..53930681e6a4 100644
--- a/drivers/iio/position/Makefile
+++ b/drivers/iio/position/Makefile
@@ -4,5 +4,6 @@
 
 # When adding new entries keep the list in alphabetical order
 
+obj-$(CONFIG_AS5600)		+= as5600.o
 obj-$(CONFIG_HID_SENSOR_CUSTOM_INTEL_HINGE) += hid-sensor-custom-intel-hinge.o
 obj-$(CONFIG_IQS624_POS)	+= iqs624-pos.o
diff --git a/drivers/iio/position/as5600.c b/drivers/iio/position/as5600.c
new file mode 100644
index 000000000000..fe716d521548
--- /dev/null
+++ b/drivers/iio/position/as5600.c
@@ -0,0 +1,373 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ams AS5600 -- 12-Bit Programmable Contactless Potentiometer
+ *
+ * Copyright (c) 2021 Frank Zago
+ * Copyright (c) 2025 Aditya Dutt
+ *
+ * datasheet
+ *    https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
+ *
+ * The rotating magnet is installed from 0.5mm to 3mm parallel to and
+ * above the chip.
+ *
+ * The raw angle value returned by the chip is [0..4095]. The channel
+ * 0 (in_angl0_raw) returns the unscaled and unmodified angle, always
+ * covering the 360 degrees. The channel 1 returns the chip adjusted
+ * angle, covering from 18 to 360 degrees, as modified by its
+ * ZPOS/MPOS/MANG values,
+ *
+ * ZPOS and MPOS can be programmed through their debugfs entries. The
+ * MANG register doesn't appear to be programmable without flashing
+ * the chip.
+ *
+ * If the DIR pin is grounded, angles will increase when the magnet is
+ * turned clockwise. If DIR is connected to Vcc, it will be the opposite.
+ *
+ * The i2c address of the device is 0x36.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+
+/* Register definitions */
+#define AS5600_REG_ZMCO              0x00
+#define     AS5600_MASK_ZMCO         GENMASK(1, 0)
+#define AS5600_REG_ZPOS_H            0x01
+#define     AS5600_MASK_ZPOS_H       GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_ZPOS_L            0x02
+#define AS5600_REG_MPOS_H            0x03
+#define     AS5600_MASK_MPOS_H       GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_MPOS_L            0x04
+#define AS5600_REG_MANG_H            0x05
+#define     AS5600_MASK_MANG_H       GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_MANG_L            0x06
+#define AS5600_REG_CONF_H            0x07
+#define     AS5600_MASK_CONF_H       GENMASK(5, 0)
+#define     AS5600_MASK_SF           GENMASK(1, 0)
+#define     AS5600_MASK_FTH          GENMASK(4, 2)
+#define     AS5600_MASK_WD           BIT(5)
+#define AS5600_REG_CONF_L            0x08
+#define     AS5600_MASK_PM           GENMASK(1, 0)
+#define     AS5600_MASK_HYST         GENMASK(3, 2)
+#define     AS5600_MASK_OUTS         GENMASK(5, 4)
+#define     AS5600_MASK_PWMF         GENMASK(7, 6)
+#define AS5600_REG_STATUS            0x0B
+#define     AS5600_MASK_STATUS       GENMASK(5, 3)
+#define     AS5600_MASK_MH           BIT(3)
+#define     AS5600_MASK_ML           BIT(4)
+#define     AS5600_MASK_MD           BIT(5)
+#define AS5600_REG_RAW_ANGLE_H       0x0C
+#define     AS5600_MASK_RAW_ANGLE_H  GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_RAW_ANGLE_L       0x0D
+#define AS5600_REG_ANGLE_H           0x0E
+#define     AS5600_MASK_ANGLE_H      GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_ANGLE_L           0x0F
+#define AS5600_REG_AGC               0x1A
+#define AS5600_REG_MAGN_H            0x1B
+#define     AS5600_MASK_MAGN_H       GENMASK(3, 0) /* bits 11:8 */
+#define AS5600_REG_MAGN_L            0x1C
+#define AS5600_REG_BURN              0xFF
+
+/* Combined 16-bit register addresses for clarity */
+#define AS5600_REG_ZPOS              0x01
+#define AS5600_REG_MPOS              0x03
+#define AS5600_REG_RAW_ANGLE         0x0C
+#define AS5600_REG_ANGLE             0x0E
+
+/* Field masks for the entire 2 byte */
+#define AS5600_FIELD_ZPOS            GENMASK(11, 0)
+#define AS5600_FIELD_MPOS            GENMASK(11, 0)
+#define AS5600_FIELD_RAW_ANGLE       GENMASK(11, 0)
+#define AS5600_FIELD_ANGLE           GENMASK(11, 0)
+
+struct as5600_priv {
+	struct i2c_client *client;
+	struct mutex lock;
+	u16 zpos;
+	u16 mpos;
+};
+
+static int as5600_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct as5600_priv *priv = iio_priv(indio_dev);
+	u16 bitmask;
+	s32 ret;
+	u16 reg;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->channel == 0) {
+			reg = AS5600_REG_RAW_ANGLE;
+			bitmask = AS5600_FIELD_RAW_ANGLE;
+		} else {
+			reg = AS5600_REG_ANGLE;
+			bitmask = AS5600_FIELD_ANGLE;
+		}
+		ret = i2c_smbus_read_word_swapped(priv->client, reg);
+
+		if (ret < 0)
+			return ret;
+		*val = ret & bitmask;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/* Always 4096 steps, but angle range varies between
+		 * 18 and 360 degrees.
+		 */
+		if (chan->channel == 0) {
+			/* Whole angle range = 2*pi / 4096 */
+			*val = 2 * 3141592;
+			*val2 = 4096000000;
+		} else {
+			s32 range;
+
+			/* MPOS - ZPOS defines the active angle selection */
+			/* Partial angle = (range / 4096) * (2*pi / 4096) */
+			mutex_lock(&priv->lock);
+			range = priv->mpos - priv->zpos;
+			mutex_unlock(&priv->lock);
+			if (range <= 0)
+				range += 4096;
+
+			*val = range * 2 * 314159;
+			*val /= 4096;
+			*val2 = 409600000;
+		}
+
+		return IIO_VAL_FRACTIONAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t as5600_reg_access_read(struct as5600_priv *priv,
+				      unsigned int reg, unsigned int *val)
+{
+	int ret;
+	u8 mask;
+
+	switch (reg) {
+	case AS5600_REG_ZMCO:
+		mask = AS5600_MASK_ZMCO;
+		break;
+	case AS5600_REG_ZPOS_H:
+		mask = AS5600_MASK_ZPOS_H;
+		break;
+	case AS5600_REG_MPOS_H:
+		mask = AS5600_MASK_MPOS_H;
+		break;
+	case AS5600_REG_MANG_H:
+		mask = AS5600_MASK_MANG_H;
+		break;
+	case AS5600_REG_CONF_H:
+		mask = AS5600_MASK_CONF_H;
+		break;
+	case AS5600_REG_STATUS:
+		mask = AS5600_MASK_STATUS;
+		break;
+	case AS5600_REG_RAW_ANGLE_H:
+		mask = AS5600_MASK_RAW_ANGLE_H;
+		break;
+	case AS5600_REG_ANGLE_H:
+		mask = AS5600_MASK_ANGLE_H;
+		break;
+	case AS5600_REG_MAGN_H:
+		mask = AS5600_MASK_MAGN_H;
+		break;
+	case AS5600_REG_ZPOS_L:
+	case AS5600_REG_MPOS_L:
+	case AS5600_REG_MANG_L:
+	case AS5600_REG_CONF_L:
+	case AS5600_REG_RAW_ANGLE_L:
+	case AS5600_REG_ANGLE_L:
+	case AS5600_REG_AGC:
+	case AS5600_REG_MAGN_L:
+		mask = 0xFF;
+		break;
+	default:
+		/* Not a readable register */
+		return -EINVAL;
+	}
+
+
+	ret = i2c_smbus_read_byte_data(priv->client, reg);
+	if (ret < 0)
+		return ret;
+
+	/* because the chip may return garbage data in the unused bits */
+	*val = ret & mask;
+	return 0;
+}
+
+static ssize_t as5600_reg_access_write(struct as5600_priv *priv,
+				       unsigned int reg, unsigned int writeval)
+{
+	int ret;
+	u8 mask;
+
+	if (writeval > 0xFF)
+		return -EINVAL;
+
+	switch (reg) {
+	case AS5600_REG_ZPOS_H:
+		mask = AS5600_MASK_ZPOS_H;
+		break;
+	case AS5600_REG_MPOS_H:
+		mask = AS5600_MASK_MPOS_H;
+		break;
+	case AS5600_REG_MANG_H:
+		mask = AS5600_MASK_MANG_H;
+		break;
+	case AS5600_REG_CONF_H:
+		mask = AS5600_MASK_CONF_H;
+		break;
+	case AS5600_REG_ZPOS_L:
+	case AS5600_REG_MPOS_L:
+	case AS5600_REG_MANG_L:
+	case AS5600_REG_CONF_L:
+	case AS5600_REG_BURN:
+		mask = 0xFF;
+		break;
+	default:
+		/* Not a writable register */
+		return -EINVAL;
+	}
+
+	ret = i2c_smbus_write_byte_data(priv->client, reg, writeval & mask);
+	if (ret < 0)
+		return ret;
+
+	/* update priv->zpos and priv->mpos */
+	mutex_lock(&priv->lock);
+	switch (reg) {
+	case AS5600_REG_ZPOS_H:
+		priv->zpos = (priv->zpos & 0x00FF) | ((writeval & mask) << 8);
+		break;
+	case AS5600_REG_ZPOS_L:
+		priv->zpos = (priv->zpos & 0xFF00) | (writeval & mask);
+		break;
+	case AS5600_REG_MPOS_H:
+		priv->mpos = (priv->mpos & 0x00FF) | ((writeval & mask) << 8);
+		break;
+	case AS5600_REG_MPOS_L:
+		priv->mpos = (priv->mpos & 0xFF00) | (writeval & mask);
+		break;
+	}
+	mutex_unlock(&priv->lock);
+	return 0;
+}
+
+static int as5600_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	struct as5600_priv *priv = iio_priv(indio_dev);
+	int ret;
+
+	if (readval) {
+		ret = as5600_reg_access_read(priv, reg, readval);
+	} else {
+		ret = as5600_reg_access_write(priv, reg, writeval);
+	}
+
+	return ret;
+}
+
+static const struct iio_chan_spec as5600_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = 0,
+	},
+	{
+		.type = IIO_ANGL,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = 1,
+	},
+};
+
+static const struct iio_info as5600_info = {
+	.read_raw = &as5600_read_raw,
+	.debugfs_reg_access = &as5600_reg_access,
+};
+
+static int as5600_probe(struct i2c_client *client)
+{
+	struct as5600_priv *priv;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	priv = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	priv->client = client;
+	mutex_init(&priv->lock);
+
+	indio_dev->info = &as5600_info;
+	indio_dev->name = "as5600";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = as5600_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as5600_channels);
+
+	ret = i2c_smbus_read_byte_data(client, AS5600_REG_STATUS);
+	if (ret < 0)
+		return ret;
+
+	/* No magnet present could be a problem. */
+	if ((ret & AS5600_MASK_MD) == 0)
+		dev_warn(&client->dev, "Magnet not detected\n");
+
+	ret = i2c_smbus_read_word_swapped(client, AS5600_REG_ZPOS);
+	if (ret < 0)
+		return ret;
+	priv->zpos = ret & AS5600_FIELD_ZPOS;
+
+	ret = i2c_smbus_read_word_swapped(client, AS5600_REG_MPOS);
+	if (ret < 0)
+		return ret;
+	priv->mpos = ret & AS5600_FIELD_MPOS;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id as5600_id[] = {
+	{ "as5600" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, as5600_id);
+
+static const struct of_device_id as5600_match[] = {
+	{ .compatible = "ams,as5600" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, as5600_match);
+
+static struct i2c_driver as5600_driver = {
+	.driver = {
+		.name = "as5600",
+		.of_match_table = as5600_match,
+	},
+	.probe = as5600_probe,
+	.id_table   = as5600_id,
+};
+
+module_i2c_driver(as5600_driver);
+
+MODULE_AUTHOR("Frank Zago <frank@zago.net>");
+MODULE_AUTHOR("Aditya Dutt <duttaditya18@gmail.com>");
+MODULE_DESCRIPTION("ams AS5600 Position Sensor");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[Frank Zago]

Hello,

Thanks for updating that driver and making the required changes. I tried to go back to it a few times but motivation was lost. 

The commit message should say "position sensor", not "angle sensor", for consistency.


> diff --git a/drivers/iio/position/as5600.c b/drivers/iio/position/as5600.c
> new file mode 100644
> index 000000000000..fe716d521548
> --- /dev/null
> +++ b/drivers/iio/position/as5600.c
...
> +	case AS5600_REG_RAW_ANGLE_L:
> +	case AS5600_REG_ANGLE_L:
> +	case AS5600_REG_AGC:
> +	case AS5600_REG_MAGN_L:
> +		mask = 0xFF;
> +		break;
> +	default:
> +		/* Not a readable register */
> +		return -EINVAL;
> +	}
> +
> +

extra empty line


I believe you could add this to the second patch:
Acked-by: Frank Zago <frank@zago.net>

Best regards,
  Frank.

Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[kernel test robot]

Hi Aditya,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.18-rc2 next-20251021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Aditya-Dutt/dt-bindings-iio-position-Add-ams-AS5600-Position-Sensor/20251021-042001
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20251020201653.86181-3-duttaditya18%40gmail.com
patch subject: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20251021/202510211921.F1RPF3gj-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 754ebc6ebb9fb9fbee7aef33478c74ea74949853)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251021/202510211921.F1RPF3gj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510211921.F1RPF3gj-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/position/as5600.c:127:12: warning: implicit conversion from 'long' to 'int' changes value from 4096000000 to -198967296 [-Wconstant-conversion]
     127 |                         *val2 = 4096000000;
         |                               ~ ^~~~~~~~~~
   1 warning generated.


vim +127 drivers/iio/position/as5600.c

    93	
    94	static int as5600_read_raw(struct iio_dev *indio_dev,
    95				   struct iio_chan_spec const *chan,
    96				   int *val, int *val2, long mask)
    97	{
    98		struct as5600_priv *priv = iio_priv(indio_dev);
    99		u16 bitmask;
   100		s32 ret;
   101		u16 reg;
   102	
   103		switch (mask) {
   104		case IIO_CHAN_INFO_RAW:
   105			if (chan->channel == 0) {
   106				reg = AS5600_REG_RAW_ANGLE;
   107				bitmask = AS5600_FIELD_RAW_ANGLE;
   108			} else {
   109				reg = AS5600_REG_ANGLE;
   110				bitmask = AS5600_FIELD_ANGLE;
   111			}
   112			ret = i2c_smbus_read_word_swapped(priv->client, reg);
   113	
   114			if (ret < 0)
   115				return ret;
   116			*val = ret & bitmask;
   117	
   118			return IIO_VAL_INT;
   119	
   120		case IIO_CHAN_INFO_SCALE:
   121			/* Always 4096 steps, but angle range varies between
   122			 * 18 and 360 degrees.
   123			 */
   124			if (chan->channel == 0) {
   125				/* Whole angle range = 2*pi / 4096 */
   126				*val = 2 * 3141592;
 > 127				*val2 = 4096000000;
   128			} else {
   129				s32 range;
   130	
   131				/* MPOS - ZPOS defines the active angle selection */
   132				/* Partial angle = (range / 4096) * (2*pi / 4096) */
   133				mutex_lock(&priv->lock);
   134				range = priv->mpos - priv->zpos;
   135				mutex_unlock(&priv->lock);
   136				if (range <= 0)
   137					range += 4096;
   138	
   139				*val = range * 2 * 314159;
   140				*val /= 4096;
   141				*val2 = 409600000;
   142			}
   143	
   144			return IIO_VAL_FRACTIONAL;
   145	
   146		default:
   147			return -EINVAL;
   148		}
   149	}
   150	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[kernel test robot]

Hi Aditya,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.18-rc2 next-20251021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Aditya-Dutt/dt-bindings-iio-position-Add-ams-AS5600-Position-Sensor/20251021-042001
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20251020201653.86181-3-duttaditya18%40gmail.com
patch subject: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor
config: powerpc-randconfig-r073-20251021 (https://download.01.org/0day-ci/archive/20251021/202510211910.UK1wOVjv-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251021/202510211910.UK1wOVjv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510211910.UK1wOVjv-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/position/as5600.c:127:12: warning: implicit conversion from 'long long' to 'int' changes value from 4096000000 to -198967296 [-Wconstant-conversion]
                           *val2 = 4096000000;
                                 ~ ^~~~~~~~~~
   1 warning generated.


vim +127 drivers/iio/position/as5600.c

    93	
    94	static int as5600_read_raw(struct iio_dev *indio_dev,
    95				   struct iio_chan_spec const *chan,
    96				   int *val, int *val2, long mask)
    97	{
    98		struct as5600_priv *priv = iio_priv(indio_dev);
    99		u16 bitmask;
   100		s32 ret;
   101		u16 reg;
   102	
   103		switch (mask) {
   104		case IIO_CHAN_INFO_RAW:
   105			if (chan->channel == 0) {
   106				reg = AS5600_REG_RAW_ANGLE;
   107				bitmask = AS5600_FIELD_RAW_ANGLE;
   108			} else {
   109				reg = AS5600_REG_ANGLE;
   110				bitmask = AS5600_FIELD_ANGLE;
   111			}
   112			ret = i2c_smbus_read_word_swapped(priv->client, reg);
   113	
   114			if (ret < 0)
   115				return ret;
   116			*val = ret & bitmask;
   117	
   118			return IIO_VAL_INT;
   119	
   120		case IIO_CHAN_INFO_SCALE:
   121			/* Always 4096 steps, but angle range varies between
   122			 * 18 and 360 degrees.
   123			 */
   124			if (chan->channel == 0) {
   125				/* Whole angle range = 2*pi / 4096 */
   126				*val = 2 * 3141592;
 > 127				*val2 = 4096000000;
   128			} else {
   129				s32 range;
   130	
   131				/* MPOS - ZPOS defines the active angle selection */
   132				/* Partial angle = (range / 4096) * (2*pi / 4096) */
   133				mutex_lock(&priv->lock);
   134				range = priv->mpos - priv->zpos;
   135				mutex_unlock(&priv->lock);
   136				if (range <= 0)
   137					range += 4096;
   138	
   139				*val = range * 2 * 314159;
   140				*val /= 4096;
   141				*val2 = 409600000;
   142			}
   143	
   144			return IIO_VAL_FRACTIONAL;
   145	
   146		default:
   147			return -EINVAL;
   148		}
   149	}
   150	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] dt-bindings: iio: position: Add ams AS5600 Position Sensor

[Conor Dooley]

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

On Tue, Oct 21, 2025 at 01:46:52AM +0530, Aditya Dutt wrote:
> The AS5600 is a Hall-based rotary magnetic position sensor using
> planar sensors that convert the magnetic field component perpendicular
> to the surface of the chip into a voltage, or a numerical value
> available through i2c.
> 
> Add dt-bindings for the sensor.
> 
> Datasheet: https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor

Looks like this device has two supplies, could you document those
please?

Additionally, this "PGO" pin - is it necessary for a driver to know what
state it is in to function correctly? If so, can the information about
which state it is in be gathered from i2c?
Should there also be an optional pgo-gpios property for the scenario
where it is not tied, but set by a GPIO, if that is even possible.

> Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>
> ---
>  .../bindings/iio/position/ams,as5600.yaml     | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml b/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
> new file mode 100644
> index 000000000000..d4c92dd41dd6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/position/ams,as5600.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/position/ams,as5600.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ams AS5600 Position Sensor
> +
> +maintainers:
> +  - Aditya Dutt <duttaditya18@gmail.com>
> +
> +description: |
> +  12-Bit Programmable Contactless Potentiometer
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ams,as5600

blank line here please.

> +  reg:
> +    maxItems: 1
> +    description: |
> +      The I2C register address of the device. Typical address for AS5600: 0x36.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ams5600@36 {

potentiometer@36

pw-bot: changes-requested

Cheers,
Conor.

> +        compatible = "ams,as5600";
> +        reg = <0x36>;
> +      };
> +    };
> +
> +...
> -- 
> 2.34.1
> 

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

Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[Jonathan Cameron]

On Tue, 21 Oct 2025 01:46:53 +0530
Aditya Dutt <duttaditya18@gmail.com> wrote:

> The AS5600 is a Hall-based rotary magnetic position sensor using
> planar sensors that convert the magnetic field component perpendicular
> to the surface of the chip into a voltage, or a numerical value
> available through i2c.
> 
> The driver registers the chip as an IIO_ANGL device.
> It also exposes the raw registers through debugfs for further configuration.
> 
> Datasheet: https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
> Co-developed-by: Frank Zago <frank@zago.net>
> Signed-off-by: Frank Zago <frank@zago.net>
> Signed-off-by: Aditya Dutt <duttaditya18@gmail.com>

Hi Aditya, Great to see this driver moving forwards.

A few comments inline on specifics but one thing I'm not sure on is whether
it is actually useful to expose the raw angle channel in real applications.
I'd kind of assume the limits etc would be programmed for a given physical
configuration so it might be sensible to just expose that scaled channel
instead?

Jonathan

> ---
>  Documentation/iio/as5600.rst  |  84 ++++++++
>  Documentation/iio/index.rst   |   1 +
>  MAINTAINERS                   |   8 +
>  drivers/iio/position/Kconfig  |  10 +
>  drivers/iio/position/Makefile |   1 +
>  drivers/iio/position/as5600.c | 373 ++++++++++++++++++++++++++++++++++
>  6 files changed, 477 insertions(+)
>  create mode 100644 Documentation/iio/as5600.rst
>  create mode 100644 drivers/iio/position/as5600.c
> 
> diff --git a/Documentation/iio/as5600.rst b/Documentation/iio/as5600.rst
> new file mode 100644
> index 000000000000..d74c4052e590
> --- /dev/null
> +++ b/Documentation/iio/as5600.rst
> @@ -0,0 +1,84 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +=================
> +ams AS5600 driver
> +=================
> +
> +
One blank line is probably enough for readability.

> +Overview
> +========
> +
> +The ams AS5600 is a 12-Bit Programmable Contactless Potentiometer. Its
> +i2c address is 0x36.
> +
> +For more information, see the datasheet at
> +
> +  https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
> +
One blank line here as well.

> +
> +Channels
> +========
> +
> +The driver provides **two channels**:
> +
> +- **Channel 0**: raw, unscaled angle measurement
> +- **Channel 1**: scaled angle measurement according to the configured
> +  ``ZPOS`` / ``MPOS`` range
> +
> +``ZPOS`` and ``MPOS`` let a user restrict the angle returned, which improves
> +the precision returned, since the angle returned is still in the 0 to
> +4095 range. The minimal angle recommended is 18 degrees.
> +
> +The following files are exposed under ``/sys/bus/iio/devices/iio:deviceX``
> +where X is the IIO index of the device.
> +
> ++----------------+-------------------------------------------------+
> +| File           | Description                                     |
> ++================+=================================================+
> +| in_angl0_raw   | Raw angle measurement                           |
> ++----------------+-------------------------------------------------+
> +| in_angl0_scale | Scale for channel 0                             |
> ++----------------+-------------------------------------------------+
> +| in_angl1_raw   | Adjusted angle measurement, scaled by ZPOS/MPOS |
> ++----------------+-------------------------------------------------+
> +| in_angl1_scale | Scale for channel 1                             |
> ++----------------+-------------------------------------------------+
> +
> +
> +Accessing the device registers
> +==============================
> +
> +The driver exposes direct register access via debugfs. This allows reading and
> +writing I2C registers for debugging or configuration.
> +
> +Assuming the device is iio:deviceX, its debugfs path will be:
> +
> +.. code-block:: sh
> +
> +  $ AS5600=/sys/kernel/debug/iio/iio:deviceX/direct_reg_access
> +
> +Locate the index of a register to access in the datasheet, then use
> +the following commands to read a value:
> +
> +.. code-block:: sh
> +
> +  $ echo <reg> > $AS5600/direct_reg_access
> +  $ cat $AS5600/direct_reg_access
> +
> +or this to write a value:
> +
> +.. code-block:: sh
> +
> +  $ echo <reg> <value> > $AS5600/direct_reg_access
> +
> +For instance, this would return the lower byte of RAW ANGLE.
> +
> +.. code-block:: sh
> +
> +  $ echo 0x0D > $AS5600/direct_reg_access
> +  $ cat $AS5600/direct_reg_access
> +
> +.. warning::
> +
> +   Register ``BURN`` (0xFF) permanently modifies device behavior.
> +   Use with caution after reading the datasheet carefully.

Great to have this documentation of useful stuff for a user. Thanks for
including it!
> diff --git a/drivers/iio/position/as5600.c b/drivers/iio/position/as5600.c
> new file mode 100644
> index 000000000000..fe716d521548
> --- /dev/null
> +++ b/drivers/iio/position/as5600.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ams AS5600 -- 12-Bit Programmable Contactless Potentiometer
> + *
> + * Copyright (c) 2021 Frank Zago
> + * Copyright (c) 2025 Aditya Dutt
> + *
> + * datasheet
> + *    https://ams-osram.com/products/sensor-solutions/position-sensors/ams-as5600-position-sensor
> + *
> + * The rotating magnet is installed from 0.5mm to 3mm parallel to and
> + * above the chip.
> + *
> + * The raw angle value returned by the chip is [0..4095]. The channel
> + * 0 (in_angl0_raw) returns the unscaled and unmodified angle, always
> + * covering the 360 degrees. The channel 1 returns the chip adjusted
> + * angle, covering from 18 to 360 degrees, as modified by its
> + * ZPOS/MPOS/MANG values,

I raise the question below on whether there is value in exposing the raw
angle.  Is userspace ever going to use it?
> + *
> + * ZPOS and MPOS can be programmed through their debugfs entries. The
> + * MANG register doesn't appear to be programmable without flashing
> + * the chip.
Whilst true that you can program them I'd not mention it explicitly. That's
kind of advanced usage that requires someone to really know what they are
doing.

> + *
> + * If the DIR pin is grounded, angles will increase when the magnet is
> + * turned clockwise. If DIR is connected to Vcc, it will be the opposite.
We need a way to know that state I think. So belongs in DT. 
We've had some recent discussions on how to handle tied GPIOs on devices
but for now you'll have to do a property for this.

> + *
> + * The i2c address of the device is 0x36.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>

Follow include what you use principles (mainly - there are a few headers
where we can always assume they should include via another).
e.g. mutex.h should be here.

> +
> +/* Register definitions */
> +#define AS5600_REG_ZMCO              0x00
> +#define     AS5600_MASK_ZMCO         GENMASK(1, 0)
> +#define AS5600_REG_ZPOS_H            0x01
> +#define     AS5600_MASK_ZPOS_H       GENMASK(3, 0) /* bits 11:8 */
Some of these will go away with simplified (less protective) debugfs
interfaces.  Probably you only need to keep the base register and the
mask of the whole thing that you have below.

> +#define AS5600_REG_ZPOS_L            0x02
> +#define AS5600_REG_MPOS_H            0x03
> +#define     AS5600_MASK_MPOS_H       GENMASK(3, 0) /* bits 11:8 */
> +#define AS5600_REG_MPOS_L            0x04
> +#define AS5600_REG_MANG_H            0x05
> +#define     AS5600_MASK_MANG_H       GENMASK(3, 0) /* bits 11:8 */
> +#define AS5600_REG_MANG_L            0x06
> +#define AS5600_REG_CONF_H            0x07
> +#define     AS5600_MASK_CONF_H       GENMASK(5, 0)
> +#define     AS5600_MASK_SF           GENMASK(1, 0)
> +#define     AS5600_MASK_FTH          GENMASK(4, 2)
> +#define     AS5600_MASK_WD           BIT(5)
> +#define AS5600_REG_CONF_L            0x08
> +#define     AS5600_MASK_PM           GENMASK(1, 0)
> +#define     AS5600_MASK_HYST         GENMASK(3, 2)
> +#define     AS5600_MASK_OUTS         GENMASK(5, 4)
> +#define     AS5600_MASK_PWMF         GENMASK(7, 6)
> +#define AS5600_REG_STATUS            0x0B
> +#define     AS5600_MASK_STATUS       GENMASK(5, 3)
These masks will probably go away anyway with simplified debugfs, but
if you had them, they should be built up from the sub fields that follow.

> +#define     AS5600_MASK_MH           BIT(3)
Make the field names include which register they are in. e.g.
#define		AS6500_STATUS_MH
No need to mention mask in a single bit, but do include that
or multi bit fields.


> +#define     AS5600_MASK_ML           BIT(4)
> +#define     AS5600_MASK_MD           BIT(5)
> +#define AS5600_REG_RAW_ANGLE_H       0x0C
> +#define     AS5600_MASK_RAW_ANGLE_H  GENMASK(3, 0) /* bits 11:8 */
> +#define AS5600_REG_RAW_ANGLE_L       0x0D
> +#define AS5600_REG_ANGLE_H           0x0E
> +#define     AS5600_MASK_ANGLE_H      GENMASK(3, 0) /* bits 11:8 */
> +#define AS5600_REG_ANGLE_L           0x0F
> +#define AS5600_REG_AGC               0x1A
> +#define AS5600_REG_MAGN_H            0x1B
> +#define     AS5600_MASK_MAGN_H       GENMASK(3, 0) /* bits 11:8 */
> +#define AS5600_REG_MAGN_L            0x1C
> +#define AS5600_REG_BURN              0xFF
> +
> +/* Combined 16-bit register addresses for clarity */

Don't bother with this.  Just use the starting address defined
above for the calls.  These to my mind just confuse things
by implying there are 16 bit registers when there aren't.

> +#define AS5600_REG_ZPOS              0x01
> +#define AS5600_REG_MPOS              0x03
> +#define AS5600_REG_RAW_ANGLE         0x0C
> +#define AS5600_REG_ANGLE             0x0E
> +
> +/* Field masks for the entire 2 byte */

As above, these are the only ones you should need for these
(after stopping masking in debugfs interfaces).

> +#define AS5600_FIELD_ZPOS            GENMASK(11, 0)
> +#define AS5600_FIELD_MPOS            GENMASK(11, 0)
> +#define AS5600_FIELD_RAW_ANGLE       GENMASK(11, 0)
> +#define AS5600_FIELD_ANGLE           GENMASK(11, 0)
> +
> +struct as5600_priv {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	u16 zpos;
> +	u16 mpos;
> +};
> +
> +static int as5600_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct as5600_priv *priv = iio_priv(indio_dev);
> +	u16 bitmask;
> +	s32 ret;
> +	u16 reg;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->channel == 0) {
> +			reg = AS5600_REG_RAW_ANGLE;
> +			bitmask = AS5600_FIELD_RAW_ANGLE;

What is the usecase for exposing this RAW angle?

We often decide not to expose channels if they are just there
for debug and aren't useful to userspace code. I'm not sure if
that is true here or not.

> +		} else {
> +			reg = AS5600_REG_ANGLE;
> +			bitmask = AS5600_FIELD_ANGLE;
> +		}
> +		ret = i2c_smbus_read_word_swapped(priv->client, reg);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = ret & bitmask;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Always 4096 steps, but angle range varies between
Wrap comments at 80 chars, not less than 70. Also
		/*
		 * Always 4096 ...
		 * ...
		 */

For multiline comments in IIO (and most of the rest of the kernel).

> +		 * 18 and 360 degrees.
> +		 */
> +		if (chan->channel == 0) {
> +			/* Whole angle range = 2*pi / 4096 */
> +			*val = 2 * 3141592;
> +			*val2 = 4096000000;
> +		} else {
> +			s32 range;
> +
> +			/* MPOS - ZPOS defines the active angle selection */
> +			/* Partial angle = (range / 4096) * (2*pi / 4096) */
Use multi line comment syntax for htis.
> +			mutex_lock(&priv->lock);
> +			range = priv->mpos - priv->zpos;

As mentioned below. I think reading these back form the device here makes more
sense than caching them.  This is not normally a fast path as typically a userspace
program only does this occasionally.


> +			mutex_unlock(&priv->lock);
> +			if (range <= 0)
> +				range += 4096;
> +
> +			*val = range * 2 * 314159;
> +			*val /= 4096;

Why not push that 4096 into val2? Overflow or something else?  If its overflow
state that here and provide a little info on why.

> +			*val2 = 409600000;
> +		}
> +
> +		return IIO_VAL_FRACTIONAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t as5600_reg_access_read( as5600_reg_access_readstruct as5600_priv *priv,
> +				      unsigned int reg, unsigned int *val)
> +{
> +	int ret;
> +	u8 mask;
> +
> +	switch (reg) {
> +	case AS5600_REG_ZMCO:
> +		mask = AS5600_MASK_ZMCO;
> +		break;
> +	case AS5600_REG_ZPOS_H:
> +		mask = AS5600_MASK_ZPOS_H;
> +		break;
> +	case AS5600_REG_MPOS_H:
> +		mask = AS5600_MASK_MPOS_H;
> +		break;
> +	case AS5600_REG_MANG_H:
> +		mask = AS5600_MASK_MANG_H;
> +		break;
> +	case AS5600_REG_CONF_H:
> +		mask = AS5600_MASK_CONF_H;
> +		break;
> +	case AS5600_REG_STATUS:
> +		mask = AAS5600_MASK_STATUSS5600_MASK_STATUS;
> +		break;
> +	case AS5600_REG_RAW_ANGLE_H:
> +		mask = AS5600_MASK_RAW_ANGLE_H;
> +		break;
> +	case AS5600_REG_ANGLE_H:
> +		mask = AS5600_MASK_ANGLE_H;
> +		break;
> +	case AS5600_REG_MAGN_H:
> +		mask = AS5600_MASK_MAGN_H;
> +		break;
> +	case AS5600_REG_ZPOS_L:
> +	case AS5600_REG_MPOS_L:
> +	case AS5600_REG_MANG_L:
> +	case AS5600_REG_CONF_L:
> +	case AS5600_REG_RAW_ANGLE_L:
> +	case AS5600_REG_ANGLE_L:
> +	case AS5600_REG_AGC:
> +	case AS5600_REG_MAGN_L:
> +		mask = 0xFF;
> +		break;
> +	default:
> +		/* Not a readable register */
> +		return -EINVAL;
> +	}
> +
> +
> +	ret = i2c_smbus_read_byte_data(priv->client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* because the chip may return garbage data in the unused bits */
> +	*val = ret & mask;

It's a debug interface only.  I don't think we care if unused bits
are garbage.  More to the point they almost certainly are not. They
are just undocumented. Potentially tied to 0 by maybe not.


> +	return 0;
> +}
> +
> +static ssize_t as5600_reg_access_write(struct as5600_priv *priv,
> +				       unsigned int reg, unsigned int writeval)
> +{
> +	int ret;
> +	u8 mask;
> +
> +	if (writeval > 0xFF)
> +		return -EINVAL;
> +
> +	switch (reg) {
> +	case AS5600_REG_ZPOS_H:
> +		mask = AS5600_MASK_ZPOS_H;

For debug write, this isn't our problem.  Also it is entirely possible someone
wants to write to bits that aren't documented and we shouldn't prevent that.

Debug interfaces let you poke things that are dangerous.


> +		break;
> +	case AS5600_REG_MPOS_H:
> +		mask = AS5600_MASK_MPOS_H;
> +		break;
> +	case AS5600_REG_MANG_H:
> +		mask = AS5600_MASK_MANG_H;
> +		break;
> +	case AS5600_REG_CONF_H:
> +		mask = AS5600_MASK_CONF_H;
> +		break;
> +	case AS5600_REG_ZPOS_L:
> +	case AS5600_REG_MPOS_L:
> +	case AS5600_REG_MANG_L:
> +	case AS5600_REG_CONF_L:
> +	case AS5600_REG_BURN:
> +		mask = 0xFF;
> +		break;
> +	default:
> +		/* Not a writable register */
> +		return -EINVAL;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(priv->client, reg, writeval & mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* update priv->zpos and priv->mpos */
> +	mutex_lock(&priv->lock);
guard(mutex)(&priv->lock);
simplifies this a little. Remember to include linux/cleanup.h as well as mutex.h for
that.

> +	switch (reg) {
> +	case AS5600_REG_ZPOS_H:
> +		priv->zpos = (priv->zpos & 0x00FF) | ((writeval & mask) << 8);

This caching in driver seems like potential overkill for the relatively slow
path places I think it is read back. Can you just query the hardware for these?
Of if you need to cache it consider regmap/ regcache for doing so.

> +		break;
> +	case AS5600_REG_ZPOS_L:
> +		priv->zpos = (priv->zpos & 0xFF00) | (writeval & mask);
> +		break;
> +	case AS5600_REG_MPOS_H:
> +		priv->mpos = (priv->mpos & 0x00FF) | ((writeval & mask) << 8);
> +		break;
> +	case AS5600_REG_MPOS_L:
> +		priv->mpos = (priv->mpos & 0xFF00) | (writeval & mask);
> +		break;
> +	}
> +	mutex_unlock(&priv->lock);
with guard() this goes).

> +	return 0;
> +}
> +
> +static int as5600_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	struct as5600_priv *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (readval) {
> +		ret = as5600_reg_access_read(priv, reg, readval);

Might as well return rather than having a local variable.

> +	} else {
> +		ret = as5600_reg_access_write(priv, reg, writeval);
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_chan_spec as5600_channels[] = {
> +	{
> +		.type = IIO_ANGL,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
Trivial but nice to align as.
		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
				      BIT(IIO_CHAN_INFO_SCALE),

as helps readability a tiny bit.

> +		.indexed = 1,
> +		.channel = 0,
> +	},
> +	{
> +		.type = IIO_ANGL,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
Same here.

> +		.indexed = 1,
> +		.channel = 1,
> +	},
> +};

> +
> +static int as5600_probe(struct i2c_client *client)
> +{
> +	struct as5600_priv *priv;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*priv));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

I don't think this is every used.  If not drop setting it.

> +	priv->client = client;
> +	mutex_init(&priv->lock);
For new code please use
	ret = devm_mutex_init(&priv->lock);
	if (ret)
		return ret;

Very small advantage if particular lock debugging is turned on.
It used to not be worth the effort of calling mutex_destroy() but now
there is this devm_ initializer it adds so little complexity we might
as well enable that.

> +
> +	indio_dev->info = &as5600_info;
> +	indio_dev->name = "as5600";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = as5600_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(as5600_channels);
> +
> +	ret = i2c_smbus_read_byte_data(client, AS5600_REG_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* No magnet present could be a problem. */

Seems a bit more than could be.  Does it make sense to carry on if this
happens?  I guess maybe there are physical systems in which the magnet
is only sometime present? If there are then the warning is going to be
a bit noisy.


> +	if ((ret & AS5600_MASK_MD) == 0)
> +		dev_warn(&client->dev, "Magnet not detected\n");
> +
> +	ret = i2c_smbus_read_word_swapped(client, AS5600_REG_ZPOS);
> +	if (ret < 0)
> +		return ret;
> +	priv->zpos = ret & AS5600_FIELD_ZPOS;
Prefer FIELD_GET() for this as then I don't need to check it is in lowest
bits.
> +
> +	ret = i2c_smbus_read_word_swapped(client, AS5600_REG_MPOS);
> +	if (ret < 0)
> +		return ret;
> +	priv->mpos = ret & AS5600_FIELD_MPOS;
FIELD_GET() here as well.

> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id as5600_id[] = {
> +	{ "as5600" },
> +	{}
	{ }

To be consistent and in general this is the standard formatting I'm
trying to encourage in IIO.

> +};
> +MODULE_DEVICE_TABLE(i2c, as5600_id);
> +
> +static const struct of_device_id as5600_match[] = {
> +	{ .compatible = "ams,as5600" },
> +	{ },
No trailing comma for these 'terminating' entries as nothing
should ever come after them.
> +};
> +MODULE_DEVICE_TABLE(of, as5600_match);
> +
> +static struct i2c_driver as5600_driver = {
> +	.driver = {
> +		.name = "as5600",
> +		.of_match_table = as5600_match,
> +	},
> +	.probe = as5600_probe,
> +	.id_table   = as5600_id,

I'd prefer just using a single space before = as it is much
easier to be consistent and void weird looking formatting like
this.

> +};
> +
> +module_i2c_driver(as5600_driver);
> +
> +MODULE_AUTHOR("Frank Zago <frank@zago.net>");
> +MODULE_AUTHOR("Aditya Dutt <duttaditya18@gmail.com>");
> +MODULE_DESCRIPTION("ams AS5600 Position Sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 07:16:27PM +0100, Jonathan Cameron wrote:
> On Tue, 21 Oct 2025 01:46:53 +0530
> Aditya Dutt <duttaditya18@gmail.com> wrote:

...

> > +		if (chan->channel == 0) {
> > +			/* Whole angle range = 2*pi / 4096 */
> > +			*val = 2 * 3141592;

Can you, please, add a definition of PI * 10^6 to units.h? We have already
users of this value and of the PI * 10^5.

...

> > +			/* Partial angle = (range / 4096) * (2*pi / 4096) */
> Use multi line comment syntax for htis.

Also you may use Greek PI (as unicode character) in the comments and messages.
We've been living decades in the unicode time!

-- 
With Best Regards,
Andy Shevchenko