Re: [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions

[Muchamad Coirul Anwar]

Hi Jonathan,

On Mon, 24 Aug 2026 at 07:07, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> This looks fine to me subject to a few little things - see inline.
>
> However I didn't take the time to decode every line of rust today so there were bits
> I simply didn't understand yet.  So for this to be able to move forward I'm going
> to need reviews from rust experts!

Thanks for the review. I'll add an explicit review request to Danilo
and Igor in the v6 cover
letter.

Brandon Saint-John has been CC'd since v2 and reviewed the series at
v3. If he has time to look at it again, a Reviewed-by from him on the
Rust abstractions would be welcome.

>
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index a56ba6309594..5dc917d92151 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -86,6 +86,7 @@ macro_rules! declare_err {
> >      declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
> >      declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
> >      declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
> > +    declare_err!(ENODATA, "No data available.");
>
> Do we have something says there must be a user in the same patch?
> A really generic thing like this in C would definitely be a patch on its
> own so that folk who care about maintaining a given file can easily see
> it without reviewing the rest of the series.
>
> So unless you can't do otherwise, break this out as a precursor patch.

I'll break it out as its own patch. The patch will note that the first
user is the AS5600 driver, which returns ENODATA when the magnet is
absent.


> >  }
> >
> >  /// Generic integer kernel error.
> > diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
> > new file mode 100644
> > index 000000000000..f1638160fed1
> > --- /dev/null
> > +++ b/rust/kernel/iio.rs
> ...
>
> > +
> > +build_iio_enum! {
> > +    /// Raw unprocessed value from the channel (`IIO_CHAN_INFO_RAW`).
> > +    ///
> > +    /// For sensors, this is typically the ADC reading or register value
> > +    /// before any scaling or offset correction.
> > +    Raw = iio_chan_info_enum_IIO_CHAN_INFO_RAW,
>
> I guess there may be a rust convention for this but from a human trying to
> read the code point of view this need a blank line here and in similar places
> where you have docs / thing documented repeated back to back.

Fair point. I'll add blank lines between the variant doc blocks and
audit the rest of iio.rs for the same pattern.


> > +    /// Scale factor to convert raw values to SI units (`IIO_CHAN_INFO_SCALE`).
> > +    ///
> > +    /// The processed value is `raw * scale`. The unit depends on the channel
> > +    /// type (e.g. V for voltage, m/s² for acceleration, rad for angle).
> > +    Scale = iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
> > +}
>
>
> > +
> > +/// C-compatible trampoline for the `iio_info.read_raw` callback.
> > +///
> > +/// # Safety
> > +///
> > +/// This function is only called by the IIO core via the `read_raw` function
> > +/// pointer in `iio_info`. The IIO core guarantees:
> > +/// - `indio_dev` is a valid `iio_dev` allocated by `iio_device_alloc`.
> > +/// - `chan` points to a valid channel spec from the device's channel array.
> > +/// - `val` is a valid non-null pointer to a writable `int`.
> > +/// - `val2` is a valid non-null pointer to a writable `int`. The IIO core
> > +///   always passes stack-allocated storage for both, regardless of whether
> > +///   the driver uses `val2` (e.g. `IIO_VAL_INT` only writes `val`; `val2`
>
> That val2 is always a valid pointer smells a bit like the c interface leaking
> into the rust.  I'm not necessarily against that being a constraint we take
> on but I'm not sure how we document that. Probably add something to the C docs.
> Any C driver relying on this today is probably buggy for other reasons.

As you suggested, I'll add a note to the C-side iio_info.read_raw
documentation in include/linux/iio/iio.h. The IIO core always passes
stack-allocated storage for both val and val2 regardless of which
IIO_VAL_* type the driver returns, so the assumption holds, it just
isn't written down anywhere. I'll also note it in the SAFETY block of
read_raw_callback.


> > +        Ok(IioVal::Fractional(v, v2)) => {
> > +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> > +            unsafe {
> > +                *val = v;
> > +                *val2 = v2.get();
>
> Why get in some places and not others?  May well be a gap in my really limited
> rust knowledge.

This came up in v3 and I answered it then but forgot to add the comment
as I said I would. IioVal::Fractional holds a NonZeroI32 denominator to
prevent division-by-zero in iio_format_value(). NonZeroI32 is a newtype
wrapper over i32, so .get() is needed to extract the inner i32. The
other variants hold plain i32 and don't need it. I'll add an inline
comment in v6.