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

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Oct 23, 2024 at 09:23:55AM -0500, Rob Herring wrote:
> 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:
> > > > +///     ]
> > > > +/// );
> > > > +///
> > > > +/// 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.

The assignment of this table in `Adapter::register` looks like this:

`pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();`

What do you think is wrong with this assignment?

> 
> > > > +
> > > > +    /// 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. 

Yes.

> 
> 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?

Fair question, I'd like to forward it to Greg. IIRC, he explicitly requested to
make the corresponding struct pci_device_id available in probe() at Kangrejos.

> 
> Rob
>