[PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

Documenting the regulator power domain properties and usage examples.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 .../bindings/power/regulator-pd.yaml          | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/regulator-pd.yaml

diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml b/Documentation/devicetree/bindings/power/regulator-pd.yaml
new file mode 100644
index 000000000000..181d2fa83f8a
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/regulator-pd.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/regulator-pd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulator Power Domain
+
+maintainers:
+  - Shenwei Wang <shenwei.wang@nxp.com>
+
+description: |
+  This describes a power domain which manages a group of regulators.
+
+allOf:
+  - $ref: power-domain.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: regulator-power-domain
+
+  '#power-domain-cells':
+    const: 1
+
+  regulator-number:
+    minimum: 1
+    maximum: 100
+    description: The count of regulator to be managed by this power domain
+
+patternProperties:
+  "regulator-[0-99]-supply$":
+    description: The regulator supply phandle to be managed by this power domain
+
+required:
+  - compatible
+  - '#power-domain-cells'
+  - regulator-number
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    reg1: regulator-1 {
+    	compatible = "regulator-fixed";
+    	regulator-name = "REG1";
+    	regulator-min-microvolt = <3000000>;
+    	regulator-max-microvolt = <3000000>;
+    	gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
+    	enable-active-high;
+    };
+
+    reg2: regulator-2 {
+    	compatible = "regulator-fixed";
+    	regulator-name = "REG2";
+    	regulator-min-microvolt = <3000000>;
+    	regulator-max-microvolt = <3000000>;
+    	gpio = <&lsio_gpio4 20 GPIO_ACTIVE_HIGH>;
+    	enable-active-high;
+    };
+
+    power-controller {
+        compatible = "regulator-power-domain";
+        #power-domain-cells = <1>;
+
+        regulator-number = <2>;
+        regulator-0-supply = <&reg1>;
+        regulator-1-supply = <&reg2>;
+    };
-- 
2.34.1

[PATCH 2/2] genpd: regulator-pd: add regulator-pd driver

[Shenwei Wang]

This driver implements a power domain by wrapping a group of regulators
into a unified genpd abstraction. The power domain is defined in the
device tree, allowing the driver framework to automatically manage
the power for the those devices which use the power domain. This removes
the need to manually adjust each individual driver to directly control
the regulators. The solution can also take the benefit of runtime PM
subsystem, and the device's power can be managed automatically when the
device is idle.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/genpd/Makefile       |   1 +
 drivers/genpd/regulator-pd.c | 145 +++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)
 create mode 100644 drivers/genpd/regulator-pd.c

diff --git a/drivers/genpd/Makefile b/drivers/genpd/Makefile
index 666753676e5c..5eacd7ea2a2d 100644
--- a/drivers/genpd/Makefile
+++ b/drivers/genpd/Makefile
@@ -15,3 +15,4 @@ obj-y					+= sunxi/
 obj-y					+= tegra/
 obj-y					+= ti/
 obj-y					+= xilinx/
+obj-y					+= regulator-pd.o
diff --git a/drivers/genpd/regulator-pd.c b/drivers/genpd/regulator-pd.c
new file mode 100644
index 000000000000..dc30a81d7e65
--- /dev/null
+++ b/drivers/genpd/regulator-pd.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2023 NXP Semiconductor, Inc.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regulator/consumer.h>
+
+struct regulator_power_domain {
+	struct generic_pm_domain genpd;
+	struct device *dev;
+	struct regulator *reg;
+	int idx;
+};
+
+#define to_regulator_power_domain(_genpd) container_of(_genpd, struct regulator_power_domain, genpd)
+
+static int regulator_power_domain_on(struct generic_pm_domain *genpd)
+{
+	struct regulator_power_domain *domain = to_regulator_power_domain(genpd);
+
+	if (IS_ERR_OR_NULL(domain->reg))
+		return -ENODEV;
+
+	return regulator_enable(domain->reg);
+}
+
+static int regulator_power_domain_off(struct generic_pm_domain *genpd)
+{
+	struct regulator_power_domain *domain = to_regulator_power_domain(genpd);
+
+	if (IS_ERR_OR_NULL(domain->reg))
+		return -ENODEV;
+
+	return regulator_disable(domain->reg);
+}
+
+static struct generic_pm_domain *
+regulator_power_domain_xlate(struct of_phandle_args *spec, void *data)
+{
+	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+	struct genpd_onecell_data *pd_data = data;
+	struct regulator_power_domain *rpd;
+	unsigned int i;
+
+	for (i = 0; i < pd_data->num_domains; i++) {
+		rpd = to_regulator_power_domain(pd_data->domains[i]);
+		if (rpd->idx == spec->args[0]) {
+			domain = &rpd->genpd;
+			break;
+		}
+	}
+
+	return domain;
+}
+
+static int regulator_power_domain_probe(struct platform_device *pdev)
+{
+	struct regulator_power_domain *gpd;
+	struct genpd_onecell_data *pd_data;
+	struct device *dev = &pdev->dev;
+	struct generic_pm_domain **pds;
+	char name[16];
+	u32 num;
+	int i;
+
+	if (of_property_read_u32(dev->of_node, "regulator-number", &num))
+		return -ENODEV;
+
+	if (num < 1)
+		return -EINVAL;
+
+	/* Limit the regulator count to <=100 */
+	if (num > 100)
+		num = 100;
+
+	pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
+	pds = devm_kcalloc(dev, num, sizeof(*pds), GFP_KERNEL);
+	gpd = devm_kcalloc(dev, num, sizeof(*gpd), GFP_KERNEL);
+
+	if (!pds || !pd_data || !gpd)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++) {
+		bool is_off = 1;
+
+		snprintf(name, 16, "regulator-%d", i);
+		gpd->reg = devm_regulator_get_optional(dev, name);
+
+		/* Let the initial pd state as is as the regulator's */
+		if (!(IS_ERR_OR_NULL(gpd->reg)))
+			is_off = (regulator_is_enabled(gpd->reg) > 0) ? 0 : 1;
+
+		pm_genpd_init(&gpd->genpd, NULL, is_off);
+
+		gpd->genpd.power_off = regulator_power_domain_off;
+		gpd->genpd.power_on = regulator_power_domain_on;
+		gpd->genpd.name = dev_name(dev);
+		gpd->dev = dev;
+		gpd->idx = i;
+		pds[i] = &gpd->genpd;
+		gpd++;
+	}
+
+	pd_data->domains = pds;
+	pd_data->num_domains = num;
+	pd_data->xlate = regulator_power_domain_xlate;
+	platform_set_drvdata(pdev, pd_data);
+
+	return of_genpd_add_provider_onecell(dev->of_node, pd_data);
+}
+
+static int regulator_power_domain_remove(struct platform_device *pdev)
+{
+	struct genpd_onecell_data *pd_data = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int i;
+
+	of_genpd_del_provider(np);
+	for (i = 0; i < pd_data->num_domains; i++)
+		pm_genpd_remove(pd_data->domains[i]);
+
+	return 0;
+}
+
+static const struct of_device_id regulator_power_domain_dt_ids[] = {
+	{ .compatible = "regulator-power-domain"},
+	{ }
+};
+
+static struct platform_driver regulator_power_domain_driver = {
+	.driver = {
+		.name = "regulator2pd",
+		.of_match_table = regulator_power_domain_dt_ids,
+	},
+	.probe = regulator_power_domain_probe,
+	.remove = regulator_power_domain_remove,
+};
+builtin_platform_driver(regulator_power_domain_driver)
+
+MODULE_AUTHOR("Shenwei Wang <shenwei.wang@nxp.com>");
+MODULE_DESCRIPTION("regulator power domain driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Rob Herring]

On Fri, Aug 18, 2023 at 10:35 AM Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
> Documenting the regulator power domain properties and usage examples.

This needs to answer why we need this.

It looks like just an abstraction layer to make regulators look like a
power domain.

Rob

RE: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Rob Herring <robh+dt@kernel.org>
> Sent: Friday, August 18, 2023 3:52 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >
> > Documenting the regulator power domain properties and usage examples.
> 
> This needs to answer why we need this.
> 
> It looks like just an abstraction layer to make regulators look like a power
> domain.
> 

Yes, it is a wrapper that allows using regulators as a power domain. This removes 
the need to add regulator operating code in each consumer device driver. As a power 
domain, the regulator will be managed automatically by the device driver framework 
and PM subsystem.

This is very useful when a device's power is controlled by a GPIO pin, which currently 
requires using the fixed-regulator to achieve the same purpose. However, the 
fixed-regulator approach may have to add code in the driver in order to use it.

Thanks,
Shenwei

> Rob

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 18/08/2023 23:06, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Rob Herring <robh+dt@kernel.org>
>> Sent: Friday, August 18, 2023 3:52 PM
>> To: Shenwei Wang <shenwei.wang@nxp.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
>> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
>> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>>>
>>> Documenting the regulator power domain properties and usage examples.
>>
>> This needs to answer why we need this.
>>
>> It looks like just an abstraction layer to make regulators look like a power
>> domain.
>>
> 
> Yes, it is a wrapper that allows using regulators as a power domain. This removes 
> the need to add regulator operating code in each consumer device driver. As a power 
> domain, the regulator will be managed automatically by the device driver framework 
> and PM subsystem.
> 
> This is very useful when a device's power is controlled by a GPIO pin, which currently 
> requires using the fixed-regulator to achieve the same purpose. However, the 
> fixed-regulator approach may have to add code in the driver in order to use it.

Why do you start discussion from zero ignoring all previous history of
this patchset?

https://lore.kernel.org/all/20220609150851.23084-1-max.oss.09@gmail.com/

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Saturday, August 19, 2023 3:04 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
> <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>
> 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
>
>
> >>
> >> This needs to answer why we need this.
> >>
> >> It looks like just an abstraction layer to make regulators look like
> >> a power domain.
> >>
> >
> > Yes, it is a wrapper that allows using regulators as a power domain.
> > This removes the need to add regulator operating code in each consumer
> > device driver. As a power domain, the regulator will be managed
> > automatically by the device driver framework and PM subsystem.
> >
> > This is very useful when a device's power is controlled by a GPIO pin,
> > which currently requires using the fixed-regulator to achieve the same
> > purpose. However, the fixed-regulator approach may have to add code in the
> driver in order to use it.
>
> Why do you start discussion from zero ignoring all previous history of this
> patchset?
>

Thank you for providing the link. After reviewing the entire thread, I still don't understand how
to proceed. What is the conclusion regarding this commonly used use case but overlooked feature
in the upstream kernel?

Thanks,
Shenwei

> https://lore.kern/
> el.org%2Fall%2F20220609150851.23084-1-
> max.oss.09%40gmail.com%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%
> 7C2cf40d23202c430302c908dba08add2e%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C638280290493921016%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&sdata=0O%2FIytE6YbJL26je7hoxEu0ayZYs07PBjfZkBDVaC1M
> %3D&reserved=0
>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Rob Herring]

On Mon, Aug 21, 2023 at 8:22 AM Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Sent: Saturday, August 19, 2023 3:04 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
> > <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >
> > 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
> >
> >
> > >>
> > >> This needs to answer why we need this.
> > >>
> > >> It looks like just an abstraction layer to make regulators look like
> > >> a power domain.
> > >>
> > >
> > > Yes, it is a wrapper that allows using regulators as a power domain.
> > > This removes the need to add regulator operating code in each consumer
> > > device driver. As a power domain, the regulator will be managed
> > > automatically by the device driver framework and PM subsystem.
> > >
> > > This is very useful when a device's power is controlled by a GPIO pin,
> > > which currently requires using the fixed-regulator to achieve the same
> > > purpose. However, the fixed-regulator approach may have to add code in the
> > driver in order to use it.
> >
> > Why do you start discussion from zero ignoring all previous history of this
> > patchset?
> >
>
> Thank you for providing the link. After reviewing the entire thread, I still don't understand how
> to proceed. What is the conclusion regarding this commonly used use case but overlooked feature
> in the upstream kernel?

Overlooked implies we missed and ignored it, but the same concept has
been submitted twice and rejected twice. What use case cannot be
supported?

The detail that power-domains get handled automatically is an
implementation detail in the kernel (currently). That could easily
change and you'd be in the same position as with regulator supplies.
We could just as easily decide to make the driver core turn on all
supplies in a node. That would give you the same "feature". Why would
you design your DT around implementation decisions of the OS?

Rob

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Rob Herring <robh+dt@kernel.org>
> Sent: Monday, August 21, 2023 1:50 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > Thank you for providing the link. After reviewing the entire thread, I
> > still don't understand how to proceed. What is the conclusion
> > regarding this commonly used use case but overlooked feature in the
> upstream kernel?
> 
> Overlooked implies we missed and ignored it, but the same concept has been
> submitted twice and rejected twice. What use case cannot be supported?
> 

No offend. :) Sorry for my poor word. To provide more context, a common use case 
example is using a GPIO pin as a power switch. The current implementation operates 
as a fixed regulator, which makes it difficult to control the on/off timing without modifying
its driver. It also lacks power management support. 

> The detail that power-domains get handled automatically is an implementation
> detail in the kernel (currently). That could easily change and you'd be in the same
> position as with regulator supplies.

The proposed regulator-pd driver follows the standard PD driver framework, so it for sure
relies on certain kernel implementation details. If those underlying implementation details 
change in the future, this driver as well as other PD drivers built on the same framework 
would need to be updated accordingly. 

> We could just as easily decide to make the driver core turn on all supplies in a
> node. That would give you the same "feature". Why would you design your DT
> around implementation decisions of the OS?
> 

This DT properties are proposed solely for this specific driver, not to hack the OS. This 
is no different than other PD drivers like gpc/scu-pd/imx93-pd.

Thanks,
Shenwei

> Rob

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 22/08/2023 17:18, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Rob Herring <robh+dt@kernel.org>
>> Sent: Monday, August 21, 2023 1:50 PM
>> To: Shenwei Wang <shenwei.wang@nxp.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
>> Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
>> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>>> Thank you for providing the link. After reviewing the entire thread, I
>>> still don't understand how to proceed. What is the conclusion
>>> regarding this commonly used use case but overlooked feature in the
>> upstream kernel?
>>
>> Overlooked implies we missed and ignored it, but the same concept has been
>> submitted twice and rejected twice. What use case cannot be supported?
>>
> 
> No offend. :) Sorry for my poor word. To provide more context, a common use case 
> example is using a GPIO pin as a power switch. The current implementation operates 
> as a fixed regulator, which makes it difficult to control the on/off timing without modifying
> its driver. 

So it is a problem of a driver?

> It also lacks power management support. 

Which is not related to bindings but implementation in given driver.

> 
>> The detail that power-domains get handled automatically is an implementation
>> detail in the kernel (currently). That could easily change and you'd be in the same
>> position as with regulator supplies.
> 
> The proposed regulator-pd driver follows the standard PD driver framework, so it for sure
> relies on certain kernel implementation details. If those underlying implementation details 
> change in the future, this driver as well as other PD drivers built on the same framework 
> would need to be updated accordingly. 

We talk about bindings which you would not be allowed to change. Thus
your case would stop working...

> 
>> We could just as easily decide to make the driver core turn on all supplies in a
>> node. That would give you the same "feature". Why would you design your DT
>> around implementation decisions of the OS?
>>
> 
> This DT properties are proposed solely for this specific driver, not to hack the OS. This 
> is no different than other PD drivers like gpc/scu-pd/imx93-pd.

I am not sure if you got Rob's point, I have feelings that not. Argument
that some OS implements something some way, is not an argument for a new
binding, barely hardware related.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 22, 2023 10:26 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
> <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >
> > No offend. :) Sorry for my poor word. To provide more context, a
> > common use case example is using a GPIO pin as a power switch. The
> > current implementation operates as a fixed regulator, which makes it
> > difficult to control the on/off timing without modifying its driver.
> 
> So it is a problem of a driver?
> 

That is arguable too. The driver may assume its power is on when probed, which 
aligns with how the PD behaves.

> > It also lacks power management support.
> 
> Which is not related to bindings but implementation in given driver.
> 

For those simple drivers, the default power management logic can handle 
power correctly without requiring any specialized implementation in the 
driver code.

> >
> >> The detail that power-domains get handled automatically is an
> >> implementation detail in the kernel (currently). That could easily
> >> change and you'd be in the same position as with regulator supplies.
> >
> > The proposed regulator-pd driver follows the standard PD driver
> > framework, so it for sure relies on certain kernel implementation
> > details. If those underlying implementation details change in the
> > future, this driver as well as other PD drivers built on the same framework
> would need to be updated accordingly.
> 
> We talk about bindings which you would not be allowed to change. Thus your
> case would stop working...
> 

As a new driver, it has to involve some new bindings especially the compatible 
string.

> >
> >> We could just as easily decide to make the driver core turn on all
> >> supplies in a node. That would give you the same "feature". Why would
> >> you design your DT around implementation decisions of the OS?
> >>
> >
> > This DT properties are proposed solely for this specific driver, not
> > to hack the OS. This is no different than other PD drivers like gpc/scu-
> pd/imx93-pd.
> 
> I am not sure if you got Rob's point, I have feelings that not. Argument that
> some OS implements something some way, is not an argument for a new
> binding, barely hardware related.
> 

Thank you for the clarification. The issue is that this driver is purely a software layer 
that wraps the regulators as a power domain. The bindings make the implementation 
clean and easy to understand.  I don't think we should add extra complex logic inside 
the driver solely to avoid introducing new bindings.

Thanks,
Shenwei

> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 22/08/2023 17:50, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, August 22, 2023 10:26 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
>> <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
>> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
>> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>>>
>>> No offend. :) Sorry for my poor word. To provide more context, a
>>> common use case example is using a GPIO pin as a power switch. The
>>> current implementation operates as a fixed regulator, which makes it
>>> difficult to control the on/off timing without modifying its driver.
>>
>> So it is a problem of a driver?
>>
> 
> That is arguable too. The driver may assume its power is on when probed, which 
> aligns with how the PD behaves.

So everything in driver... no discussion about bindings.

> 
>>> It also lacks power management support.
>>
>> Which is not related to bindings but implementation in given driver.
>>
> 
> For those simple drivers, the default power management logic can handle 
> power correctly without requiring any specialized implementation in the 
> driver code.

You can create any drivers you wish or change existing ones. I don't see
a problem here.

> 
>>>
>>>> The detail that power-domains get handled automatically is an
>>>> implementation detail in the kernel (currently). That could easily
>>>> change and you'd be in the same position as with regulator supplies.
>>>
>>> The proposed regulator-pd driver follows the standard PD driver
>>> framework, so it for sure relies on certain kernel implementation
>>> details. If those underlying implementation details change in the
>>> future, this driver as well as other PD drivers built on the same framework
>> would need to be updated accordingly.
>>
>> We talk about bindings which you would not be allowed to change. Thus your
>> case would stop working...
>>
> 
> As a new driver, it has to involve some new bindings especially the compatible 
> string.

I am not talking about this. I do not speak about creating new bindings,
but changing them.
> 
>>>
>>>> We could just as easily decide to make the driver core turn on all
>>>> supplies in a node. That would give you the same "feature". Why would
>>>> you design your DT around implementation decisions of the OS?
>>>>
>>>
>>> This DT properties are proposed solely for this specific driver, not
>>> to hack the OS. This is no different than other PD drivers like gpc/scu-
>> pd/imx93-pd.
>>
>> I am not sure if you got Rob's point, I have feelings that not. Argument that
>> some OS implements something some way, is not an argument for a new
>> binding, barely hardware related.
>>
> 
> Thank you for the clarification. The issue is that this driver is purely a software layer 
> that wraps the regulators as a power domain. The bindings make the implementation 
> clean and easy to understand.  I don't think we should add extra complex logic inside 
> the driver solely to avoid introducing new bindings.

Since bindings are not for software layers, I don't think we should be
talking about them just to avoid introducing driver changes.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, August 22, 2023 10:58 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
> <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml
> file
> 
> >
> > That is arguable too. The driver may assume its power is on when
> > probed, which aligns with how the PD behaves.
> 
> So everything in driver... no discussion about bindings.
> 

That's true only when you have a PD driver for it. 

> >
> >>> It also lacks power management support.
> >>
> >> Which is not related to bindings but implementation in given driver.
> >>
> >
> > For those simple drivers, the default power management logic can
> > handle power correctly without requiring any specialized
> > implementation in the driver code.
> 
> You can create any drivers you wish or change existing ones. I don't see a
> problem here.
> 

That's what this patch set does.

> >
> >>>
> >
> > As a new driver, it has to involve some new bindings especially the
> > compatible string.
> 
> I am not talking about this. I do not speak about creating new bindings, but
> changing them.

I'm a bit confused by your statement. The patch only creates some necessary 
new bindings for the new driver. It doesn't change any existing bindings.

> >
> >>>
> > Thank you for the clarification. The issue is that this driver is
> > purely a software layer that wraps the regulators as a power domain.
> > The bindings make the implementation clean and easy to understand.  I
> > don't think we should add extra complex logic inside the driver solely to avoid
> introducing new bindings.
> 
> Since bindings are not for software layers, I don't think we should be talking
> about them just to avoid introducing driver changes.
> 

While it is a software layer, it is a driver that requires bindings. I don't think 
you would recommend hardcoding those properties inside the driver itself.

Thanks,
Shenwei

> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Ulf Hansson]

On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
> Documenting the regulator power domain properties and usage examples.

As Rob and Krzysztof already pointed out, I agree that this binding
looks a bit questionable.

Rather than adding a new DT binding, why can't we just use the
existing way of describing a platform specific power-domain provider?
This still looks platform specific to me.

Kind regards
Uffe

>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  .../bindings/power/regulator-pd.yaml          | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/regulator-pd.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml b/Documentation/devicetree/bindings/power/regulator-pd.yaml
> new file mode 100644
> index 000000000000..181d2fa83f8a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/regulator-pd.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/regulator-pd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Regulator Power Domain
> +
> +maintainers:
> +  - Shenwei Wang <shenwei.wang@nxp.com>
> +
> +description: |
> +  This describes a power domain which manages a group of regulators.
> +
> +allOf:
> +  - $ref: power-domain.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: regulator-power-domain
> +
> +  '#power-domain-cells':
> +    const: 1
> +
> +  regulator-number:
> +    minimum: 1
> +    maximum: 100
> +    description: The count of regulator to be managed by this power domain
> +
> +patternProperties:
> +  "regulator-[0-99]-supply$":
> +    description: The regulator supply phandle to be managed by this power domain
> +
> +required:
> +  - compatible
> +  - '#power-domain-cells'
> +  - regulator-number
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    reg1: regulator-1 {
> +       compatible = "regulator-fixed";
> +       regulator-name = "REG1";
> +       regulator-min-microvolt = <3000000>;
> +       regulator-max-microvolt = <3000000>;
> +       gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
> +       enable-active-high;
> +    };
> +
> +    reg2: regulator-2 {
> +       compatible = "regulator-fixed";
> +       regulator-name = "REG2";
> +       regulator-min-microvolt = <3000000>;
> +       regulator-max-microvolt = <3000000>;
> +       gpio = <&lsio_gpio4 20 GPIO_ACTIVE_HIGH>;
> +       enable-active-high;
> +    };
> +
> +    power-controller {
> +        compatible = "regulator-power-domain";
> +        #power-domain-cells = <1>;
> +
> +        regulator-number = <2>;
> +        regulator-0-supply = <&reg1>;
> +        regulator-1-supply = <&reg2>;
> +    };
> --
> 2.34.1
>

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Thursday, August 24, 2023 4:27 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> 
> 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
> 
> 
> On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote:
> >
> > Documenting the regulator power domain properties and usage examples.
> 
> As Rob and Krzysztof already pointed out, I agree that this binding looks a bit
> questionable.
> 
> Rather than adding a new DT binding, why can't we just use the existing way of
> describing a platform specific power-domain provider?

Can you please provide more details on how you thought we should implement this
feature using the existing way? Very appreciate if you could provide a simple example.

> This still looks platform specific to me.

What does platform specific exactly mean here?  I want to make sure I understand 
what you were referring to.

Thanks,
Shenwei

> 
> Kind regards
> Uffe
> 
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  .../bindings/power/regulator-pd.yaml          | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/power/regulator-pd.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/power/regulator-pd.yaml
> > b/Documentation/devicetree/bindings/power/regulator-pd.yaml
> > new file mode 100644
> > index 000000000000..181d2fa83f8a

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Ulf Hansson]

On Thu, 24 Aug 2023 at 18:35, Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: Thursday, August 24, 2023 4:27 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >
> > 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
> >
> >
> > On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com> wrote:
> > >
> > > Documenting the regulator power domain properties and usage examples.
> >
> > As Rob and Krzysztof already pointed out, I agree that this binding looks a bit
> > questionable.
> >
> > Rather than adding a new DT binding, why can't we just use the existing way of
> > describing a platform specific power-domain provider?
>
> Can you please provide more details on how you thought we should implement this
> feature using the existing way? Very appreciate if you could provide a simple example.
>
> > This still looks platform specific to me.
>
> What does platform specific exactly mean here?  I want to make sure I understand
> what you were referring to.

There are plenty of examples of how a platform specific genpd provider
looks in DT. You may have a look a imx platforms for example.

git grep "#power-domain-cells" arch/arm/boot/dts/nxp/imx

The genpd provider then needs to be a consumer of the resources it
needs. In this case a couple of regulators it seems like.

[...]

Kind regards
Uffe

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Friday, August 25, 2023 7:25 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> 
> 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
> 
> 
> On Thu, 24 Aug 2023 at 18:35, Shenwei Wang <shenwei.wang@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Sent: Thursday, August 24, 2023 4:27 AM
> > > To: Shenwei Wang <shenwei.wang@nxp.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark
> > > Brown <broonie@kernel.org>; imx@lists.linux.dev;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>
> > > Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd
> > > yaml file
> > >
> > > 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
> > >
> > >
> > > On Fri, 18 Aug 2023 at 17:35, Shenwei Wang <shenwei.wang@nxp.com>
> wrote:
> > > >
> > > > Documenting the regulator power domain properties and usage examples.
> > >
> > > As Rob and Krzysztof already pointed out, I agree that this binding
> > > looks a bit questionable.
> > >
> > > Rather than adding a new DT binding, why can't we just use the
> > > existing way of describing a platform specific power-domain provider?
> >
> > Can you please provide more details on how you thought we should
> > implement this feature using the existing way? Very appreciate if you could
> provide a simple example.
> >
> > > This still looks platform specific to me.
> >
> > What does platform specific exactly mean here?  I want to make sure I
> > understand what you were referring to.
> 
> There are plenty of examples of how a platform specific genpd provider looks in
> DT. You may have a look a imx platforms for example.
> 
> git grep "#power-domain-cells" arch/arm/boot/dts/nxp/imx
> 
> The genpd provider then needs to be a consumer of the resources it needs. In
> this case a couple of regulators it seems like.
> 

If I understood your reply correctly,  it seems that the current implementation of 
regulator-pd is what you have described. Please correct me if I'm mistaken.

The following are the diff of scu-pd and this regulator-pd.

    power-controller {						    power-controller {
        compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";      |	        compatible = "regulator-power-domain";
        #power-domain-cells = <1>;				        #power-domain-cells = <1>;
							      >
							      >	        regulator-number = <2>;
							      >	        regulator-0-supply = <&reg1>;
							      >	        regulator-1-supply = <&reg2>;
    };								    };

Are you suggesting to move the regulator-pd to the imx directory and add a company prefix
to the compatible string?

Thanks,
Shenwei

> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 25/08/2023 17:44, Shenwei Wang wrote:
>>
>> The genpd provider then needs to be a consumer of the resources it needs. In
>> this case a couple of regulators it seems like.
>>
> 
> If I understood your reply correctly,  it seems that the current implementation of 
> regulator-pd is what you have described. Please correct me if I'm mistaken.
> 
> The following are the diff of scu-pd and this regulator-pd.
> 
>     power-controller {						    power-controller {
>         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";      |	        compatible = "regulator-power-domain";
>         #power-domain-cells = <1>;				        #power-domain-cells = <1>;
> 							      >
> 							      >	        regulator-number = <2>;
> 							      >	        regulator-0-supply = <&reg1>;
> 							      >	        regulator-1-supply = <&reg2>;
>     };								    };
> 
> Are you suggesting to move the regulator-pd to the imx directory and add a company prefix
> to the compatible string?

There is no such part of iMX processor as such regulator-power-domain,
so I don't recommend that approach. DTS nodes represent hardware, not
your SW layers.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Ulf Hansson]

On Sat, 26 Aug 2023 at 19:31, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/08/2023 17:44, Shenwei Wang wrote:
> >>
> >> The genpd provider then needs to be a consumer of the resources it needs. In
> >> this case a couple of regulators it seems like.
> >>
> >
> > If I understood your reply correctly,  it seems that the current implementation of
> > regulator-pd is what you have described. Please correct me if I'm mistaken.
> >
> > The following are the diff of scu-pd and this regulator-pd.
> >
> >     power-controller {                                                    power-controller {
> >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";      |               compatible = "regulator-power-domain";
> >         #power-domain-cells = <1>;                                    #power-domain-cells = <1>;
> >                                                             >
> >                                                             >         regulator-number = <2>;
> >                                                             >         regulator-0-supply = <&reg1>;
> >                                                             >         regulator-1-supply = <&reg2>;
> >     };                                                                    };
> >
> > Are you suggesting to move the regulator-pd to the imx directory and add a company prefix
> > to the compatible string?
>
> There is no such part of iMX processor as such regulator-power-domain,
> so I don't recommend that approach. DTS nodes represent hardware, not
> your SW layers.

I would agree if this was pure SW layers, but I don't think it is. At
least, if I have understood the earlier discussions correctly [1],
there are certainly one or more power-domains here. The power-domains
just happen to be powered through something that can be modelled as a
regular regulator(s). No?

Note that, we already have other power-domains that are consumers of
regulators too.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20220609150851.23084-1-max.oss.09@gmail.com/

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 11:59, Ulf Hansson wrote:
> On Sat, 26 Aug 2023 at 19:31, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 25/08/2023 17:44, Shenwei Wang wrote:
>>>>
>>>> The genpd provider then needs to be a consumer of the resources it needs. In
>>>> this case a couple of regulators it seems like.
>>>>
>>>
>>> If I understood your reply correctly,  it seems that the current implementation of
>>> regulator-pd is what you have described. Please correct me if I'm mistaken.
>>>
>>> The following are the diff of scu-pd and this regulator-pd.
>>>
>>>     power-controller {                                                    power-controller {
>>>         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";      |               compatible = "regulator-power-domain";
>>>         #power-domain-cells = <1>;                                    #power-domain-cells = <1>;
>>>                                                             >
>>>                                                             >         regulator-number = <2>;
>>>                                                             >         regulator-0-supply = <&reg1>;
>>>                                                             >         regulator-1-supply = <&reg2>;
>>>     };                                                                    };
>>>
>>> Are you suggesting to move the regulator-pd to the imx directory and add a company prefix
>>> to the compatible string?
>>
>> There is no such part of iMX processor as such regulator-power-domain,
>> so I don't recommend that approach. DTS nodes represent hardware, not
>> your SW layers.
> 
> I would agree if this was pure SW layers, but I don't think it is. At
> least, if I have understood the earlier discussions correctly [1],
> there are certainly one or more power-domains here. The power-domains
> just happen to be powered through something that can be modelled as a
> regular regulator(s). No?

No. It was for controlling power of multiple devices, supplied by
multiple different or similar regulators, where Linux drivers for these
devices (so not even all drivers...) do not have regulator control. The
bindings for these devices allow power-domains, but not regulator.

There are no multiple power domains in the problem. Even term "power
domain" is questionable here, because we tend to look power domain as
part of SoC. Here it is some selected part of the circuitry, like few
totally independent devices which share purpose and power rails.

But more important is my first paragraph - this is purely to avoid
adding regulators to these devices.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Monday, August 28, 2023 4:59 AM
> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Shenwei Wang <shenwei.wang@nxp.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> > > If I understood your reply correctly,  it seems that the current
> > > implementation of regulator-pd is what you have described. Please correct
> me if I'm mistaken.
> > >
> > > The following are the diff of scu-pd and this regulator-pd.
> > >
> > >     power-controller {                                                    power-controller {
> > >         compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";      |               compatible
> = "regulator-power-domain";
> > >         #power-domain-cells = <1>;                                    #power-domain-cells =
> <1>;
> > >                                                             >
> > >                                                             >         regulator-number = <2>;
> > >                                                             >         regulator-0-supply = <&reg1>;
> > >                                                             >         regulator-1-supply = <&reg2>;
> > >     };                                                                    };
> > >
> > > Are you suggesting to move the regulator-pd to the imx directory and
> > > add a company prefix to the compatible string?
> >
> > There is no such part of iMX processor as such regulator-power-domain,
> > so I don't recommend that approach. DTS nodes represent hardware, not
> > your SW layers.
>
> I would agree if this was pure SW layers, but I don't think it is. At least, if I have
> understood the earlier discussions correctly [1], there are certainly one or more
> power-domains here. The power-domains just happen to be powered through
> something that can be modelled as a regular regulator(s). No?
>
> Note that, we already have other power-domains that are consumers of
> regulators too.

Can you please point me an example? I would be happy to use the existing implementation
if it fits this use case.

Thanks,
Shenwei

>
> Kind regards
> Uffe
>
> [1]
> https://lore.kern/
> el.org%2Fall%2F20220609150851.23084-1-
> max.oss.09%40gmail.com%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%
> 7Cebeaa5d30be14033509f08dba7ad7dbd%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638288135797275723%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&sdata=re2A5wbng9haHoMKTbPICh%2Fh9VJVsG66H9jybd2fN
> k8%3D&reserved=0

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Saturday, August 26, 2023 12:32 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >                                                             >
> >                                                             >         regulator-number = <2>;
> >                                                             >         regulator-0-supply = <&reg1>;
> >                                                             >         regulator-1-supply = <&reg2>;
> >     };                                                                    };
> >
> > Are you suggesting to move the regulator-pd to the imx directory and
> > add a company prefix to the compatible string?
> 
> There is no such part of iMX processor as such regulator-power-domain, so I
> don't recommend that approach. DTS nodes represent hardware, not your SW
> layers.
> 

That's not always the case, as we do sometimes need a virtual device. 
As an example, the "regulator-fixed" acts as a software abstraction layer to create virtual regulator 
devices by interfacing with the underlying GPIO drivers.
Similarly, "regulator-pd" provides a software abstraction layer for virtual PD devices built on 
top of existing regulator drivers.
When looking at the conceptual purpose, regulator-fixed and regulator-pd are comparable in 
that they both offer software abstraction layers for virtual devices."

Thanks,
Shenwei

> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 16:04, Shenwei Wang wrote:

>>> Are you suggesting to move the regulator-pd to the imx directory and
>>> add a company prefix to the compatible string?
>>
>> There is no such part of iMX processor as such regulator-power-domain, so I
>> don't recommend that approach. DTS nodes represent hardware, not your SW
>> layers.
>>
> 
> That's not always the case, as we do sometimes need a virtual device. 
> As an example, the "regulator-fixed" acts as a software abstraction layer to create virtual regulator 
> devices by interfacing with the underlying GPIO drivers.

Not true. This is a real regulator device. Real hardware on the board.
You can even see and touch it.

> Similarly, "regulator-pd" provides a software abstraction layer for virtual PD devices built on 
> top of existing regulator drivers.

This is not related to regulator-fixed at all.

> When looking at the conceptual purpose, regulator-fixed and regulator-pd are comparable in 
> that they both offer software abstraction layers for virtual devices."

No. regulator-fixed is a real device. Yours is not.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, August 28, 2023 12:11 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> On 28/08/2023 16:04, Shenwei Wang wrote:
> 
> >>> Are you suggesting to move the regulator-pd to the imx directory and
> >>> add a company prefix to the compatible string?
> >>
> >> There is no such part of iMX processor as such
> >> regulator-power-domain, so I don't recommend that approach. DTS nodes
> >> represent hardware, not your SW layers.
> >>
> >
> > That's not always the case, as we do sometimes need a virtual device.
> > As an example, the "regulator-fixed" acts as a software abstraction
> > layer to create virtual regulator devices by interfacing with the underlying
> GPIO drivers.
> 
> Not true. This is a real regulator device. Real hardware on the board.
> You can even see and touch it.
> 

The physical hardware component is the GPIO pin, which is what you can only touch. 
The regulator functions virtually through software layer above of the GPIO driver. While 
we may call it a "regulator" or whatever else, this cannot obscure the fact that the underlying 
hardware is just a GPIO pin being used in a specialized way.

Thanks,
Shenwei

> > Similarly, "regulator-pd" provides a software abstraction layer for
> > virtual PD devices built on top of existing regulator drivers.
> 
> This is not related to regulator-fixed at all.
> 
> > When looking at the conceptual purpose, regulator-fixed and
> > regulator-pd are comparable in that they both offer software abstraction
> layers for virtual devices."
> 
> No. regulator-fixed is a real device. Yours is not.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 20:39, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Monday, August 28, 2023 12:11 PM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
>> <ulf.hansson@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
>> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>> On 28/08/2023 16:04, Shenwei Wang wrote:
>>
>>>>> Are you suggesting to move the regulator-pd to the imx directory and
>>>>> add a company prefix to the compatible string?
>>>>
>>>> There is no such part of iMX processor as such
>>>> regulator-power-domain, so I don't recommend that approach. DTS nodes
>>>> represent hardware, not your SW layers.
>>>>
>>>
>>> That's not always the case, as we do sometimes need a virtual device.
>>> As an example, the "regulator-fixed" acts as a software abstraction
>>> layer to create virtual regulator devices by interfacing with the underlying
>> GPIO drivers.
>>
>> Not true. This is a real regulator device. Real hardware on the board.
>> You can even see and touch it.
>>
> 
> The physical hardware component is the GPIO pin, which is what you can only touch. 

No. The regulator is the chip.

> The regulator functions virtually through software layer above of the GPIO driver. While 
> we may call it a "regulator" or whatever else, this cannot obscure the fact that the underlying 
> hardware is just a GPIO pin being used in a specialized way.

The regulator is some tiny little box, you can touch and called
ti,tps51632 or similar.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, August 28, 2023 1:42 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >>>>> Are you suggesting to move the regulator-pd to the imx directory
> >>>>> and add a company prefix to the compatible string?
> >>>>
> >>>> There is no such part of iMX processor as such
> >>>> regulator-power-domain, so I don't recommend that approach. DTS
> >>>> nodes represent hardware, not your SW layers.
> >>>>
> >>>
> >>> That's not always the case, as we do sometimes need a virtual device.
> >>> As an example, the "regulator-fixed" acts as a software abstraction
> >>> layer to create virtual regulator devices by interfacing with the
> >>> underlying
> >> GPIO drivers.
> >>
> >> Not true. This is a real regulator device. Real hardware on the board.
> >> You can even see and touch it.
> >>
> >
> > The physical hardware component is the GPIO pin, which is what you can only
> touch.
> 
> No. The regulator is the chip.
> 

In the definition of dts node below, where is the chip? The real hardware is just a GPIO Pin.
    reg1: regulator-1 {
    	compatible = "regulator-fixed";
    	regulator-name = "REG1";
    	regulator-min-microvolt = <3000000>;
    	regulator-max-microvolt = <3000000>;
    	gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
    	enable-active-high;
    };

> > The regulator functions virtually through software layer above of the
> > GPIO driver. While we may call it a "regulator" or whatever else, this
> > cannot obscure the fact that the underlying hardware is just a GPIO pin being
> used in a specialized way.
> 
> The regulator is some tiny little box, you can touch and called
> ti,tps51632 or similar.
> 

We are talking about the specific "regulator-fixed" driver, why did you bring up "ti,tps51632" here?

Thanks,
Shenwei

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 20:50, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Monday, August 28, 2023 1:42 PM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
>> <ulf.hansson@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
>> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>>>>>>> Are you suggesting to move the regulator-pd to the imx directory
>>>>>>> and add a company prefix to the compatible string?
>>>>>>
>>>>>> There is no such part of iMX processor as such
>>>>>> regulator-power-domain, so I don't recommend that approach. DTS
>>>>>> nodes represent hardware, not your SW layers.
>>>>>>
>>>>>
>>>>> That's not always the case, as we do sometimes need a virtual device.
>>>>> As an example, the "regulator-fixed" acts as a software abstraction
>>>>> layer to create virtual regulator devices by interfacing with the
>>>>> underlying
>>>> GPIO drivers.
>>>>
>>>> Not true. This is a real regulator device. Real hardware on the board.
>>>> You can even see and touch it.
>>>>
>>>
>>> The physical hardware component is the GPIO pin, which is what you can only
>> touch.
>>
>> No. The regulator is the chip.
>>
> 
> In the definition of dts node below, where is the chip? The real hardware is just a GPIO Pin.
>     reg1: regulator-1 {
>     	compatible = "regulator-fixed";
>     	regulator-name = "REG1";
>     	regulator-min-microvolt = <3000000>;
>     	regulator-max-microvolt = <3000000>;
>     	gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
>     	enable-active-high;
>     };

There is a chip. This is the chip. If you have there only GPIO pin, then
your DTS is just wrong. Drop it. If you learn from wrong DTS, then sure,
power-domain-regulator seems like great idea...

> 
>>> The regulator functions virtually through software layer above of the
>>> GPIO driver. While we may call it a "regulator" or whatever else, this
>>> cannot obscure the fact that the underlying hardware is just a GPIO pin being
>> used in a specialized way.
>>
>> The regulator is some tiny little box, you can touch and called
>> ti,tps51632 or similar.
>>
> 
> We are talking about the specific "regulator-fixed" driver, why did you bring up "ti,tps51632" here?

Just an example. Can be TPS123098.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, August 28, 2023 1:53 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >>>>>>> Are you suggesting to move the regulator-pd to the imx directory
> >>>>>>> and add a company prefix to the compatible string?
> >>>>>>
> >>>>>> There is no such part of iMX processor as such
> >>>>>> regulator-power-domain, so I don't recommend that approach. DTS
> >>>>>> nodes represent hardware, not your SW layers.
> >>>>>>
> >>>>>
> >>>>> That's not always the case, as we do sometimes need a virtual device.
> >>>>> As an example, the "regulator-fixed" acts as a software
> >>>>> abstraction layer to create virtual regulator devices by
> >>>>> interfacing with the underlying
> >>>> GPIO drivers.
> >>>>
> >>>> Not true. This is a real regulator device. Real hardware on the board.
> >>>> You can even see and touch it.
> >>>>
> >>>
> >>> The physical hardware component is the GPIO pin, which is what you
> >>> can only
> >> touch.
> >>
> >> No. The regulator is the chip.
> >>
> >
> > In the definition of dts node below, where is the chip? The real hardware is just
> a GPIO Pin.
> >     reg1: regulator-1 {
> >       compatible = "regulator-fixed";
> >       regulator-name = "REG1";
> >       regulator-min-microvolt = <3000000>;
> >       regulator-max-microvolt = <3000000>;
> >       gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
> >       enable-active-high;
> >     };
> 
> There is a chip. This is the chip. If you have there only GPIO pin, then your DTS is
> just wrong. Drop it. If you learn from wrong DTS, then sure, power-domain-
> regulator seems like great idea...
> 

When you talk about the chip, can you please be more specific?

Regarding the dts node, how about the example in the fixed-regulator.yaml under the bindings directory.

    reg_1v8: regulator-1v8 {
      compatible = "regulator-fixed";
      regulator-name = "1v8";
      regulator-min-microvolt = <1800000>;
      regulator-max-microvolt = <1800000>;
      gpio = <&gpio1 16 0>;
      startup-delay-us = <70000>;
      enable-active-high;
      regulator-boot-on;
      gpio-open-drain;
      vin-supply = <&parent_reg>;
    };

If you take a look at the fixed regulator driver (fixed.c), I don't think you'll find anything related to a hardware 
component (chip) other than the GPIO Pin.

Thanks,
Shenwei

> >
> >>> The regulator functions virtually through software layer above of
> >>> the GPIO driver. While we may call it a "regulator" or whatever
> >>> else, this cannot obscure the fact that the underlying hardware is
> >>> just a GPIO pin being
> >> used in a specialized way.
> >>
> >> The regulator is some tiny little box, you can touch and called
> >> ti,tps51632 or similar.
> >>
> >
> > We are talking about the specific "regulator-fixed" driver, why did you bring up
> "ti,tps51632" here?
> 
> Just an example. Can be TPS123098.
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 21:09, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Monday, August 28, 2023 1:53 PM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
>> <ulf.hansson@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
>> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
>> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
>>>>>>>>> Are you suggesting to move the regulator-pd to the imx directory
>>>>>>>>> and add a company prefix to the compatible string?
>>>>>>>>
>>>>>>>> There is no such part of iMX processor as such
>>>>>>>> regulator-power-domain, so I don't recommend that approach. DTS
>>>>>>>> nodes represent hardware, not your SW layers.
>>>>>>>>
>>>>>>>
>>>>>>> That's not always the case, as we do sometimes need a virtual device.
>>>>>>> As an example, the "regulator-fixed" acts as a software
>>>>>>> abstraction layer to create virtual regulator devices by
>>>>>>> interfacing with the underlying
>>>>>> GPIO drivers.
>>>>>>
>>>>>> Not true. This is a real regulator device. Real hardware on the board.
>>>>>> You can even see and touch it.
>>>>>>
>>>>>
>>>>> The physical hardware component is the GPIO pin, which is what you
>>>>> can only
>>>> touch.
>>>>
>>>> No. The regulator is the chip.
>>>>
>>>
>>> In the definition of dts node below, where is the chip? The real hardware is just
>> a GPIO Pin.
>>>     reg1: regulator-1 {
>>>       compatible = "regulator-fixed";
>>>       regulator-name = "REG1";
>>>       regulator-min-microvolt = <3000000>;
>>>       regulator-max-microvolt = <3000000>;
>>>       gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
>>>       enable-active-high;
>>>     };
>>
>> There is a chip. This is the chip. If you have there only GPIO pin, then your DTS is
>> just wrong. Drop it. If you learn from wrong DTS, then sure, power-domain-
>> regulator seems like great idea...
>>
> 
> When you talk about the chip, can you please be more specific?

What to say more? The device node you quoted above is the regulator. You
brought specific example and now claim this is not a regulator, but just
GPIO. Please fix your DTS.

> 
> Regarding the dts node, how about the example in the fixed-regulator.yaml under the bindings directory.

That's an example, how is it related to anything?

> 
>     reg_1v8: regulator-1v8 {
>       compatible = "regulator-fixed";
>       regulator-name = "1v8";
>       regulator-min-microvolt = <1800000>;
>       regulator-max-microvolt = <1800000>;
>       gpio = <&gpio1 16 0>;
>       startup-delay-us = <70000>;
>       enable-active-high;
>       regulator-boot-on;
>       gpio-open-drain;
>       vin-supply = <&parent_reg>;
>     };
> 
> If you take a look at the fixed regulator driver (fixed.c), I don't think you'll find anything related to a hardware 
> component (chip) other than the GPIO Pin.

That's a driver. How is it related to this discussion? Bindings are not
about drivers.


Best regards,
Krzysztof

RE: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, August 28, 2023 2:12 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> >>>>>>>>> Are you suggesting to move the regulator-pd to the imx
> >>>>>>>>> directory and add a company prefix to the compatible string?
> >>>>>>>>
> >>>>>>>> There is no such part of iMX processor as such
> >>>>>>>> regulator-power-domain, so I don't recommend that approach. DTS
> >>>>>>>> nodes represent hardware, not your SW layers.
> >>>>>>>>
> >>>>>>>
> >>>>>>> That's not always the case, as we do sometimes need a virtual device.
> >>>>>>> As an example, the "regulator-fixed" acts as a software
> >>>>>>> abstraction layer to create virtual regulator devices by
> >>>>>>> interfacing with the underlying
> >>>>>> GPIO drivers.
> >>>>>>
> >>>>>> Not true. This is a real regulator device. Real hardware on the board.
> >>>>>> You can even see and touch it.
> >>>>>>
> >>>>>
> >>>>> The physical hardware component is the GPIO pin, which is what you
> >>>>> can only
> >>>> touch.
> >>>>
> >>>> No. The regulator is the chip.
> >>>>
> >>>
> >>> In the definition of dts node below, where is the chip? The real
> >>> hardware is just
> >> a GPIO Pin.
> >>>     reg1: regulator-1 {
> >>>       compatible = "regulator-fixed";
> >>>       regulator-name = "REG1";
> >>>       regulator-min-microvolt = <3000000>;
> >>>       regulator-max-microvolt = <3000000>;
> >>>       gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
> >>>       enable-active-high;
> >>>     };
> >>
> >> There is a chip. This is the chip. If you have there only GPIO pin,
> >> then your DTS is just wrong. Drop it. If you learn from wrong DTS,
> >> then sure, power-domain- regulator seems like great idea...
> >>
> >
> > When you talk about the chip, can you please be more specific?
> 
> What to say more? The device node you quoted above is the regulator. You
> brought specific example and now claim this is not a regulator, but just GPIO.
> Please fix your DTS.
> 

The fixed-regulator is a virtual regulator driver that uses the GPIO pin. You claimed this 
as a hardware chip.

The regulator-pd driver also uses the same GPIO pin. You now claimed this as a software layer.

What's your standard?

Thanks,
Shenwei

> >
> > Regarding the dts node, how about the example in the fixed-regulator.yaml
> under the bindings directory.
> 
> That's an example, how is it related to anything?
> 
> >
> >     reg_1v8: regulator-1v8 {
> >       compatible = "regulator-fixed";
> >       regulator-name = "1v8";
> >       regulator-min-microvolt = <1800000>;
> >       regulator-max-microvolt = <1800000>;
> >       gpio = <&gpio1 16 0>;
> >       startup-delay-us = <70000>;
> >       enable-active-high;
> >       regulator-boot-on;
> >       gpio-open-drain;
> >       vin-supply = <&parent_reg>;
> >     };
> >
> > If you take a look at the fixed regulator driver (fixed.c), I don't
> > think you'll find anything related to a hardware component (chip) other than
> the GPIO Pin.
> 
> That's a driver. How is it related to this discussion? Bindings are not about
> drivers.
> 
> 
> Best regards,
> Krzysztof

Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Krzysztof Kozlowski]

On 28/08/2023 21:23, Shenwei Wang wrote:
>>>>>     reg1: regulator-1 {
>>>>>       compatible = "regulator-fixed";
>>>>>       regulator-name = "REG1";
>>>>>       regulator-min-microvolt = <3000000>;
>>>>>       regulator-max-microvolt = <3000000>;
>>>>>       gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
>>>>>       enable-active-high;
>>>>>     };
>>>>
>>>> There is a chip. This is the chip. If you have there only GPIO pin,
>>>> then your DTS is just wrong. Drop it. If you learn from wrong DTS,
>>>> then sure, power-domain- regulator seems like great idea...
>>>>
>>>
>>> When you talk about the chip, can you please be more specific?
>>
>> What to say more? The device node you quoted above is the regulator. You
>> brought specific example and now claim this is not a regulator, but just GPIO.
>> Please fix your DTS.
>>
> 
> The fixed-regulator is a virtual regulator driver that uses the GPIO pin. 

We do not talk about drivers but bindings and DTS. Why do you bring
again drivers, all the time?

> You claimed this
> as a hardware chip.

??? Sorry, this is getting boring. The DTS-snippet is a hardware chip.
If it is not, then drop it from your DTS. I insist. Srsly, third time I
insist.


> 
> The regulator-pd driver also uses the same GPIO pin. 

Again, what is with the drivers? Can you stop bringing it to the discussion?

> You now claimed this as a software layer.

???

> 
> What's your standard?

I don't think there is anything more to say. You clearly do not
understand what is DTS, schematics and how the actual hardware looks like.

I am not going to respond more to this patchset (which is a clear NAK
just in case).

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, August 28, 2023 2:31 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml
> > The fixed-regulator is a virtual regulator driver that uses the GPIO pin.
> 
> We do not talk about drivers but bindings and DTS. Why do you bring again
> drivers, all the time?
> 
> > You claimed this
> > as a hardware chip.
> 
> ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip.
> If it is not, then drop it from your DTS. I insist. Srsly, third time I insist.
> 
> 
> >
> > The regulator-pd driver also uses the same GPIO pin.
> 
> Again, what is with the drivers? Can you stop bringing it to the discussion?
> 

I have to admit you have a real talent for debate.

Thanks,
Shenwei


> > You now claimed this as a software layer.
> 
> ???
> 
> >
> > What's your standard?
> 
> I don't think there is anything more to say. You clearly do not understand what is
> DTS, schematics and how the actual hardware looks like.
> 
> I am not going to respond more to this patchset (which is a clear NAK just in
> case).
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Rob Herring]

On Mon, Aug 28, 2023 at 2:49 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Sent: Monday, August 28, 2023 2:31 PM
> > To: Shenwei Wang <shenwei.wang@nxp.com>; Ulf Hansson
> > <ulf.hansson@linaro.org>
> > Cc: Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> > Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml
> > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin.
> >
> > We do not talk about drivers but bindings and DTS. Why do you bring again
> > drivers, all the time?
> >
> > > You claimed this
> > > as a hardware chip.
> >
> > ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip.
> > If it is not, then drop it from your DTS. I insist. Srsly, third time I insist.
> >
> >
> > >
> > > The regulator-pd driver also uses the same GPIO pin.
> >
> > Again, what is with the drivers? Can you stop bringing it to the discussion?
> >
>
> I have to admit you have a real talent for debate.

It takes 2...

You've gotten feedback from multiple people that your proposal is not
going to be accepted. The prior attempt of the same thing had similar
feedback from even more people. Please go re-read the responses until
you understand.

For fixed-regulator, I can tell you very easily what the h/w looks like:

Vfix---|gate|---Vfix-gated
            |
GPIO--------|

'gate' here may be a chip or discrete transistor. That's a very common
board level component.

If you want to discuss this any further, describe the h/w in terms of
simplified schematics. Otherwise, there is nothing more to discuss.

Rob

Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file

[Shenwei Wang]

> -----Original Message-----
> From: Rob Herring <robh+dt@kernel.org>
> Sent: Monday, August 28, 2023 4:14 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Ulf Hansson
> <ulf.hansson@linaro.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> imx@lists.linux.dev; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] dt-bindings: power: Add regulator-pd yaml file
> > > > The fixed-regulator is a virtual regulator driver that uses the GPIO pin.
> > >
> > > We do not talk about drivers but bindings and DTS. Why do you bring
> > > again drivers, all the time?
> > >
> > > > You claimed this
> > > > as a hardware chip.
> > >
> > > ??? Sorry, this is getting boring. The DTS-snippet is a hardware chip.
> > > If it is not, then drop it from your DTS. I insist. Srsly, third time I insist.
> > >
> > >
> > > >
> > > > The regulator-pd driver also uses the same GPIO pin.
> > >
> > > Again, what is with the drivers? Can you stop bringing it to the discussion?
> > >
> >
> > I have to admit you have a real talent for debate.
> 
> It takes 2...
> 
> You've gotten feedback from multiple people that your proposal is not going to
> be accepted. The prior attempt of the same thing had similar feedback from
> even more people. Please go re-read the responses until you understand.
> 
> For fixed-regulator, I can tell you very easily what the h/w looks like:
> 
> Vfix---|gate|---Vfix-gated
>             |
> GPIO--------|
> 
> 'gate' here may be a chip or discrete transistor. That's a very common board
> level component.
> 

The difference is in how we model the hardware. In your example, you model the GPIO 
as a simple switch to fit the fixed regulator use case. However, we could also model the 
same GPIO as a power domain if we consider the device connected to it. 
This allows for more nuanced hardware modeling based on the context and components 
involved.

Regulator-1 -+-> [Device A] 

This give you one regulator(via GPIO Pin) and one power domain (Device A).

The following are the example diagram given by the power domain overview doc:

                     Regulator-1 -+-> Regulator-2 -+-> [Consumer A]
                                  |
                                  +-> [Consumer B]

                   This gives us two regulators and two power domains:

                   - Domain 1: Regulator-2, Consumer B.
                   - Domain 2: Consumer A.

Thanks,
Shenwei

> If you want to discuss this any further, describe the h/w in terms of simplified
> schematics. Otherwise, there is nothing more to discuss.
> 
> Rob