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

[Igor Korotin <igor.korotin@linux.dev>]

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)
     }
 }

Cheers
Igor