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

[Muchamad Coirul Anwar]

On Fri, 29 May 2026 05:37:00 +0000
Brandon Saint-John <branstj@xxxxxxxxx> wrote:

> > +//! Datasheet: https://ams.com/documents/20143/36005/AS5600_DS000365_5-00.pdf
>
> Small nitpick, but going to the above link doesn't resolve to a pdf.
> The link that works as of today for me is:
> https://look.ams-osram.com/m/7059eac7531a86fd/original/AS5600-DS000365.pdf

I'll update in v4. Looks like ams reorganized under the ams-osram.com domain.

> > +fn err_enodata() -> Error {
> > +    Error::from_errno(-(ENODATA as i32))
> > +}
>
> Later on it's probably better to add ENODATA to kernel::error::code
> instead of the helper.

Agreed, the `err_enodata()` helper is a workaround. Will check if
ENODATA can be added to kernel::error::code directly for v4, or at
minimum leave a TODO comment.

> > +#[derive(Clone, Copy)]
> > +struct As5600Io(*mut i2c_client);
>
> You can replace the *mut i2c_client with an ARef<I2cClient>. The
> kernel::impl_device_context_into_aref! macro is run on &I2cClient<Core>
> so you can call ARef::from on dev instead of casting it to the raw pointer
> to hold it.
>
> Then it saves you from repeating a few different parts, like redoing unsafe
> impls, recasting back to I2cClient<Core> in try_readN, etc.

Good suggestion. I'll restructure around `ARef<I2cClient>` in v4, it
eliminates the As5600Io wrapper, the manual Send/Sync impls, and the
unsafe casts entirely.

> > +impl IoCapable<u16> for As5600Io {}
>
> None of the read_u16 or IoCapable<u16> are used at this point, so those
> traits/methods could be dropped.

Per Jonathan's feedback, I'm switching to a word read for the angle
register, so `try_read16` will actually be used in v4. But the As5600Io
wrapper (and its IoCapable impls) goes away regardless since I'm moving
to `ARef<I2cClient>` which already has the Io impl from patch 1.

> As a side note, in the most recent rust-next branch, there are a few changes
> with IoCapable so maybe worth rebasing at some point to get those changes.
> IoCapable isn't a marker trait anymore so I get compile errors trying to
> rebase there.

I checked and you're right. `IoCapable` changed upstream, the empty
marker impls in patch 1 won't compile against rust-next anymore.
Will provide proper implementations for I2cClient in v4.

> > +#[pin_data]
> > +struct As5600Priv<T> {
> > +    #[pin]
> > +    io_lock: Mutex<As5600HwState<T>>,
> > +    channels: KBox<[iio_chan_spec; 1]>,
> > +}
>
> Could As5600Priv/HwState hold the As5600Io directly instead of a generic T?
> As5600Priv was not generic over T in v2, and since As5600Io implements
> Io/IoCapable and the sensor only uses the I2C bus it doesn't seem like it
> needs to be generic, at least at the moment.

I will make this change in v4. I agree that simplifying the struct to
use a concrete
type is the better approach here.With ARef<I2cClient> replacing
As5600Io, the struct
becomes As5600Priv { io_lock: Mutex<As5600HwState>, ... } with no type
parameter."

> > +            m if m == iio_chan_info_enum_IIO_CHAN_INFO_RAW as isize => {
>
> Ideally in the future, the iio_chan_info_enum_* variants can be wrapped in
> an Rust enum with an #[repr] attribute. At this stage, match isn't as
> useful as it could be since you still need to call "if m == ...".

Makes sense as future work for the IIO abstraction. Would make the match
exhaustive and eliminate the catch-all arm.

Thanks,
Coirul