From mboxrd@z Thu Jan 1 00:00:00 1970 From: gregkh@linuxfoundation.org (Greg KH) Date: Sun, 15 Jan 2017 12:04:55 +0100 Subject: [PATCH net-next v3 05/10] drivers: base: Add device_find_class() In-Reply-To: <20170114214713.28109-6-f.fainelli@gmail.com> References: <20170114214713.28109-1-f.fainelli@gmail.com> <20170114214713.28109-6-f.fainelli@gmail.com> Message-ID: <20170115110455.GE26374@kroah.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > --- > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751014AbdAOLEn (ORCPT ); Sun, 15 Jan 2017 06:04:43 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:54936 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750812AbdAOLEm (ORCPT ); Sun, 15 Jan 2017 06:04:42 -0500 Date: Sun, 15 Jan 2017 12:04:55 +0100 From: Greg KH To: Florian Fainelli Cc: netdev@vger.kernel.org, Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Gregory Clement , Russell King , Vivien Didelot , "David S. Miller" , "moderated list:ARM SUB-ARCHITECTURES" , open list Subject: Re: [PATCH net-next v3 05/10] drivers: base: Add device_find_class() Message-ID: <20170115110455.GE26374@kroah.com> References: <20170114214713.28109-1-f.fainelli@gmail.com> <20170114214713.28109-6-f.fainelli@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170114214713.28109-6-f.fainelli@gmail.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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