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@xxxxxxxxxx> 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
>
