| Message ID | 1539080245-25818-1-git-send-email-svellattu@mvista.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] Driver core: add bus_find_device_by_fwnode | expand |
On Tue, Oct 9, 2018 at 12:18 PM Silesh C V <svellattu@mvista.com> wrote: > > Some drivers need to find the device on a bus having a specific firmware > node. Currently, such drivers have their own implementations to do this. > Provide a helper similar to bus_find_device_by_name so that each driver > does not have to reinvent this. > > Signed-off-by: Silesh C V <svellattu@mvista.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Changes since v2: > - make use of dev_fwnode in match_fwnode. > > drivers/base/bus.c | 20 ++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index 8bfd27e..a2f39db 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -17,6 +17,7 @@ > #include <linux/string.h> > #include <linux/mutex.h> > #include <linux/sysfs.h> > +#include <linux/property.h> > #include "base.h" > #include "power/power.h" > > @@ -373,6 +374,25 @@ struct device *bus_find_device_by_name(struct bus_type *bus, > } > EXPORT_SYMBOL_GPL(bus_find_device_by_name); > > +static int match_fwnode(struct device *dev, void *fwnode) > +{ > + return dev_fwnode(dev) == fwnode; > +} > + > +/** > + * bus_find_device_by_fwnode - device iterator for locating a particular device > + * having a specific firmware node > + * @bus: bus type > + * @start: Device to begin with > + * @fwnode: firmware node of the device to match > + */ > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, > + struct fwnode_handle *fwnode) > +{ > + return bus_find_device(bus, start, (void *)fwnode, match_fwnode); > +} > +EXPORT_SYMBOL_GPL(bus_find_device_by_fwnode); > + > /** > * subsys_find_device_by_id - find a device with a specific enumeration number > * @subsys: subsystem > diff --git a/include/linux/device.h b/include/linux/device.h > index 8f88254..09384f6 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -171,6 +171,9 @@ struct device *bus_find_device(struct bus_type *bus, struct device *start, > struct device *bus_find_device_by_name(struct bus_type *bus, > struct device *start, > const char *name); > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, > + struct device *start, > + struct fwnode_handle *fwnode); > struct device *subsys_find_device_by_id(struct bus_type *bus, unsigned int id, > struct device *hint); > int bus_for_each_drv(struct bus_type *bus, struct device_driver *start, > -- > 1.9.1 >
On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote: > Some drivers need to find the device on a bus having a specific firmware > node. Currently, such drivers have their own implementations to do this. > Provide a helper similar to bus_find_device_by_name so that each driver > does not have to reinvent this. > > Signed-off-by: Silesh C V <svellattu@mvista.com> Looks good in general, however: We recently had this discussion in I2C world about using the parent if the (logical) device has a NULL fw_node [1]. I don't know if the other subsystems you modify use logical devices as well? If no, it seems we need an additional check for the parent in the I2C core only. If yes, this might be considered in your patchset? Thanks, Wolfram [1] http://patchwork.ozlabs.org/patch/974584/
On Tue, Oct 09, 2018 at 01:02:10PM +0200, Wolfram Sang wrote: > We recently had this discussion in I2C world about using the parent if > the (logical) device has a NULL fw_node [1]. I don't know if the other > subsystems you modify use logical devices as well? If no, it seems we > need an additional check for the parent in the I2C core only. If yes, > this might be considered in your patchset? SPI has a logical controller device as well.
Hi Silesh, On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote: > Some drivers need to find the device on a bus having a specific firmware > node. Currently, such drivers have their own implementations to do this. > Provide a helper similar to bus_find_device_by_name so that each driver > does not have to reinvent this. > > Signed-off-by: Silesh C V <svellattu@mvista.com> > --- > Changes since v2: > - make use of dev_fwnode in match_fwnode. > > drivers/base/bus.c | 20 ++++++++++++++++++++ > include/linux/device.h | 3 +++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index 8bfd27e..a2f39db 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -17,6 +17,7 @@ > #include <linux/string.h> > #include <linux/mutex.h> > #include <linux/sysfs.h> > +#include <linux/property.h> > #include "base.h" > #include "power/power.h" > > @@ -373,6 +374,25 @@ struct device *bus_find_device_by_name(struct bus_type *bus, > } > EXPORT_SYMBOL_GPL(bus_find_device_by_name); > > +static int match_fwnode(struct device *dev, void *fwnode) > +{ > + return dev_fwnode(dev) == fwnode; > +} > + > +/** > + * bus_find_device_by_fwnode - device iterator for locating a particular device > + * having a specific firmware node > + * @bus: bus type > + * @start: Device to begin with > + * @fwnode: firmware node of the device to match > + */ > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, > + struct fwnode_handle *fwnode) I get the following when running checkpatch on your set: mpoirier@xps15:~/work/linaro/coresight/kernel-maint$ ./scripts/checkpatch.pl 0001-Driver-core-add-bus_find_device_by_fwnode.patch WARNING: line over 80 characters #45: FILE: drivers/base/bus.c:389: +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, total: 0 errors, 1 warnings, 41 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. 0001-Driver-core-add-bus_find_device_by_fwnode.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. Regards, Mathieu > +{ > + return bus_find_device(bus, start, (void *)fwnode, match_fwnode); > +} > +EXPORT_SYMBOL_GPL(bus_find_device_by_fwnode); > + > /** > * subsys_find_device_by_id - find a device with a specific enumeration number > * @subsys: subsystem > diff --git a/include/linux/device.h b/include/linux/device.h > index 8f88254..09384f6 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -171,6 +171,9 @@ struct device *bus_find_device(struct bus_type *bus, struct device *start, > struct device *bus_find_device_by_name(struct bus_type *bus, > struct device *start, > const char *name); > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, > + struct device *start, > + struct fwnode_handle *fwnode); > struct device *subsys_find_device_by_id(struct bus_type *bus, unsigned int id, > struct device *hint); > int bus_for_each_drv(struct bus_type *bus, struct device_driver *start, > -- > 1.9.1 >
On Tue, Oct 9, 2018 at 7:27 PM Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > Hi Silesh, > > On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote: > > Some drivers need to find the device on a bus having a specific firmware > > node. Currently, such drivers have their own implementations to do this. > > Provide a helper similar to bus_find_device_by_name so that each driver > > does not have to reinvent this. > > > > Signed-off-by: Silesh C V <svellattu@mvista.com> > > --- > > Changes since v2: > > - make use of dev_fwnode in match_fwnode. > > > > drivers/base/bus.c | 20 ++++++++++++++++++++ > > include/linux/device.h | 3 +++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > > index 8bfd27e..a2f39db 100644 > > --- a/drivers/base/bus.c > > +++ b/drivers/base/bus.c > > @@ -17,6 +17,7 @@ > > #include <linux/string.h> > > #include <linux/mutex.h> > > #include <linux/sysfs.h> > > +#include <linux/property.h> > > #include "base.h" > > #include "power/power.h" > > > > @@ -373,6 +374,25 @@ struct device *bus_find_device_by_name(struct bus_type *bus, > > } > > EXPORT_SYMBOL_GPL(bus_find_device_by_name); > > > > +static int match_fwnode(struct device *dev, void *fwnode) > > +{ > > + return dev_fwnode(dev) == fwnode; > > +} > > + > > +/** > > + * bus_find_device_by_fwnode - device iterator for locating a particular device > > + * having a specific firmware node > > + * @bus: bus type > > + * @start: Device to begin with > > + * @fwnode: firmware node of the device to match > > + */ > > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, > > + struct fwnode_handle *fwnode) > > I get the following when running checkpatch on your set: > > mpoirier@xps15:~/work/linaro/coresight/kernel-maint$ ./scripts/checkpatch.pl > 0001-Driver-core-add-bus_find_device_by_fwnode.patch > WARNING: line over 80 characters Lines longer than 80 chars often are legitimate. No need to send extra reports about those cases in general. Thanks, Rafael
On Tue, 9 Oct 2018 at 11:39, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Oct 9, 2018 at 7:27 PM Mathieu Poirier > <mathieu.poirier@linaro.org> wrote: > > > > Hi Silesh, > > > > On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote: > > > Some drivers need to find the device on a bus having a specific firmware > > > node. Currently, such drivers have their own implementations to do this. > > > Provide a helper similar to bus_find_device_by_name so that each driver > > > does not have to reinvent this. > > > > > > Signed-off-by: Silesh C V <svellattu@mvista.com> > > > --- > > > Changes since v2: > > > - make use of dev_fwnode in match_fwnode. > > > > > > drivers/base/bus.c | 20 ++++++++++++++++++++ > > > include/linux/device.h | 3 +++ > > > 2 files changed, 23 insertions(+) > > > > > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > > > index 8bfd27e..a2f39db 100644 > > > --- a/drivers/base/bus.c > > > +++ b/drivers/base/bus.c > > > @@ -17,6 +17,7 @@ > > > #include <linux/string.h> > > > #include <linux/mutex.h> > > > #include <linux/sysfs.h> > > > +#include <linux/property.h> > > > #include "base.h" > > > #include "power/power.h" > > > > > > @@ -373,6 +374,25 @@ struct device *bus_find_device_by_name(struct bus_type *bus, > > > } > > > EXPORT_SYMBOL_GPL(bus_find_device_by_name); > > > > > > +static int match_fwnode(struct device *dev, void *fwnode) > > > +{ > > > + return dev_fwnode(dev) == fwnode; > > > +} > > > + > > > +/** > > > + * bus_find_device_by_fwnode - device iterator for locating a particular device > > > + * having a specific firmware node > > > + * @bus: bus type > > > + * @start: Device to begin with > > > + * @fwnode: firmware node of the device to match > > > + */ > > > +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, > > > + struct fwnode_handle *fwnode) > > > > I get the following when running checkpatch on your set: > > > > mpoirier@xps15:~/work/linaro/coresight/kernel-maint$ ./scripts/checkpatch.pl > > 0001-Driver-core-add-bus_find_device_by_fwnode.patch > > WARNING: line over 80 characters > > Lines longer than 80 chars often are legitimate. No need to send > extra reports about those cases in general. In this case I don't see a reason not to abide to the guideline. Wrapping the function declaration to 80 characters would be easy without effecting code readability. Mathieu > > Thanks, > Rafael
Hello Wolfram, On Tue, Oct 9, 2018 at 4:32 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote: > > Some drivers need to find the device on a bus having a specific firmware > > node. Currently, such drivers have their own implementations to do this. > > Provide a helper similar to bus_find_device_by_name so that each driver > > does not have to reinvent this. > > > > Signed-off-by: Silesh C V <svellattu@mvista.com> > > Looks good in general, however: > > We recently had this discussion in I2C world about using the parent if > the (logical) device has a NULL fw_node [1]. I don't know if the other > subsystems you modify use logical devices as well? If no, it seems we > need an additional check for the parent in the I2C core only. If yes, > this might be considered in your patchset? We can add an additional check for dev_fwnode(dev->parent) if match for dev_fwnode(dev) fails in match_fwnode callback. Please correct me if I am wrong. But then, we will be doing these two comparisons for each device (until a match is found) on a bus for a bus_find_device iteration(mostly unnecessarily because I could not find any "match node" callbacks (for bus_find_device) currently doing this). So, wouldn't it be better to add this check in exceptional cases (like in the case of I2C) in the respective subsystems itself ? Thanks, Silesh
Hello Mathieu, Thank you for the review. On Tue, Oct 9, 2018 at 11:18 PM Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > > In this case I don't see a reason not to abide to the guideline. > Wrapping the function declaration to 80 characters would be easy > without effecting code readability. > I had seen this warning but thought it looked better as is. But now, as I have to resend the patches after rebasing on linux-next, I will fix this too in v4. Thanks, Silesh
diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 8bfd27e..a2f39db 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -17,6 +17,7 @@ #include <linux/string.h> #include <linux/mutex.h> #include <linux/sysfs.h> +#include <linux/property.h> #include "base.h" #include "power/power.h" @@ -373,6 +374,25 @@ struct device *bus_find_device_by_name(struct bus_type *bus, } EXPORT_SYMBOL_GPL(bus_find_device_by_name); +static int match_fwnode(struct device *dev, void *fwnode) +{ + return dev_fwnode(dev) == fwnode; +} + +/** + * bus_find_device_by_fwnode - device iterator for locating a particular device + * having a specific firmware node + * @bus: bus type + * @start: Device to begin with + * @fwnode: firmware node of the device to match + */ +struct device *bus_find_device_by_fwnode(struct bus_type *bus, struct device *start, + struct fwnode_handle *fwnode) +{ + return bus_find_device(bus, start, (void *)fwnode, match_fwnode); +} +EXPORT_SYMBOL_GPL(bus_find_device_by_fwnode); + /** * subsys_find_device_by_id - find a device with a specific enumeration number * @subsys: subsystem diff --git a/include/linux/device.h b/include/linux/device.h index 8f88254..09384f6 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -171,6 +171,9 @@ struct device *bus_find_device(struct bus_type *bus, struct device *start, struct device *bus_find_device_by_name(struct bus_type *bus, struct device *start, const char *name); +struct device *bus_find_device_by_fwnode(struct bus_type *bus, + struct device *start, + struct fwnode_handle *fwnode); struct device *subsys_find_device_by_id(struct bus_type *bus, unsigned int id, struct device *hint); int bus_for_each_drv(struct bus_type *bus, struct device_driver *start,
Some drivers need to find the device on a bus having a specific firmware node. Currently, such drivers have their own implementations to do this. Provide a helper similar to bus_find_device_by_name so that each driver does not have to reinvent this. Signed-off-by: Silesh C V <svellattu@mvista.com> --- Changes since v2: - make use of dev_fwnode in match_fwnode. drivers/base/bus.c | 20 ++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 23 insertions(+)