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

[Muchamad Coirul Anwar]

On Thu, 28 May 2026 17:08:00 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> > Features:
> > - Circuit breaker pattern for I/O error resilience
> I'm not sure why this is particularly useful, but maybe I'm missing something.

> 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.

Fair enough. It's over-engineered for what this driver does. Simple -EIO
propagation is enough since the sensor is read-only and the Mutex
already limits bus access to one reader at a time. I'll remove the circuit
breaker in v4. The read_raw path will just propagate errors from
try_read8/try_read16 directly.

> 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.

Agreed. Since this driver is read-only today, the whole mechanism is
solving a problem that doesn't really exist here. If device recovery
patterns come up as a broader R4L discussion topic, happy to participate.

> > +                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.

Switching to `try_read16()` (calls `i2c_smbus_read_word_data`) plus
`swap_bytes()` for the byte order, then mask:

    const AS5600_RAW_ANGLE_MASK: u16 = 0x0FFF;

    let raw = client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
    let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;

Rust doesn't have FIELD_GET yet, but a named constant serves the same
documentation purpose. Single call, no manual byte assembly.

> > +                // 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
>
> 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.

Yes, the AS5600 supports word reads over SMBus. I was being overly
cautious with the byte-at-a-time approach based on the datasheet's
latch mechanism description, but a word read achieves the same atomicity
from the hardware side. Using it in v4.

> > +        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.

This should use `kernel::bits::bit()` for consistency.
The status register check already uses `bit_u8(5)` but the channel
setup didn't follow the same pattern. I'll fix this in v4.

> > +        dev_info!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");
>
> Too noisy. dev_dbg() at most.

I will change this to dev_dbg!().

Thanks,
Coirul