* [PATCH 0/2] New driver for ams AS5600 Position Sensor @ 2025-10-20 20:16 Aditya Dutt 2025-10-20 20:16 ` [PATCH 1/2] dt-bindings: iio: position: Add " Aditya Dutt 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt 0 siblings, 2 replies; 9+ messages in thread From: Aditya Dutt @ 2025-10-20 20:16 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago Cc: Aditya Dutt, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: iio: position: Add ams AS5600 Position Sensor 2025-10-20 20:16 [PATCH 0/2] New driver for ams AS5600 Position Sensor Aditya Dutt @ 2025-10-20 20:16 ` Aditya Dutt 2025-10-22 17:50 ` Conor Dooley 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt 1 sibling, 1 reply; 9+ messages in thread From: Aditya Dutt @ 2025-10-20 20:16 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago Cc: Aditya Dutt, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: position: Add ams AS5600 Position Sensor 2025-10-20 20:16 ` [PATCH 1/2] dt-bindings: iio: position: Add " Aditya Dutt @ 2025-10-22 17:50 ` Conor Dooley 0 siblings, 0 replies; 9+ messages in thread From: Conor Dooley @ 2025-10-22 17:50 UTC (permalink / raw) To: Aditya Dutt Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago, linux-kernel, linux-iio, devicetree, linux-doc [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-20 20:16 [PATCH 0/2] New driver for ams AS5600 Position Sensor Aditya Dutt 2025-10-20 20:16 ` [PATCH 1/2] dt-bindings: iio: position: Add " Aditya Dutt @ 2025-10-20 20:16 ` Aditya Dutt 2025-10-20 23:45 ` Frank Zago ` (3 more replies) 1 sibling, 4 replies; 9+ messages in thread From: Aditya Dutt @ 2025-10-20 20:16 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago Cc: Aditya Dutt, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt @ 2025-10-20 23:45 ` Frank Zago 2025-10-21 11:38 ` kernel test robot ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Frank Zago @ 2025-10-20 23:45 UTC (permalink / raw) To: Aditya Dutt, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet Cc: linux-kernel, linux-iio, devicetree, linux-doc 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt 2025-10-20 23:45 ` Frank Zago @ 2025-10-21 11:38 ` kernel test robot 2025-10-21 12:14 ` kernel test robot 2025-10-23 18:16 ` Jonathan Cameron 3 siblings, 0 replies; 9+ messages in thread From: kernel test robot @ 2025-10-21 11:38 UTC (permalink / raw) To: Aditya Dutt, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago Cc: oe-kbuild-all, Aditya Dutt, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt 2025-10-20 23:45 ` Frank Zago 2025-10-21 11:38 ` kernel test robot @ 2025-10-21 12:14 ` kernel test robot 2025-10-23 18:16 ` Jonathan Cameron 3 siblings, 0 replies; 9+ messages in thread From: kernel test robot @ 2025-10-21 12:14 UTC (permalink / raw) To: Aditya Dutt, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago Cc: oe-kbuild-all, Aditya Dutt, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt ` (2 preceding siblings ...) 2025-10-21 12:14 ` kernel test robot @ 2025-10-23 18:16 ` Jonathan Cameron 2025-10-23 18:32 ` Andy Shevchenko 3 siblings, 1 reply; 9+ messages in thread From: Jonathan Cameron @ 2025-10-23 18:16 UTC (permalink / raw) To: Aditya Dutt Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago, linux-kernel, linux-iio, devicetree, linux-doc 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"); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor 2025-10-23 18:16 ` Jonathan Cameron @ 2025-10-23 18:32 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2025-10-23 18:32 UTC (permalink / raw) To: Jonathan Cameron Cc: Aditya Dutt, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Frank Zago, linux-kernel, linux-iio, devicetree, linux-doc 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-10-23 18:33 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-20 20:16 [PATCH 0/2] New driver for ams AS5600 Position Sensor Aditya Dutt 2025-10-20 20:16 ` [PATCH 1/2] dt-bindings: iio: position: Add " Aditya Dutt 2025-10-22 17:50 ` Conor Dooley 2025-10-20 20:16 ` [PATCH 2/2] iio: position: Add support for ams AS5600 angle sensor Aditya Dutt 2025-10-20 23:45 ` Frank Zago 2025-10-21 11:38 ` kernel test robot 2025-10-21 12:14 ` kernel test robot 2025-10-23 18:16 ` Jonathan Cameron 2025-10-23 18:32 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).