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
>
