| Message ID | 20230906024011.17488-2-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: scmi: add SCMI pinctrl protocol support | expand |
Hi AKASHI, On Tue, 5 Sept 2023 at 20:41, AKASHI Takahiro <takahiro.akashi@linaro.org> 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 <takahiro.akashi@linaro.org> > --- > 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
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 > <takahiro.akashi@linaro.org> 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 <takahiro.akashi@linaro.org> > > --- > > 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
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; }
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 <takahiro.akashi@linaro.org> --- drivers/firmware/scmi/scmi_agent-uclass.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)