-----Original Message----- From: Leon Romanovsky leon@kernel.org Sent: Tuesday, October 6, 2020 10:23 AM To: Ertman, David M david.m.ertman@intel.com Cc: alsa-devel@alsa-project.org; tiwai@suse.de; broonie@kernel.org; linux- rdma@vger.kernel.org; jgg@nvidia.com; dledford@redhat.com; netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; gregkh@linuxfoundation.org; ranjani.sridharan@linux.intel.com; pierre- louis.bossart@linux.intel.com; fred.oh@linux.intel.com; parav@mellanox.com; Saleem, Shiraz shiraz.saleem@intel.com; Williams, Dan J dan.j.williams@intel.com; Patil, Kiran kiran.patil@intel.com 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@intel.com Signed-off-by: Kiran Patil kiran.patil@intel.com Co-developed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Shiraz Saleem shiraz.saleem@intel.com Reviewed-by: Parav Pandit parav@mellanox.com Reviewed-by: Dan Williams dan.j.williams@intel.com Signed-off-by: Dave Ertman david.m.ertman@intel.com
<...>
+/**
- __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?
-DaveE
- return driver_register(&ancildrv->driver);
+} +EXPORT_SYMBOL_GPL(__ancillary_driver_register);
Thanks