Re: [RFC PATCH v4 3/3] iio: position: add Rust driver for ams AS5600

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  7 Jul 2026 22:15:42 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add a Rust driver for the ams AS5600 12-bit magnetic rotary position
> sensor. The driver exposes in_angl_raw and in_angl_scale via the IIO
> sysfs interface.
> 
> Features:
> - ARef<I2cClient> for safe refcounted I2C client access
> - Mutex-serialized status + angle read sequence
> - Static channel spec (module-level const)
> - No magnet validation at probe (deferred to read_raw per IIO convention)
> - Error propagation via ? operator (no recovery state machine)
> 
> The byte order for the AS5600's big-endian registers is handled via
> swap_bytes() in-driver. This is equivalent to C's
> i2c_smbus_read_word_swapped(). The long-term solution is regmap-rs
> where endianness is configured once at the transport level.
> 
> Tested on BeagleBone Black (AM335x) with AS5600 on i2c-2 (0x36).
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>

Sadly my rust remains very limited :(  So you both end up at the back
of my review queue and I can't provide much useful review!

> ---
>  drivers/iio/position/Kconfig   |  14 +++
>  drivers/iio/position/Makefile  |   1 +
>  drivers/iio/position/as5600.rs | 181 +++++++++++++++++++++++++++++++++
>  3 files changed, 196 insertions(+)
>  create mode 100644 drivers/iio/position/as5600.rs
> 
> diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
> index 1576a6380b53..573d241676bf 100644
> --- a/drivers/iio/position/Kconfig
> +++ b/drivers/iio/position/Kconfig
> @@ -6,6 +6,20 @@
>  
>  menu "Linear and angular position sensors"
>  
> +config AS5600
> +	tristate "ams AS5600 magnetic rotary position sensor"
> +	depends on I2C && IIO && RUST

The IIO dependence should be implicit given it is buried
deep in a menu gated on that.

> +	help
> +	  Say Y here to build support for the ams AS5600 12-bit

This Say Y language is odd as it sort of disagrees with the M
section below.  Start off with what it is then finish up with
any suggestions on Y vs M.

> +	  magnetic rotary position sensor with IIO channel support
> +	  (in_angl_raw and in_angl_scale).
> +
> +	  This is a Rust driver that exposes the 12-bit raw angle

I think the aim is that no one configuring the kernel should even
know what the language used.

> +	  and radian scale via the IIO subsystem.

I'd skip the IIO reference here given to get to this help typically
someone already navigated down into the IIO menus.

> +
> +	  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/as5600.rs b/drivers/iio/position/as5600.rs
> new file mode 100644
> index 000000000000..7445398c86b9
> --- /dev/null
> +++ b/drivers/iio/position/as5600.rs

> +
> +static AS5600_CHANNELS: As5600Channels = As5600Channels({
> +    // SAFETY: `iio_chan_spec` is a repr(C) struct where all-zeroes is valid
> +    // (integers default to 0, pointers to NULL).
> +    let mut chan: iio_chan_spec = unsafe { core::mem::zeroed() };
> +    chan.type_ = iio_chan_type_IIO_ANGL;
> +    // TODO: Use kernel::bits equivalent once bit_usize exists
> +    chan.info_mask_separate = (1usize << iio_chan_info_enum_IIO_CHAN_INFO_RAW)
> +        | (1usize << iio_chan_info_enum_IIO_CHAN_INFO_SCALE);

No nice BIT() equivalent?   Those names end up rather repetitive with
most of it coming twice.

I don't suppose there is any way to avoid that?

> +    [chan]
> +});
> +
> +impl IioDriver for As5600Priv {
> +    fn read_raw(&self, _chan: *const iio_chan_spec, mask: isize) -> Result<IioVal> {
> +        const INFO_RAW: isize = iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize;
> +        const INFO_SCALE: isize = iio_chan_info_enum_IIO_CHAN_INFO_SCALE as isize;
> +        match mask {
> +            // IIO_CHAN_INFO_RAW: read the 12-bit raw angle value.
> +            INFO_RAW => {
> +                let hw = self.io_lock.lock();
> +
> +                // Read status register to verify magnet presence before
> +                // reading the angle.
> +                let status = hw.client.try_read8(AS5600_REG_STATUS as usize)?;
> +
> +                // Check magnet presence (MD bit). Without a magnet the angle
> +                // register contains stale/invalid data.
> +                if (status & AS5600_STATUS_MD) == 0 {
> +                    return Err(ENODATA);
> +                }
> +
> +                // Word read at register 0x0C: SMBus read_word_data returns LE,

That seems wrong.  It assumes LE data on the wire, but should return in
the CPU endianness.  Hence it's a swap in all cases, rather than an be16_to_cpu()
which would get it backwards on a big endian platform.


> +                // AS5600 stores angle big-endian, so swap_bytes() is needed.
> +                // Mutex ensures status + angle read is atomic.
> +                // NOTE: Equivalent to C's i2c_smbus_read_word_swapped().
> +                // Long-term, regmap-rs with val_format_endian=Big handles
> +                // this transparently at configuration level.
> +                let raw = hw.client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
> +                let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;
> +                Ok(IioVal::Int(angle as i32))
> +            }
> +            // IIO_CHAN_INFO_SCALE: radians per LSB, 2*pi / 4096 = 0.001533981.
> +            INFO_SCALE => {
> +                Ok(IioVal::IntPlusNano(0, 1533981))
> +            }
> +            _ => Err(EINVAL),
> +        }
> +    }
> +
> +    fn channels(&self) -> &[iio_chan_spec] {
> +        &AS5600_CHANNELS.0
> +    }
> +}