Thanks for the review Leon.
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.
I was under impression that this name is going to be changed.
It's part of the opens stated in the cover letter.
[...]
- const struct my_driver my_drv = {
.ancillary_drv = {.driver = {.name = "myancillarydrv",Why do we need to give control over driver name to the driver authors? It can be problematic if author puts name that already exists.
Good point. When I used the ancillary_devices for my own SoundWire test, the driver name didn't seem specifically meaningful but needed to be set to something, what mattered was the id_table. Just thinking aloud, maybe we can add prefixing with KMOD_BUILD, as we've done already to avoid collisions between device names?
[...]
+int __ancillary_device_add(struct ancillary_device *ancildev, const char *modname) +{
- struct device *dev = &ancildev->dev;
- int ret;
- if (!modname) {
pr_err("ancillary device modname is NULL\n");return -EINVAL;- }
- ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev->name, ancildev->id);
- if (ret) {
pr_err("ancillary device dev_set_name failed: %d\n", ret);return ret;- }
- ret = device_add(dev);
- if (ret)
dev_err(dev, "adding ancillary device failed!: %d\n", ret);- return ret;
+}
Sorry, but this is very strange API that requires users to put internal call to "dev" that is buried inside "struct ancillary_device".
For example in your next patch, you write this "put_device(&cdev->ancildev.dev);"
I'm pretty sure that the amount of bugs in error unwind will be astonishing, so if you are doing wrappers over core code, better do not pass complexity to the users.
In initial reviews, there was pushback on adding wrappers that don't do anything except for a pointer indirection.
Others had concerns that the API wasn't balanced and blurring layers.
Both points have merits IMHO. Do we want wrappers for everything and completely hide the low-level device?
+EXPORT_SYMBOL_GPL(__ancillary_device_add);
+static int ancillary_probe_driver(struct device *dev) +{
- struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
- struct ancillary_device *ancildev = to_ancillary_dev(dev);
- int ret;
- ret = dev_pm_domain_attach(dev, true);
- if (ret) {
dev_warn(dev, "Failed to attach to PM Domain : %d\n", ret);return ret;- }
- ret = ancildrv->probe(ancildev, ancillary_match_id(ancildrv->id_table, ancildev));
I don't think that you need to call ->probe() if ancillary_match_id() returned NULL and probably that check should be done before dev_pm_domain_attach().
we'll look into this.
- if (ret)
dev_pm_domain_detach(dev, true);- return ret;
+}
+static int ancillary_remove_driver(struct device *dev) +{
- struct ancillary_driver *ancildrv = to_ancillary_drv(dev->driver);
- struct ancillary_device *ancildev = to_ancillary_dev(dev);
- int ret;
- ret = ancildrv->remove(ancildev);
- dev_pm_domain_detach(dev, true);
- return ret;
You returned an error to user and detached from PM, what will user do with this information? Should user ignore it? retry?
That comment was also provided in earlier reviews. In practice the error is typically ignored so there was a suggestion to move the return type to void, that could be done if this was desired by the majority.
[...]
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 5b08a473cdba..7d596dc30833 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -838,4 +838,12 @@ struct mhi_device_id { kernel_ulong_t driver_data; };
+#define ANCILLARY_NAME_SIZE 32 +#define ANCILLARY_MODULE_PREFIX "ancillary:"
+struct ancillary_device_id {
- char name[ANCILLARY_NAME_SIZE];
I hope that this be enough.
Are you suggesting a different value to allow for a longer string?