[PATCH 1/9] of: property: add of_graph_get_next_port()
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Thu Aug 8 08:06:26 UTC 2024
On 06/08/2024 07:58, Kuninori Morimoto wrote:
> We have endpoint base functions
> - of_graph_get_next_device_endpoint()
> - of_graph_get_device_endpoint_count()
> - for_each_of_graph_device_endpoint()
>
> Here, for_each_of_graph_device_endpoint() loop finds each endpoints
>
> ports {
> port at 0 {
> (1) endpoint {...};
> };
> port at 1 {
> (2) endpoint {...};
> };
> ...
> };
>
> In above case, it finds endpoint as (1) -> (2) -> ...
>
> Basically, user/driver knows which port is used for what, but not in
> all cases. For example on flexible/generic driver case, how many ports
> are used is not fixed.
>
> For example Sound Generic Card driver which is used from many venders
> can't know how many ports are used. Because the driver is very
> flexible/generic, it is impossible to know how many ports are used,
> it depends on each vender SoC and/or its used board.
>
> And more, the port can have multi endpoints. For example Generic Sound
> Card case, it supports many type of connection between CPU / Codec, and
> some of them uses multi endpoint in one port.
> Then, Generic Sound Card want to handle each connection via "port"
> instead of "endpoint".
> But, it is very difficult to handle each "port" via
> for_each_of_graph_device_endpoint(). Getting "port" by using
> of_get_parent() from "endpoint" doesn't work. see below.
>
> ports {
> port at 0 {
> (1) endpoint at 0 {...};
> (2) endpoint at 1 {...};
> };
> port at 1 {
> (3) endpoint {...};
> };
> ...
> };
>
> In the same time, same reason, we want to handle "ports" same as "port".
>
> node {
> => ports at 0 {
> port at 0 {
> endpoint at 0 {...};
> endpoint at 1 {...};
> ...
> };
> port at 1 {
> endpoint at 0 {...};
> endpoint at 1 {...};
> ...
> };
> ...
> };
> => ports at 1 {
> ...
> };
> };
>
> Add "ports" / "port" base functions.
> For above case, we can use
>
> for_each_of_graph_ports(node, ports) {
> for_each_of_graph_port(ports, port) {
> ...
> }
> }
>
> This loop works in case of "node" doesn't have "ports" also.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx at renesas.com>
> ---
> drivers/of/property.c | 86 ++++++++++++++++++++++++++++++++++++++++
> include/linux/of_graph.h | 47 ++++++++++++++++++++++
> 2 files changed, 133 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 164d77cb9445..3b2d09c0376a 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -625,8 +625,76 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
> }
> EXPORT_SYMBOL(of_graph_get_port_by_id);
>
> +/**
> + * of_graph_get_next_ports() - get next ports node.
> + * @parent: pointer to the parent device node
> + * @ports: current ports node, or NULL to get first
> + *
> + * Return: A 'ports' node pointer with refcount incremented. Refcount
> + * of the passed @prev node is decremented.
No "prev" argument in the code.
The of_graph_get_next_endpoint() function uses "previous" as the
argument name (well, the function declaration uses "previous", the
implementation uses "prev"...), and I would use the same naming here.
Also of_graph_get_next_endpoint() talks about "previous endpoint node",
whereas here it's "current ports node". I'd use the same style here, so
"previous ports node".
The same comments for the of_graph_get_next_port().
> + */
> +struct device_node *of_graph_get_next_ports(struct device_node *parent,
> + struct device_node *ports)
> +{
> + if (!parent)
> + return NULL;
> +
> + if (!ports) {
> + ports = of_get_child_by_name(parent, "ports");
> +
> + /* use parent as its ports of this device if it not exist */
I think this needs to be described in the kernel doc. I understand the
need for this, but it's somewhat counter-intuitive that this returns the
parent node if there are no ports nodes, so it must be highlighted in
the documentation.
I wonder if a bit more complexity here would be good... I think here we
could:
- If there are no 'ports' nodes in the parent, but there is a 'port'
node in the parent, return the parent node
- If there are no 'ports' nor 'port' nodes in the parent, return NULL
> + if (!ports) {
> + ports = parent;
> + of_node_get(ports);
> + }
You could just do "ports = of_node_get(parent);"
> +
> + return ports;
> + }
> +
> + do {
> + ports = of_get_next_child(parent, ports);
> + if (!ports)
> + break;
> + } while (!of_node_name_eq(ports, "ports"));
> +
> + return ports;
> +}
> +EXPORT_SYMBOL(of_graph_get_next_ports);
> +
> +/**
> + * of_graph_get_next_port() - get next port node.
> + * @parent: pointer to the parent device node
> + * @port: current port node, or NULL to get first
> + *
> + * Return: A 'port' node pointer with refcount incremented. Refcount
> + * of the passed @prev node is decremented.
> + */
> +struct device_node *of_graph_get_next_port(struct device_node *parent,
> + struct device_node *port)
> +{
> + if (!parent)
> + return NULL;
> +
> + if (!port) {
> + struct device_node *ports __free(device_node) =
> + of_graph_get_next_ports(parent, NULL);
> +
> + return of_get_child_by_name(ports, "port");
> + }
> +
> + do {
> + port = of_get_next_child(parent, port);
> + if (!port)
> + break;
> + } while (!of_node_name_eq(port, "port"));
> +
> + return port;
> +}
Hmm... So if I call this with of_graph_get_next_port(dev_node, NULL)
(dev_node being the device node of the device), it'll give me the first
port in the first ports node, or the first port in the dev_node if there
are no ports nodes?
And if I then continue iterating with of_graph_get_next_port(dev_node,
prev_port)... The call will return NULL if the dev_node contains "ports"
node (because the dev_node does not contain any "port" nodes)?
So if I understand right, of_graph_get_next_port() must always be called
with a parent that contains port nodes. Sometimes that's the device's
node (if there's just one port) and sometimes that's ports node. If it's
called with a parent that contains ports node, it will not work correctly.
If the above is right, then should this just return
"of_get_child_by_name(parent, "port")" if !port, instead of calling
of_graph_get_next_ports()?
Or maybe I'm just getting confused here. But in any case, I think it
would be very good to describe the behavior on the kernel doc for the
different ports/port structure cases (also for
of_graph_get_next_ports()), and be clear on what the parameters can be,
i.e. what kind of device nodes can be given as parent, and how the
function iterates over the ports.
> +EXPORT_SYMBOL(of_graph_get_next_port);
> +
> /**
> * of_graph_get_next_endpoint() - get next endpoint node
> + *
Extra change.
> * @parent: pointer to the parent device node
> * @prev: previous endpoint node, or NULL to get first
> *
> @@ -823,6 +891,24 @@ unsigned int of_graph_get_endpoint_count(const struct device_node *np)
> }
> EXPORT_SYMBOL(of_graph_get_endpoint_count);
>
> +/**
> + * of_graph_get_port_count() - get count of port
Perhaps "get the number of port nodes".
> + * @np: pointer to the parent device node
> + *
> + * Return: count of port of this device node
> + */
> +unsigned int of_graph_get_port_count(struct device_node *np)
> +{
> + struct device_node *port = NULL;
> + int num = 0;
> +
> + for_each_of_graph_port(np, port)
> + num++;
> +
> + return num;
> +}
I my analysis above is right, calling of_graph_get_port_count(dev_node)
will return 1, if the dev_node contains "ports" node which contains one
or more "port" nodes.
> +EXPORT_SYMBOL(of_graph_get_port_count);
> +
> /**
> * of_graph_get_remote_node() - get remote parent device_node for given port/endpoint
> * @node: pointer to parent device_node containing graph port/endpoint
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index a4bea62bfa29..30169b50b042 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -37,14 +37,42 @@ struct of_endpoint {
> for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
> child = of_graph_get_next_endpoint(parent, child))
>
> +/**
> + * for_each_of_graph_ports - iterate over every ports in a device node
> + * @parent: parent device node containing ports
> + * @child: loop variable pointing to the current ports node
> + *
> + * When breaking out of the loop, of_node_put(child) has to be called manually.
> + */
> +#define for_each_of_graph_ports(parent, child) \
> + for (child = of_graph_get_next_ports(parent, NULL); child != NULL; \
> + child = of_graph_get_next_ports(parent, child))
> +
> +/**
> + * for_each_of_graph_port - iterate over every port in a device or ports node
> + * @parent: parent device or ports node containing port
> + * @child: loop variable pointing to the current port node
> + *
> + * When breaking out of the loop, of_node_put(child) has to be called manually.
> + */
> +#define for_each_of_graph_port(parent, child) \
> + for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
> + child = of_graph_get_next_port(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,
> struct of_endpoint *endpoint);
> +
Extra change.
> unsigned int of_graph_get_endpoint_count(const struct device_node *np);
> +unsigned int of_graph_get_port_count(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_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_endpoint_by_regs(
> const struct device_node *parent, int port_reg, int reg);
> struct device_node *of_graph_get_remote_endpoint(
> @@ -73,6 +101,11 @@ static inline unsigned int of_graph_get_endpoint_count(const struct device_node
> return 0;
> }
>
> +static inline unsigned int of_graph_get_port_count(struct device_node *np)
> +{
> + return 0;
> +}
> +
> static inline struct device_node *of_graph_get_port_by_id(
> struct device_node *node, u32 id)
> {
> @@ -86,6 +119,20 @@ static inline struct device_node *of_graph_get_next_endpoint(
> return NULL;
> }
>
> +static inline struct device_node *of_graph_get_next_ports(
> + struct device_node *parent,
> + struct device_node *previous)
> +{
> + return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_next_port(
> + 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)
> {
Tomi
More information about the dri-devel
mailing list