Message ID | 20250203084906.681418-13-apatel@ventanamicro.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Linux SBI MPXY and RPMI drivers | expand |
Context | Check | Description |
---|---|---|
bjorn/pre-ci_am | success | Success |
bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
bjorn/checkpatch | success | checkpatch |
bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
bjorn/header-inline | success | header-inline |
bjorn/kdoc | success | kdoc |
bjorn/module-param | success | module-param |
bjorn/verify-fixes | success | verify-fixes |
bjorn/verify-signedoff | success | verify-signedoff |
On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > From: Sunil V L <sunilvl@ventanamicro.com> > > fwnode_get_reference_args() which is common for both DT and ACPI passes > a property name like #mbox-cells which needs to be fetched from the > reference node to determine the number of arguments needed for the > property. However, the ACPI version of this function doesn't support > this and simply ignores the parameter passed from the wrapper function. > Add support for dynamically finding number of arguments by reading the > nargs property value. Update the callers to pass extra parameter. I don't like this (implementation). It seems that we basically have two parameters which values are duplicating each other. This is error prone API and confusing in the cases when both are defined. If you want property, add a new API that takes const char *nargs and relies on the property be present. Your patch becomes much simpler, and solution is robust against potential confusion on how to treat the corner cases. Note, Rafael might have different opinion and his has the last word. But here just my view on the implementation details.
On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > a property name like #mbox-cells which needs to be fetched from the > > reference node to determine the number of arguments needed for the > > property. However, the ACPI version of this function doesn't support > > this and simply ignores the parameter passed from the wrapper function. > > Add support for dynamically finding number of arguments by reading the > > nargs property value. Update the callers to pass extra parameter. > > I don't like this (implementation). Agree. > It seems that we basically have two parameters which values are duplicating > each other. This is error prone API and confusing in the cases when both are > defined. If you want property, add a new API that takes const char *nargs > and relies on the property be present. Also this is not really needed for ACPI case because it has types so it can distinguish references from integer. Having separate property for this just makes things more complex than they need to be IMHO.
On Mon, Feb 03, 2025 at 12:58:40PM +0200, Mika Westerberg wrote: > On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > > a property name like #mbox-cells which needs to be fetched from the > > > reference node to determine the number of arguments needed for the > > > property. However, the ACPI version of this function doesn't support > > > this and simply ignores the parameter passed from the wrapper function. > > > Add support for dynamically finding number of arguments by reading the > > > nargs property value. Update the callers to pass extra parameter. > > > > I don't like this (implementation). > > Agree. > > > It seems that we basically have two parameters which values are duplicating > > each other. This is error prone API and confusing in the cases when both are > > defined. If you want property, add a new API that takes const char *nargs > > and relies on the property be present. > > Also this is not really needed for ACPI case because it has types so it can > distinguish references from integer. Having separate property for this just > makes things more complex than they need to be IMHO. Thanks! Andy and Mika for your kind feedback. I agree that having both property name and nargs is confusing and also ACPI would not need nargs_prop. In fact, I think ACPI doesn't need even nargs integer value as well from the caller since all integers after the reference are counted as arguments. However, the issue is acpi_get_ref_args() assumes that caller passes valid num_args. But typically the common fwnode_property_get_reference_args() doesn't usually pass both valid values. So, should fwnode_property_get_reference_args() pass both nargs_prop (for DT) and nargs (for ACPI). Or do you mean it is better to remove the check for num_args in the loop inside acpi_get_ref_args() function? Thanks, Sunil
On Mon, Feb 03, 2025 at 05:54:18PM +0530, Sunil V L wrote: > On Mon, Feb 03, 2025 at 12:58:40PM +0200, Mika Westerberg wrote: > > On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > > > a property name like #mbox-cells which needs to be fetched from the > > > > reference node to determine the number of arguments needed for the > > > > property. However, the ACPI version of this function doesn't support > > > > this and simply ignores the parameter passed from the wrapper function. > > > > Add support for dynamically finding number of arguments by reading the > > > > nargs property value. Update the callers to pass extra parameter. > > > > > > I don't like this (implementation). > > > > Agree. > > > > > It seems that we basically have two parameters which values are duplicating > > > each other. This is error prone API and confusing in the cases when both are > > > defined. If you want property, add a new API that takes const char *nargs > > > and relies on the property be present. > > > > Also this is not really needed for ACPI case because it has types so it can > > distinguish references from integer. Having separate property for this just > > makes things more complex than they need to be IMHO. > > Thanks! Andy and Mika for your kind feedback. I agree that having both > property name and nargs is confusing and also ACPI would not need > nargs_prop. In fact, I think ACPI doesn't need even nargs integer value > as well from the caller since all integers after the reference are > counted as arguments. However, the issue is acpi_get_ref_args() assumes > that caller passes valid num_args. But typically the common > fwnode_property_get_reference_args() doesn't usually pass both valid > values. So, should fwnode_property_get_reference_args() pass both > nargs_prop (for DT) and nargs (for ACPI). Or do you mean it is better to > remove the check for num_args in the loop inside acpi_get_ref_args() > function? Can you show an example of a case you are trying to solve with this? So far we have been able to go with the current implementation so why this is needed now?
On Mon, Feb 03, 2025 at 02:36:58PM +0200, Mika Westerberg wrote: > On Mon, Feb 03, 2025 at 05:54:18PM +0530, Sunil V L wrote: > > On Mon, Feb 03, 2025 at 12:58:40PM +0200, Mika Westerberg wrote: > > > On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > > > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > > > > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > > > > a property name like #mbox-cells which needs to be fetched from the > > > > > reference node to determine the number of arguments needed for the > > > > > property. However, the ACPI version of this function doesn't support > > > > > this and simply ignores the parameter passed from the wrapper function. > > > > > Add support for dynamically finding number of arguments by reading the > > > > > nargs property value. Update the callers to pass extra parameter. > > > > > > > > I don't like this (implementation). > > > > > > Agree. > > > > > > > It seems that we basically have two parameters which values are duplicating > > > > each other. This is error prone API and confusing in the cases when both are > > > > defined. If you want property, add a new API that takes const char *nargs > > > > and relies on the property be present. > > > > > > Also this is not really needed for ACPI case because it has types so it can > > > distinguish references from integer. Having separate property for this just > > > makes things more complex than they need to be IMHO. > > > > Thanks! Andy and Mika for your kind feedback. I agree that having both > > property name and nargs is confusing and also ACPI would not need > > nargs_prop. In fact, I think ACPI doesn't need even nargs integer value > > as well from the caller since all integers after the reference are > > counted as arguments. However, the issue is acpi_get_ref_args() assumes > > that caller passes valid num_args. But typically the common > > fwnode_property_get_reference_args() doesn't usually pass both valid > > values. So, should fwnode_property_get_reference_args() pass both > > nargs_prop (for DT) and nargs (for ACPI). Or do you mean it is better to > > remove the check for num_args in the loop inside acpi_get_ref_args() > > function? > > Can you show an example of a case you are trying to solve with this? So far > we have been able to go with the current implementation so why this is > needed now? Basically one can call fwnode_property_get_reference_args() irrespective of DT/ACPI. The case we are trying is like below. if (fwnode_property_get_reference_args(dev->fwnode, "mboxes", "#mbox-cells", 0, index, &fwspec)) { ... } As you can see this works for DT since OF interface handles "#mbox-cells". But since nargs is passed as 0, it won't work for ACPI due to the reason I mentioned earlier. Mandating to pass both "#mbox-cell" and valid nargs count looks redundant to me. Thanks, Sunil
On Mon, Feb 03, 2025 at 07:21:50PM +0530, Sunil V L wrote: > On Mon, Feb 03, 2025 at 02:36:58PM +0200, Mika Westerberg wrote: > > On Mon, Feb 03, 2025 at 05:54:18PM +0530, Sunil V L wrote: > > > On Mon, Feb 03, 2025 at 12:58:40PM +0200, Mika Westerberg wrote: > > > > On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > > > > > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > > > > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > > > > > > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > > > > > a property name like #mbox-cells which needs to be fetched from the > > > > > > reference node to determine the number of arguments needed for the > > > > > > property. However, the ACPI version of this function doesn't support > > > > > > this and simply ignores the parameter passed from the wrapper function. > > > > > > Add support for dynamically finding number of arguments by reading the > > > > > > nargs property value. Update the callers to pass extra parameter. > > > > > > > > > > I don't like this (implementation). > > > > > > > > Agree. > > > > > > > > > It seems that we basically have two parameters which values are duplicating > > > > > each other. This is error prone API and confusing in the cases when both are > > > > > defined. If you want property, add a new API that takes const char *nargs > > > > > and relies on the property be present. > > > > > > > > Also this is not really needed for ACPI case because it has types so it can > > > > distinguish references from integer. Having separate property for this just > > > > makes things more complex than they need to be IMHO. > > > > > > Thanks! Andy and Mika for your kind feedback. I agree that having both > > > property name and nargs is confusing and also ACPI would not need > > > nargs_prop. In fact, I think ACPI doesn't need even nargs integer value > > > as well from the caller since all integers after the reference are > > > counted as arguments. However, the issue is acpi_get_ref_args() assumes > > > that caller passes valid num_args. But typically the common > > > fwnode_property_get_reference_args() doesn't usually pass both valid > > > values. So, should fwnode_property_get_reference_args() pass both > > > nargs_prop (for DT) and nargs (for ACPI). Or do you mean it is better to > > > remove the check for num_args in the loop inside acpi_get_ref_args() > > > function? > > > > Can you show an example of a case you are trying to solve with this? So far > > we have been able to go with the current implementation so why this is > > needed now? > > Basically one can call fwnode_property_get_reference_args() > irrespective of DT/ACPI. The case we are trying is like below. > > if (fwnode_property_get_reference_args(dev->fwnode, "mboxes", > "#mbox-cells", 0, index, &fwspec)) { > ... > } > > As you can see this works for DT since OF interface handles > "#mbox-cells". But since nargs is passed as 0, it won't work for ACPI > due to the reason I mentioned earlier. > > Mandating to pass both "#mbox-cell" and valid nargs count looks > redundant to me. Ah, interesting. The original change that introduces this 3e3119d3088f ("device property: Introduce fwnode_property_get_reference_args") hadn't been reviewed by Mika or me, that's probably why we are not familiar with. Since interface is already established, I would recommend to fix this as proposed, i.e. with a new API. This is the way to match how OF seems to be doing.
On Mon, Feb 03, 2025 at 04:39:11PM +0200, Andy Shevchenko wrote: > On Mon, Feb 03, 2025 at 07:21:50PM +0530, Sunil V L wrote: > > On Mon, Feb 03, 2025 at 02:36:58PM +0200, Mika Westerberg wrote: > > > On Mon, Feb 03, 2025 at 05:54:18PM +0530, Sunil V L wrote: > > > > On Mon, Feb 03, 2025 at 12:58:40PM +0200, Mika Westerberg wrote: > > > > > On Mon, Feb 03, 2025 at 11:43:26AM +0200, Andy Shevchenko wrote: > > > > > > On Mon, Feb 03, 2025 at 02:19:01PM +0530, Anup Patel wrote: > > > > > > > From: Sunil V L <sunilvl@ventanamicro.com> > > > > > > > > > > > > > > fwnode_get_reference_args() which is common for both DT and ACPI passes > > > > > > > a property name like #mbox-cells which needs to be fetched from the > > > > > > > reference node to determine the number of arguments needed for the > > > > > > > property. However, the ACPI version of this function doesn't support > > > > > > > this and simply ignores the parameter passed from the wrapper function. > > > > > > > Add support for dynamically finding number of arguments by reading the > > > > > > > nargs property value. Update the callers to pass extra parameter. > > > > > > > > > > > > I don't like this (implementation). > > > > > > > > > > Agree. > > > > > > > > > > > It seems that we basically have two parameters which values are duplicating > > > > > > each other. This is error prone API and confusing in the cases when both are > > > > > > defined. If you want property, add a new API that takes const char *nargs > > > > > > and relies on the property be present. > > > > > > > > > > Also this is not really needed for ACPI case because it has types so it can > > > > > distinguish references from integer. Having separate property for this just > > > > > makes things more complex than they need to be IMHO. > > > > > > > > Thanks! Andy and Mika for your kind feedback. I agree that having both > > > > property name and nargs is confusing and also ACPI would not need > > > > nargs_prop. In fact, I think ACPI doesn't need even nargs integer value > > > > as well from the caller since all integers after the reference are > > > > counted as arguments. However, the issue is acpi_get_ref_args() assumes > > > > that caller passes valid num_args. But typically the common > > > > fwnode_property_get_reference_args() doesn't usually pass both valid > > > > values. So, should fwnode_property_get_reference_args() pass both > > > > nargs_prop (for DT) and nargs (for ACPI). Or do you mean it is better to > > > > remove the check for num_args in the loop inside acpi_get_ref_args() > > > > function? > > > > > > Can you show an example of a case you are trying to solve with this? So far > > > we have been able to go with the current implementation so why this is > > > needed now? > > > > Basically one can call fwnode_property_get_reference_args() > > irrespective of DT/ACPI. The case we are trying is like below. > > > > if (fwnode_property_get_reference_args(dev->fwnode, "mboxes", > > "#mbox-cells", 0, index, &fwspec)) { > > ... > > } > > > > As you can see this works for DT since OF interface handles > > "#mbox-cells". But since nargs is passed as 0, it won't work for ACPI > > due to the reason I mentioned earlier. > > > > Mandating to pass both "#mbox-cell" and valid nargs count looks > > redundant to me. > > Ah, interesting. The original change that introduces this 3e3119d3088f ("device > property: Introduce fwnode_property_get_reference_args") hadn't been reviewed > by Mika or me, that's probably why we are not familiar with. > > Since interface is already established, I would recommend to fix > this as proposed, i.e. with a new API. This is the way to match > how OF seems to be doing. For the reference see implementation of of_fwnode_get_reference_args() if (nargs_prop) ret = of_parse_phandle_with_args(to_of_node(fwnode), prop, nargs_prop, index, &of_args); else ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop, nargs, index, &of_args);
On Mon, Feb 03, 2025 at 04:41:35PM +0200, Andy Shevchenko wrote: > > Ah, interesting. The original change that introduces this 3e3119d3088f ("device > > property: Introduce fwnode_property_get_reference_args") hadn't been reviewed > > by Mika or me, that's probably why we are not familiar with. > > > > Since interface is already established, I would recommend to fix > > this as proposed, i.e. with a new API. This is the way to match > > how OF seems to be doing. > > For the reference see implementation of of_fwnode_get_reference_args() > > if (nargs_prop) > ret = of_parse_phandle_with_args(to_of_node(fwnode), prop, > nargs_prop, index, &of_args); > else > ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop, > nargs, index, &of_args); > > Thanks!. I can do similar. But the change in __acpi_node_get_property_reference() will be still required since that is the place where the actual decoding of AML object is done. That would be similar to __of_parse_phandle_with_args() as well. Hope that is fine. Thanks, Sunil
On Tue, Feb 04, 2025 at 10:28:03PM +0530, Sunil V L wrote: > On Mon, Feb 03, 2025 at 04:41:35PM +0200, Andy Shevchenko wrote: > > > Ah, interesting. The original change that introduces this 3e3119d3088f ("device > > > property: Introduce fwnode_property_get_reference_args") hadn't been reviewed > > > by Mika or me, that's probably why we are not familiar with. > > > > > > Since interface is already established, I would recommend to fix > > > this as proposed, i.e. with a new API. This is the way to match > > > how OF seems to be doing. > > > > For the reference see implementation of of_fwnode_get_reference_args() > > > > if (nargs_prop) > > ret = of_parse_phandle_with_args(to_of_node(fwnode), prop, > > nargs_prop, index, &of_args); > > else > > ret = of_parse_phandle_with_fixed_args(to_of_node(fwnode), prop, > > nargs, index, &of_args); > > > > > Thanks!. I can do similar. But the change in > __acpi_node_get_property_reference() will be still required since that > is the place where the actual decoding of AML object is done. That would > be similar to __of_parse_phandle_with_args() as well. Hope that is fine. You don't need that. Split the core part to local static helper that takes whatever it needs, but being not visible to anyone outside drivers/acpi/property.c. And build the current helper and new visible one on the basis of this split. For better reviewing and maintaining you can split this approach to two patches: 1) preparatory by splitting a new local helper; 2) the introduction of a new API.
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 98d93ed58315..ddea5dec70bd 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -887,6 +887,9 @@ static struct fwnode_handle *acpi_parse_string_ref(const struct fwnode_handle *f * @fwnode: Firmware node to get the property from * @propname: Name of the property * @index: Index of the reference to return + * @nargs_prop: The name of the property telling the number of arguments + * in the referred node. NULL if @num_args is known, otherwise + * @num_args is ignored. * @num_args: Maximum number of arguments after each reference * @args: Location to store the returned reference with optional arguments * (may be NULL) @@ -919,13 +922,14 @@ static struct fwnode_handle *acpi_parse_string_ref(const struct fwnode_handle *f * Return: %0 on success, negative error code on failure. */ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, - const char *propname, size_t index, size_t num_args, + const char *propname, size_t index, const char *nargs_prop, size_t num_args, struct fwnode_reference_args *args) { const union acpi_object *element, *end; const union acpi_object *obj; const struct acpi_device_data *data; struct fwnode_handle *ref_fwnode; + struct acpi_device *ref_adev; struct acpi_device *device; int ret, idx = 0; @@ -1012,6 +1016,13 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, element->string.pointer); if (!ref_fwnode) return -EINVAL; + if (nargs_prop) { + ref_adev = to_acpi_device_node(ref_fwnode); + if (!acpi_dev_get_property(ref_adev, nargs_prop, + ACPI_TYPE_INTEGER, &obj)) { + num_args = obj->integer.value; + } + } element++; @@ -1565,7 +1576,7 @@ acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode, struct fwnode_reference_args *args) { return __acpi_node_get_property_reference(fwnode, prop, index, - args_count, args); + nargs_prop, args_count, args); } static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode) diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 1f9fe50bba00..de8e4d081539 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -839,7 +839,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode, int ret; memset(&args, 0, sizeof(args)); - ret = __acpi_node_get_property_reference(fwnode, propname, index, 3, + ret = __acpi_node_get_property_reference(fwnode, propname, index, NULL, 3, &args); if (ret) { struct acpi_device *adev; diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index ccd54c089bab..7afd78061e6e 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -1790,7 +1790,7 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode) memset(&args, 0, sizeof(args)); - ret = __acpi_node_get_property_reference(fwnode, "pwms", 0, 3, &args); + ret = __acpi_node_get_property_reference(fwnode, "pwms", 0, NULL, 3, &args); if (ret < 0) return ERR_PTR(ret); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 4e495b29c640..b9fd3c812e1f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1296,8 +1296,9 @@ static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) int acpi_dev_get_property(const struct acpi_device *adev, const char *name, acpi_object_type type, const union acpi_object **obj); int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, - const char *name, size_t index, size_t num_args, - struct fwnode_reference_args *args); + const char *name, size_t index, + const char *nargs_prop, size_t num_args, + struct fwnode_reference_args *args); static inline int acpi_node_get_property_reference( const struct fwnode_handle *fwnode, @@ -1305,7 +1306,7 @@ static inline int acpi_node_get_property_reference( struct fwnode_reference_args *args) { return __acpi_node_get_property_reference(fwnode, name, index, - NR_FWNODE_REFERENCE_ARGS, args); + NULL, NR_FWNODE_REFERENCE_ARGS, args); } static inline bool acpi_dev_has_props(const struct acpi_device *adev) @@ -1400,8 +1401,9 @@ static inline int acpi_dev_get_property(struct acpi_device *adev, static inline int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, - const char *name, size_t index, size_t num_args, - struct fwnode_reference_args *args) + const char *name, size_t index, + const char *nargs_prop, size_t num_args, + struct fwnode_reference_args *args) { return -ENXIO; }