On Thu, Oct 8, 2020 at 3:04 PM Ertman, David M <david.m.ertman@xxxxxxxxx> wrote: > > > -----Original Message----- > > From: Leon Romanovsky <leon@xxxxxxxxxx> > > Sent: Tuesday, October 6, 2020 10:23 AM > > To: Ertman, David M <david.m.ertman@xxxxxxxxx> > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; broonie@xxxxxxxxxx; linux- > > rdma@xxxxxxxxxxxxxxx; jgg@xxxxxxxxxx; dledford@xxxxxxxxxx; > > netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; > > gregkh@xxxxxxxxxxxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx; pierre- > > louis.bossart@xxxxxxxxxxxxxxx; fred.oh@xxxxxxxxxxxxxxx; > > parav@xxxxxxxxxxxx; Saleem, Shiraz <shiraz.saleem@xxxxxxxxx>; Williams, > > Dan J <dan.j.williams@xxxxxxxxx>; Patil, Kiran <kiran.patil@xxxxxxxxx> > > Subject: Re: [PATCH v2 1/6] Add ancillary bus support > > > > On Mon, Oct 05, 2020 at 11:24:41AM -0700, Dave Ertman wrote: > > > Add support for the Ancillary Bus, ancillary_device and ancillary_driver. > > > It enables drivers to create an ancillary_device and bind an > > > ancillary_driver to it. > > > > > > The bus supports probe/remove shutdown and suspend/resume callbacks. > > > Each ancillary_device has a unique string based id; driver binds to > > > an ancillary_device based on this id through the bus. > > > > > > Co-developed-by: Kiran Patil <kiran.patil@xxxxxxxxx> > > > Signed-off-by: Kiran Patil <kiran.patil@xxxxxxxxx> > > > Co-developed-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx> > > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx> > > > Co-developed-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx> > > > Signed-off-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx> > > > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> > > > Reviewed-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx> > > > Reviewed-by: Parav Pandit <parav@xxxxxxxxxxxx> > > > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > > Signed-off-by: Dave Ertman <david.m.ertman@xxxxxxxxx> > > > --- > > > > <...> > > > > > +/** > > > + * __ancillary_driver_register - register a driver for ancillary bus devices > > > + * @ancildrv: ancillary_driver structure > > > + * @owner: owning module/driver > > > + */ > > > +int __ancillary_driver_register(struct ancillary_driver *ancildrv, struct > > module *owner) > > > +{ > > > + if (WARN_ON(!ancildrv->probe) || WARN_ON(!ancildrv->remove) > > || > > > + WARN_ON(!ancildrv->shutdown) || WARN_ON(!ancildrv- > > >id_table)) > > > + return -EINVAL; > > > > In our driver ->shutdown is empty, it will be best if ancillary bus will > > do "if (->remove) ..->remove()" pattern. > > > > Yes, looking it over, only the probe needs to mandatory. I will change the others to the > conditional model, and adjust the WARN_ONs. > > > > > + > > > + ancildrv->driver.owner = owner; > > > + ancildrv->driver.bus = &ancillary_bus_type; > > > + ancildrv->driver.probe = ancillary_probe_driver; > > > + ancildrv->driver.remove = ancillary_remove_driver; > > > + ancildrv->driver.shutdown = ancillary_shutdown_driver; > > > + > > > > I think that this part is wrong, probe/remove/shutdown functions should > > come from ancillary_bus_type. > > From checking other usage cases, this is the model that is used for probe, remove, > and shutdown in drivers. Here is the example from Greybus. > > int greybus_register_driver(struct greybus_driver *driver, struct module *owner, > const char *mod_name) > { > int retval; > > if (greybus_disabled()) > return -ENODEV; > > driver->driver.bus = &greybus_bus_type; > driver->driver.name = driver->name; > driver->driver.probe = greybus_probe; > driver->driver.remove = greybus_remove; > driver->driver.owner = owner; > driver->driver.mod_name = mod_name; > > > > You are overwriting private device_driver > > callbacks that makes impossible to make container_of of ancillary_driver > > to chain operations. > > > > I am sorry, you lost me here. you cannot perform container_of on the callbacks > because they are pointers, but if you are referring to going from device_driver > to the auxiliary_driver, that is what happens in auxiliary_probe_driver in the > very beginning. > > static int auxiliary_probe_driver(struct device *dev) > 145 { > 146 struct auxiliary_driver *auxdrv = to_auxiliary_drv(dev->driver); > 147 struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > > Did I miss your meaning? I think you're misunderstanding the cases when the bus_type.{probe,remove} is used vs the driver.{probe,remove} callbacks. The bus_type callbacks are to implement a pattern where the 'probe' and 'remove' method are typed to the bus device type. For example 'struct pci_dev *' instead of raw 'struct device *'. See this conversion of dax bus as an example of going from raw 'struct device *' typed probe/remove to dax-device typed probe/remove: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=75797273189d