[PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[Satya Priya]

Satya Priya (6):
  dt-bindings: regulator: Add pm8008 regulator bindings
  dt-bindings: mfd: pm8008: Add regulators node
  mfd: pm8008: Add mfd cell struct to register LDOs
  regulator: Add a regulator driver for the PM8008 PMIC
  arm64: dts: qcom: pm8008: Add base dts file
  arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

 .../devicetree/bindings/mfd/qcom,pm8008.yaml       |  49 ++++-
 .../bindings/regulator/qcom,pm8008-regulator.yaml  |  31 +++
 arch/arm64/boot/dts/qcom/pm8008.dtsi               |  46 ++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |  66 ++++++
 drivers/mfd/qcom-pm8008.c                          |  59 +++++-
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-pm8008-regulator.c          | 234 +++++++++++++++++++++
 8 files changed, 485 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

-- 
2.7.4

[PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Satya Priya]

Add bindings for pm8008 pmic regulators.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
 - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
   resolve dtschema errors. Removed regulator-min-microvolt and 
   regulator-max-microvolt properties.

Changes in V3:
 - As per Rob's comments added standard unit suffix for mindropout property,
   added blank lines where required and added description for reg property.

Changes in V4:
 - Changed compatible string to "com,pm8008-regulators"
 - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
   separate patch.

Changes in V5:
 - Removed the separate compatible for pm8008 regulator driver.
 - Moved the supply nodes to chip level.
 - Removed min-dropout property.

 .../bindings/regulator/qcom,pm8008-regulator.yaml  | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
new file mode 100644
index 0000000..0098845
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
+
+maintainers:
+  - Satya Priya <skakit@codeaurora.org>
+
+description:
+  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
+  containing 7 LDO regulators.
+
+patternProperties:
+  "^LDO[1-7]$":
+    type: object
+    $ref: "regulator.yaml#"
+    description: PM8008 regulator peripherals of PM8008 regulator device
+
+    properties:
+      regulator-name: true
+
+    required:
+      - regulator-name
+
+    unevaluatedProperties: false
+
+additionalProperties: false
+...
-- 
2.7.4

Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Rob Herring]

On Tue, 08 Feb 2022 20:22:15 +0530, Satya Priya wrote:
> Add bindings for pm8008 pmic regulators.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V2:
>  - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
>    resolve dtschema errors. Removed regulator-min-microvolt and
>    regulator-max-microvolt properties.
> 
> Changes in V3:
>  - As per Rob's comments added standard unit suffix for mindropout property,
>    added blank lines where required and added description for reg property.
> 
> Changes in V4:
>  - Changed compatible string to "com,pm8008-regulators"
>  - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
>    separate patch.
> 
> Changes in V5:
>  - Removed the separate compatible for pm8008 regulator driver.
>  - Moved the supply nodes to chip level.
>  - Removed min-dropout property.
> 
>  .../bindings/regulator/qcom,pm8008-regulator.yaml  | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Stephen Boyd]

Quoting Satya Priya (2022-02-08 06:52:15)
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> new file mode 100644
> index 0000000..0098845
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
> +
> +maintainers:
> +  - Satya Priya <skakit@codeaurora.org>
> +
> +description:
> +  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
> +  containing 7 LDO regulators.
> +
> +patternProperties:
> +  "^LDO[1-7]$":

Any reason it needs to be capitalized vs. lowercase ldo?

> +    type: object
> +    $ref: "regulator.yaml#"
> +    description: PM8008 regulator peripherals of PM8008 regulator device
> +
> +    properties:
> +      regulator-name: true
> +
> +    required:
> +      - regulator-name

Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Satya Priya Kakitapalli (Temp)]

On 2/10/2022 6:54 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:15)
>> diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> new file mode 100644
>> index 0000000..0098845
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> @@ -0,0 +1,31 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
>> +
>> +maintainers:
>> +  - Satya Priya <skakit@codeaurora.org>
>> +
>> +description:
>> +  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
>> +  containing 7 LDO regulators.
>> +
>> +patternProperties:
>> +  "^LDO[1-7]$":
> Any reason it needs to be capitalized vs. lowercase ldo?


I'll change this to lowercase.


>> +    type: object
>> +    $ref: "regulator.yaml#"
>> +    description: PM8008 regulator peripherals of PM8008 regulator device
>> +
>> +    properties:
>> +      regulator-name: true
>> +
>> +    required:
>> +      - regulator-name

Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Bjorn Andersson]

On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:

> Add bindings for pm8008 pmic regulators.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V2:
>  - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
>    resolve dtschema errors. Removed regulator-min-microvolt and 
>    regulator-max-microvolt properties.
> 
> Changes in V3:
>  - As per Rob's comments added standard unit suffix for mindropout property,
>    added blank lines where required and added description for reg property.
> 
> Changes in V4:
>  - Changed compatible string to "com,pm8008-regulators"
>  - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
>    separate patch.
> 
> Changes in V5:
>  - Removed the separate compatible for pm8008 regulator driver.
>  - Moved the supply nodes to chip level.
>  - Removed min-dropout property.
> 
>  .../bindings/regulator/qcom,pm8008-regulator.yaml  | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> new file mode 100644
> index 0000000..0098845
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
> +
> +maintainers:
> +  - Satya Priya <skakit@codeaurora.org>
> +
> +description:
> +  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
> +  containing 7 LDO regulators.
> +
> +patternProperties:
> +  "^LDO[1-7]$":

Please make this lower case, to match all other regulator bindings.

> +    type: object
> +    $ref: "regulator.yaml#"
> +    description: PM8008 regulator peripherals of PM8008 regulator device
> +
> +    properties:
> +      regulator-name: true
> +
> +    required:
> +      - regulator-name

Why is regulator-name a (and the only) required property?

Regards,
Bjorn

> +
> +    unevaluatedProperties: false
> +
> +additionalProperties: false
> +...
> -- 
> 2.7.4
> 

Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings

[Satya Priya Kakitapalli (Temp)]

On 2/11/2022 6:13 AM, Bjorn Andersson wrote:
> On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
>
>> Add bindings for pm8008 pmic regulators.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>> Changes in V2:
>>   - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
>>     resolve dtschema errors. Removed regulator-min-microvolt and
>>     regulator-max-microvolt properties.
>>
>> Changes in V3:
>>   - As per Rob's comments added standard unit suffix for mindropout property,
>>     added blank lines where required and added description for reg property.
>>
>> Changes in V4:
>>   - Changed compatible string to "com,pm8008-regulators"
>>   - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
>>     separate patch.
>>
>> Changes in V5:
>>   - Removed the separate compatible for pm8008 regulator driver.
>>   - Moved the supply nodes to chip level.
>>   - Removed min-dropout property.
>>
>>   .../bindings/regulator/qcom,pm8008-regulator.yaml  | 31 ++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> new file mode 100644
>> index 0000000..0098845
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> @@ -0,0 +1,31 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
>> +
>> +maintainers:
>> +  - Satya Priya <skakit@codeaurora.org>
>> +
>> +description:
>> +  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
>> +  containing 7 LDO regulators.
>> +
>> +patternProperties:
>> +  "^LDO[1-7]$":
> Please make this lower case, to match all other regulator bindings.


Okay.


>> +    type: object
>> +    $ref: "regulator.yaml#"
>> +    description: PM8008 regulator peripherals of PM8008 regulator device
>> +
>> +    properties:
>> +      regulator-name: true
>> +
>> +    required:
>> +      - regulator-name
> Why is regulator-name a (and the only) required property?


It is not a required property, I'll correct this.


> Regards,
> Bjorn
>
>> +
>> +    unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +...
>> -- 
>> 2.7.4
>>

[PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node

[Satya Priya]

Add regulators node and their supply nodes. Add separate compatible
"qcom,pm8008-regulators" to differentiate between pm8008 infra
and pm8008 regulators mfd devices.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V2:
 - As per Rob's comments changed "pm8008[a-z]?-regulator" to
   "^pm8008[a-z]?-regulators".

Changes in V3:
 - Fixed bot errors.
 - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to
   "regulators".

Changes in V4:
 - Changed compatible string to "qcom,pm8008-regulators"

Changes in V5:
 - Remove compatible for regulators node.
 - Move supply nodes of the regulators to chip level.

 .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 49 +++++++++++++++++++---
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..fbe84e4 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -16,7 +16,9 @@ description: |
 
 properties:
   compatible:
-    const: qcom,pm8008
+    enum:
+      - qcom,pm8008
+      - qcom,pm8008-regulators
 
   reg:
     description:
@@ -44,6 +46,25 @@ properties:
   "#size-cells":
     const: 0
 
+  vdd_l1_l2-supply:
+    description: Input supply phandle of ldo1 and ldo2 regulators.
+
+  vdd_l3_l4-supply:
+    description: Input supply phandle of ldo3 and ldo4 regulators.
+
+  vdd_l5-supply:
+    description: Input supply phandle of ldo5 regulator.
+
+  vdd_l6-supply:
+    description: Input supply phandle of ldo6 regulator.
+
+  vdd_l7-supply:
+    description: Input supply phandle of ldo7 regulator.
+
+  regulators:
+    type: object
+    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
+
 patternProperties:
   "^gpio@[0-9a-f]+$":
     type: object
@@ -88,10 +109,8 @@ patternProperties:
 required:
   - compatible
   - reg
-  - interrupts
   - "#address-cells"
   - "#size-cells"
-  - "#interrupt-cells"
 
 additionalProperties: false
 
@@ -102,7 +121,7 @@ examples:
     qupv3_se13_i2c {
       #address-cells = <1>;
       #size-cells = <0>;
-      pm8008i@8 {
+      pm8008_infra: pm8008@8 {
         compatible = "qcom,pm8008";
         reg = <0x8>;
         #address-cells = <1>;
@@ -123,6 +142,26 @@ examples:
           #interrupt-cells = <2>;
         };
       };
-    };
 
+      pm8008_regulators: pm8008@9 {
+        compatible = "qcom,pm8008";
+        reg = <0x9>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+        vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+        vdd_l5-supply = <&vreg_bob>;
+        vdd_l6-supply = <&vreg_bob>;
+        vdd_l7-supply = <&vreg_bob>;
+
+        regulators {
+          pm8008_l1: LDO1 {
+            regulator-name = "pm8008_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+          };
+        };
+      };
+    };
 ...
-- 
2.7.4

Re: [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node

[Rob Herring]

On Tue, 08 Feb 2022 20:22:16 +0530, Satya Priya wrote:
> Add regulators node and their supply nodes. Add separate compatible
> "qcom,pm8008-regulators" to differentiate between pm8008 infra
> and pm8008 regulators mfd devices.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes in V2:
>  - As per Rob's comments changed "pm8008[a-z]?-regulator" to
>    "^pm8008[a-z]?-regulators".
> 
> Changes in V3:
>  - Fixed bot errors.
>  - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to
>    "regulators".
> 
> Changes in V4:
>  - Changed compatible string to "qcom,pm8008-regulators"
> 
> Changes in V5:
>  - Remove compatible for regulators node.
>  - Move supply nodes of the regulators to chip level.
> 
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 49 +++++++++++++++++++---
>  1 file changed, 44 insertions(+), 5 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs

[Satya Priya]

Register mfd cell ID and name for each of the 7 pm8008
LDOs to probe them through mfd driver without needing
a separate compatible for regulator driver.

Also, add a different compatible for the mfd node that
contains regulators to make sure that the LDOs are
registered with the correct mfd device.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V5:
 - Changes newly added from V5.

 drivers/mfd/qcom-pm8008.c | 59 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f..e8569cc 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
@@ -27,6 +28,37 @@
 #define INT_EN_CLR_OFFSET		0x16
 #define INT_LATCHED_STS_OFFSET		0x18
 
+static const struct mfd_cell pm8008_regulator_devs[] = {
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 0,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 1,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 2,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 3,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 4,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 5,
+	},
+	{
+		.name = "qcom,pm8008-regulators",
+		.id = 6,
+	},
+};
+
 enum {
 	PM8008_MISC,
 	PM8008_TEMP_ALARM,
@@ -35,6 +67,17 @@ enum {
 	PM8008_NUM_PERIPHS,
 };
 
+enum {
+	PM8008_INFRA,
+	PM8008_REGULATORS,
+};
+
+static const struct of_device_id pm8008_match[] = {
+	{ .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
+	{ .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
+	{ },
+};
+
 #define PM8008_PERIPH_0_BASE	0x900
 #define PM8008_PERIPH_1_BASE	0x2400
 #define PM8008_PERIPH_2_BASE	0xC000
@@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
 {
 	int rc;
 	struct pm8008_data *chip;
+	const struct of_device_id *id;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
 			dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
 	}
 
+	id = of_match_node(pm8008_match, chip->dev->of_node);
+	if (id->data == (void *)PM8008_REGULATORS) {
+		rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,
+				ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
+		if (rc) {
+			dev_err(chip->dev, "Failed to add children: %d\n", rc);
+			return rc;
+		}
+	}
+
 	return devm_of_platform_populate(chip->dev);
 }
 
-static const struct of_device_id pm8008_match[] = {
-	{ .compatible = "qcom,pm8008", },
-	{ },
-};
-
 static struct i2c_driver pm8008_mfd_driver = {
 	.driver = {
 		.name = "pm8008",
-- 
2.7.4

Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs

[Stephen Boyd]

Quoting Satya Priya (2022-02-08 06:52:17)
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index c472d7f..e8569cc 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -8,6 +8,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of_platform.h>
> @@ -27,6 +28,37 @@
>  #define INT_EN_CLR_OFFSET              0x16
>  #define INT_LATCHED_STS_OFFSET         0x18
>
> +static const struct mfd_cell pm8008_regulator_devs[] = {

Is there some way to not allocate this structure statically forever?

> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 0,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 1,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 2,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 3,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 4,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 5,
> +       },
> +       {
> +               .name = "qcom,pm8008-regulators",
> +               .id = 6,
> +       },
> +};
> +
>  enum {
>         PM8008_MISC,
>         pm8008_temp_alarm,
> @@ -35,6 +67,17 @@ enum {
>         PM8008_NUM_PERIPHS,
>  };
>
> +enum {
> +       PM8008_INFRA,
> +       PM8008_REGULATORS,
> +};
> +
> +static const struct of_device_id pm8008_match[] = {
> +       { .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
> +       { .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
> +       { },

Nitpick: Drop , on {} so nothing can come after without causing compile
error.

> +};
> +
>  #define PM8008_PERIPH_0_BASE   0x900
>  #define PM8008_PERIPH_1_BASE   0x2400
>  #define PM8008_PERIPH_2_BASE   0xC000
> @@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
>  {
>         int rc;
>         struct pm8008_data *chip;
> +       const struct of_device_id *id;
>
>         chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>         if (!chip)
> @@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
>                         dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>         }
>
> +       id = of_match_node(pm8008_match, chip->dev->of_node);

Use device_get_match_data()? And then use (uintptr_t) casts to check for
the enum? Using device_get_match_data() allows us to avoid moving the
pm8008_match table.

> +       if (id->data == (void *)PM8008_REGULATORS) {

	enum <your_name_here> dev_type = device_get_match_data(dev);

	if (dev_type == PM8008_REGULATORS)

> +               rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,

Why not devm_mfd_add_devices()?

> +                               ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
> +               if (rc) {
> +                       dev_err(chip->dev, "Failed to add children: %d\n", rc);
> +                       return rc;
> +               }
> +       }
> +
>         return devm_of_platform_populate(chip->dev);
>  }
>
> -static const struct of_device_id pm8008_match[] = {
> -       { .compatible = "qcom,pm8008", },
> -       { },
> -};

This should have a MODULE_DEVICE_TABLE(of, pm8008_match) here.

> -
>  static struct i2c_driver pm8008_mfd_driver = {
>         .driver = {
>                 .name = "pm8008",

Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs

[Satya Priya Kakitapalli (Temp)]

On 2/10/2022 7:02 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:17)
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index c472d7f..e8569cc 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/interrupt.h>
>>   #include <linux/irq.h>
>>   #include <linux/irqdomain.h>
>> +#include <linux/mfd/core.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>>   #include <linux/of_platform.h>
>> @@ -27,6 +28,37 @@
>>   #define INT_EN_CLR_OFFSET              0x16
>>   #define INT_LATCHED_STS_OFFSET         0x18
>>
>> +static const struct mfd_cell pm8008_regulator_devs[] = {
> Is there some way to not allocate this structure statically forever?


I think No.

I found that some of the drivers are just using one cell with .name to 
match with regulator driver and then probing regulators using a loop. 
I'll do that too.

static const struct mfd_cell pm8008_regulator_devs[] = {
         {
                 .name = "qcom,pm8008-regulators",
         },
  };

>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 0,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 1,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 2,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 3,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 4,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 5,
>> +       },
>> +       {
>> +               .name = "qcom,pm8008-regulators",
>> +               .id = 6,
>> +       },
>> +};
>> +
>>   enum {
>>          PM8008_MISC,
>>          pm8008_temp_alarm,
>> @@ -35,6 +67,17 @@ enum {
>>          PM8008_NUM_PERIPHS,
>>   };
>>
>> +enum {
>> +       PM8008_INFRA,
>> +       PM8008_REGULATORS,
>> +};
>> +
>> +static const struct of_device_id pm8008_match[] = {
>> +       { .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
>> +       { .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
>> +       { },
> Nitpick: Drop , on {} so nothing can come after without causing compile
> error.


Okay.


>> +};
>> +
>>   #define PM8008_PERIPH_0_BASE   0x900
>>   #define PM8008_PERIPH_1_BASE   0x2400
>>   #define PM8008_PERIPH_2_BASE   0xC000
>> @@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
>>   {
>>          int rc;
>>          struct pm8008_data *chip;
>> +       const struct of_device_id *id;
>>
>>          chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>          if (!chip)
>> @@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
>>                          dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>>          }
>>
>> +       id = of_match_node(pm8008_match, chip->dev->of_node);
> Use device_get_match_data()? And then use (uintptr_t) casts to check for
> the enum? Using device_get_match_data() allows us to avoid moving the
> pm8008_match table.


Okay.


>> +       if (id->data == (void *)PM8008_REGULATORS) {
> 	enum <your_name_here> dev_type = device_get_match_data(dev);
>
> 	if (dev_type == PM8008_REGULATORS)
>
>> +               rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,
> Why not devm_mfd_add_devices()?


Okay.


>> +                               ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
>> +               if (rc) {
>> +                       dev_err(chip->dev, "Failed to add children: %d\n", rc);
>> +                       return rc;
>> +               }
>> +       }
>> +
>>          return devm_of_platform_populate(chip->dev);
>>   }
>>
>> -static const struct of_device_id pm8008_match[] = {
>> -       { .compatible = "qcom,pm8008", },
>> -       { },
>> -};
> This should have a MODULE_DEVICE_TABLE(of, pm8008_match) here.


Okay.


>> -
>>   static struct i2c_driver pm8008_mfd_driver = {
>>          .driver = {
>>                  .name = "pm8008",

Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs

[Lee Jones]

On Fri, 11 Feb 2022, Satya Priya Kakitapalli (Temp) wrote:

> 
> On 2/10/2022 7:02 AM, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-02-08 06:52:17)
> > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > index c472d7f..e8569cc 100644
> > > --- a/drivers/mfd/qcom-pm8008.c
> > > +++ b/drivers/mfd/qcom-pm8008.c
> > > @@ -8,6 +8,7 @@
> > >   #include <linux/interrupt.h>
> > >   #include <linux/irq.h>
> > >   #include <linux/irqdomain.h>
> > > +#include <linux/mfd/core.h>
> > >   #include <linux/module.h>
> > >   #include <linux/of_device.h>
> > >   #include <linux/of_platform.h>
> > > @@ -27,6 +28,37 @@
> > >   #define INT_EN_CLR_OFFSET              0x16
> > >   #define INT_LATCHED_STS_OFFSET         0x18
> > > 
> > > +static const struct mfd_cell pm8008_regulator_devs[] = {
> > Is there some way to not allocate this structure statically forever?
> 
> 
> I think No.
> 
> I found that some of the drivers are just using one cell with .name to match
> with regulator driver and then probing regulators using a loop. I'll do that
> too.
> 
> static const struct mfd_cell pm8008_regulator_devs[] = {
>         {
>                 .name = "qcom,pm8008-regulators",
>         },
>  };

Please use MFD_CELL_NAME() for these.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs

[Satya Priya Kakitapalli (Temp)]

Hi,


On 2/11/2022 7:04 PM, Lee Jones wrote:
> On Fri, 11 Feb 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 2/10/2022 7:02 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-02-08 06:52:17)
>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>> index c472d7f..e8569cc 100644
>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>> @@ -8,6 +8,7 @@
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/irq.h>
>>>>    #include <linux/irqdomain.h>
>>>> +#include <linux/mfd/core.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/of_platform.h>
>>>> @@ -27,6 +28,37 @@
>>>>    #define INT_EN_CLR_OFFSET              0x16
>>>>    #define INT_LATCHED_STS_OFFSET         0x18
>>>>
>>>> +static const struct mfd_cell pm8008_regulator_devs[] = {
>>> Is there some way to not allocate this structure statically forever?
>>
>> I think No.
>>
>> I found that some of the drivers are just using one cell with .name to match
>> with regulator driver and then probing regulators using a loop. I'll do that
>> too.
>>
>> static const struct mfd_cell pm8008_regulator_devs[] = {
>>          {
>>                  .name = "qcom,pm8008-regulators",
>>          },
>>   };
> Please use MFD_CELL_NAME() for these.


Okay.

[PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Satya Priya]

Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
containing 7 LDO regulators.  Add a PM8008 regulator driver to
support PMIC regulator management via the regulator framework.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
 As per Mark's comments
 - Using regmap helpers for regulator enable/disable and is_enabled APIs
 - Changed pr_err to dev_err wherever possible.
 - Removed init_voltage property as it is not used.
 - Removed if check for registering LDOs
 - Other minor changes.

Changes in V3:
 As per Stephen's comments, 
 - Removed unused includes
 - Removed PM8008_MAX_LDO macro.
 - Removed pm8008_read/write APIs, using regmap_bulk_read/write APIs
 - Using le16_to_cpu/cpu_to_le16 APIs in pm8008_regulator_get/set_voltage
 - Consolidated all probe related functions into single probe function.
 - Added of_parse_cb call back and removed regulator-name matching loop.
 - Fixed other minor nits.

Changes in V4:
 - Removed unused members like rdev and of_node from pm8008_regulator struct.
 - Replaced set_voltage with set_voltage_sel
 - Removed init_data configuration as it is not needed.
 - Removed few other unused assignments from probe

Changes in V5:
 - Removed Compatible string.
 - Changed the probe function accordingly to probe LDOs using mfd driver.
 - Added max headrooms for LDOs and removed the part reading min-dropout from DT.
 - Added base reg values in the regulator_data struct instead of reading it from DT.

 drivers/regulator/Kconfig                 |   9 ++
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-pm8008-regulator.c | 234 ++++++++++++++++++++++++++++++
 3 files changed, 244 insertions(+)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 22503e4..4372ad7 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -925,6 +925,15 @@ config REGULATOR_PWM
 	  This driver supports PWM controlled voltage regulators. PWM
 	  duty cycle can increase or decrease the voltage.
 
+config REGULATOR_QCOM_PM8008
+	tristate "Qualcomm Technologies, Inc. PM8008 PMIC regulators"
+	depends on MFD_QCOM_PM8008
+	help
+	  Select this option to get support for the voltage regulators
+	  of Qualcomm Technologies, Inc. PM8008 PMIC chip. PM8008 has 7 LDO
+	  regulators. This driver provides support for basic operations like
+	  set/get voltage and enable/disable.
+
 config REGULATOR_QCOM_RPM
 	tristate "Qualcomm RPM regulator driver"
 	depends on MFD_QCOM_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 2e1b087..9ab3ad7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
 obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
new file mode 100644
index 0000000..86043b4
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define STARTUP_DELAY_USEC		20
+#define VSET_STEP_MV			8
+#define VSET_STEP_UV			(VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base)		((base) + 0x46)
+#define ENABLE_BIT			BIT(7)
+
+#define LDO_STATUS1_REG(base)		((base) + 0x08)
+#define VREG_READY_BIT			BIT(7)
+
+#define LDO_VSET_LB_REG(base)		((base) + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base)	((base) + 0x3b)
+#define DEFAULT_VOLTAGE_STEPPER_RATE	38400
+#define STEP_RATE_MASK			GENMASK(1, 0)
+
+#define PM8008_NUM_LDOS			7
+
+struct regulator_data {
+	const char			*name;
+	const char			*supply_name;
+	u16				base;
+	int				min_uv;
+	int				max_uv;
+	int				min_dropout_uv;
+	const struct linear_range	*voltage_range;
+};
+
+struct pm8008_regulator {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct regulator_desc	rdesc;
+	u16			base;
+	int			step_rate;
+};
+
+static const struct linear_range nldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
+};
+
+static const struct linear_range pldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
+};
+
+static const struct regulator_data reg_data[] = {
+	/* name  parent       base   min_uv  max_uv  headroom_uv voltage_range */
+	{ "LDO1", "vdd_l1_l2", 0x4000,  528000, 1504000, 225000, nldo_ranges, },
+	{ "LDO2", "vdd_l1_l2", 0x4100,  528000, 1504000, 225000, nldo_ranges, },
+	{ "LDO3", "vdd_l3_l4", 0x4200, 1504000, 3400000, 300000, pldo_ranges, },
+	{ "LDO4", "vdd_l3_l4", 0x4300, 1504000, 3400000, 300000, pldo_ranges, },
+	{ "LDO5", "vdd_l5",    0x4400, 1504000, 3400000, 200000, pldo_ranges, },
+	{ "LDO6", "vdd_l6",    0x4500, 1504000, 3400000, 200000, pldo_ranges, },
+	{ "LDO7", "vdd_l7",    0x4600, 1504000, 3400000, 200000, pldo_ranges, },
+};
+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	__le16 mV;
+	int rc;
+
+	rc = regmap_bulk_read(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+	if (rc < 0) {
+		dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return le16_to_cpu(mV) * 1000;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+							int mV)
+{
+	int rc;
+	u16 vset_raw;
+
+	vset_raw = cpu_to_le16(mV);
+
+	rc = regmap_bulk_write(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base),
+			(const void *)&vset_raw, sizeof(vset_raw));
+	if (rc < 0) {
+		dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+				int old_uV, int new_uv)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+	return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+					unsigned int selector)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc, mV;
+
+	/* voltage control register is set with voltage in millivolts */
+	mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
+						1000);
+	if (mV < 0)
+		return mV;
+
+	rc = pm8008_write_voltage(pm8008_reg, mV);
+	if (rc < 0)
+		return rc;
+
+	dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
+	return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.set_voltage_sel	= pm8008_regulator_set_voltage,
+	.get_voltage		= pm8008_regulator_get_voltage,
+	.list_voltage		= regulator_list_voltage_linear,
+	.set_voltage_time	= pm8008_regulator_set_voltage_time,
+};
+
+static int pm8008_regulator_of_parse(struct device_node *node,
+			const struct regulator_desc *desc,
+			struct regulator_config *config)
+{
+	struct pm8008_regulator *pm8008_reg = config->driver_data;
+	int rc;
+	unsigned int reg;
+
+	/* get slew rate */
+	rc = regmap_bulk_read(pm8008_reg->regmap,
+			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
+	if (rc < 0) {
+		dev_err(pm8008_reg->dev,
+			"%s: failed to read step rate configuration rc=%d\n",
+			pm8008_reg->rdesc.name, rc);
+		return rc;
+	}
+	reg &= STEP_RATE_MASK;
+	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
+
+	return 0;
+}
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int id = pdev->id % PM8008_NUM_LDOS;
+	struct regulator_dev    *rdev;
+	struct pm8008_regulator *pm8008_reg;
+	struct regmap *regmap;
+	struct regulator_config reg_config = {};
+	int rc;
+
+	dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "parent regmap is missing\n");
+		return -EINVAL;
+	}
+
+	pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+	if (!pm8008_reg)
+		return -ENOMEM;
+
+	pm8008_reg->regmap = regmap;
+	pm8008_reg->dev = dev;
+	pm8008_reg->base = reg_data[id].base;
+
+	pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+	pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
+	pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+	pm8008_reg->rdesc.name = reg_data[id].name;
+	pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
+	pm8008_reg->rdesc.of_match = reg_data[id].name;
+	pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
+	pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
+	pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
+	pm8008_reg->rdesc.n_voltages
+		= ((reg_data[id].max_uv - reg_data[id].min_uv)
+			/ pm8008_reg->rdesc.uV_step) + 1;
+	pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
+	pm8008_reg->rdesc.n_linear_ranges = 1;
+	pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
+	pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+	pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
+
+	reg_config.dev = dev->parent;
+	reg_config.driver_data = pm8008_reg;
+
+	rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, &reg_config);
+	if (IS_ERR(rdev)) {
+		rc = PTR_ERR(rdev);
+		dev_err(dev, "%s: failed to register regulator rc=%d\n",
+				reg_data[id].name, rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static struct platform_driver pm8008_regulator_driver = {
+	.driver	= {
+		.name		= "qcom,pm8008-regulators",
+	},
+	.probe	= pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]

On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:

> +static int pm8008_regulator_of_parse(struct device_node *node,
> +			const struct regulator_desc *desc,
> +			struct regulator_config *config)
> +{
> +	struct pm8008_regulator *pm8008_reg = config->driver_data;
> +	int rc;
> +	unsigned int reg;
> +
> +	/* get slew rate */
> +	rc = regmap_bulk_read(pm8008_reg->regmap,
> +			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
> +	if (rc < 0) {
> +		dev_err(pm8008_reg->dev,
> +			"%s: failed to read step rate configuration rc=%d\n",
> +			pm8008_reg->rdesc.name, rc);
> +		return rc;
> +	}
> +	reg &= STEP_RATE_MASK;
> +	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> +	return 0;

This is not doing any parsing of any DT properties at all, it is just
reading a default value back from the hardware.  This shouldn't be in
the of_parse() callback, it should be done on probe() or something so
that if someone adds ACPI support or whatever there's no surprise
breakage, and so that we've got this configured even if there's no DT
bindings for the specific regulator.

> +}
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int id = pdev->id % PM8008_NUM_LDOS;
> +	struct regulator_dev    *rdev;
> +	struct pm8008_regulator *pm8008_reg;
> +	struct regmap *regmap;
> +	struct regulator_config reg_config = {};
> +	int rc;
> +
> +	dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "parent regmap is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +	if (!pm8008_reg)
> +		return -ENOMEM;
> +
> +	pm8008_reg->regmap = regmap;
> +	pm8008_reg->dev = dev;
> +	pm8008_reg->base = reg_data[id].base;
> +
> +	pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
> +	pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
> +	pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
> +	pm8008_reg->rdesc.name = reg_data[id].name;
> +	pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
> +	pm8008_reg->rdesc.of_match = reg_data[id].name;
> +	pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
> +	pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
> +	pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
> +	pm8008_reg->rdesc.n_voltages
> +		= ((reg_data[id].max_uv - reg_data[id].min_uv)
> +			/ pm8008_reg->rdesc.uV_step) + 1;
> +	pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
> +	pm8008_reg->rdesc.n_linear_ranges = 1;
> +	pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> +	pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
> +	pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
> +
> +	reg_config.dev = dev->parent;
> +	reg_config.driver_data = pm8008_reg;
> +
> +	rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, &reg_config);
> +	if (IS_ERR(rdev)) {
> +		rc = PTR_ERR(rdev);
> +		dev_err(dev, "%s: failed to register regulator rc=%d\n",
> +				reg_data[id].name, rc);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver pm8008_regulator_driver = {
> +	.driver	= {
> +		.name		= "qcom,pm8008-regulators",
> +	},
> +	.probe	= pm8008_regulator_probe,
> +};
> +
> +module_platform_driver(pm8008_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Satya Priya Kakitapalli (Temp)]

On 2/9/2022 7:48 PM, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:
>
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> +			const struct regulator_desc *desc,
>> +			struct regulator_config *config)
>> +{
>> +	struct pm8008_regulator *pm8008_reg = config->driver_data;
>> +	int rc;
>> +	unsigned int reg;
>> +
>> +	/* get slew rate */
>> +	rc = regmap_bulk_read(pm8008_reg->regmap,
>> +			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
>> +	if (rc < 0) {
>> +		dev_err(pm8008_reg->dev,
>> +			"%s: failed to read step rate configuration rc=%d\n",
>> +			pm8008_reg->rdesc.name, rc);
>> +		return rc;
>> +	}
>> +	reg &= STEP_RATE_MASK;
>> +	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> +	return 0;
> This is not doing any parsing of any DT properties at all, it is just
> reading a default value back from the hardware.  This shouldn't be in
> the of_parse() callback, it should be done on probe() or something so
> that if someone adds ACPI support or whatever there's no surprise
> breakage, and so that we've got this configured even if there's no DT
> bindings for the specific regulator.


Okay, I'll move it to probe.


>
>> +}
>> +
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int id = pdev->id % PM8008_NUM_LDOS;
>> +	struct regulator_dev    *rdev;
>> +	struct pm8008_regulator *pm8008_reg;
>> +	struct regmap *regmap;
>> +	struct regulator_config reg_config = {};
>> +	int rc;
>> +
>> +	dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
>> +
>> +	regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!regmap) {
>> +		dev_err(dev, "parent regmap is missing\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
>> +	if (!pm8008_reg)
>> +		return -ENOMEM;
>> +
>> +	pm8008_reg->regmap = regmap;
>> +	pm8008_reg->dev = dev;
>> +	pm8008_reg->base = reg_data[id].base;
>> +
>> +	pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> +	pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
>> +	pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> +	pm8008_reg->rdesc.name = reg_data[id].name;
>> +	pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
>> +	pm8008_reg->rdesc.of_match = reg_data[id].name;
>> +	pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
>> +	pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
>> +	pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
>> +	pm8008_reg->rdesc.n_voltages
>> +		= ((reg_data[id].max_uv - reg_data[id].min_uv)
>> +			/ pm8008_reg->rdesc.uV_step) + 1;
>> +	pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
>> +	pm8008_reg->rdesc.n_linear_ranges = 1;
>> +	pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
>> +	pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
>> +	pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
>> +
>> +	reg_config.dev = dev->parent;
>> +	reg_config.driver_data = pm8008_reg;
>> +
>> +	rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, &reg_config);
>> +	if (IS_ERR(rdev)) {
>> +		rc = PTR_ERR(rdev);
>> +		dev_err(dev, "%s: failed to register regulator rc=%d\n",
>> +				reg_data[id].name, rc);
>> +		return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver pm8008_regulator_driver = {
>> +	.driver	= {
>> +		.name		= "qcom,pm8008-regulators",
>> +	},
>> +	.probe	= pm8008_regulator_probe,
>> +};
>> +
>> +module_platform_driver(pm8008_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.7.4
>>

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Stephen Boyd]

Quoting Satya Priya (2022-02-08 06:52:18)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..86043b4
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +
> +#define STARTUP_DELAY_USEC             20
> +#define VSET_STEP_MV                   8
> +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
> +#define ENABLE_BIT                     BIT(7)
> +
> +#define LDO_STATUS1_REG(base)          ((base) + 0x08)
> +#define VREG_READY_BIT                 BIT(7)
> +
> +#define LDO_VSET_LB_REG(base)          ((base) + 0x40)
> +
> +#define LDO_STEPPER_CTL_REG(base)      ((base) + 0x3b)
> +#define DEFAULT_VOLTAGE_STEPPER_RATE   38400
> +#define STEP_RATE_MASK                 GENMASK(1, 0)
> +
> +#define PM8008_NUM_LDOS                        7
> +
> +struct regulator_data {
> +       const char                      *name;
> +       const char                      *supply_name;
> +       u16                             base;
> +       int                             min_uv;
> +       int                             max_uv;
> +       int                             min_dropout_uv;
> +       const struct linear_range       *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> +       struct device           *dev;
> +       struct regmap           *regmap;
> +       struct regulator_desc   rdesc;
> +       u16                     base;
> +       int                     step_rate;
> +};
> +
> +static const struct linear_range nldo_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
> +};
> +
> +static const struct linear_range pldo_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
> +};
> +
> +static const struct regulator_data reg_data[] = {
> +       /* name  parent       base   min_uv  max_uv  headroom_uv voltage_range */
> +       { "LDO1", "vdd_l1_l2", 0x4000,  528000, 1504000, 225000, nldo_ranges, },
> +       { "LDO2", "vdd_l1_l2", 0x4100,  528000, 1504000, 225000, nldo_ranges, },
> +       { "LDO3", "vdd_l3_l4", 0x4200, 1504000, 3400000, 300000, pldo_ranges, },
> +       { "LDO4", "vdd_l3_l4", 0x4300, 1504000, 3400000, 300000, pldo_ranges, },
> +       { "LDO5", "vdd_l5",    0x4400, 1504000, 3400000, 200000, pldo_ranges, },
> +       { "LDO6", "vdd_l6",    0x4500, 1504000, 3400000, 200000, pldo_ranges, },
> +       { "LDO7", "vdd_l7",    0x4600, 1504000, 3400000, 200000, pldo_ranges, },
> +};
> +
> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +       __le16 mV;
> +       int rc;
> +
> +       rc = regmap_bulk_read(pm8008_reg->regmap,
> +                       LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> +       if (rc < 0) {
> +               dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
> +               return rc;
> +       }
> +
> +       return le16_to_cpu(mV) * 1000;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> +                                                       int mV)
> +{
> +       int rc;
> +       u16 vset_raw;
> +
> +       vset_raw = cpu_to_le16(mV);

sparse should complain here that an le16 is degrading to a u16. Please
make vset_raw an __le16 as well.

> +
> +       rc = regmap_bulk_write(pm8008_reg->regmap,
> +                       LDO_VSET_LB_REG(pm8008_reg->base),
> +                       (const void *)&vset_raw, sizeof(vset_raw));
> +       if (rc < 0) {
> +               dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);

Do we really need this? It could spam the logs in theory. We have
tracepoints on regmap that could be used to figure out that some
read/write failed. I'd like to see a plain

	return regmap_bulk_write(...)

> +               return rc;
> +       }
> +
> +       return 0;
> +}
> +
> +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
> +                               int old_uV, int new_uv)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> +       return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
> +}
> +
> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> +                                       unsigned int selector)
> +{
> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +       int rc, mV;
> +
> +       /* voltage control register is set with voltage in millivolts */
> +       mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
> +                                               1000);

Aren't there linear range APIs that can avoid any div roundups here?

> +       if (mV < 0)
> +               return mV;
> +
> +       rc = pm8008_write_voltage(pm8008_reg, mV);
> +       if (rc < 0)
> +               return rc;
> +
> +       dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
> +       return 0;
> +}
> +
> +static const struct regulator_ops pm8008_regulator_ops = {
> +       .enable                 = regulator_enable_regmap,
> +       .disable                = regulator_disable_regmap,
> +       .is_enabled             = regulator_is_enabled_regmap,
> +       .set_voltage_sel        = pm8008_regulator_set_voltage,
> +       .get_voltage            = pm8008_regulator_get_voltage,
> +       .list_voltage           = regulator_list_voltage_linear,
> +       .set_voltage_time       = pm8008_regulator_set_voltage_time,
> +};
> +
> +static int pm8008_regulator_of_parse(struct device_node *node,
> +                       const struct regulator_desc *desc,
> +                       struct regulator_config *config)
> +{
> +       struct pm8008_regulator *pm8008_reg = config->driver_data;
> +       int rc;
> +       unsigned int reg;
> +
> +       /* get slew rate */
> +       rc = regmap_bulk_read(pm8008_reg->regmap,
> +                       LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
> +       if (rc < 0) {
> +               dev_err(pm8008_reg->dev,
> +                       "%s: failed to read step rate configuration rc=%d\n",
> +                       pm8008_reg->rdesc.name, rc);
> +               return rc;
> +       }
> +       reg &= STEP_RATE_MASK;
> +       pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> +       return 0;
> +}
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       int id = pdev->id % PM8008_NUM_LDOS;
> +       struct regulator_dev    *rdev;
> +       struct pm8008_regulator *pm8008_reg;
> +       struct regmap *regmap;
> +       struct regulator_config reg_config = {};
> +       int rc;
> +
> +       dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);

Why can't we probe one regulators (plural) platform device instead of 8
regulator platform devices? A 'struct device' isn't exactly small and
it would be simpler to probe all the regulators in a single loop.

> +
> +       regmap = dev_get_regmap(dev->parent, NULL);
> +       if (!regmap) {
> +               dev_err(dev, "parent regmap is missing\n");

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Bjorn Andersson]

On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
[..]
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int id = pdev->id % PM8008_NUM_LDOS;

Why does this driver look completely different from all the other
Qualcomm regulator drivers that we already have, and why do you register
one platform_device per regulator?

The fundamental difference in design makes it hard to maintain and
you're wasting quite a bit of memory with the unnecessary
platfrom_device objects.

Regards,
Bjorn

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Satya Priya Kakitapalli (Temp)]

On 2/11/2022 6:27 AM, Bjorn Andersson wrote:
> On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> [..]
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int id = pdev->id % PM8008_NUM_LDOS;
> Why does this driver look completely different from all the other
> Qualcomm regulator drivers that we already have, and why do you register
> one platform_device per regulator?


Based on Mark's suggestions and the discussion happened here [1], I've 
changed the design like this.

[1] 
https://patchwork.kernel.org/project/linux-arm-msm/patch/1637314953-4215-3-git-send-email-quic_c_skakit@quicinc.com/


> The fundamental difference in design makes it hard to maintain and


> you're wasting quite a bit of memory with the unnecessary
> platfrom_device objects.


I'm going to change this. I will add only one cell with .name to match 
with the regulator driver and probe all the regulators using a single 
loop. That way we don't waste lot of memory by registering multiple 
regulator platform devices.


> Regards,
> Bjorn

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Matti Vaittinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1771 bytes --]

Hi Satya,

It's always nice to see new PMIC drivers :) I just one question after 
reading your patch - please ignore it if it has already been discussed 
before - for some reason this version was caught by my filters where the 
previous versions didn't. It means I do not know the full history.

On 2/8/22 16:52, Satya Priya wrote:
> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
> containing 7 LDO regulators.  Add a PM8008 regulator driver to
> support PMIC regulator management via the regulator framework.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---

snip

> +
> +static int pm8008_regulator_of_parse(struct device_node *node,
> +			const struct regulator_desc *desc,
> +			struct regulator_config *config)
> +{
> +	struct pm8008_regulator *pm8008_reg = config->driver_data;
> +	int rc;
> +	unsigned int reg;
> +
> +	/* get slew rate */
> +	rc = regmap_bulk_read(pm8008_reg->regmap,
> +			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
> +	if (rc < 0) {
> +		dev_err(pm8008_reg->dev,
> +			"%s: failed to read step rate configuration rc=%d\n",
> +			pm8008_reg->rdesc.name, rc);
> +		return rc;
> +	}
> +	reg &= STEP_RATE_MASK;
> +	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> +	return 0;
> +}

I wonder why this is done in the of_parse_cb? Could this perhaps be done 
directly in probe - I don't think this is actually parsing the 
device_node properties, right?

Overall this looks pretty nice to me.

Best Regards
	-- Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 5335 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC

[Satya Priya Kakitapalli (Temp)]

Hi Matti,


Thanks for reviewing the patches!


On 2/11/2022 4:31 PM, Matti Vaittinen wrote:
> Hi Satya,
>
> It's always nice to see new PMIC drivers :) I just one question after 
> reading your patch - please ignore it if it has already been discussed 
> before - for some reason this version was caught by my filters where 
> the previous versions didn't. It means I do not know the full history.
> On 2/8/22 16:52, Satya Priya wrote:
>> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
>> containing 7 LDO regulators.  Add a PM8008 regulator driver to
>> support PMIC regulator management via the regulator framework.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>
> snip
>
>> +
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> +            const struct regulator_desc *desc,
>> +            struct regulator_config *config)
>> +{
>> +    struct pm8008_regulator *pm8008_reg = config->driver_data;
>> +    int rc;
>> +    unsigned int reg;
>> +
>> +    /* get slew rate */
>> +    rc = regmap_bulk_read(pm8008_reg->regmap,
>> +            LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
>> +    if (rc < 0) {
>> +        dev_err(pm8008_reg->dev,
>> +            "%s: failed to read step rate configuration rc=%d\n",
>> +            pm8008_reg->rdesc.name, rc);
>> +        return rc;
>> +    }
>> +    reg &= STEP_RATE_MASK;
>> +    pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> +    return 0;
>> +}
>
> I wonder why this is done in the of_parse_cb? Could this perhaps be 
> done directly in probe - I don't think this is actually parsing the 
> device_node properties, right?
>

Right, I will move this part to probe. In the previous version there was 
some code here which did the DT parsing, now that I removed all that, I 
should move this to probe.


Thanks,

Satya Priya


> Overall this looks pretty nice to me.
>
> Best Regards
>     -- Matti
>

[PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file

[Satya Priya]

Add base DTS file for pm8008 with infra and regulator nodes.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V4:
 - This is newly added in V4, to add all the pm8008 common stuff.

Changes in V5:
 - Changed the mfd node names from pm8008_chip to pm8008_infra and
   pm8008_ldo to pm8008_regulators as they re more appropriate.
 - Changed the compatible for pm8008@9 mfd node to differentiate from
   pm8008@8 node in driver.
 - Removed compatible for regulators node.
 - Removed reg property for LDOs and added in driver.

 arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi

diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
new file mode 100644
index 0000000..8e04983
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2021, The Linux Foundation. All rights reserved.
+
+pm8008_infra: pm8008@8 {
+	compatible = "qcom,pm8008";
+	reg = <0x8>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+};
+
+pm8008_regulators: pm8008@9 {
+	compatible = "qcom,pm8008-regulators";
+	reg = <0x9>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	regulators {
+		pm8008_l1: LDO1 {
+			regulator-name = "pm8008_l1";
+		};
+
+		pm8008_l2: LDO2 {
+			regulator-name = "pm8008_l2";
+		};
+
+		pm8008_l3: LDO3 {
+			regulator-name = "pm8008_l3";
+		};
+
+		pm8008_l4: LDO4 {
+			regulator-name = "pm8008_l4";
+		};
+
+		pm8008_l5: LDO5 {
+			regulator-name = "pm8008_l5";
+		};
+
+		pm8008_l6: LDO6 {
+			regulator-name = "pm8008_l6";
+		};
+
+		pm8008_l7: LDO7 {
+			regulator-name = "pm8008_l7";
+		};
+	};
+};
-- 
2.7.4

Re: [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file

[Stephen Boyd]

Quoting Satya Priya (2022-02-08 06:52:19)
> Add base DTS file for pm8008 with infra and regulator nodes.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V4:
>  - This is newly added in V4, to add all the pm8008 common stuff.
>
> Changes in V5:
>  - Changed the mfd node names from pm8008_chip to pm8008_infra and
>    pm8008_ldo to pm8008_regulators as they re more appropriate.
>  - Changed the compatible for pm8008@9 mfd node to differentiate from
>    pm8008@8 node in driver.
>  - Removed compatible for regulators node.
>  - Removed reg property for LDOs and added in driver.
>
>  arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> new file mode 100644
> index 0000000..8e04983
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +// Copyright (c) 2021, The Linux Foundation. All rights reserved.
> +
> +pm8008_infra: pm8008@8 {
> +       compatible = "qcom,pm8008";
> +       reg = <0x8>;
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +};
> +
> +pm8008_regulators: pm8008@9 {
> +       compatible = "qcom,pm8008-regulators";
> +       reg = <0x9>;
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       regulators {

What is the point of the 'regulators' node? Why can't we just put LDO1
directly underneath the node that has the "qcom,pm8008-regulators"
compatible?

> +               pm8008_l1: LDO1 {
> +                       regulator-name = "pm8008_l1";
> +               };
> +

Re: [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file

[Satya Priya Kakitapalli (Temp)]

On 2/10/2022 7:11 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:19)
>> Add base DTS file for pm8008 with infra and regulator nodes.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>> Changes in V4:
>>   - This is newly added in V4, to add all the pm8008 common stuff.
>>
>> Changes in V5:
>>   - Changed the mfd node names from pm8008_chip to pm8008_infra and
>>     pm8008_ldo to pm8008_regulators as they re more appropriate.
>>   - Changed the compatible for pm8008@9 mfd node to differentiate from
>>     pm8008@8 node in driver.
>>   - Removed compatible for regulators node.
>>   - Removed reg property for LDOs and added in driver.
>>
>>   arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
>> new file mode 100644
>> index 0000000..8e04983
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
>> @@ -0,0 +1,46 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +// Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> +
>> +pm8008_infra: pm8008@8 {
>> +       compatible = "qcom,pm8008";
>> +       reg = <0x8>;
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +};
>> +
>> +pm8008_regulators: pm8008@9 {
>> +       compatible = "qcom,pm8008-regulators";
>> +       reg = <0x9>;
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +
>> +       regulators {
> What is the point of the 'regulators' node? Why can't we just put LDO1
> directly underneath the node that has the "qcom,pm8008-regulators"
> compatible?


It is not mandatory to have it but I think it makes the layout look a 
little better. And it seems to be commonly used upstream.


>> +               pm8008_l1: LDO1 {
>> +                       regulator-name = "pm8008_l1";
>> +               };
>> +

[PATCH V5 6/6] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Satya Priya]

Add pm8008_infra and pm8008_regulators support for sc7280 idp.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
 - As per Stephen's comments, replaced '_' with '-' for node names.

Changes in V3:
 - Changed the regulator node names as l1, l2 etc
 - Changed "pm8008-regulators" to "regulators"
 - Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt"

Changes in V4:
 - Moved all common stuff to pm8008.dtsi and added board specific configurations here.

Changes in V5:
 - Changed the node names as per pm8008.dtsi
 - Moved supply nodes to chip level (mfd node).
 - Removed the regulator-mindropout property.

 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index ecbf2b8..371ad19 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -263,6 +263,62 @@
 	};
 };
 
+&i2c1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	#include "pm8008.dtsi"
+};
+
+&pm8008_infra {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pm8008_active>;
+};
+
+&pm8008_regulators {
+	vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+	vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+	vdd_l5-supply = <&vreg_bob>;
+	vdd_l6-supply = <&vreg_bob>;
+	vdd_l7-supply = <&vreg_bob>;
+};
+
+&pm8008_l1 {
+	regulator-min-microvolt = <950000>;
+	regulator-max-microvolt = <1300000>;
+};
+
+&pm8008_l2 {
+	regulator-min-microvolt = <950000>;
+	regulator-max-microvolt = <1250000>;
+};
+
+&pm8008_l3 {
+	regulator-min-microvolt = <1650000>;
+	regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l4 {
+	regulator-min-microvolt = <1504000>;
+	regulator-max-microvolt = <1600000>;
+};
+
+&pm8008_l5 {
+	regulator-min-microvolt = <2600000>;
+	regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l6 {
+	regulator-min-microvolt = <2600000>;
+	regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l7 {
+	regulator-min-microvolt = <3000000>;
+	regulator-max-microvolt = <3544000>;
+};
+
 &qfprom {
 	vcc-supply = <&vreg_l1c_1p8>;
 };
@@ -375,6 +431,16 @@
 	drive-strength = <2>;
 };
 
+&pm8350c_gpios {
+	pm8008_active: pm8008_active {
+		pins = "gpio4";
+		function = "normal";
+		bias-disable;
+		output-high;
+		power-source = <0>;
+	};
+};
+
 &qspi_cs0 {
 	bias-disable;
 };
-- 
2.7.4