| Message ID | 20170114214713.28109-6-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jan 14, 2017 at 01:47:08PM -0800, Florian Fainelli wrote: > Add a helper function to lookup a device reference given a class name. > This is a preliminary patch to remove adhoc code from net/dsa/dsa.c and > make it more generic. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/base/core.c | 19 +++++++++++++++++++ > include/linux/device.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 020ea7f05520..3dd6047c10d8 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -2065,6 +2065,25 @@ struct device *device_find_child(struct device *parent, void *data, > } > EXPORT_SYMBOL_GPL(device_find_child); > > +static int dev_is_class(struct device *dev, void *class) > +{ > + if (dev->class != NULL && !strcmp(dev->class->name, class)) > + return 1; > + > + return 0; > +} > + > +struct device *device_find_class(struct device *parent, char *class) Why are you using the char * for a class, and not just a pointer to "struct class"? That seems to be the most logical one, no need to rely on string comparisons here. Also, what is this being used for? You aren't trying to walk up the device heirachy to find a specific "type" of device, are you? If so, ugh, I ranted about this in the past when the hyperv driver was trying to do such a thing... > +{ > + if (dev_is_class(parent, class)) { > + get_device(parent); > + return parent; > + } > + > + return device_find_child(parent, class, dev_is_class); You are trying to find a peer device with the same parent that belongs to a specific class? Again, what is this being used for? And all exported driver core functions should have full kerneldoc information for them so that people know how to use them, and what the constraints are (see device_find_child() as an example.) Please do that here as well because you are returning a pointer to a structure with the reference count incremented, callers need to know that. thanks, greg k-h
On 01/15/2017 03:04 AM, Greg KH wrote: > On Sat, Jan 14, 2017 at 01:47:08PM -0800, Florian Fainelli wrote: >> Add a helper function to lookup a device reference given a class name. >> This is a preliminary patch to remove adhoc code from net/dsa/dsa.c and >> make it more generic. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/base/core.c | 19 +++++++++++++++++++ >> include/linux/device.h | 1 + >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 020ea7f05520..3dd6047c10d8 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -2065,6 +2065,25 @@ struct device *device_find_child(struct device *parent, void *data, >> } >> EXPORT_SYMBOL_GPL(device_find_child); >> >> +static int dev_is_class(struct device *dev, void *class) >> +{ >> + if (dev->class != NULL && !strcmp(dev->class->name, class)) >> + return 1; >> + >> + return 0; >> +} >> + >> +struct device *device_find_class(struct device *parent, char *class) > > Why are you using the char * for a class, and not just a pointer to > "struct class"? That seems to be the most logical one, no need to rely > on string comparisons here. A more reflective name of what that does would probably be device_find_by_class_name() or something alike. > > Also, what is this being used for? You aren't trying to walk up the > device heirachy to find a specific "type" of device, are you? If so, > ugh, I ranted about this in the past when the hyperv driver was trying > to do such a thing... What's a better way to do that though? > >> +{ >> + if (dev_is_class(parent, class)) { >> + get_device(parent); >> + return parent; >> + } >> + >> + return device_find_child(parent, class, dev_is_class); > > You are trying to find a peer device with the same parent that belongs > to a specific class? Correct, network devices, and MDIO bus devices usually (always?) set dev.parent. > > Again, what is this being used for? See my other replies in patches 6, 7 and how it is used in patches 8 and 10 for instance. > > And all exported driver core functions should have full kerneldoc > information for them so that people know how to use them, and what the > constraints are (see device_find_child() as an example.) Please do that > here as well because you are returning a pointer to a structure with the > reference count incremented, callers need to know that. Sure.
diff --git a/drivers/base/core.c b/drivers/base/core.c index 020ea7f05520..3dd6047c10d8 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2065,6 +2065,25 @@ struct device *device_find_child(struct device *parent, void *data, } EXPORT_SYMBOL_GPL(device_find_child); +static int dev_is_class(struct device *dev, void *class) +{ + if (dev->class != NULL && !strcmp(dev->class->name, class)) + return 1; + + return 0; +} + +struct device *device_find_class(struct device *parent, char *class) +{ + if (dev_is_class(parent, class)) { + get_device(parent); + return parent; + } + + return device_find_child(parent, class, dev_is_class); +} +EXPORT_SYMBOL_GPL(device_find_class); + int __init devices_init(void) { devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL); diff --git a/include/linux/device.h b/include/linux/device.h index 491b4c0ca633..8d37f5ecb972 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1120,6 +1120,7 @@ extern int device_for_each_child_reverse(struct device *dev, void *data, int (*fn)(struct device *dev, void *data)); extern struct device *device_find_child(struct device *dev, void *data, int (*match)(struct device *dev, void *data)); +extern struct device *device_find_class(struct device *parent, char *class); extern int device_rename(struct device *dev, const char *new_name); extern int device_move(struct device *dev, struct device *new_parent, enum dpm_order dpm_order);
Add a helper function to lookup a device reference given a class name. This is a preliminary patch to remove adhoc code from net/dsa/dsa.c and make it more generic. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/base/core.c | 19 +++++++++++++++++++ include/linux/device.h | 1 + 2 files changed, 20 insertions(+)