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

On Mon, 3 Aug 2026 at 08:13, Jonathan Cameron <[email protected]> wrote:

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

Thanks for the review, Jonathan. Every review teaches me something regardless.

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


I'll drop "&& IIO" from the depends line and rewrite the help text.
Device description first, Y/M hint at the end, and no mention of the
implementation language.


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


There's no bit_usize in kernel::bits yet. I'm considering adding it
for v5 since the change is small.



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

You're right that the comment was misleading. That was my mistake. The
swap is unconditional and has nothing to do with CPU endianness. I'll
rewrite the comment to reflect that.

Coirul

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