Re: [RFC 1/6] firmware: scmi: fix protocol enumeration logic

[AKASHI Takahiro]

Hi Simon,

On Sun, Sep 10, 2023 at 01:13:26PM -0600, Simon Glass wrote:
> Hi AKASHI,
> 
> On Tue, 5 Sept 2023 at 20:41, AKASHI Takahiro
>  wrote:
> >
> > The original logic in enumerating all the protocols accidentally
> > modifies a *loop* variable, node, at Voltage domain protocol.
> > So subsequent protocol nodes in a device tree won't be detected.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  drivers/firmware/scmi/scmi_agent-uclass.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c 
> > b/drivers/firmware/scmi/scmi_agent-uclass.c
> > index 46a2933d51a4..79584c00a066 100644
> > --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> > +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> > @@ -422,8 +422,11 @@ static int scmi_bind_protocols(struct udevice *dev)
> > case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> > if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI) &&
> > scmi_protocol_is_supported(dev, protocol_id)) {
> > -   node = ofnode_find_subnode(node, 
> > "regulators");
> > -   if (!ofnode_valid(node)) {
> > +   ofnode sub_node;
> > +
> > +   sub_node = ofnode_find_subnode(node,
> > +  
> > "regulators");
> > +   if (!ofnode_valid(sub_node)) {
> > dev_err(dev, "no regulators 
> > node\n");
> > return -ENXIO;
> > }
> > --
> > 2.34.1
> >
> 
> This function is very ugly...could we instead rely on driver model to


I think you are talking about scmi_bind_protocols() itself,
not the hunk above in my patch, which is merely a bug fix
in the existing code.
If so,

> bind the devices? It seems unfortunate to have to write code for
> something which could be done with no extra code.

We will discuss in the threads on your comment [1].

[1] https://lists.denx.de/pipermail/u-boot/2023-September/530200.html

-Takahiro Akashi


> Regards,
> Simon

Re: [RFC 1/6] firmware: scmi: fix protocol enumeration logic

[Simon Glass]

Hi AKASHI,

On Tue, 5 Sept 2023 at 20:41, AKASHI Takahiro
 wrote:
>
> The original logic in enumerating all the protocols accidentally
> modifies a *loop* variable, node, at Voltage domain protocol.
> So subsequent protocol nodes in a device tree won't be detected.
>
> Signed-off-by: AKASHI Takahiro 
> ---
>  drivers/firmware/scmi/scmi_agent-uclass.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c 
> b/drivers/firmware/scmi/scmi_agent-uclass.c
> index 46a2933d51a4..79584c00a066 100644
> --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> @@ -422,8 +422,11 @@ static int scmi_bind_protocols(struct udevice *dev)
> case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI) &&
> scmi_protocol_is_supported(dev, protocol_id)) {
> -   node = ofnode_find_subnode(node, 
> "regulators");
> -   if (!ofnode_valid(node)) {
> +   ofnode sub_node;
> +
> +   sub_node = ofnode_find_subnode(node,
> +  "regulators");
> +   if (!ofnode_valid(sub_node)) {
> dev_err(dev, "no regulators node\n");
> return -ENXIO;
> }
> --
> 2.34.1
>

This function is very ugly...could we instead rely on driver model to
bind the devices? It seems unfortunate to have to write code for
something which could be done with no extra code.

Regards,
Simon

[RFC 1/6] firmware: scmi: fix protocol enumeration logic

[AKASHI Takahiro]

The original logic in enumerating all the protocols accidentally
modifies a *loop* variable, node, at Voltage domain protocol.
So subsequent protocol nodes in a device tree won't be detected.

Signed-off-by: AKASHI Takahiro 
---
 drivers/firmware/scmi/scmi_agent-uclass.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c 
b/drivers/firmware/scmi/scmi_agent-uclass.c
index 46a2933d51a4..79584c00a066 100644
--- a/drivers/firmware/scmi/scmi_agent-uclass.c
+++ b/drivers/firmware/scmi/scmi_agent-uclass.c
@@ -422,8 +422,11 @@ static int scmi_bind_protocols(struct udevice *dev)
case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI) &&
scmi_protocol_is_supported(dev, protocol_id)) {
-   node = ofnode_find_subnode(node, "regulators");
-   if (!ofnode_valid(node)) {
+   ofnode sub_node;
+
+   sub_node = ofnode_find_subnode(node,
+  "regulators");
+   if (!ofnode_valid(sub_node)) {
dev_err(dev, "no regulators node\n");
return -ENXIO;
}
-- 
2.34.1