| Message ID | 20201215164315.3666-11-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 12316 this patch: 12316 |
| netdev/kdoc | success | Errors and warnings before: 5 this patch: 5 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 39 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 11702 this patch: 11702 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Hi Calvin, Thank you for the patch. On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote: > Using fwnode_get_id(), get the reg property value for DT node > and get the _ADR object value for ACPI node. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > Changes in v2: None > > drivers/base/property.c | 26 ++++++++++++++++++++++++++ > include/linux/property.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 4c43d30145c6..1c50e17ae879 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) > return fwnode_call_ptr_op(fwnode, get_name_prefix); > } > > +/** > + * fwnode_get_id - Get the id of a fwnode. > + * @fwnode: firmware node > + * @id: id of the fwnode > + * Is the concept of fwnode ID documented clearly somewhere ? I think this function should otherwise have more documentation, at least to explain what the ID is. > + * Returns 0 on success or a negative errno. > + */ > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > +{ > + unsigned long long adr; > + acpi_status status; > + > + if (is_of_node(fwnode)) { > + return of_property_read_u32(to_of_node(fwnode), "reg", id); > + } else if (is_acpi_node(fwnode)) { > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > + METHOD_NAME__ADR, NULL, &adr); > + if (ACPI_FAILURE(status)) > + return -ENODATA; Would it make sense to standardize error codes ? of_property_read_u32() can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of this function would be very interested to tell those three cases apart. Maybe we should return -EINVAL in all error cases ? Or maybe different error codes to mean "the backend doesn't support the concept of IDs", and "the device doesn't have an ID" ? > + *id = (u32)adr; > + return 0; > + } > + return -EINVAL; > +} > +EXPORT_SYMBOL_GPL(fwnode_get_id); > + > /** > * fwnode_get_parent - Return parent firwmare node > * @fwnode: Firmware whose parent is retrieved > diff --git a/include/linux/property.h b/include/linux/property.h > index 2d4542629d80..92d405cf2b07 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, > > const char *fwnode_get_name(const struct fwnode_handle *fwnode); > const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode); > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id); > struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode); > struct fwnode_handle *fwnode_get_next_parent( > struct fwnode_handle *fwnode);
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Using fwnode_get_id(), get the reg property value for DT node > and get the _ADR object value for ACPI node. and -> or ... > +/** > + * fwnode_get_id - Get the id of a fwnode. > + * @fwnode: firmware node > + * @id: id of the fwnode > + * > + * Returns 0 on success or a negative errno. > + */ > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > +{ > + unsigned long long adr; > + acpi_status status; > + > + if (is_of_node(fwnode)) { > + return of_property_read_u32(to_of_node(fwnode), "reg", id); ACPI nodes can hold reg property as well. I would rather think about ret = fwnode_property_read_u32(fwnode, "reg", id) if (!(ret && is_acpi_node(fwnode))) return ret; > + } else if (is_acpi_node(fwnode)) { Redundant 'else' > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > + METHOD_NAME__ADR, NULL, &adr); > + if (ACPI_FAILURE(status)) > + return -ENODATA; I'm wondering if it compiles when CONFIG_ACPI=n. > + *id = (u32)adr; > + return 0; > + } > + return -EINVAL; > +}
On Tue, Dec 15, 2020 at 7:00 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote: > > Using fwnode_get_id(), get the reg property value for DT node > > and get the _ADR object value for ACPI node. ... > > +/** > > + * fwnode_get_id - Get the id of a fwnode. > > + * @fwnode: firmware node > > + * @id: id of the fwnode > > Is the concept of fwnode ID documented clearly somewhere ? I think this > function should otherwise have more documentation, at least to explain > what the ID is. I'm afraid that OF has no clear concept of this either. It's usually used as a unique ID for the children of some device (like MFD) and can represent a lot of things. But I agree it should be documented. Rob, any ideas about this? > > + * Returns 0 on success or a negative errno. > > + */ > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > > +{ > > + unsigned long long adr; > > + acpi_status status; > > + > > + if (is_of_node(fwnode)) { > > + return of_property_read_u32(to_of_node(fwnode), "reg", id); > > + } else if (is_acpi_node(fwnode)) { > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > + METHOD_NAME__ADR, NULL, &adr); > > + if (ACPI_FAILURE(status)) > > + return -ENODATA; > > Would it make sense to standardize error codes ? of_property_read_u32() > can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of > this function would be very interested to tell those three cases apart. > Maybe we should return -EINVAL in all error cases ? Or maybe different > error codes to mean "the backend doesn't support the concept of IDs", > and "the device doesn't have an ID" ? We may actually get mapping to all three if first we check for the method/name existence followed by value check. But I don't think we need to bloat this simple one. > > + *id = (u32)adr; > > + return 0; > > + } > > + return -EINVAL; > > +}
Hi Laurent, Thanks for reviewing. On Tue, Dec 15, 2020 at 07:00:28PM +0200, Laurent Pinchart wrote: > Hi Calvin, > > Thank you for the patch. > > On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote: > > Using fwnode_get_id(), get the reg property value for DT node > > and get the _ADR object value for ACPI node. > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > > > Changes in v2: None > > > > drivers/base/property.c | 26 ++++++++++++++++++++++++++ > > include/linux/property.h | 1 + > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > index 4c43d30145c6..1c50e17ae879 100644 > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) > > return fwnode_call_ptr_op(fwnode, get_name_prefix); > > } > > > > +/** > > + * fwnode_get_id - Get the id of a fwnode. > > + * @fwnode: firmware node > > + * @id: id of the fwnode > > + * > > Is the concept of fwnode ID documented clearly somewhere ? I think this > function should otherwise have more documentation, at least to explain > what the ID is. Agree. Will add more info here. > > > + * Returns 0 on success or a negative errno. > > + */ > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > > +{ > > + unsigned long long adr; > > + acpi_status status; > > + > > + if (is_of_node(fwnode)) { > > + return of_property_read_u32(to_of_node(fwnode), "reg", id); > > + } else if (is_acpi_node(fwnode)) { > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > + METHOD_NAME__ADR, NULL, &adr); > > + if (ACPI_FAILURE(status)) > > + return -ENODATA; > > Would it make sense to standardize error codes ? of_property_read_u32() > can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of > this function would be very interested to tell those three cases apart. > Maybe we should return -EINVAL in all error cases ? Or maybe different > error codes to mean "the backend doesn't support the concept of IDs", > and "the device doesn't have an ID" ? I think it make sense to return just -EINVAL. Will take care in v3. Thanks Calvin
On Tue, Dec 15, 2020 at 07:45:16PM +0200, Andy Shevchenko wrote: > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > Using fwnode_get_id(), get the reg property value for DT node > > and get the _ADR object value for ACPI node. > > and -> or > > ... > > > +/** > > + * fwnode_get_id - Get the id of a fwnode. > > + * @fwnode: firmware node > > + * @id: id of the fwnode > > + * > > + * Returns 0 on success or a negative errno. > > + */ > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > > +{ > > + unsigned long long adr; > > + acpi_status status; > > + > > + if (is_of_node(fwnode)) { > > + return of_property_read_u32(to_of_node(fwnode), "reg", id); > > ACPI nodes can hold reg property as well. I would rather think about > > ret = fwnode_property_read_u32(fwnode, "reg", id) > if (!(ret && is_acpi_node(fwnode))) > return ret; > Got it. Will rework on it. > > + } else if (is_acpi_node(fwnode)) { > > Redundant 'else' > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > + METHOD_NAME__ADR, NULL, &adr); > > + if (ACPI_FAILURE(status)) > > + return -ENODATA; > > I'm wondering if it compiles when CONFIG_ACPI=n. Correct. It doesn't compile for non-ACPI case. Will resolve it. Thanks Calvin
diff --git a/drivers/base/property.c b/drivers/base/property.c index 4c43d30145c6..1c50e17ae879 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) return fwnode_call_ptr_op(fwnode, get_name_prefix); } +/** + * fwnode_get_id - Get the id of a fwnode. + * @fwnode: firmware node + * @id: id of the fwnode + * + * Returns 0 on success or a negative errno. + */ +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) +{ + unsigned long long adr; + acpi_status status; + + if (is_of_node(fwnode)) { + return of_property_read_u32(to_of_node(fwnode), "reg", id); + } else if (is_acpi_node(fwnode)) { + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), + METHOD_NAME__ADR, NULL, &adr); + if (ACPI_FAILURE(status)) + return -ENODATA; + *id = (u32)adr; + return 0; + } + return -EINVAL; +} +EXPORT_SYMBOL_GPL(fwnode_get_id); + /** * fwnode_get_parent - Return parent firwmare node * @fwnode: Firmware whose parent is retrieved diff --git a/include/linux/property.h b/include/linux/property.h index 2d4542629d80..92d405cf2b07 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, const char *fwnode_get_name(const struct fwnode_handle *fwnode); const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode); +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id); struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode); struct fwnode_handle *fwnode_get_next_parent( struct fwnode_handle *fwnode);
Using fwnode_get_id(), get the reg property value for DT node and get the _ADR object value for ACPI node. Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v2: None drivers/base/property.c | 26 ++++++++++++++++++++++++++ include/linux/property.h | 1 + 2 files changed, 27 insertions(+)