Message-ID: <Z0XBbLb8NRQg_dek@cassiopeiae>
Date: Tue, 26 Nov 2024 13:39:08 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Rob Herring <robh@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com,
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	daniel.almeida@...labora.com, saravanak@...gle.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 15/16] rust: platform: add basic platform device /
 driver abstractions

On Wed, Oct 30, 2024 at 07:23:47AM -0500, Rob Herring wrote:
> On Mon, Oct 28, 2024 at 5:15 AM Danilo Krummrich <dakr@...nel.org> wrote:
> >
> > 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?
> 
> Every bus implementation will need the DT and ACPI tables, so they
> should not be declared and assigned in platform driver code, but in
> the generic device/driver abstractions just like the underlying C
> code. The one here should be for platform_device_id. You could put all
> 3 tables here, but that's going to be a lot of duplication I think.

That's indeed true. But I'm not sure that at this point we need a generalized
`Driver` abstraction just for assigning the DT and ACPI tables.

Maybe it's better to do this in a subsequent series?

> 
> > >
> > > > > > +
> > > > > > +    /// 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.
> 
> Which table gets passed in though? Is the IdInfo parameter generic and
> can be platform_device_id, of_device_id or acpi_device_id? Not sure if
> that's possible in rust or not.

Not sure I can follow you here.

The `IdInfo` parameter is of a type given by the driver for driver specific data
for a certain ID table entry.

It's analogue to resolving `pci_device_id::driver_data` in C.

> 
> PCI is the exception, not the rule here, in that it only matches with
> pci_device_id. At least I think that is the case currently, but it is
> entirely possible we may want to do ACPI/DT matching like every other
> bus. There are cases where PCI devices are described in DT.
> 
> Rob
> 

Powered by blists - more mailing lists