| Message ID | 87fryhklhb.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | of: property: add port base loop | expand |
Hi Sakari > > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy > > > > to expand fwnode side functions if of_graph side function exist ? > > > > > > That would be one way to do that, yes, but I suggested using the existing > > > endpoint iterators as that would keep the firmware specific implementation > > > more simple. The (slight) drawback is that for each node returned, you'd > > > need to check its parent (i.e. port node) is the same as the port you're > > > interested in. The alternative may involve reworking the struct > > > fwnode_operations interface somewhat, including swnode, DT and ACPI > > > implementations. > > > > > > > But we still need the of_* versions, don't we, for patches 4 to 13? > > Yes, my comment was indeed about the fwnode property API only. Thank you for your suggestion. But I'm not familiar with fwnode, and it seems we still need of_*, I will keep current style (= non fwnode) in v3 Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Mon, Jan 29, 2024 at 03:02:19PM +0200, Laurent Pinchart wrote: > 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@renesas.com> > > > --- > > > 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. I think we should get rid of or minimize of_graph_get_next_endpoint() in its current form. In general, drivers should be asking for a specific port number because their function is fixed in the binding. Iterating over endpoints within a port is okay as that's usually a selecting 1 of N operation. Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and wrong. I also added of_graph_get_remote_node() for this reason and cleaned a lot of these (in DRM) up some time ago. Because in the end, a driver generally just wants the remote device it is connected to and details of parsing the graph should be mostly opaque. Wouldn't something like this work for this case: #define for_each_port_endpoint_of_node(parent, port, child) \ for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \ child = of_get_next_child(parent, child)) Rob
On Mon, Jan 29, 2024 at 12:54:44AM +0000, Kuninori Morimoto wrote: > We have endpoint base functions > - of_graph_get_next_endpoint() > - of_graph_get_endpoint_count() > - for_each_endpoint_of_node() > > Here, for_each_endpoint_of_node() loop finds each endpoints > > ports { > port@0 { > (1) endpoint {...}; > }; > port@1 { > (2) endpoint {...}; > }; > ... > }; > > In above case, for_each_endpoint_of_node() loop 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" by > for_each_endpoint_of_node(). Getting "port" by using of_get_parent() > from "endpoint" doesn't work. see below. > > ports { > port@0 { > (1) endpoint@0 {...}; > (2) endpoint@1 {...}; > }; > port@1 { > (3) endpoint {...}; > }; > ... > }; > > Add "port" base functions. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/of/property.c | 48 ++++++++++++++++++++++++++++++++++++++++ > include/linux/of_graph.h | 21 ++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index afdaefbd03f6..9e670e99dbbb 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -631,6 +631,42 @@ 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_port() - get next port node > + * @parent: pointer to the parent device node > + * @port: current port node, or NULL to get first > + * > + * Return: An 'port' node pointer with refcount incremented. Refcount > + * of the passed @prev node is decremented. > + */ > +struct device_node *of_graph_get_next_port(const struct device_node *parent, > + struct device_node *port) > +{ > + if (!parent) > + return NULL; > + > + if (!port) { > + struct device_node *node; > + > + node = of_get_child_by_name(parent, "ports"); > + if (node) { > + parent = node; > + of_node_put(node); The original code had this right, but here you have it wrong. You are releasing ports here, but then using it... > + } > + > + return of_get_child_by_name(parent, "port"); ...here. You have to get the child before you can put the parent. > + } > + > + do { > + port = of_get_next_child(parent, port); > + if (!port) > + break; > + } while (!of_node_name_eq(port, "port")); > + > + return port; > +} > +EXPORT_SYMBOL(of_graph_get_next_port); > +
On Mon, Jan 29, 2024 at 12:54:44AM +0000, Kuninori Morimoto wrote: > We have endpoint base functions > - of_graph_get_next_endpoint() > - of_graph_get_endpoint_count() > - for_each_endpoint_of_node() I also noticed that your mail setup has an issue. You have some utf-8 encoded email names, but the headers say it is ascii. git-send-email should do the right thing here, but maybe Exchange is messing things up. Rob
Hi Rob Thank you for review > > +/** > > + * 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: An 'port' node pointer with refcount incremented. Refcount > > + * of the passed @prev node is decremented. > > + */ > > +struct device_node *of_graph_get_next_port(const struct device_node *parent, > > + struct device_node *port) > > +{ > > + if (!parent) > > + return NULL; > > + > > + if (!port) { > > + struct device_node *node; > > + > > + node = of_get_child_by_name(parent, "ports"); > > + if (node) { > > + parent = node; > > + of_node_put(node); > > The original code had this right, but here you have it wrong. > > You are releasing ports here, but then using it... > > > + } > > + > > + return of_get_child_by_name(parent, "port"); > > ...here. You have to get the child before you can put the parent. You are reviewing v2, and it was already fixed in v3 Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Rob > I think we should get rid of or minimize of_graph_get_next_endpoint() in > its current form. In general, drivers should be asking for a specific > port number because their function is fixed in the binding. Iterating > over endpoints within a port is okay as that's usually a selecting 1 of > N operation. > > Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) > which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). > Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and > wrong. > > I also added of_graph_get_remote_node() for this reason and cleaned a > lot of these (in DRM) up some time ago. Because in the end, a driver > generally just wants the remote device it is connected to and details of > parsing the graph should be mostly opaque. > > Wouldn't something like this work for this case: > > #define for_each_port_endpoint_of_node(parent, port, child) \ > for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \ > child = of_get_next_child(parent, child)) I see. I will split this patch-set to like below - patch-set for reduce/remove to using current next_endpoint() - patch-set for rename current next_endpoint() to next_device_endpoint() - patch-set for adding new next_port_endpoint() Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Morimoto-san, On Tue, Jan 30, 2024 at 11:24:07PM +0000, Kuninori Morimoto wrote: > > Hi Sakari > > > > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy > > > > > to expand fwnode side functions if of_graph side function exist ? > > > > > > > > That would be one way to do that, yes, but I suggested using the existing > > > > endpoint iterators as that would keep the firmware specific implementation > > > > more simple. The (slight) drawback is that for each node returned, you'd > > > > need to check its parent (i.e. port node) is the same as the port you're > > > > interested in. The alternative may involve reworking the struct > > > > fwnode_operations interface somewhat, including swnode, DT and ACPI > > > > implementations. > > > > > > > > > > But we still need the of_* versions, don't we, for patches 4 to 13? > > > > Yes, my comment was indeed about the fwnode property API only. > > Thank you for your suggestion. > But I'm not familiar with fwnode, and it seems we still need of_*, > I will keep current style (= non fwnode) in v3 The fwnode API should be kept in sync with the OF (and other firmware specific) API. Merging your set in its current form would leave fwnode API impaired. Therefore I'd very much prefer to see this set add similar fwnode APIs, too.
Hi Sakari > > Thank you for your suggestion. > > But I'm not familiar with fwnode, and it seems we still need of_*, > > I will keep current style (= non fwnode) in v3 > > The fwnode API should be kept in sync with the OF (and other firmware > specific) API. Merging your set in its current form would leave fwnode API > impaired. Therefore I'd very much prefer to see this set add similar fwnode > APIs, too. I will keep current fwnode API behavior, but I can't test it. Now, I'm separating the patch-set into small stages. There is no problem for a while, but I think I will ask you to test it in the final stage. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hello Morimoto-san, On Mon, Feb 05, 2024 at 05:31:25AM +0000, Kuninori Morimoto wrote: > > Hi Sakari > > > > Thank you for your suggestion. > > > But I'm not familiar with fwnode, and it seems we still need of_*, > > > I will keep current style (= non fwnode) in v3 > > > > The fwnode API should be kept in sync with the OF (and other firmware > > specific) API. Merging your set in its current form would leave fwnode API > > impaired. Therefore I'd very much prefer to see this set add similar fwnode > > APIs, too. > > I will keep current fwnode API behavior, but I can't test it. The fwnode API is an abstraction layer on top of the OF or ACPI APIs, and allows drivers to work on both without needing to support OF and ACPI explicitly and separately. You should be able to convert the drivers you're using to the fwnode API, and it should work exactly the same as when using the OF-specific functions. That will give you a way to test the API. For instance, if you look at the drivers/media/platform/rcar_drif.c driver, you will see if (!fwnode_property_read_u32(fwnode, "sync-active", &val)) which, on OF platforms, is equivalent to if (!of_property_read_u32(np, "sync-active", &val)) This particular driver will never be used on an ACPI-based system, but drivers are still encouraged to use the fwnode API. > Now, I'm separating the patch-set into small stages. > There is no problem for a while, but I think I will ask you to test it in the > final stage.