| Message ID | 87fryhklhb.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | of: property: add port base loop | expand |
Hi Sakari > > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port, > > + struct device_node *endpoint) > > +{ > > + if (!port) > > + return NULL; > > of_get_next_child() returns NULL if node is NULL, hence there's no need to > check this. Thanks, will fix in v3 Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Morimoto-san, On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: > > Hi Laurent, Sakari > > Thank you for your review > > > > The strategy sounds good to me. However, I'm wondering if you shouldn't > > > take one more step in the core, and implement these as fwnode > > > operations. Or is there a reason why OF is special, and iterating over > > > ports would be useful for drivers on OF systems but not on other types > > > of systems ? > > > > I'd prefer that, too. > > It is very easy reason, because I'm not fwnode user ;P > 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.
On 30/01/2024 09:31, Sakari Ailus wrote: > Hi Morimoto-san, > > On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: >> >> Hi Laurent, Sakari >> >> Thank you for your review >> >>>> The strategy sounds good to me. However, I'm wondering if you shouldn't >>>> take one more step in the core, and implement these as fwnode >>>> operations. Or is there a reason why OF is special, and iterating over >>>> ports would be useful for drivers on OF systems but not on other types >>>> of systems ? >>> >>> I'd prefer that, too. >> >> It is very easy reason, because I'm not fwnode user ;P >> 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? Tomi
On Tue, Jan 30, 2024 at 09:37:42AM +0200, Tomi Valkeinen wrote: > On 30/01/2024 09:31, Sakari Ailus wrote: > > Hi Morimoto-san, > > > > On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: > > > > > > Hi Laurent, Sakari > > > > > > Thank you for your review > > > > > > > > The strategy sounds good to me. However, I'm wondering if you shouldn't > > > > > take one more step in the core, and implement these as fwnode > > > > > operations. Or is there a reason why OF is special, and iterating over > > > > > ports would be useful for drivers on OF systems but not on other types > > > > > of systems ? > > > > > > > > I'd prefer that, too. > > > > > > It is very easy reason, because I'm not fwnode user ;P > > > 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.
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
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