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

[Muchamad Coirul Anwar]

On Sun, 25 Oct 2026 at 00:39, Igor Korotin <igor.korotin@xxxxxxxxx> wrote:
>
> Hi Coirul,
>
> Here's the full diff, applied on top of your v6 patches 3 and 4.
>
> diff --git a/drivers/iio/position/as5600.rs b/drivers/iio/position/as5600.rs
> index b72775241e81..f2897fbf8aa2 100644
> --- a/drivers/iio/position/as5600.rs
> +++ b/drivers/iio/position/as5600.rs
> @@ -36,7 +36,8 @@
>          Device,
>          IioChanInfo,
>          IioDriver,
> -        IioVal, //
> +        IioVal,
> +        Registered, //
>      },
>      io::Io,
>      module_i2c_driver,
> @@ -65,14 +66,12 @@
>
>  i2c_device_table!(
>      I2C_TABLE,
> -    MODULE_I2C_TABLE,
>      <As5600 as Driver>::IdInfo,
>      [(DeviceId::new(c"as5600"), ())]
>  );
>
>  of_device_table!(
>      OF_TABLE,
> -    MODULE_OF_TABLE,
>      <As5600 as Driver>::IdInfo,
>      [(of::DeviceId::new(c"ams,as5600"), ())]
>  );
> @@ -97,25 +96,29 @@ unsafe impl Sync for As5600Channels {}
>  });
>
>  #[pin_data]
> -struct As5600Priv {
> +struct As5600Priv<'a> {
> +    client: &'a I2cClient<Bound>,
>      #[pin]
>      io_lock: Mutex<()>,
>  }
>
> +// SAFETY: `I2cClient<Bound>` methods go through kernel I2C core functions
> +// that provide their own synchronization (bus locking); no `&self` method
> +// exposes interior mutability. The borrowed `client` field can therefore be
> +// safely accessed and shared across threads like the rest of `As5600Priv`.
> +unsafe impl Send for As5600Priv<'_> {}
> +// SAFETY: See above.
> +unsafe impl Sync for As5600Priv<'_> {}
> +
>  // SAFETY: AS5600_CHANNELS contains one valid, immutable IIO_ANGL channel
>  // exposing only RAW and SCALE. It has no event or extension callbacks,
>  // and all optional pointer fields are null.
> -unsafe impl IioDriver for As5600Priv {
> -    fn read_raw(
> -        &self,
> -        parent: &kernel::device::Device<Bound>,
> -        _chan: *const iio_chan_spec,
> -        info: IioChanInfo,
> -    ) -> Result<IioVal> {
> +unsafe impl IioDriver for As5600Priv<'_> {
> +    fn read_raw(&self, _chan: *const iio_chan_spec, info: IioChanInfo) -> Result<IioVal> {
>          match info {
>              IioChanInfo::Raw => {
>                  let _guard = self.io_lock.lock();
> -                let client: &I2cClient<Bound> = parent.try_into()?;
> +                let client = self.client;
>
>                  let io = client.smbus_io();
>                  // Read status register to verify magnet presence before
> @@ -142,14 +145,17 @@ fn channels(&self) -> &'static [iio_chan_spec] {
>      }
>  }
>
> +struct As5600;
> +
>  #[pin_data]
> -struct As5600 {
> -    _registration: (),
> +struct As5600Data<'bound> {
> +    #[pin]
> +    _iio_dev: Device<As5600Priv<'bound>, Registered>,
>  }
>
>  impl Driver for As5600 {
>      type IdInfo = ();
> -    type Data<'bound> = As5600;
> +    type Data<'bound> = As5600Data<'bound>;
>
>      const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>> = Some(&I2C_TABLE);
>      const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> @@ -162,15 +168,22 @@ fn probe<'bound>(
>          dev: &'bound I2cClient<Core<'_>>,
>          _id_info: Option<&'bound Self::IdInfo>,
>      ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> -        try_pin_init!(As5600 {
> -            _registration: {
> +        try_pin_init!(As5600Data {
> +            _iio_dev: {
> +                // Deref coercion: I2cClient<Core<'_>> -> I2cClient<Bound>.
> +                let client: &'bound I2cClient<Bound> = dev;
> +
>                  let priv_init = pin_init!(As5600Priv {
> +                    client,
>                      io_lock <- new_mutex!(()),
>                  });
>
> -                Device::build_device(dev.as_ref(), c"as5600", INDIO_DIRECT_MODE, priv_init)?
> -                    .register(&crate::THIS_MODULE)?;
> +                let unregistered =
> +                    Device::build_device(dev.as_ref(), c"as5600", INDIO_DIRECT_MODE, priv_init)?;
> +                let registered =
> +                    unregistered.register(kernel::module::this_module::<crate::LocalModule>())?;
>                  dev_dbg!(dev.as_ref(), "AS5600 magnetic position sensor ready\n");
> +                registered
>              }
>          })
>      }
> diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
> index 9ad2f50948de..4648424c7600 100644
> --- a/rust/kernel/iio.rs
> +++ b/rust/kernel/iio.rs
> @@ -21,7 +21,6 @@
>          self,
>          Bound, //
>      },
> -    devres,
>      error::{
>          code::*,
>          to_result,
> @@ -159,15 +158,12 @@ pub enum IioVal {
>  pub unsafe trait IioDriver: Send + Sync {
>      /// Called by the IIO core to read channel information.
>      ///
> -    /// `parent` is the parent device supplied to [`Device::build_device`].
>      /// `chan` identifies the channel, and `info` selects the requested
> -    /// information, such as its raw value or scale.
> -    fn read_raw(
> -        &self,
> -        parent: &device::Device<Bound>,
> -        chan: *const iio_chan_spec,
> -        info: IioChanInfo,
> -    ) -> Result<IioVal>;
> +    /// information, such as its raw value or scale. Implementors must
> +    /// capture whatever bus-specific resources they need at registration
> +    /// time (see [`Device::build_device`]) rather than re-deriving them
> +    /// from a generic parent reference here.
> +    fn read_raw(&self, chan: *const iio_chan_spec, info: IioChanInfo) -> Result<IioVal>;
>
>      /// Returns the channel specifications for this driver.
>      ///
> @@ -204,16 +200,6 @@ fn channels(&self) -> &'static [iio_chan_spec] {
>      // initialized `T` that remains valid for the duration of this callback.
>      let driver = unsafe { &*priv_ptr };
>
> -    // SAFETY: `indio_dev` is valid by the callback contract. Its parent
> -    // was supplied as a bound device to `build_device`.
> -    //
> -    // During registration, the builder holds that bound parent borrow.
> -    // After ownership is transferred, parent devres runs the registered
> -    // device's destructor, which unregisters the IIO device and drains
> -    // the supported read_raw paths before destroying the private data.
> -    // The parent therefore remains bound throughout this callback.
> -    let parent = unsafe { device::Device::<Bound>::from_raw((*indio_dev).dev.parent) };
> -
>      // `info` is `long` from C. Negative values and values > u32::MAX
>      // are unsupported iio_chan_info variants; both map to EINVAL.
>      let info_enum = match u32::try_from(info)
> @@ -224,7 +210,7 @@ fn channels(&self) -> &'static [iio_chan_spec] {
>          None => return EINVAL.to_errno(),
>      };
>
> -    match driver.read_raw(parent, chan, info_enum) {
> +    match driver.read_raw(chan, info_enum) {
>          Ok(IioVal::Int(v)) => {
>              // SAFETY: `val` is valid per the function's Safety contract above.
>              // `val2` is not written; `IIO_VAL_INT` signals to the IIO core
> @@ -271,11 +257,8 @@ fn channels(&self) -> &'static [iio_chan_spec] {
>  pub struct Registered;
>
>  /// An initialized IIO device awaiting registration.
> -///
> -/// The parent remains bound until this builder is consumed or dropped.
> -pub struct UnregisteredDevice<'a, T: IioDriver> {
> +pub struct UnregisteredDevice<T: IioDriver> {
>      device: Device<T>,
> -    parent: &'a device::Device<Bound>,
>  }
>
>  /// An owned IIO device with in-place initialized driver data.
> @@ -363,12 +346,12 @@ impl<T: IioDriver> Device<T> {
>      /// Allocation and initialization errors are propagated.
>      ///
>      /// Call [`UnregisteredDevice::register`] to register the device.
> -    pub fn build_device<'a, E>(
> -        parent: &'a device::Device<Bound>,
> +    pub fn build_device<E>(
> +        parent: &device::Device<Bound>,
>          name: &'static CStr,
>          modes: u32,
>          init: impl PinInit<T, E>,
> -    ) -> Result<UnregisteredDevice<'a, T>>
> +    ) -> Result<UnregisteredDevice<T>>
>      where
>          Error: From<E>,
>      {
> @@ -406,7 +389,7 @@ pub fn build_device<'a, E>(
>          // large enough for `T`. The private data remains at a fixed address in
>          // the separate IIO allocation until it is dropped. On initialization
>          // failure, the error path only frees the allocation.
> -        let init_result = unsafe { init.__pinned_init(priv_ptr) };
> +        let init_result = unsafe { pin_init::raw_try_init(priv_ptr, init) };
>          if let Err(e) = init_result {
>              // SAFETY: `pin_init` guarantees partial-init rollback internally.
>              // `priv_` memory was not fully initialized, so we only free the
> @@ -446,21 +429,23 @@ pub fn build_device<'a, E>(
>              (*indio_dev).modes = modes;
>          }
>
> -        Ok(UnregisteredDevice { device, parent })
> +        Ok(UnregisteredDevice { device })
>      }
>  }
>
> -impl<T: IioDriver + 'static> UnregisteredDevice<'_, T> {
> -    /// Registers the IIO device and transfers ownership to parent devres.
> +impl<T: IioDriver> UnregisteredDevice<T> {
> +    /// Registers the IIO device.
>      ///
> -    /// Parent resource teardown unregisters the device and completes its
> -    /// supported `read_raw` callbacks before destroying the private data.
> +    /// The caller owns the returned, pinned [`Device<T, Registered>`] and is
> +    /// responsible for keeping it alive for as long as the registration
> +    /// should remain visible to userspace; dropping it unregisters the
> +    /// device and tears down the private data.
>      ///
>      /// On failure, the device is released and any successful registration is
>      /// undone.
>      #[inline]
> -    pub fn register(self, module: &'static ThisModule) -> Result {
> -        let Self { device, parent } = self;
> +    pub fn register(self, module: &'static ThisModule) -> Result<Device<T, Registered>> {
> +        let Self { device } = self;
>
>          // SAFETY: `device.indio_dev` is a valid, fully-initialized `iio_dev`
>          // allocated by `iio_device_alloc` with its name, info vtable, channels,
> @@ -475,6 +460,6 @@ pub fn register(self, module: &'static ThisModule) -> Result {
>          };
>          forget(device);
>
> -        devres::register(parent, registered, GFP_KERNEL)
> +        Ok(registered)
>      }
>  }


Hi Igor,

Thank you for your help. I really appreciate it. I've applied your
changes locally, and the build succeeds.

I hadn't realized that pin-init had changed upstream and that your
patch also adapted the AS5600 and IIO code to those changes. My version
appears to have fallen quite far behind upstream. Catching up on my own
might have taken me weeks, perhaps even longer if I had run into other
issues along the way.

This also seems to address the earlier discussion around the IIO design
and the TryFrom implementation. I plan to test it on my hardware this
weekend.

Thank you so much,
Coirul


> Cheers
> Igor