On Fri, Apr 18, 2025 at 10:52:00AM +0800, Peng Fan wrote:
>On Wed, Apr 09, 2025 at 06:42:14AM +0000, Alice Guo (OSS) wrote:
>>> -----邮件原件-----
>>> 发件人: Marek Vasut <ma...@denx.de>
>>> 发送时间: 2025年4月9日 2:31
>>> 收件人: Alice Guo (OSS) <alice....@oss.nxp.com>; u-boot@lists.denx.de
>>> 抄送: Alice Guo <alice....@nxp.com>; Ilias Apalodimas
>>> <ilias.apalodi...@linaro.org>; Jaehoon Chung <jh80.ch...@samsung.com>;
>>> Tom Rini <tr...@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 <ma...@denx.de>
>>> >> 发送时间: 2025年3月22日 9:45
>>> >> 收件人: u-boot@lists.denx.de
>>> >> 抄送: Marek Vasut <ma...@denx.de>; Alice Guo <alice....@nxp.com>;
>>> Ilias
>>> >> Apalodimas <ilias.apalodi...@linaro.org>; Jaehoon Chung
>>> >> <jh80.ch...@samsung.com>; Tom Rini <tr...@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 <ma...@denx.de>
>>> >> ---
>>> >> Cc: Alice Guo <alice....@nxp.com>
>>> >> Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org>
>>> >> Cc: Jaehoon Chung <jh80.ch...@samsung.com>
>>> >> Cc: Tom Rini <tr...@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");
This patch has issue, it will always return failure here.
Regards,
Peng
>> 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
>>
