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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 24 May 2026 20:28:22 +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:
> - Circuit breaker pattern for I/O error resilience

I'm not sure why this is particularly useful, but maybe I'm missing something.

> - Mutex-serialized multi-byte angle read sequence
> - Automatic recovery from bus failures (Poisoned -> Normal)

I mention this below - it works (sort of) because right now you just
do reads with no state changes.  In general device recovery is way
more complex than what you do.

Various other comments inline. Some are wish list things that may
or may not have rust equivalents of what we'd do in C.

> - No magnet validation at probe time (deferred to read_raw)
> 
> Tested on BeagleBone Black (AM335x) with AS5600 on i2c-2 (0x36).
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>
> ---
>  drivers/iio/position/as5600.rs | 289 +++++++++++++++++++++++++++++++++
>  1 file changed, 289 insertions(+)
>  create mode 100644 drivers/iio/position/as5600.rs

> +
> +#[derive(Clone, Copy)]
> +struct As5600Io(*mut i2c_client);
> +
> +/// Tracks the health state of the hardware bus to prevent I/O storms.
I think we need a separate discussion on how to do device recovery.
It is at best a black art and very device specific if bus corruption occurred.
On a read you can maybe get away with this but on a write we have no idea
if it wrote or not and that write may or may not have had side effects.

If you lose a write, the only always correct thing to do is a device
reset.  Other stuff is very device specific.

No writes in here yet which helps ;)

> +#[derive(Clone, Copy, PartialEq, Eq)]
> +enum DeviceState {
> +    Normal,
> +    Poisoned,
> +}


> +
> +impl<T: Io + IoCapable<u8>> As5600HwState<T> {
> +    /// Performs a dummy read to probe bus health after an I/O failure.

If you got a write failure, this recovers the bus but leaves you not knowing
if the write succeeded or not. I'm not sure how helpful it is.

> +    ///
> +    /// Returns `EIO` in all cases — the caller should always propagate the error.
> +    /// The side effect determines recovery behavior:
> +    /// - If the dummy read **succeeds**: state is reset to `Normal`, meaning the
> +    ///   next `read_raw` call will attempt normal operation directly.
> +    /// - If the dummy read **fails**: state is set to `Poisoned`, meaning the
> +    ///   next `read_raw` call will attempt recovery before normal operation.
> +    fn handle_io_error(&mut self) -> Error {
> +        match self.io.try_read8(AS5600_REG_STATUS as usize) {
> +            Ok(_) => {
> +                self.state = DeviceState::Normal;
> +                EIO
> +            }
> +            Err(_) => {
> +                self.state = DeviceState::Poisoned;
> +                EIO
> +            }
> +        }
> +    }
> +}
> +
> +// SAFETY: `As5600Priv<T>` is `Send` and `Sync` because:
> +// - `T: IoCapable<u8>` is a marker trait with no interior mutability.
> +//   The underlying `As5600Io` wrapper's Send/Sync is guaranteed by its
> +//   manual impls (serialized via Mutex + I2C adapter lock).
> +// - `channels` is a heap-allocated array (`KBox`) with no interior mutability.
> +// - `io_lock: Mutex<As5600HwState<T>>` provides synchronized interior mutability.
> +// - `DeviceState` is a plain enum without interior mutability (Send + Sync
> +//   implicitly).
> +// All concurrent access to hardware goes through the `Mutex` guard.
> +// The `Unpin` bound is strictly required because `kernel::sync::lock::Guard`
> +// only implements `DerefMut` for `T: Unpin`. Without it, state mutation fails.
> +unsafe impl<T: IoCapable<u8> + Unpin> Send for As5600Priv<T> {}
> +unsafe impl<T: IoCapable<u8> + Unpin> Sync for As5600Priv<T> {}
> +
> +impl<T: Io + IoCapable<u8> + Unpin> IioDriver for As5600Priv<T> {
> +    fn read_raw(&self, _chan: *const iio_chan_spec, mask: isize) -> Result<IioVal> {
> +        match mask {
> +            // IIO_CHAN_INFO_RAW — read the 12-bit raw angle value.
> +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {
> +                let mut hw_guard = self.io_lock.lock();
> +
> +                // If the bus was previously poisoned, attempt a single recovery
> +                // read before proceeding with normal operation.
> +                let status = if hw_guard.state == DeviceState::Poisoned {
> +                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
> +                        Ok(s) => {
> +                            hw_guard.state = DeviceState::Normal;
> +                            s
> +                        }
> +                        Err(_) => return Err(EIO),
> +                    }
> +                } else {
> +                    match hw_guard.io.try_read8(AS5600_REG_STATUS as usize) {
> +                        Ok(s) => s,
> +                        Err(_) => return Err(hw_guard.handle_io_error()),
> +                    }
> +                };
> +
> +                // Check magnet presence (MD bit). Without a magnet the angle
> +                // register contains stale/invalid data.
> +                if (status & AS5600_STATUS_MD) == 0 {
> +                    return Err(err_enodata());
> +                }
> +
> +                // Read the 12-bit angle as two bytes. The AS5600 hardware
> +                // freezes the internal angle value on reading the high byte
> +                // until the low byte is read — the Mutex ensures this
> +                // sequence is not interleaved by concurrent readers.

Would be very unusual if the device does this but doesn't support some form of larger
read. Would be nice to be able to do that in a rust driver.

> +                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
> +                    Ok(v) => v as u16,
> +                    Err(_) => return Err(hw_guard.handle_io_error()),
> +                };
> +                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
> +                    Ok(v) => v as u16,
> +                    Err(_) => return Err(hw_guard.handle_io_error()),
> +                };
> +
> +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;

This is the sort of thing we'd never do in a C driver because we have well defined meaningful
functions / macros for doing this.  I'd like to see something equivalent in the rust code of.

   Bulk read int a two byte array. i2c_smbus_read_word_data() or swapped variant.
   Unaligned endian read  get_unaligned_be16()
   Masking to extract the 12 bits that are valid. FIELD_GET() whatever.

Might not matter in this driver but once you get 24bit reads or bigger doing open coded
version is not particularly readable.

> +                Ok(IioVal::Int(angle as i32))
> +            }
> +            // IIO_CHAN_INFO_SCALE — radians per LSB: 2π / 4096 ≈ 0.001533981.
> +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_SCALE as isize => {
> +                Ok(IioVal::IntPlusNano(0, 1533981))
> +            }
> +            _ => Err(kernel::error::code::EINVAL),
> +        }
> +    }
> +
> +    fn channels(&self) -> &[iio_chan_spec] {
> +        &self.channels[..]
> +    }
> +}
> +
> +struct As5600 {
> +    _iio_dev: Device<As5600Priv<As5600Io>, Registered>,
> +}
> +
> +impl Driver for As5600 {
> +    type IdInfo = ();
> +    const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    #[allow(refining_impl_trait)]
> +    fn probe(dev: &I2cClient<Core>, _id_info: Option<&Self::IdInfo>) -> Result<Self> {
> +        // SAFETY: `iio_chan_spec` is a C struct whose fields are all integers
> +        // and pointers. Zero is a valid initialization for all of them.
> +        let mut channels_alloc = kernel::alloc::KBox::new(
> +            [unsafe { core::mem::zeroed::<iio_chan_spec>() }],
> +            kernel::alloc::flags::GFP_KERNEL,
> +        )?;
> +
> +        channels_alloc[0].info_mask_separate = (1 << iio_chan_info_enum_IIO_CHAN_INFO_RAW)
> +            | (1 << iio_chan_info_enum_IIO_CHAN_INFO_SCALE);

I believe there is a kernel rust equivalent of BIT(). Can you use that here.

> +        channels_alloc[0].type_ = iio_chan_type_IIO_ANGL;
> +
> +        let client_ptr = dev as *const _ as *mut i2c_client;
> +
> +        let priv_init = pin_init!(As5600Priv {
> +            io_lock <- new_mutex!(As5600HwState {
> +                io: As5600Io(client_ptr),
> +                state: DeviceState::Normal,
> +            }),
> +            channels: channels_alloc,
> +        });
> +
> +        let iio_dev = Device::build_device(dev.as_ref(), c"as5600", priv_init)?;
> +        let iio_dev_registered = iio_dev.register(&crate::THIS_MODULE)?;
> +
> +        dev_info!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");

Too noisy. dev_dbg() at most.

> +        Ok(As5600 {
> +            _iio_dev: iio_dev_registered,
> +        })
> +    }
> +}