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

["Danilo Krummrich" <dakr@kernel.org>]

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.

> +}
> +
> +// 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
> +        ),
> +    ]
> +);