On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote:
> On 29/01/2024 02:54, Kuninori Morimoto wrote:
> > We already have of_graph_get_next_endpoint(), but it is not intuitive
> > to use.
> >
> > (X) node {
> > (Y) ports {
> > port@0 { endpoint { remote-endpoint = ...; };};
> > (A1) port@1 { endpoint { remote-endpoint = ...; };
> > (A2) endpoint { remote-endpoint = ...; };};
> > (B) port@2 { endpoint { remote-endpoint = ...; };};
> > };
> > };
> >
> > For example, if I want to handle port@1's 2 endpoints (= A1, A2),
> > I want to use like below
> >
> > A1 = of_graph_get_next_endpoint(port1, NULL);
> > A2 = of_graph_get_next_endpoint(port1, A1);
> >
> > But 1st one will be error, because of_graph_get_next_endpoint() requested
> > "parent" means "node" (X) or "ports" (Y), not "port".
> > Below are OK
> >
> > of_graph_get_next_endpoint(node, NULL); // node/ports/port@0/endpoint
> > of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint
> >
> > In other words, we can't handle A1/A2 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 A1 pointer for some way, and if I want to
> > handle port@1 things, I would like use it like below
> >
> > /*
> > * "endpoint" is now A1, and handle port1 things here,
> > * but we don't know how many endpoints port1 has.
> > *
> > * Because "endpoint" is non NULL, we can use port1
> > * as of_graph_get_next_endpoint(port1, xxx)
> > */
> > do {
> > /* do something for port1 specific things here */
> > } while (endpoint = of_graph_get_next_endpoint(port1, endpoint))
> >
> > But it also not worked as I expected.
> > I expect it will be A1 -> A2 -> NULL,
> > but it will be A1 -> A2 -> B, because of_graph_get_next_endpoint()
> > will fetch endpoint beyond the port.
> >
> > It is not useful on generic driver like Generic Sound Card.
> > It uses of_get_next_child() instead for now, but it is not intuitive,
> > and not check node name (= "endpoint").
> >
> > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw()
> >
> > of_graph_get_next_endpoint_raw(port1, NULL); // A1
> > of_graph_get_next_endpoint_raw(port1, A1); // A2
> > of_graph_get_next_endpoint_raw(port1, A2); // NULL
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> > ---
> > drivers/of/property.c | 26 +++++++++++++++++++++++++-
> > include/linux/of_graph.h | 2 ++
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 14ffd199c9b1..37dbb1b0e742 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent,
> > }
> > EXPORT_SYMBOL(of_graph_get_next_port);
> >
> > +/**
> > + * of_graph_get_next_endpoint_raw() - get next endpoint node
>
> How about "of_graph_get_next_port_endpoint()"?
We may want to also rename the existing of_graph_get_next_endpoint()
function to of_graph_next_dev_endpoint() then. It would be a tree-wide
patch, which is always annoying to get reviewed and merged, so if Rob
would prefer avoiding the rename, I'm fine with that.
> > + * @port: pointer to the target port node
> > + * @endpoint: current endpoint node, or NULL to get first
> > + *
> > + * Return: An 'endpoint' node pointer with refcount incremented. Refcount
> > + * of the passed @prev node is decremented.
> > + */
>
> It might be good to highlight here the difference to the
> of_graph_get_next_endpoint().
Yes, and the documentation of of_graph_get_next_endpoint() shoul also be
improved.
> > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> > + struct device_node *endpoint)
> > +{
> > + if (!port)
> > + return NULL;
> > +
> > + do {
> > + endpoint = of_get_next_child(port, endpoint);
> > + if (!endpoint)
> > + break;
> > + } while (!of_node_name_eq(endpoint, "endpoint"));
> > +
> > + return endpoint;
> > +}
> > +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw);
> > +
> > /**
> > * of_graph_get_next_endpoint() - get next endpoint node
> > * @parent: pointer to the parent device node
> > @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> > * getting the next child. If the previous endpoint is NULL this
> > * will return the first child.
> > */
> > - endpoint = of_get_next_child(port, prev);
> > + endpoint = of_graph_get_next_endpoint_raw(port, prev);
> > if (endpoint) {
> > of_node_put(port);
> > return endpoint;
> > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> > index fff598640e93..427905a6e8c3 100644
> > --- a/include/linux/of_graph.h
> > +++ b/include/linux/of_graph.h
> > @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np);
> > struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id);
> > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> > struct device_node *previous);
> > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> > + struct device_node *prev);
> > struct device_node *of_graph_get_next_port(const struct device_node *parent,
> > struct device_node *previous);
> > struct device_node *of_graph_get_endpoint_by_regs(
--
Regards,
Laurent Pinchart
