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

[Jonathan Cameron]

On Fri, 28 Aug 2026 13:53:11 +0700
Muchamad Coirul Anwar <muchamadcoirulanwar@xxxxxxxxx> wrote:

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

Sounds good.

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

Perfect. Thanks.  There are a lot of little things like this that we
don't document well on the C side of things.  Big advantage of your
work is it'll help us clean some of that up.

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

Ah.  This is my lack of rust knowledge (and forgetfulness).  I'd avoid
putting too many comments in for the benefit of folk like me who are
still failing to find the time to learn rust :(  

Jonathan