Re: [PATCH 06/15] device property: Add support for remote endpoints

Rob Herring
2017-01-27T21:45:46Z

On Fri, Jan 27, 2017 at 10:03 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> This follows DT implementation of of_graph_* APIs but we call them
> fwnode_graph_* instead. For DT nodes the existing of_graph_* implementation
> will be used. For ACPI we use the new ACPI graph implementation instead.
>
> This commit includes code from Sakari Ailus.

Then it should have your S-o-b. Actually, either way it should have it.

Whether or not copying DT graphs is a good idea or not, I'll let
someone else chime in on that...

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/base/property.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/property.h | 9 ++++
> 2 files changed, 131 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0011cf..cf602c3 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -15,6 +15,7 @@
> #include <linux/kernel.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/of_graph.h>
> #include <linux/property.h>
> #include <linux/etherdevice.h>
> #include <linux/phy.h>
> @@ -1111,3 +1112,124 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
> return device_get_mac_addr(dev, "address", addr, alen);
> }
> EXPORT_SYMBOL(device_get_mac_address);
> +
> +/**
> + * device_graph_get_next_endpoint - Get next endpoint firmware node
> + * @fwnode: Pointer to the parent firmware node
> + * @prev: Previous endpoint node or %NULL to get the first
> + *
> + * Returns an endpoint firmware node pointer or %NULL if no more endpoints
> + * are available.
> + */
> +struct fwnode_handle *
> +fwnode_graph_get_next_endpoint(struct fwnode_handle *fwnode,
> + struct fwnode_handle *prev)
> +{
> + struct fwnode_handle *endpoint = NULL;
> +
> + if (is_of_node(fwnode)) {
> + struct device_node *node;
> +
> + node = of_graph_get_next_endpoint(to_of_node(fwnode),
> + to_of_node(prev));
> +
> + if (node)
> + endpoint = &node->fwnode;
> + } else if (is_acpi_node(fwnode)) {
> + endpoint = acpi_graph_get_next_endpoint(fwnode, prev);
> + if (IS_ERR(endpoint))
> + endpoint = NULL;
> + }

This is an ugly pattern IMO. Why not define an ops struct and set it
to OF or ACPI rather than check on every function call with
is_{of,acpi}_node()?

The of_graph_* functions need some better helpers too, so duplicating
them 2 more times is not ideal. Basically, drivers are left to do too
much of the walking of nodes themselves.

Rob