| Message ID | 1449047368-5768-3-git-send-email-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Wed, Dec 02, 2015 at 05:09:26PM +0800, Kefeng Wang wrote: > With of_parse_phandle() and acpi_dev_get_reference_device(), we can introduce > a universal helper device_get_reference_node() to read and parse a device > property and return a pointer to the resulting firmware node. What's happening to make this fwnode stuff actually usable? Whenever I've looked at it from the DT perspective, it looks very much like a half-baked train wreck - although the struct device_node contains a fwnode, of_node_init() initialises it partially, and we have a way to convert _from_ a fwnode to a device_node (to_of_node), nothing sets the struct device fwnode pointer. Whenever I've looked at this, it's always led me to the conclusion that the way that fwnode stuff has currently been written, I'm supposed to get the device_node, and then use the embedded fwnode directly, which I'm pretty sure misses the point of fwnode (as it means any driver code making use of this is tied to DT.) This patch seems to confirm my suspicions that it's just wrong - trying to use device_get_reference_node() here will fail in a DT based setup, because (I assume) dev_fwnode(dev) returns dev->fwnode which is NULL there. I don't know, maybe there is an intention that fwnode should not be used for DT? Maybe someone who knows what the intentions are behind this fwnode stuff can enlighten the situation, and maybe sort out this apparent oversight which IMHO should've been spotted in the initial fwnode review?
On Thursday, December 03, 2015 03:28:42 PM Russell King - ARM Linux wrote: > On Wed, Dec 02, 2015 at 05:09:26PM +0800, Kefeng Wang wrote: > > With of_parse_phandle() and acpi_dev_get_reference_device(), we can introduce > > a universal helper device_get_reference_node() to read and parse a device > > property and return a pointer to the resulting firmware node. > > What's happening to make this fwnode stuff actually usable? Whenever > I've looked at it from the DT perspective, it looks very much like a > half-baked train wreck - although the struct device_node contains a > fwnode, of_node_init() initialises it partially, and we have a way to > convert _from_ a fwnode to a device_node (to_of_node), nothing sets > the struct device fwnode pointer. > > Whenever I've looked at this, it's always led me to the conclusion > that the way that fwnode stuff has currently been written, I'm > supposed to get the device_node, and then use the embedded fwnode > directly, which I'm pretty sure misses the point of fwnode (as it > means any driver code making use of this is tied to DT.) > > This patch seems to confirm my suspicions that it's just wrong - > trying to use device_get_reference_node() here will fail in a DT > based setup, because (I assume) dev_fwnode(dev) returns dev->fwnode > which is NULL there. > > I don't know, maybe there is an intention that fwnode should not be > used for DT? No, there isn't. > Maybe someone who knows what the intentions are behind this fwnode stuff > can enlighten the situation, and maybe sort out this apparent oversight > which IMHO should've been spotted in the initial fwnode review? At that time there was a plan (or maybe just and intention, I'm not sure to be honest) to introduce and accessor macro for the of_node pointer in struct device and make the users of it access it via that macro, which then would make it possible to switch over to using dev->fwnode in the DT case (by changing the implementation of that macro). That hasn't materialized as clearly visible, but still can be done AFAICS, although finding a volunteer for that work may be hard I suppose. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, December 04, 2015 12:29:12 AM Rafael J. Wysocki wrote: > On Thursday, December 03, 2015 03:28:42 PM Russell King - ARM Linux wrote: > > On Wed, Dec 02, 2015 at 05:09:26PM +0800, Kefeng Wang wrote: > > > With of_parse_phandle() and acpi_dev_get_reference_device(), we can introduce > > > a universal helper device_get_reference_node() to read and parse a device > > > property and return a pointer to the resulting firmware node. > > > > What's happening to make this fwnode stuff actually usable? Whenever > > I've looked at it from the DT perspective, it looks very much like a > > half-baked train wreck - although the struct device_node contains a > > fwnode, of_node_init() initialises it partially, and we have a way to > > convert _from_ a fwnode to a device_node (to_of_node), nothing sets > > the struct device fwnode pointer. > > > > Whenever I've looked at this, it's always led me to the conclusion > > that the way that fwnode stuff has currently been written, I'm > > supposed to get the device_node, and then use the embedded fwnode > > directly, which I'm pretty sure misses the point of fwnode (as it > > means any driver code making use of this is tied to DT.) > > > > This patch seems to confirm my suspicions that it's just wrong - > > trying to use device_get_reference_node() here will fail in a DT > > based setup, because (I assume) dev_fwnode(dev) returns dev->fwnode > > which is NULL there. > > > > I don't know, maybe there is an intention that fwnode should not be > > used for DT? > > No, there isn't. > > > Maybe someone who knows what the intentions are behind this fwnode stuff > > can enlighten the situation, and maybe sort out this apparent oversight > > which IMHO should've been spotted in the initial fwnode review? > > At that time there was a plan (or maybe just and intention, I'm not sure to > be honest) to introduce and accessor macro for the of_node pointer in struct > device and make the users of it access it via that macro, which then would > make it possible to switch over to using dev->fwnode in the DT case (by > changing the implementation of that macro). > > That hasn't materialized as clearly visible, but still can be done AFAICS, > although finding a volunteer for that work may be hard I suppose. That said if there are real problems with this (other than aesthetics), I actually may go off and do it, but I can't promise that to happen soon. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Russell, On Thu, Dec 03, 2015 at 03:28:42PM +0000, Russell King - ARM Linux wrote: > On Wed, Dec 02, 2015 at 05:09:26PM +0800, Kefeng Wang wrote: > > With of_parse_phandle() and acpi_dev_get_reference_device(), we can introduce > > a universal helper device_get_reference_node() to read and parse a device > > property and return a pointer to the resulting firmware node. > > What's happening to make this fwnode stuff actually usable? Whenever > I've looked at it from the DT perspective, it looks very much like a > half-baked train wreck - although the struct device_node contains a > fwnode, of_node_init() initialises it partially, and we have a way to > convert _from_ a fwnode to a device_node (to_of_node), nothing sets > the struct device fwnode pointer. I might have missed why that pointer is left to NULL in DT, it is not that complicated to set it up and may actually simplify things in the process. Rafael certainly knows better than I do, it could have been done in commit: 9017f25254e4 ("driver core: Implement device property accessors through fwnode ones") except that as you noticed the dev_fwnode has to be special cased since the OF core does not set the fwnode pointer (there must be a reason why, if there is not a patch to implement that is simple). On DT the fwnode_handle is allocated in the struct device_node that contains it but I still do not see why the fwnode pointer in struct device can't be made to point at it. > Whenever I've looked at this, it's always led me to the conclusion > that the way that fwnode stuff has currently been written, I'm > supposed to get the device_node, and then use the embedded fwnode > directly, which I'm pretty sure misses the point of fwnode (as it > means any driver code making use of this is tied to DT.) > > This patch seems to confirm my suspicions that it's just wrong - > trying to use device_get_reference_node() here will fail in a DT > based setup, because (I assume) dev_fwnode(dev) returns dev->fwnode > which is NULL there. No, dev_fwnode is special cased for OF (have a look at it in /drivers/base/property.c), I share your question on why it has to be so, actually this would make dev_fwnode() even simpler. > I don't know, maybe there is an intention that fwnode should not be > used for DT? That's the question we need to get answered :), it might well be that we can simply update the OF populate code to initialize the pointer in the struct device. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/property.c b/drivers/base/property.c index 1325ff2..6517140 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -568,6 +568,46 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev, EXPORT_SYMBOL_GPL(device_get_next_child_node); /** + * fwnode_get_reference_node - Find the firmware node referenced + * @fwnode: Firmware node to get the property from. + * @propname: Name of the property + * @index: Index of the reference + * + * Returns referenced fwnode handler pointer, or an NULL if not found + */ +struct fwnode_handle *fwnode_get_reference_node(struct fwnode_handle *fwnode, + const char *propname, int index) +{ + if (is_of_node(fwnode)) { + struct device_node *np; + np = of_parse_phandle(to_of_node(fwnode), propname, index); + return np ? &np->fwnode : NULL; + } else if (is_acpi_node(fwnode)) { + struct acpi_device *adev; + adev = acpi_dev_get_reference_device(to_acpi_device_node(fwnode), + propname, index); + return adev ? acpi_fwnode_handle(adev) : NULL; + } + return NULL; +} +EXPORT_SYMBOL_GPL(fwnode_get_reference_node); + +/** + * dev_get_reference_node - Find the firmware node referenced + * @dev: Device to get the property from. + * @propname: Name of the property + * @index: Index of the reference + * + * Returns referenced fwnode handler pointer, or an NULL if not found + */ +struct fwnode_handle *device_get_reference_node(struct device *dev, + const char *propname, int index) +{ + return fwnode_get_reference_node(dev_fwnode(dev), propname, index); +} +EXPORT_SYMBOL_GPL(device_get_reference_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/linux/property.h b/include/linux/property.h index 0a3705a..d77e6a2 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -70,6 +70,12 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode, int fwnode_property_match_string(struct fwnode_handle *fwnode, const char *propname, const char *string); +struct fwnode_handle *fwnode_get_reference_node(struct fwnode_handle *fwnode, + const char *propname, int index); + +struct fwnode_handle *device_get_reference_node(struct device *dev, + const char *propname, int index); + struct fwnode_handle *device_get_next_child_node(struct device *dev, struct fwnode_handle *child);
With of_parse_phandle() and acpi_dev_get_reference_device(), we can introduce a universal helper device_get_reference_node() to read and parse a device property and return a pointer to the resulting firmware node. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- drivers/base/property.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 6 ++++++ 2 files changed, 46 insertions(+)