Re: [RFC PATCH v1 4/4] rust: add PL031 RTC driver

[Ke Sun <sk.alvin.x@gmail.com>]

On 1/4/26 21:10, Danilo Krummrich wrote:
> On Sun Jan 4, 2026 at 7:06 AM CET, Ke Sun wrote:
>> +/// PL031 RTC driver private data.
>> +#[pin_data(PinnedDrop)]
>> +struct Pl031DrvData {
>> +    #[pin]
>> +    base: Devres<IoMem<0>>,
> Please do not use 0 as generic argument, this should likely be RTC_YLR + 0x4
> (assuming that this register has a width of 32 bit).
>
> It allows you to perform register accesses until RTC_YLR + 0x4 with infallible
> accessors, since the call to IoMem::new() will validate that the memory region
> has at least a size of RTC_YLR + 0x4.
>
>> +    variant: VendorVariant,
>> +    /// RTC device reference for interrupt handler.
>> +    ///
>> +    /// Set in `init_rtcdevice` and remains valid for the driver's lifetime
>> +    /// because the RTC device is managed by devres.
>> +    rtc_device: Option<ARef<RtcDevice>>,
> I don't see a reason for a separate init_rtcdevice() method. Creating the RTC
> device should happen in probe(), which also gets you rid of this odd Option.
>
Hi Danilo,

I encountered an issue while refactoring the RTC abstraction.

Following the platform driver implementation, the AMBA driver stores its 
drvdata in amba_device->dev. However,
the RTC driver also stores its drvdata in the parent device (which is 
also amba_device->dev), causing a conflict.
This was already encountered in v1, which is why the design was awkward.

Do you have any suggestions on how to address this?

Here is part of the code:

  // rust/kernel/amba.rs
impl<T: Driver + 'static> Adapter<T> {
     extern "C" fn probe_callback(
         adev: *mut bindings::amba_device,
         id: *const bindings::amba_id,
     ) -> kernel::ffi::c_int {
         let adev_internal = unsafe { 
&*adev.cast::<Device<device::CoreInternal>>() };
         let info = Self::amba_id_info(adev_internal, id);

         from_result(|| {
             let data = T::probe(adev_internal, info);

             // Referring to the implementation in platform.rs, here 
`dev_set_drvdata(&adev->dev, data)`
             // will clobber the value that has already been set in 
`amba::Driver::probe`.
             adev_internal.as_ref().set_drvdata(data)?;
             Ok(0)
         })
   }
}

// rust/kernel/rtc.rs
impl<T: RtcOps> Adapter<T> {
     unsafe extern "C" fn read_time(
         dev: *mut bindings::device,      // **pointer to the `struct 
device` embedded in  a `struct amba_device`
         tm: *mut bindings::rtc_time,
     ) -> c_int {
         let bound_dev = unsafe { 
device::Device::<device::Bound>::from_raw(dev) };
         let rtc_tm = unsafe { &mut *tm.cast::<RtcTime>() };

         match T::read_time(bound_dev, rtc_tm) {
             Ok(()) => 0,
             Err(err) => err.to_errno(),
         }
   }
}

// drivers/rtc/rtc_pl031_rust.rs
impl amba::Driver for Pl031AmbaDriver {
     ...
     fn probe(
         adev: &amba::Device<Core>,
         id_info: Option<&Self::IdInfo>,
     ) -> impl PinInit<Self, Error> {
         let dev = adev.as_ref();
         let io_request = adev.io_request().ok_or(ENODEV)?;
         ...
         // Allocate RTC device.
         let rtcdev = RtcDevice::new::<Pl031DrvData>(dev)?;
         let rtcdev_clone = rtcdev.clone();

         // Set driver data with RTC device reference.
         rtcdev.set_drvdata(try_pin_init!(Pl031DrvData {
             base <- IoMem::new(io_request),
             variant,
             rtcdev: rtcdev_clone,
         }))?;
         ...
         // Register RTC device.
         // **If CONFIG_RTC_HCTOSYS is enabled and the device is rtc0, 
rtc_read_time will be
         // called during the registration process. This requires 
drvdata to be set up before registration.
         Registration::register(dev, rtcdev)?;

         Ok(Pl031AmbaDriver)
     }
}

Best regards,
Ke Sun

>> +}
>> +
>> +// SAFETY: `Pl031DrvData` contains only `Send`/`Sync` types: `Devres` (Send+Sync),
>> +// `VendorVariant` (Copy), and `Option<ARef<RtcDevice>>` (Send+Sync because `RtcDevice` is
>> +// Send+Sync).
>> +unsafe impl Send for Pl031DrvData {}
>> +// SAFETY: `Pl031DrvData` contains only `Send`/`Sync` types: `Devres` (Send+Sync),
>> +// `VendorVariant` (Copy), and `Option<ARef<RtcDevice>>` (Send+Sync because `RtcDevice` is
>> +// Send+Sync).
>> +unsafe impl Sync for Pl031DrvData {}
> Why not implement Send + Sync for RtcDevice then?
>
>> +// Use AMBA device table for matching
>> +kernel::amba_device_table!(
>> +    ID_TABLE,
>> +    MODULE_ID_TABLE,
>> +    <Pl031DrvData as rtc::DriverGeneric<rtc::AmbaBus>>::IdInfo,
>> +    [
>> +        (
>> +            amba::DeviceId::new_with_data(0x00041031, 0x000fffff, Pl031Variant::ARM.to_usize()),
>> +            Pl031Variant::ARM
>> +        ),
>> +        (
>> +            amba::DeviceId::new_with_data(0x00180031, 0x00ffffff, Pl031Variant::STV1.to_usize()),
>> +            Pl031Variant::STV1
>> +        ),
>> +        (
>> +            amba::DeviceId::new_with_data(0x00280031, 0x00ffffff, Pl031Variant::STV2.to_usize()),
> Why a constructor new_with_data() if you already store data through the generic
> device ID mechanism right below?
>
>> +            Pl031Variant::STV2
>> +        ),
>> +    ]
>> +);