diff mbox series

[v3,15/16] rust: platform: add basic platform device / driver abstractions

Message ID 20241022213221.2383-16-dakr@kernel.org (mailing list archive)
State New
Headers show
Series Device / Driver PCI / Platform Rust abstractions | expand

Commit Message

Danilo Krummrich Oct. 22, 2024, 9:31 p.m. UTC
Implement the basic platform bus abstractions required to write a basic
platform driver. This includes the following data structures:

The `platform::Driver` trait represents the interface to the driver and
provides `pci::Driver::probe` for the driver to implement.

The `platform::Device` abstraction represents a `struct platform_device`.

In order to provide the platform bus specific parts to a generic
`driver::Registration` the `driver::RegistrationOps` trait is implemented
by `platform::Adapter`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/platform.c         |  13 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/platform.rs         | 217 ++++++++++++++++++++++++++++++++
 6 files changed, 234 insertions(+)
 create mode 100644 rust/helpers/platform.c
 create mode 100644 rust/kernel/platform.rs

Comments

Rob Herring Oct. 22, 2024, 11:47 p.m. UTC | #1
On Tue, Oct 22, 2024 at 11:31:52PM +0200, Danilo Krummrich wrote:
> Implement the basic platform bus abstractions required to write a basic
> platform driver. This includes the following data structures:
> 
> The `platform::Driver` trait represents the interface to the driver and
> provides `pci::Driver::probe` for the driver to implement.
> 
> The `platform::Device` abstraction represents a `struct platform_device`.
> 
> In order to provide the platform bus specific parts to a generic
> `driver::Registration` the `driver::RegistrationOps` trait is implemented
> by `platform::Adapter`.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  MAINTAINERS                     |   1 +
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/platform.c         |  13 ++
>  rust/kernel/lib.rs              |   1 +
>  rust/kernel/platform.rs         | 217 ++++++++++++++++++++++++++++++++
>  6 files changed, 234 insertions(+)
>  create mode 100644 rust/helpers/platform.c
>  create mode 100644 rust/kernel/platform.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 87eb9a7869eb..173540375863 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6985,6 +6985,7 @@ F:	rust/kernel/device.rs
>  F:	rust/kernel/device_id.rs
>  F:	rust/kernel/devres.rs
>  F:	rust/kernel/driver.rs
> +F:	rust/kernel/platform.rs
>  
>  DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
>  M:	Nishanth Menon <nm@ti.com>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 312f03cbdce9..217c776615b9 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -18,6 +18,7 @@
>  #include <linux/of_device.h>
>  #include <linux/pci.h>
>  #include <linux/phy.h>
> +#include <linux/platform_device.h>
>  #include <linux/refcount.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 8bc6e9735589..663cdc2a45e0 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -17,6 +17,7 @@
>  #include "kunit.c"
>  #include "mutex.c"
>  #include "page.c"
> +#include "platform.c"
>  #include "pci.c"
>  #include "rbtree.c"
>  #include "rcu.c"
> diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
> new file mode 100644
> index 000000000000..ab9b9f317301
> --- /dev/null
> +++ b/rust/helpers/platform.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/platform_device.h>
> +
> +void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
> +{
> +	return platform_get_drvdata(pdev);
> +}
> +
> +void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
> +{
> +	platform_set_drvdata(pdev, data);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5946f59f1688..9e8dcd6d7c01 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -53,6 +53,7 @@
>  pub mod net;
>  pub mod of;
>  pub mod page;
> +pub mod platform;
>  pub mod prelude;
>  pub mod print;
>  pub mod rbtree;
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> new file mode 100644
> index 000000000000..addf5356f44f
> --- /dev/null
> +++ b/rust/kernel/platform.rs
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for the platform bus.
> +//!
> +//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
> +
> +use crate::{
> +    bindings, container_of, device,
> +    device_id::RawDeviceId,
> +    driver,
> +    error::{to_result, Result},
> +    of,
> +    prelude::*,
> +    str::CStr,
> +    types::{ARef, ForeignOwnable},
> +    ThisModule,
> +};
> +
> +/// An adapter for the registration of platform drivers.
> +pub struct Adapter<T: Driver>(T);
> +
> +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> +    type RegType = bindings::platform_driver;
> +
> +    fn register(
> +        pdrv: &mut Self::RegType,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result {
> +        pdrv.driver.name = name.as_char_ptr();
> +        pdrv.probe = Some(Self::probe_callback);
> +
> +        // Both members of this union are identical in data layout and semantics.
> +        pdrv.__bindgen_anon_1.remove = Some(Self::remove_callback);
> +        pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();
> +
> +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +        to_result(unsafe { bindings::__platform_driver_register(pdrv, module.0) })
> +    }
> +
> +    fn unregister(pdrv: &mut Self::RegType) {
> +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> +        unsafe { bindings::platform_driver_unregister(pdrv) };
> +    }
> +}
> +
> +impl<T: Driver + 'static> Adapter<T> {
> +    fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
> +        let table = T::ID_TABLE;
> +        let id = T::of_match_device(pdev)?;
> +
> +        Some(table.info(id.index()))
> +    }
> +
> +    extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> core::ffi::c_int {
> +        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
> +        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
> +        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
> +        // call above.
> +        let mut pdev = unsafe { Device::from_dev(dev) };
> +
> +        let info = Self::id_info(&pdev);
> +        match T::probe(&mut pdev, info) {
> +            Ok(data) => {
> +                // Let the `struct platform_device` own a reference of the driver's private data.
> +                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> +                // `struct platform_device`.
> +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +            }
> +            Err(err) => return Error::to_errno(err),
> +        }
> +
> +        0
> +    }
> +
> +    extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
> +        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> +        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> +
> +        // SAFETY: `remove_callback` is only ever called after a successful call to
> +        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> +        // `KBox<T>` pointer created through `KBox::into_foreign`.
> +        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
> +    }
> +}
> +
> +/// Declares a kernel module that exposes a single platform driver.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// kernel::module_platform_driver! {
> +///     type: MyDriver,
> +///     name: "Module name",
> +///     author: "Author name",
> +///     description: "Description",
> +///     license: "GPL v2",
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! module_platform_driver {
> +    ($($f:tt)*) => {
> +        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
> +    };
> +}
> +
> +/// IdTable type for platform drivers.
> +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
> +
> +/// The platform driver trait.
> +///
> +/// # Example
> +///
> +///```
> +/// # use kernel::{bindings, c_str, of, platform};
> +///
> +/// struct MyDriver;
> +///
> +/// kernel::of_device_table!(
> +///     OF_TABLE,
> +///     MODULE_OF_TABLE,
> +///     <MyDriver as platform::Driver>::IdInfo,
> +///     [
> +///         (of::DeviceId::new(c_str!("redhat,my-device")), ())

All compatible strings have to be documented as do vendor prefixes and 
I don't think "redhat" is one. An exception is you can use 
"test,<whatever>" and not document it.

There's a check for undocumented compatibles. I guess I'll have to add 
rust parsing to it...

BTW, how do you compile this code in the kernel? 

> +///     ]
> +/// );
> +///
> +/// impl platform::Driver for MyDriver {
> +///     type IdInfo = ();
> +///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> +///
> +///     fn probe(
> +///         _pdev: &mut platform::Device,
> +///         _id_info: Option<&Self::IdInfo>,
> +///     ) -> Result<Pin<KBox<Self>>> {
> +///         Err(ENODEV)
> +///     }
> +/// }
> +///```
> +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> +/// the `Adapter` documentation for an example.
> +pub trait Driver {
> +    /// The type holding information about each device id supported by the driver.
> +    ///
> +    /// TODO: Use associated_type_defaults once stabilized:
> +    ///
> +    /// type IdInfo: 'static = ();
> +    type IdInfo: 'static;
> +
> +    /// The table of device ids supported by the driver.
> +    const ID_TABLE: IdTable<Self::IdInfo>;
> +
> +    /// Platform driver probe.
> +    ///
> +    /// Called when a new platform device is added or discovered.
> +    /// Implementers should attempt to initialize the device here.
> +    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> +
> +    /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
> +    fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {

Is this visible to drivers? It shouldn't be. I just removed most of the 
calls of the C version earlier this year. Drivers should only need the 
match data. The preferred driver C API is device_get_match_data(). That 
is firmware agnostic and works for DT, ACPI and old platform 
driver_data. Obviously, ACPI is not supported here, but it will be soon 
enough. We could perhaps get away with not supporting the platform 
driver_data because that's generally not used on anything in the last 10 
years.

Another potential issue is keeping the match logic for probe and the 
match logic for the data in sync. If you implement your own logic here 
in rust and probe is using the C version, they might not be the same. 
Best case, we have 2 implementations of the same thing.

> +        let table = Self::ID_TABLE;
> +
> +        // SAFETY:
> +        // - `table` has static lifetime, hence it's valid for read,
> +        // - `dev` is guaranteed to be valid while it's alive, and so is
> +        //   `pdev.as_dev().as_raw()`.
> +        let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), pdev.as_dev().as_raw()) };

of_match_device depends on CONFIG_OF. There's an empty static inline, 
but seems bindgen can't handle those. Prior versions added a helper 
function, but that's going to scale terribly. Can we use an annotation 
for CONFIG_OF here (assuming we can't get rid of using this directly)?

> +
> +        if raw_id.is_null() {
> +            None
> +        } else {
> +            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
> +            // does not add additional invariants, so it's safe to transmute.
> +            Some(unsafe { &*raw_id.cast::<of::DeviceId>() })
> +        }
> +    }
> +}
Danilo Krummrich Oct. 23, 2024, 6:44 a.m. UTC | #2
On Tue, Oct 22, 2024 at 06:47:12PM -0500, Rob Herring wrote:
> On Tue, Oct 22, 2024 at 11:31:52PM +0200, Danilo Krummrich wrote:
> > Implement the basic platform bus abstractions required to write a basic
> > platform driver. This includes the following data structures:
> > 
> > The `platform::Driver` trait represents the interface to the driver and
> > provides `pci::Driver::probe` for the driver to implement.
> > 
> > The `platform::Device` abstraction represents a `struct platform_device`.
> > 
> > In order to provide the platform bus specific parts to a generic
> > `driver::Registration` the `driver::RegistrationOps` trait is implemented
> > by `platform::Adapter`.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  MAINTAINERS                     |   1 +
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/helpers/helpers.c          |   1 +
> >  rust/helpers/platform.c         |  13 ++
> >  rust/kernel/lib.rs              |   1 +
> >  rust/kernel/platform.rs         | 217 ++++++++++++++++++++++++++++++++
> >  6 files changed, 234 insertions(+)
> >  create mode 100644 rust/helpers/platform.c
> >  create mode 100644 rust/kernel/platform.rs
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 87eb9a7869eb..173540375863 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6985,6 +6985,7 @@ F:	rust/kernel/device.rs
> >  F:	rust/kernel/device_id.rs
> >  F:	rust/kernel/devres.rs
> >  F:	rust/kernel/driver.rs
> > +F:	rust/kernel/platform.rs
> >  
> >  DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
> >  M:	Nishanth Menon <nm@ti.com>
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 312f03cbdce9..217c776615b9 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -18,6 +18,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/pci.h>
> >  #include <linux/phy.h>
> > +#include <linux/platform_device.h>
> >  #include <linux/refcount.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 8bc6e9735589..663cdc2a45e0 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -17,6 +17,7 @@
> >  #include "kunit.c"
> >  #include "mutex.c"
> >  #include "page.c"
> > +#include "platform.c"
> >  #include "pci.c"
> >  #include "rbtree.c"
> >  #include "rcu.c"
> > diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
> > new file mode 100644
> > index 000000000000..ab9b9f317301
> > --- /dev/null
> > +++ b/rust/helpers/platform.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/platform_device.h>
> > +
> > +void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
> > +{
> > +	return platform_get_drvdata(pdev);
> > +}
> > +
> > +void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
> > +{
> > +	platform_set_drvdata(pdev, data);
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 5946f59f1688..9e8dcd6d7c01 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -53,6 +53,7 @@
> >  pub mod net;
> >  pub mod of;
> >  pub mod page;
> > +pub mod platform;
> >  pub mod prelude;
> >  pub mod print;
> >  pub mod rbtree;
> > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > new file mode 100644
> > index 000000000000..addf5356f44f
> > --- /dev/null
> > +++ b/rust/kernel/platform.rs
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Abstractions for the platform bus.
> > +//!
> > +//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
> > +
> > +use crate::{
> > +    bindings, container_of, device,
> > +    device_id::RawDeviceId,
> > +    driver,
> > +    error::{to_result, Result},
> > +    of,
> > +    prelude::*,
> > +    str::CStr,
> > +    types::{ARef, ForeignOwnable},
> > +    ThisModule,
> > +};
> > +
> > +/// An adapter for the registration of platform drivers.
> > +pub struct Adapter<T: Driver>(T);
> > +
> > +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> > +    type RegType = bindings::platform_driver;
> > +
> > +    fn register(
> > +        pdrv: &mut Self::RegType,
> > +        name: &'static CStr,
> > +        module: &'static ThisModule,
> > +    ) -> Result {
> > +        pdrv.driver.name = name.as_char_ptr();
> > +        pdrv.probe = Some(Self::probe_callback);
> > +
> > +        // Both members of this union are identical in data layout and semantics.
> > +        pdrv.__bindgen_anon_1.remove = Some(Self::remove_callback);
> > +        pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();
> > +
> > +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> > +        to_result(unsafe { bindings::__platform_driver_register(pdrv, module.0) })
> > +    }
> > +
> > +    fn unregister(pdrv: &mut Self::RegType) {
> > +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> > +        unsafe { bindings::platform_driver_unregister(pdrv) };
> > +    }
> > +}
> > +
> > +impl<T: Driver + 'static> Adapter<T> {
> > +    fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
> > +        let table = T::ID_TABLE;
> > +        let id = T::of_match_device(pdev)?;
> > +
> > +        Some(table.info(id.index()))
> > +    }
> > +
> > +    extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> core::ffi::c_int {
> > +        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
> > +        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
> > +        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
> > +        // call above.
> > +        let mut pdev = unsafe { Device::from_dev(dev) };
> > +
> > +        let info = Self::id_info(&pdev);
> > +        match T::probe(&mut pdev, info) {
> > +            Ok(data) => {
> > +                // Let the `struct platform_device` own a reference of the driver's private data.
> > +                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> > +                // `struct platform_device`.
> > +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > +            }
> > +            Err(err) => return Error::to_errno(err),
> > +        }
> > +
> > +        0
> > +    }
> > +
> > +    extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
> > +        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> > +        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> > +
> > +        // SAFETY: `remove_callback` is only ever called after a successful call to
> > +        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> > +        // `KBox<T>` pointer created through `KBox::into_foreign`.
> > +        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
> > +    }
> > +}
> > +
> > +/// Declares a kernel module that exposes a single platform driver.
> > +///
> > +/// # Examples
> > +///
> > +/// ```ignore
> > +/// kernel::module_platform_driver! {
> > +///     type: MyDriver,
> > +///     name: "Module name",
> > +///     author: "Author name",
> > +///     description: "Description",
> > +///     license: "GPL v2",
> > +/// }
> > +/// ```
> > +#[macro_export]
> > +macro_rules! module_platform_driver {
> > +    ($($f:tt)*) => {
> > +        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
> > +    };
> > +}
> > +
> > +/// IdTable type for platform drivers.
> > +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
> > +
> > +/// The platform driver trait.
> > +///
> > +/// # Example
> > +///
> > +///```
> > +/// # use kernel::{bindings, c_str, of, platform};
> > +///
> > +/// struct MyDriver;
> > +///
> > +/// kernel::of_device_table!(
> > +///     OF_TABLE,
> > +///     MODULE_OF_TABLE,
> > +///     <MyDriver as platform::Driver>::IdInfo,
> > +///     [
> > +///         (of::DeviceId::new(c_str!("redhat,my-device")), ())
> 
> All compatible strings have to be documented as do vendor prefixes and 
> I don't think "redhat" is one. An exception is you can use 
> "test,<whatever>" and not document it.

Yeah, I copied that from the sample driver, where it's probably wrong too.

I guess "vendor,device" would be illegal as well?

> 
> There's a check for undocumented compatibles. I guess I'll have to add 
> rust parsing to it...
> 
> BTW, how do you compile this code in the kernel? 

You mean this example? It gets compiled as a KUnit doctest, but it obvously
doesn't execute anything, so it's a compile only test.

> 
> > +///     ]
> > +/// );
> > +///
> > +/// impl platform::Driver for MyDriver {
> > +///     type IdInfo = ();
> > +///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> > +///
> > +///     fn probe(
> > +///         _pdev: &mut platform::Device,
> > +///         _id_info: Option<&Self::IdInfo>,
> > +///     ) -> Result<Pin<KBox<Self>>> {
> > +///         Err(ENODEV)
> > +///     }
> > +/// }
> > +///```
> > +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> > +/// the `Adapter` documentation for an example.
> > +pub trait Driver {
> > +    /// The type holding information about each device id supported by the driver.
> > +    ///
> > +    /// TODO: Use associated_type_defaults once stabilized:
> > +    ///
> > +    /// type IdInfo: 'static = ();
> > +    type IdInfo: 'static;
> > +
> > +    /// The table of device ids supported by the driver.
> > +    const ID_TABLE: IdTable<Self::IdInfo>;
> > +
> > +    /// Platform driver probe.
> > +    ///
> > +    /// Called when a new platform device is added or discovered.
> > +    /// Implementers should attempt to initialize the device here.
> > +    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> > +
> > +    /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
> > +    fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {
> 
> Is this visible to drivers? It shouldn't be.

Yeah, I think we should just remove it. Looking at struct of_device_id, it
doesn't contain any useful information for a driver. I think when I added this I
was a bit in "autopilot" mode from the PCI stuff, where struct pci_device_id is
useful to drivers.

> I just removed most of the 
> calls of the C version earlier this year. Drivers should only need the 
> match data. The preferred driver C API is device_get_match_data(). That 
> is firmware agnostic and works for DT, ACPI and old platform 
> driver_data. Obviously, ACPI is not supported here, but it will be soon 
> enough. We could perhaps get away with not supporting the platform 
> driver_data because that's generally not used on anything in the last 10 
> years.

Otherwise `of_match_device` is only used in `probe_callback` to get the device
info structure, which we indeed should use device_get_match_data() for.

> 
> Another potential issue is keeping the match logic for probe and the 
> match logic for the data in sync. If you implement your own logic here 
> in rust and probe is using the C version, they might not be the same. 
> Best case, we have 2 implementations of the same thing.
> 
> > +        let table = Self::ID_TABLE;
> > +
> > +        // SAFETY:
> > +        // - `table` has static lifetime, hence it's valid for read,
> > +        // - `dev` is guaranteed to be valid while it's alive, and so is
> > +        //   `pdev.as_dev().as_raw()`.
> > +        let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), pdev.as_dev().as_raw()) };
> 
> of_match_device depends on CONFIG_OF. There's an empty static inline, 
> but seems bindgen can't handle those. Prior versions added a helper 
> function, but that's going to scale terribly. Can we use an annotation 
> for CONFIG_OF here (assuming we can't get rid of using this directly)?
> 
> > +
> > +        if raw_id.is_null() {
> > +            None
> > +        } else {
> > +            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
> > +            // does not add additional invariants, so it's safe to transmute.
> > +            Some(unsafe { &*raw_id.cast::<of::DeviceId>() })
> > +        }
> > +    }
> > +}
>
Rob Herring Oct. 23, 2024, 2:23 p.m. UTC | #3
On Wed, Oct 23, 2024 at 08:44:42AM +0200, Danilo Krummrich wrote:
> On Tue, Oct 22, 2024 at 06:47:12PM -0500, Rob Herring wrote:
> > On Tue, Oct 22, 2024 at 11:31:52PM +0200, Danilo Krummrich wrote:
> > > Implement the basic platform bus abstractions required to write a basic
> > > platform driver. This includes the following data structures:
> > > 
> > > The `platform::Driver` trait represents the interface to the driver and
> > > provides `pci::Driver::probe` for the driver to implement.
> > > 
> > > The `platform::Device` abstraction represents a `struct platform_device`.
> > > 
> > > In order to provide the platform bus specific parts to a generic
> > > `driver::Registration` the `driver::RegistrationOps` trait is implemented
> > > by `platform::Adapter`.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > >  MAINTAINERS                     |   1 +
> > >  rust/bindings/bindings_helper.h |   1 +
> > >  rust/helpers/helpers.c          |   1 +
> > >  rust/helpers/platform.c         |  13 ++
> > >  rust/kernel/lib.rs              |   1 +
> > >  rust/kernel/platform.rs         | 217 ++++++++++++++++++++++++++++++++
> > >  6 files changed, 234 insertions(+)
> > >  create mode 100644 rust/helpers/platform.c
> > >  create mode 100644 rust/kernel/platform.rs
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 87eb9a7869eb..173540375863 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -6985,6 +6985,7 @@ F:	rust/kernel/device.rs
> > >  F:	rust/kernel/device_id.rs
> > >  F:	rust/kernel/devres.rs
> > >  F:	rust/kernel/driver.rs
> > > +F:	rust/kernel/platform.rs
> > >  
> > >  DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
> > >  M:	Nishanth Menon <nm@ti.com>
> > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > > index 312f03cbdce9..217c776615b9 100644
> > > --- a/rust/bindings/bindings_helper.h
> > > +++ b/rust/bindings/bindings_helper.h
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/of_device.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/phy.h>
> > > +#include <linux/platform_device.h>
> > >  #include <linux/refcount.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/slab.h>
> > > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > > index 8bc6e9735589..663cdc2a45e0 100644
> > > --- a/rust/helpers/helpers.c
> > > +++ b/rust/helpers/helpers.c
> > > @@ -17,6 +17,7 @@
> > >  #include "kunit.c"
> > >  #include "mutex.c"
> > >  #include "page.c"
> > > +#include "platform.c"
> > >  #include "pci.c"
> > >  #include "rbtree.c"
> > >  #include "rcu.c"
> > > diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
> > > new file mode 100644
> > > index 000000000000..ab9b9f317301
> > > --- /dev/null
> > > +++ b/rust/helpers/platform.c
> > > @@ -0,0 +1,13 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <linux/platform_device.h>
> > > +
> > > +void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
> > > +{
> > > +	return platform_get_drvdata(pdev);
> > > +}
> > > +
> > > +void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
> > > +{
> > > +	platform_set_drvdata(pdev, data);
> > > +}
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index 5946f59f1688..9e8dcd6d7c01 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -53,6 +53,7 @@
> > >  pub mod net;
> > >  pub mod of;
> > >  pub mod page;
> > > +pub mod platform;
> > >  pub mod prelude;
> > >  pub mod print;
> > >  pub mod rbtree;
> > > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > > new file mode 100644
> > > index 000000000000..addf5356f44f
> > > --- /dev/null
> > > +++ b/rust/kernel/platform.rs
> > > @@ -0,0 +1,217 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +//! Abstractions for the platform bus.
> > > +//!
> > > +//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
> > > +
> > > +use crate::{
> > > +    bindings, container_of, device,
> > > +    device_id::RawDeviceId,
> > > +    driver,
> > > +    error::{to_result, Result},
> > > +    of,
> > > +    prelude::*,
> > > +    str::CStr,
> > > +    types::{ARef, ForeignOwnable},
> > > +    ThisModule,
> > > +};
> > > +
> > > +/// An adapter for the registration of platform drivers.
> > > +pub struct Adapter<T: Driver>(T);
> > > +
> > > +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> > > +    type RegType = bindings::platform_driver;
> > > +
> > > +    fn register(
> > > +        pdrv: &mut Self::RegType,
> > > +        name: &'static CStr,
> > > +        module: &'static ThisModule,
> > > +    ) -> Result {
> > > +        pdrv.driver.name = name.as_char_ptr();
> > > +        pdrv.probe = Some(Self::probe_callback);
> > > +
> > > +        // Both members of this union are identical in data layout and semantics.
> > > +        pdrv.__bindgen_anon_1.remove = Some(Self::remove_callback);
> > > +        pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();
> > > +
> > > +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> > > +        to_result(unsafe { bindings::__platform_driver_register(pdrv, module.0) })
> > > +    }
> > > +
> > > +    fn unregister(pdrv: &mut Self::RegType) {
> > > +        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> > > +        unsafe { bindings::platform_driver_unregister(pdrv) };
> > > +    }
> > > +}
> > > +
> > > +impl<T: Driver + 'static> Adapter<T> {
> > > +    fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
> > > +        let table = T::ID_TABLE;
> > > +        let id = T::of_match_device(pdev)?;
> > > +
> > > +        Some(table.info(id.index()))
> > > +    }
> > > +
> > > +    extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> core::ffi::c_int {
> > > +        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
> > > +        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
> > > +        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
> > > +        // call above.
> > > +        let mut pdev = unsafe { Device::from_dev(dev) };
> > > +
> > > +        let info = Self::id_info(&pdev);
> > > +        match T::probe(&mut pdev, info) {
> > > +            Ok(data) => {
> > > +                // Let the `struct platform_device` own a reference of the driver's private data.
> > > +                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> > > +                // `struct platform_device`.
> > > +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > > +            }
> > > +            Err(err) => return Error::to_errno(err),
> > > +        }
> > > +
> > > +        0
> > > +    }
> > > +
> > > +    extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
> > > +        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> > > +        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> > > +
> > > +        // SAFETY: `remove_callback` is only ever called after a successful call to
> > > +        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> > > +        // `KBox<T>` pointer created through `KBox::into_foreign`.
> > > +        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
> > > +    }
> > > +}
> > > +
> > > +/// Declares a kernel module that exposes a single platform driver.
> > > +///
> > > +/// # Examples
> > > +///
> > > +/// ```ignore
> > > +/// kernel::module_platform_driver! {
> > > +///     type: MyDriver,
> > > +///     name: "Module name",
> > > +///     author: "Author name",
> > > +///     description: "Description",
> > > +///     license: "GPL v2",
> > > +/// }
> > > +/// ```
> > > +#[macro_export]
> > > +macro_rules! module_platform_driver {
> > > +    ($($f:tt)*) => {
> > > +        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
> > > +    };
> > > +}
> > > +
> > > +/// IdTable type for platform drivers.
> > > +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
> > > +
> > > +/// The platform driver trait.
> > > +///
> > > +/// # Example
> > > +///
> > > +///```
> > > +/// # use kernel::{bindings, c_str, of, platform};
> > > +///
> > > +/// struct MyDriver;
> > > +///
> > > +/// kernel::of_device_table!(
> > > +///     OF_TABLE,
> > > +///     MODULE_OF_TABLE,
> > > +///     <MyDriver as platform::Driver>::IdInfo,
> > > +///     [
> > > +///         (of::DeviceId::new(c_str!("redhat,my-device")), ())
> > 
> > All compatible strings have to be documented as do vendor prefixes and 
> > I don't think "redhat" is one. An exception is you can use 
> > "test,<whatever>" and not document it.
> 
> Yeah, I copied that from the sample driver, where it's probably wrong too.
> 
> I guess "vendor,device" would be illegal as well?

Yes.

> > There's a check for undocumented compatibles. I guess I'll have to add 
> > rust parsing to it...
> > 
> > BTW, how do you compile this code in the kernel? 
> 
> You mean this example? It gets compiled as a KUnit doctest, but it obvously
> doesn't execute anything, so it's a compile only test.

Yes. That's a question for my own education.

> > 
> > > +///     ]
> > > +/// );
> > > +///
> > > +/// impl platform::Driver for MyDriver {
> > > +///     type IdInfo = ();
> > > +///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> > > +///
> > > +///     fn probe(
> > > +///         _pdev: &mut platform::Device,
> > > +///         _id_info: Option<&Self::IdInfo>,
> > > +///     ) -> Result<Pin<KBox<Self>>> {
> > > +///         Err(ENODEV)
> > > +///     }
> > > +/// }
> > > +///```
> > > +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> > > +/// the `Adapter` documentation for an example.
> > > +pub trait Driver {
> > > +    /// The type holding information about each device id supported by the driver.
> > > +    ///
> > > +    /// TODO: Use associated_type_defaults once stabilized:
> > > +    ///
> > > +    /// type IdInfo: 'static = ();
> > > +    type IdInfo: 'static;
> > > +
> > > +    /// The table of device ids supported by the driver.
> > > +    const ID_TABLE: IdTable<Self::IdInfo>;

Another thing. I don't think this is quite right. Well, this part is 
fine, but assigning the DT table to it is not. The underlying C code has 
2 id tables in struct device_driver (DT and ACPI) and then the bus 
specific one in the struct ${bus}_driver.

> > > +
> > > +    /// Platform driver probe.
> > > +    ///
> > > +    /// Called when a new platform device is added or discovered.
> > > +    /// Implementers should attempt to initialize the device here.
> > > +    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> > > +
> > > +    /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
> > > +    fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {
> > 
> > Is this visible to drivers? It shouldn't be.
> 
> Yeah, I think we should just remove it. Looking at struct of_device_id, it
> doesn't contain any useful information for a driver. I think when I added this I
> was a bit in "autopilot" mode from the PCI stuff, where struct pci_device_id is
> useful to drivers.

TBC, you mean other than *data, right? If so, I agree. 

The DT type and name fields are pretty much legacy, so I don't think the 
rust bindings need to worry about them until someone converts Sparc and 
PowerMac drivers to rust (i.e. never).

I would guess the PCI cases might be questionable, too. Like DT, drivers 
may be accessing the table fields, but that's not best practice. All the 
match fields are stored in pci_dev, so why get them from the match 
table? 

Rob
Dirk Behme Oct. 24, 2024, 9:11 a.m. UTC | #4
Hi Danilo,

On 22.10.2024 23:31, Danilo Krummrich wrote:
> Implement the basic platform bus abstractions required to write a basic
> platform driver. This includes the following data structures:
> 
> The `platform::Driver` trait represents the interface to the driver and
> provides `pci::Driver::probe` for the driver to implement.
> 
> The `platform::Device` abstraction represents a `struct platform_device`.
> 
> In order to provide the platform bus specific parts to a generic
> `driver::Registration` the `driver::RegistrationOps` trait is implemented
> by `platform::Adapter`.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>   MAINTAINERS                     |   1 +
>   rust/bindings/bindings_helper.h |   1 +
>   rust/helpers/helpers.c          |   1 +
>   rust/helpers/platform.c         |  13 ++
>   rust/kernel/lib.rs              |   1 +
>   rust/kernel/platform.rs         | 217 ++++++++++++++++++++++++++++++++
>   6 files changed, 234 insertions(+)
>   create mode 100644 rust/helpers/platform.c
>   create mode 100644 rust/kernel/platform.rs
...
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> new file mode 100644
> index 000000000000..addf5356f44f
> --- /dev/null
> +++ b/rust/kernel/platform.rs
....
> +/// Declares a kernel module that exposes a single platform driver.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// kernel::module_platform_driver! {
> +///     type: MyDriver,
> +///     name: "Module name",
> +///     author: "Author name",
> +///     description: "Description",
> +///     license: "GPL v2",
> +/// }
> +/// ```
> +#[macro_export]
> +macro_rules! module_platform_driver {
> +    ($($f:tt)*) => {
> +        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
> +    };
> +}
> +
> +/// IdTable type for platform drivers.
> +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
> +
> +/// The platform driver trait.
> +///
> +/// # Example
> +///
> +///```
> +/// # use kernel::{bindings, c_str, of, platform};
> +///
> +/// struct MyDriver;
> +///
> +/// kernel::of_device_table!(
> +///     OF_TABLE,
> +///     MODULE_OF_TABLE,

It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names 
used here. Shouldn't they be somehow driver specific, e.g. 
OF_TABLE_MYDRIVER and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the 
other examples/samples in this patch series. Found that while using the 
*same* somewhere else ;)

Best regards

Dirk
Fabien Parent Oct. 27, 2024, 4:32 a.m. UTC | #5
Hi Danilo,

On Tue Oct 22, 2024 at 2:31 PM PDT, Danilo Krummrich wrote:
> +/// An adapter for the registration of platform drivers.
> +pub struct Adapter<T: Driver>(T);

...

> +/// The platform driver trait.
> +///
> +/// # Example
> +///
> +///```
> +/// # use kernel::{bindings, c_str, of, platform};
> +///
> +/// struct MyDriver;
> +///
> +/// kernel::of_device_table!(
> +///     OF_TABLE,
> +///     MODULE_OF_TABLE,
> +///     <MyDriver as platform::Driver>::IdInfo,
> +///     [
> +///         (of::DeviceId::new(c_str!("redhat,my-device")), ())
> +///     ]
> +/// );
> +///
> +/// impl platform::Driver for MyDriver {
> +///     type IdInfo = ();
> +///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> +///
> +///     fn probe(
> +///         _pdev: &mut platform::Device,
> +///         _id_info: Option<&Self::IdInfo>,
> +///     ) -> Result<Pin<KBox<Self>>> {
> +///         Err(ENODEV)
> +///     }
> +/// }
> +///```
...
> +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> +/// the `Adapter` documentation for an example.

The `Adapter` doesn't have any examples, but there is one right
above this paragraph. Anyway I think the example is in the right place and
should not be moved over there.

I think this paragraph should be moved above the example and the last
sentence should be deleted.

Fabien
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 87eb9a7869eb..173540375863 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6985,6 +6985,7 @@  F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
+F:	rust/kernel/platform.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <nm@ti.com>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 312f03cbdce9..217c776615b9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -18,6 +18,7 @@ 
 #include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
+#include <linux/platform_device.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 8bc6e9735589..663cdc2a45e0 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -17,6 +17,7 @@ 
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
+#include "platform.c"
 #include "pci.c"
 #include "rbtree.c"
 #include "rcu.c"
diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
new file mode 100644
index 000000000000..ab9b9f317301
--- /dev/null
+++ b/rust/helpers/platform.c
@@ -0,0 +1,13 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/platform_device.h>
+
+void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
+{
+	return platform_get_drvdata(pdev);
+}
+
+void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
+{
+	platform_set_drvdata(pdev, data);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5946f59f1688..9e8dcd6d7c01 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -53,6 +53,7 @@ 
 pub mod net;
 pub mod of;
 pub mod page;
+pub mod platform;
 pub mod prelude;
 pub mod print;
 pub mod rbtree;
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
new file mode 100644
index 000000000000..addf5356f44f
--- /dev/null
+++ b/rust/kernel/platform.rs
@@ -0,0 +1,217 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the platform bus.
+//!
+//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
+
+use crate::{
+    bindings, container_of, device,
+    device_id::RawDeviceId,
+    driver,
+    error::{to_result, Result},
+    of,
+    prelude::*,
+    str::CStr,
+    types::{ARef, ForeignOwnable},
+    ThisModule,
+};
+
+/// An adapter for the registration of platform drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::platform_driver;
+
+    fn register(
+        pdrv: &mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        pdrv.driver.name = name.as_char_ptr();
+        pdrv.probe = Some(Self::probe_callback);
+
+        // Both members of this union are identical in data layout and semantics.
+        pdrv.__bindgen_anon_1.remove = Some(Self::remove_callback);
+        pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe { bindings::__platform_driver_register(pdrv, module.0) })
+    }
+
+    fn unregister(pdrv: &mut Self::RegType) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::platform_driver_unregister(pdrv) };
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
+        let table = T::ID_TABLE;
+        let id = T::of_match_device(pdev)?;
+
+        Some(table.info(id.index()))
+    }
+
+    extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> core::ffi::c_int {
+        // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
+        let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
+        // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
+        // call above.
+        let mut pdev = unsafe { Device::from_dev(dev) };
+
+        let info = Self::id_info(&pdev);
+        match T::probe(&mut pdev, info) {
+            Ok(data) => {
+                // Let the `struct platform_device` own a reference of the driver's private data.
+                // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
+                // `struct platform_device`.
+                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+            }
+            Err(err) => return Error::to_errno(err),
+        }
+
+        0
+    }
+
+    extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
+        // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
+        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
+
+        // SAFETY: `remove_callback` is only ever called after a successful call to
+        // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+        // `KBox<T>` pointer created through `KBox::into_foreign`.
+        let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+    }
+}
+
+/// Declares a kernel module that exposes a single platform driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// kernel::module_platform_driver! {
+///     type: MyDriver,
+///     name: "Module name",
+///     author: "Author name",
+///     description: "Description",
+///     license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_platform_driver {
+    ($($f:tt)*) => {
+        $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
+    };
+}
+
+/// IdTable type for platform drivers.
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
+
+/// The platform driver trait.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, c_str, of, platform};
+///
+/// struct MyDriver;
+///
+/// kernel::of_device_table!(
+///     OF_TABLE,
+///     MODULE_OF_TABLE,
+///     <MyDriver as platform::Driver>::IdInfo,
+///     [
+///         (of::DeviceId::new(c_str!("redhat,my-device")), ())
+///     ]
+/// );
+///
+/// impl platform::Driver for MyDriver {
+///     type IdInfo = ();
+///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
+///
+///     fn probe(
+///         _pdev: &mut platform::Device,
+///         _id_info: Option<&Self::IdInfo>,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         Err(ENODEV)
+///     }
+/// }
+///```
+/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
+/// the `Adapter` documentation for an example.
+pub trait Driver {
+    /// The type holding information about each device id supported by the driver.
+    ///
+    /// TODO: Use associated_type_defaults once stabilized:
+    ///
+    /// type IdInfo: 'static = ();
+    type IdInfo: 'static;
+
+    /// The table of device ids supported by the driver.
+    const ID_TABLE: IdTable<Self::IdInfo>;
+
+    /// Platform driver probe.
+    ///
+    /// Called when a new platform device is added or discovered.
+    /// Implementers should attempt to initialize the device here.
+    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
+
+    /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
+    fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {
+        let table = Self::ID_TABLE;
+
+        // SAFETY:
+        // - `table` has static lifetime, hence it's valid for read,
+        // - `dev` is guaranteed to be valid while it's alive, and so is
+        //   `pdev.as_dev().as_raw()`.
+        let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), pdev.as_dev().as_raw()) };
+
+        if raw_id.is_null() {
+            None
+        } else {
+            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
+            // does not add additional invariants, so it's safe to transmute.
+            Some(unsafe { &*raw_id.cast::<of::DeviceId>() })
+        }
+    }
+}
+
+/// The platform device representation.
+///
+/// A platform device is based on an always reference counted `device:Device` instance. Cloning a
+/// platform device, hence, also increments the base device' reference count.
+///
+/// # Invariants
+///
+/// `Device` holds a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
+/// member of a `struct platform_device`.
+#[derive(Clone)]
+pub struct Device(ARef<device::Device>);
+
+impl Device {
+    /// Convert a raw kernel device into a `Device`
+    ///
+    /// # Safety
+    ///
+    /// `dev` must be an `Aref<device::Device>` whose underlying `bindings::device` is a member of a
+    /// `bindings::platform_device`.
+    unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
+        Self(dev)
+    }
+
+    fn as_dev(&self) -> &device::Device {
+        &self.0
+    }
+
+    fn as_raw(&self) -> *mut bindings::platform_device {
+        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
+        // embedded in `struct platform_device`.
+        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
+    }
+}
+
+impl AsRef<device::Device> for Device {
+    fn as_ref(&self) -> &device::Device {
+        &self.0
+    }
+}