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

[Shuai Liu]

Hi Coirul,

Thanks for the series. I read through v6, including Igor's registration
rework proposal in this thread. The comments below do not overlap with
that rework and should carry over to v7 unchanged, as far as I can tell.

1. clippy warnings

`make LLVM=1 CLIPPY=1` reports four warnings on this series, which the
Rust CI gates on. Three are `clippy::ptr_as_ptr` in the IIO abstraction
and one is `clippy::useless_conversion` in the driver:

    --- a/rust/kernel/iio.rs
    +++ b/rust/kernel/iio.rs
    @@ -202,1 +202,1 @@
    -    let priv_ptr = unsafe { (*indio_dev).priv_ as *mut T };
    +    let priv_ptr = unsafe { (*indio_dev).priv_.cast::<T>() };

    (the same pattern at lines 334 and 397; line 334 is already inside
    an unsafe block and just needs `.cast::<T>()`)

    --- a/drivers/iio/position/as5600.rs
    +++ b/drivers/iio/position/as5600.rs
    @@ -133,1 +133,1 @@
    -                Ok(IioVal::Int(angle as i32))
    +                Ok(IioVal::Int(i32::from(angle)))

2. ignored doctest in the SMBus word helper

The example on `smbus_read_word_swapped()` (rust/kernel/i2c.rs:841) is
marked "ignore" and references driver-specific facts (the AS5600).
Ignored examples are not compiled or run by rusttest, so the helper
currently ships with no tested usage. A generic, compilable example
works here:

    --- a/rust/kernel/i2c.rs
    +++ b/rust/kernel/i2c.rs
    @@ -841,8 +841,15 @@
     /// ```
    -/// // AS5600 stores the 12-bit raw angle at command byte 0x0C with its
    -/// // high byte first, so the SMBus (low byte first) word result must
    -/// // be swapped.
    -/// let raw = client.smbus_read_word_swapped(0x0C)?;
    -/// # Ok::<(), Error>(())
    +/// use kernel::{i2c::I2cClient, prelude::*};
    +///
    +/// fn read_hi_first_register(client: &I2cClient<kernel::device::Bound>) -> Result<u16> {
    +///     // A device whose 16-bit register selected by command byte 0x0C
    +///     // transmits the high byte first needs the SMBus (low byte first)
    +///     // word swapped.
    +///     client.smbus_read_word_swapped(0x0C)
    +/// }
     /// ```

3. repeated unsafe blocks in the SMBus access helpers

Patch 2 implements the `FallibleIoCapable` impls for `u8`/`u16` with
five near-identical unsafe blocks wrapping `bindings::i2c_smbus_*`
calls plus error conversion. Extracting four small safe helpers would
put the safety argument in one place and turn each call site into a
one-liner:

    fn smbus_read_byte(client: &I2cClient<device::Bound>, reg: u8)
        -> Result<u8>;
    fn smbus_write_byte(client: &I2cClient<device::Bound>, reg: u8, value: u8)
        -> Result;
    fn smbus_read_word_data(client: &I2cClient<device::Bound>, reg: u8)
        -> Result<u16>;
    fn smbus_write_word_data(client: &I2cClient<device::Bound>, reg: u8, value: u16)
        -> Result;

For example, `io_try_write` for `u8` then reduces to
`smbus_write_byte(client, reg, value)`. Happy to send the full diff if
that is useful as a starting point.

With 1 and 2 applied (and 3 sketched on top), `make LLVM=1 CLIPPY=1
drivers/iio/position/as5600.o` builds with no warnings and the rusttest
doctests pass.

Thanks,
Shuai Liu