Re: 回复: 回复: [EXT] [PATCH] power: regulator: scmi: Move regulator subnode hack to scmi_regulator

[Peng Fan <peng.fan@oss.nxp.com>]

On Wed, Apr 09, 2025 at 06:42:14AM +0000, Alice Guo (OSS) wrote:
>> -----邮件原件-----
>> 发件人: Marek Vasut <marex@denx.de>
>> 发送时间: 2025年4月9日 2:31
>> 收件人: Alice Guo (OSS) <alice.guo@oss.nxp.com>; u-boot@lists.denx.de
>> 抄送: Alice Guo <alice.guo@nxp.com>; Ilias Apalodimas
>> <ilias.apalodimas@linaro.org>; Jaehoon Chung <jh80.chung@samsung.com>;
>> Tom Rini <trini@konsulko.com>
>> 主题: Re: 回复: [EXT] [PATCH] power: regulator: scmi: Move regulator
>> subnode hack to scmi_regulator
>> 
>> On 4/3/25 12:56 PM, Alice Guo (OSS) wrote:
>> >> -----邮件原件-----
>> >> 发件人: Marek Vasut <marex@denx.de>
>> >> 发送时间: 2025年3月22日 9:45
>> >> 收件人: u-boot@lists.denx.de
>> >> 抄送: Marek Vasut <marex@denx.de>; Alice Guo <alice.guo@nxp.com>;
>> Ilias
>> >> Apalodimas <ilias.apalodimas@linaro.org>; Jaehoon Chung
>> >> <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
>> >> 主题: [EXT] [PATCH] power: regulator: scmi: Move regulator subnode hack
>> >> to scmi_regulator
>> >>
>> >> Caution: This is an external email. Please take care when clicking
>> >> links or opening attachments. When in doubt, report the message using
>> >> the 'Report this email' button
>> >>
>> >>
>> >> The current code attempts to bind scmi_voltage_domain to regulator
>> >> subnode of the SCMI protocol node, so scmi_voltage_domain can then
>> >> bind regulators directly to subnodes of its node. This kind of
>> >> behavior should not be in core code, move it into scmi_voltage_domain
>> >> driver code. Let the driver descend into regulator node and bind regulators to
>> its subnodes.
>> >>
>> >> Fixes: 1f213ee4dbf2 ("firmware: scmi: voltage regulator")
>> >> Signed-off-by: Marek Vasut <marex@denx.de>
>> >> ---
>> >> Cc: Alice Guo <alice.guo@nxp.com>
>> >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> >> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>> >> Cc: Tom Rini <trini@konsulko.com>
>> >> Cc: u-boot@lists.denx.de
>> >> ---
>> >>   drivers/firmware/scmi/scmi_agent-uclass.c | 8 +-------
>> >> drivers/power/regulator/scmi_regulator.c  | 6 ++++++
>> >>   2 files changed, 7 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c
>> >> b/drivers/firmware/scmi/scmi_agent-uclass.c
>> >> index 8c907c3b032..e6e43ae936a 100644
>> >> --- a/drivers/firmware/scmi/scmi_agent-uclass.c
>> >> +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
>> >> @@ -427,14 +427,8 @@ static int scmi_bind_protocols(struct udevice *dev)
>> >>                          break;
>> >>                  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)) {
>> >> -                                       dev_err(dev, "no
>> regulators
>> >> node\n");
>> >> -                                       return -ENXIO;
>> >> -                               }
>> >> +                           scmi_protocol_is_supported(dev,
>> >> + protocol_id))
>> >>                                  drv =
>> >> DM_DRIVER_GET(scmi_voltage_domain);
>> >> -                       }
>> >>                          break;
>> >>                  default:
>> >>                          break;
>> >> diff --git a/drivers/power/regulator/scmi_regulator.c
>> >> b/drivers/power/regulator/scmi_regulator.c
>> >> index 99f6506f162..2550b27246f 100644
>> >> --- a/drivers/power/regulator/scmi_regulator.c
>> >> +++ b/drivers/power/regulator/scmi_regulator.c
>> >> @@ -178,6 +178,12 @@ static int scmi_regulator_bind(struct udevice *dev)
>> >>          ofnode node;
>> >>          int ret;
>> >>
>> >> +       node = ofnode_find_subnode(node, "regulators");
>> >> +       if (!ofnode_valid(node)) {
>> >> +               dev_err(dev, "no regulators node\n");
>> >> +               return -ENXIO;
>> >> +       }
>> >> +
>> >>          drv = DM_DRIVER_GET(scmi_regulator);
>> >>
>> >>          ofnode_for_each_subnode(node, dev_ofnode(dev)) {
>> >> --
>> >> 2.47.2
>> >
>> > Hi Marek,
>> >
>> > Is there a problem in your patch? Should it be changed like this:
>> What kind of problem ? Can you please elaborate ?
>
>I think that in the scmi_regulator_bind() function, the second parameter passed into ofnode_for_each_subnode() should be the node named "regulators".
>
>static int scmi_regulator_bind(struct udevice *dev)
>{
>	struct driver *drv;
>	ofnode regul_node;
>	ofnode node;
>	int ret;
>
>	regul_node = ofnode_find_subnode(dev_ofnode(dev), "regulators");

Right. I could correct this when apply the patch.

Thanks,
Peng

>	if (!ofnode_valid(regul_node)) {
>		dev_err(dev, "no regulators node\n");
>		return -ENXIO;
>	}
>
>	drv = DM_DRIVER_GET(scmi_regulator);
>
>	ofnode_for_each_subnode(node, regul_node) {
>		ret = device_bind(dev, drv, ofnode_get_name(node),
>				  NULL, node, NULL);
>		if (ret)
>			return ret;
>	}
>
>	return 0;
>}
>
>Best Regards,
>Alice Guo
>