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

[Jonathan Cameron <jic23@kernel.org>]

> > > +                let angle_h = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_H as usize) {
> > > +                    Ok(v) => v as u16,
> > > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > > +                };
> > > +                let angle_l = match hw_guard.io.try_read8(AS5600_REG_RAW_ANGLE_L as usize) {
> > > +                    Ok(v) => v as u16,
> > > +                    Err(_) => return Err(hw_guard.handle_io_error()),
> > > +                };
> > > +
> > > +                let angle = (angle_h << 8 | angle_l) & 0x0FFF;  
> >
> > This is the sort of thing we'd never do in a C driver because we have well
> > defined meaningful functions / macros for doing this.  I'd like to see
> > something equivalent in the rust code of.
> >    Bulk read int a two byte array. i2c_smbus_read_word_data() or swapped variant.
> >    Unaligned endian read  get_unaligned_be16()
> >    Masking to extract the 12 bits that are valid. FIELD_GET() whatever.  
> 
> Switching to `try_read16()` (calls `i2c_smbus_read_word_data`) plus
> `swap_bytes()` for the byte order, then mask:
> 
>     const AS5600_RAW_ANGLE_MASK: u16 = 0x0FFF;
> 
>     let raw = client.try_read16(AS5600_REG_RAW_ANGLE_H as usize)?;
>     let angle = raw.swap_bytes() & AS5600_RAW_ANGLE_MASK;

That swap goes back to a pattern we ripped out of the C code years ago
and why we have the smbus swapped functions and regmap support fort htat.

If a given part always does the bytes in opposite byte order of smbus
then it should be handled as part of the read function rather than every
word read having to be followed by a swap.  Here you only have one so
it doesn't look that bad, but for some other devices this is the common
call sequence to ready almost anything.

> 
> Rust doesn't have FIELD_GET yet, but a named constant serves the same
> documentation purpose. Single call, no manual byte assembly.

A named constant serves only part of the purpose. The main gain from FIELD_GET()
is we don't have to go check if a shift is also needed.  I'd strongly
support work on getting something similar for rust as it makes for a lot
more consistent and readable driver code.

Basically I want all the useful helper stuff we've built up in C to be
available in Rust.  In cases like this one I would prefer there was never
a legacy of doing it any other way!

Jonathan