On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:
> We already have of_graph_get_next_endpoint(), but it is not
> intuitive to use in some case.
Can of_graph_get_next_endpoint() users be replaced with your new
helpers? I'd really like to get rid of the 3 remaining users.
>
> (X) node {
> (Y) ports {
> (P0) port@0 { endpoint { remote-endpoint = ...; };};
> (P10) port@1 { endpoint { remote-endpoint = ...; };
> (P11) endpoint { remote-endpoint = ...; };};
> (P2) port@2 { endpoint { remote-endpoint = ...; };};
> };
> };
>
> For example, if I want to handle port@1's 2 endpoints (= P10, P11),
> I want to use like below
>
> P10 = of_graph_get_next_endpoint(port1, NULL);
> P11 = of_graph_get_next_endpoint(port1, P10);
>
> But 1st one will be error, because of_graph_get_next_endpoint()
> requested "parent" means "node" (X) or "ports" (Y), not "port".
> Below works, but it will get P0
>
> /* These will be node/ports/port@0/endpoint */
> P0 = of_graph_get_next_endpoint(node, NULL);
> P0 = of_graph_get_next_endpoint(ports, NULL);
>
> In other words, we can't handle P10/P11 directly via
> of_graph_get_next_endpoint() so far.
>
> There is another non intuitive behavior on of_graph_get_next_endpoint().
> In case of if I could get P10 pointer for some way, and if I want to
> handle port@1 things, I would like use it like below
>
> /*
> * "ep" is now P10, and handle port1 things here,
> * but we don't know how many endpoints port1 has.
> *
> * Because "ep" is non NULL now, we can use port1
> * as of_graph_get_next_endpoint(port1, xxx)
> */
> do {
> /* do something for port1 specific things here */
> } while (ep = of_graph_get_next_endpoint(port1, ep))
>
> But it also not worked as I expected.
> I expect it will be P10 -> P11 -> NULL,
> but it will be P10 -> P11 -> P2, because
> of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port".
>
> It is not useful on generic driver.
> It uses of_get_next_child() instead for now, but it is not intuitive.
> And it doesn't check node name (= "endpoint").
>
> To handle endpoint more intuitive, create of_graph_get_next_port_endpoint()
>
> of_graph_get_next_port_endpoint(port1, NULL); // P10
> of_graph_get_next_port_endpoint(port1, P10); // P11
> of_graph_get_next_port_endpoint(port1, P11); // NULL
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> drivers/of/property.c | 22 ++++++++++++++++++++++
> include/linux/of_graph.h | 20 ++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index aec6ac9f70064..90820e43bc973 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -719,6 +719,28 @@ struct device_node *of_graph_get_next_port(struct device_node *parent,
> }
> EXPORT_SYMBOL(of_graph_get_next_port);
>
> +/**
> + * of_graph_get_next_port_endpoint() - get next endpoint node in port.
> + * If it reached to end of the port, it will return NULL.
> + * @port: pointer to the target port node
> + * @prev: previous endpoint node, or NULL to get first
> + *
> + * Return: An 'endpoint' node pointer with refcount incremented. Refcount
> + * of the passed @prev node is decremented.
> + */
> +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> + struct device_node *prev)
> +{
> + do {
> + prev = of_get_next_child(port, prev);
> + if (!prev)
> + break;
> + } while (!of_node_name_eq(prev, "endpoint"));
Really, this check is validation as no other name is valid in a
port node. The kernel is not responsible for validation, but okay.
However, if we are going to keep this, might as well make it WARN().
> +
> + return prev;
> +}
> +EXPORT_SYMBOL(of_graph_get_next_port_endpoint);
> +
> /**
> * of_graph_get_next_endpoint() - get next endpoint node
> * @parent: pointer to the parent device node
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index a6b91577700a8..967ee14a1ff37 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -59,6 +59,17 @@ struct of_endpoint {
> for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
> child = of_graph_get_next_port(parent, child))
>
> +/**
> + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
> + * @parent: parent port node
> + * @child: loop variable pointing to the current endpoint node
> + *
> + * When breaking out of the loop, of_node_put(child) has to be called manually.
No need for this requirement anymore. Use cleanup.h so this is
automatic.
> + */
> +#define for_each_of_graph_port_endpoint(parent, child) \
> + for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \
> + child = of_graph_get_next_port_endpoint(parent, child))
> +
> #ifdef CONFIG_OF
> bool of_graph_is_present(const struct device_node *node);
> int of_graph_parse_endpoint(const struct device_node *node,
> @@ -72,6 +83,8 @@ struct device_node *of_graph_get_next_ports(struct device_node *parent,
> struct device_node *ports);
> struct device_node *of_graph_get_next_port(struct device_node *parent,
> struct device_node *port);
> +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> + struct device_node *prev);
> struct device_node *of_graph_get_endpoint_by_regs(
> const struct device_node *parent, int port_reg, int reg);
> struct device_node *of_graph_get_remote_endpoint(
> @@ -132,6 +145,13 @@ static inline struct device_node *of_graph_get_next_port(
> return NULL;
> }
>
> +static inline struct device_node *of_graph_get_next_port_endpoint(
> + const struct device_node *parent,
> + struct device_node *previous)
> +{
> + return NULL;
> +}
> +
> static inline struct device_node *of_graph_get_endpoint_by_regs(
> const struct device_node *parent, int port_reg, int reg)
> {
> --
> 2.43.0
>
