| Message ID | 7105258db927c65dfa41a8abeef4b9735871d023.1464802265.git.Adam.Thomson.Opensource@diasemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/9/2016 5:13 PM, Adam Thomson wrote: > For device nodes in both DT and ACPI, it possible to have named > child nodes which contain properties (an existing example being > gpio-leds). This adds a function to find a named child node for > a device which can be used by drivers for property retrieval. > > For ACPI data node name matching, a helper function is also added > which returns false if CONFIG_ACPI is not set, otherwise it > performs a string comparison on the data node name. This avoids > using the acpi_data_node struct for non CONFIG_ACPI builds, > which would otherwise cause a build failure. > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > Tested-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com> For some reason that didn't make it into the linux-acpi list, or at least I can't see it there. > --- > > Changes in v2: > - Rebase to v4.7-rc1 > > drivers/base/property.c | 28 ++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 7 +++++++ > include/linux/acpi.h | 6 ++++++ > include/linux/property.h | 3 +++ > 4 files changed, 44 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index f38c21d..573b361 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -888,6 +888,34 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev, > EXPORT_SYMBOL_GPL(device_get_next_child_node); > > /** > + * device_get_named_child_node - Return first matching named child node handle > + * @dev: Device to find the named child node for. > + * @childname: String to match child node name against. > + */ > +struct fwnode_handle *device_get_named_child_node(struct device *dev, > + const char *childname) > +{ > + struct fwnode_handle *child; > + > + /* > + * Find first matching named child node of this device. > + * For ACPI this will be a data only sub-node. > + */ > + device_for_each_child_node(dev, child) { > + if (is_of_node(child)) { > + if (!strcasecmp(to_of_node(child)->name, childname)) Why do you use strcasecmp() here? > + return child; > + } else if (is_acpi_data_node(child)) { > + if (acpi_data_node_match(child, childname)) > + return child; > + } > + } > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(device_get_named_child_node); > + > +/** > * fwnode_handle_put - Drop reference to a device node > * @fwnode: Pointer to the device node to drop the reference to. > * > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 788c6c3..993bdd0 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -420,6 +420,13 @@ static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwn > container_of(fwnode, struct acpi_data_node, fwnode) : NULL; > } > > +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, > + const char *name) > +{ > + return is_acpi_data_node(fwnode) ? > + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false; > +} Is there any particular reason to introduce this function instead of doing the test in device_get_named_child_node() directly? > + > static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev) > { > return &adev->fwnode; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 288fac5..03039c4 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -568,6 +568,12 @@ static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwn > return NULL; > } > > +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, > + const char *name) > +{ > + return false; > +} > + > static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev) > { > return NULL; > diff --git a/include/linux/property.h b/include/linux/property.h > index ecab11e..3a2f9ae 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -77,6 +77,9 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev, > for (child = device_get_next_child_node(dev, NULL); child; \ > child = device_get_next_child_node(dev, child)) > > +struct fwnode_handle *device_get_named_child_node(struct device *dev, > + const char *childname); > + > void fwnode_handle_put(struct fwnode_handle *fwnode); > > unsigned int device_get_child_node_count(struct device *dev); > -- Mika, Heikki, Andy, any feedback on this one?
On 10 June 2016 00:11, Rafael J. Wysocki wrote: > For some reason that didn't make it into the linux-acpi list, or at > least I can't see it there. That's strange. I'm not a subscriber to that mailing list, but I assume that shouldn't matter here? Strangely though the only mailing list these seem to have made it to is the ALSA one. :( Will see if I can find out why as I've not seen this problem before. > > + * device_get_named_child_node - Return first matching named child node > handle > > + * @dev: Device to find the named child node for. > > + * @childname: String to match child node name against. > > + */ > > +struct fwnode_handle *device_get_named_child_node(struct device *dev, > > + const char *childname) > > +{ > > + struct fwnode_handle *child; > > + > > + /* > > + * Find first matching named child node of this device. > > + * For ACPI this will be a data only sub-node. > > + */ > > + device_for_each_child_node(dev, child) { > > + if (is_of_node(child)) { > > + if (!strcasecmp(to_of_node(child)->name, childname)) > > Why do you use strcasecmp() here? DT node names are case insensitive. The of.h header does provide a helper macro which is equivalent to this, but that macro is part of the '#ifdef CONFIG_OF' block. If I were to use it then it would cause non-DT builds to fail. I opted for strcasecmp() directly as I didn't think for just this one scenario it made sense to reorganise the of.h header with regards to the helper macros. Of course if there are other opinions on this then am happy to listen. > > +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, > > + const char *name) > > +{ > > + return is_acpi_data_node(fwnode) ? > > + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false; > > +} > > Is there any particular reason to introduce this function instead of > doing the test in device_get_named_child_node() directly? Again this is a build related design option (I mention it in the patch description). In a non-DT build there is no access to the acpi_data_node struct (returned by to_acpi_data_node() call) so if we call this in that scenario then the build will fail. I could have added some #ifdefs to the device_get_named_child_node() function directly, but that would have been a bit messy I think. To me it made more sense to have this helper function which can be called regardless of build type, and for non-ACPI builds its definition always returns false.
On Fri, Jun 10, 2016 at 09:58:39AM +0000, Opensource [Adam Thomson] wrote: > On 10 June 2016 00:11, Rafael J. Wysocki wrote: > > For some reason that didn't make it into the linux-acpi list, or at > > least I can't see it there. > That's strange. I'm not a subscriber to that mailing list, but I assume that > shouldn't matter here? Strangely though the only mailing list these seem to have > made it to is the ALSA one. :( Will see if I can find out why as I've not > seen this problem before. You have a very large number of people on copy, a lot of the lists have restrictions on the number of recipients.
On 13 June 2016 09:47, Mark Brown wrote: > > That's strange. I'm not a subscriber to that mailing list, but I assume that > > shouldn't matter here? Strangely though the only mailing list these seem to have > > made it to is the ALSA one. :( Will see if I can find out why as I've not > > seen this problem before. > > You have a very large number of people on copy, a lot of the lists have > restrictions on the number of recipients. Thanks Mark. Good to know for future mails.
Hi Adam, (comment below and adding Rob to the cc:) On 06/10/16 02:58, Opensource [Adam Thomson] wrote: > On 10 June 2016 00:11, Rafael J. Wysocki wrote: > >> For some reason that didn't make it into the linux-acpi list, or at >> least I can't see it there. > > That's strange. I'm not a subscriber to that mailing list, but I assume that > shouldn't matter here? Strangely though the only mailing list these seem to have > made it to is the ALSA one. :( Will see if I can find out why as I've not > seen this problem before. > >>> + * device_get_named_child_node - Return first matching named child node >> handle >>> + * @dev: Device to find the named child node for. >>> + * @childname: String to match child node name against. >>> + */ >>> +struct fwnode_handle *device_get_named_child_node(struct device *dev, >>> + const char *childname) >>> +{ >>> + struct fwnode_handle *child; >>> + >>> + /* >>> + * Find first matching named child node of this device. >>> + * For ACPI this will be a data only sub-node. >>> + */ >>> + device_for_each_child_node(dev, child) { >>> + if (is_of_node(child)) { >>> + if (!strcasecmp(to_of_node(child)->name, childname)) >> >> Why do you use strcasecmp() here? > > DT node names are case insensitive. The of.h header does provide a helper macro > which is equivalent to this, but that macro is part of the '#ifdef CONFIG_OF' > block. If I were to use it then it would cause non-DT builds to fail. I opted > for strcasecmp() directly as I didn't think for just this one scenario it made > sense to reorganise the of.h header with regards to the helper macros. Of course > if there are other opinions on this then am happy to listen. DT node names are not always case insensitive. Please us of_node_cmp(). -Frank > >>> +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, >>> + const char *name) >>> +{ >>> + return is_acpi_data_node(fwnode) ? >>> + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false; >>> +} >> >> Is there any particular reason to introduce this function instead of >> doing the test in device_get_named_child_node() directly? > > Again this is a build related design option (I mention it in the patch > description). In a non-DT build there is no access to the acpi_data_node struct > (returned by to_acpi_data_node() call) so if we call this in that scenario then > the build will fail. I could have added some #ifdefs to the > device_get_named_child_node() function directly, but that would have been a bit > messy I think. To me it made more sense to have this helper function which can > be called regardless of build type, and for non-ACPI builds its definition > always returns false. >
On 13 June 2016 20:33, Frank Rowand wrote: > > DT node names are case insensitive. The of.h header does provide a helper macro > > which is equivalent to this, but that macro is part of the '#ifdef CONFIG_OF' > > block. If I were to use it then it would cause non-DT builds to fail. I opted > > for strcasecmp() directly as I didn't think for just this one scenario it made > > sense to reorganise the of.h header with regards to the helper macros. Of course > > if there are other opinions on this then am happy to listen. > > DT node names are not always case insensitive. Please us of_node_cmp(). > > -Frank Ok, fair enough. I'll have to move those definitions in the of.h header out of the CONFIG_OF block then.
diff --git a/drivers/base/property.c b/drivers/base/property.c index f38c21d..573b361 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -888,6 +888,34 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev, EXPORT_SYMBOL_GPL(device_get_next_child_node); /** + * device_get_named_child_node - Return first matching named child node handle + * @dev: Device to find the named child node for. + * @childname: String to match child node name against. + */ +struct fwnode_handle *device_get_named_child_node(struct device *dev, + const char *childname) +{ + struct fwnode_handle *child; + + /* + * Find first matching named child node of this device. + * For ACPI this will be a data only sub-node. + */ + device_for_each_child_node(dev, child) { + if (is_of_node(child)) { + if (!strcasecmp(to_of_node(child)->name, childname)) + return child; + } else if (is_acpi_data_node(child)) { + if (acpi_data_node_match(child, childname)) + return child; + } + } + + return NULL; +} +EXPORT_SYMBOL_GPL(device_get_named_child_node); + +/** * fwnode_handle_put - Drop reference to a device node * @fwnode: Pointer to the device node to drop the reference to. * diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 788c6c3..993bdd0 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -420,6 +420,13 @@ static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwn container_of(fwnode, struct acpi_data_node, fwnode) : NULL; } +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, + const char *name) +{ + return is_acpi_data_node(fwnode) ? + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false; +} + static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev) { return &adev->fwnode; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 288fac5..03039c4 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -568,6 +568,12 @@ static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwn return NULL; } +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode, + const char *name) +{ + return false; +} + static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev) { return NULL; diff --git a/include/linux/property.h b/include/linux/property.h index ecab11e..3a2f9ae 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -77,6 +77,9 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev, for (child = device_get_next_child_node(dev, NULL); child; \ child = device_get_next_child_node(dev, child)) +struct fwnode_handle *device_get_named_child_node(struct device *dev, + const char *childname); + void fwnode_handle_put(struct fwnode_handle *fwnode); unsigned int device_get_child_node_count(struct device *dev);