[PATCH V7 0/5] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[PATCH V7 0/5] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[Satya Priya]

Satya Priya (5):
  dt-bindings: mfd: pm8008: Add pm8008 regulators
  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       |  50 ++++-
 arch/arm64/boot/dts/qcom/pm8008.dtsi               |  44 +++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |  66 +++++++
 drivers/mfd/qcom-pm8008.c                          |  27 ++-
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-pm8008-regulator.c          | 205 +++++++++++++++++++++
 7 files changed, 393 insertions(+), 9 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

-- 
2.7.4

[PATCH V7 1/5] dt-bindings: mfd: pm8008: Add pm8008 regulators

[Satya Priya]

Add regulators 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>
---
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.

Changes in V6:
 - No changes.

Changes in V7:
 - Removed the intermediate regulators node and added ldos
   directly under mfd node.

 .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 50 +++++++++++++++++++---
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..6b3b53e 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,21 @@ 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.
+
 patternProperties:
   "^gpio@[0-9a-f]+$":
     type: object
@@ -85,13 +102,16 @@ patternProperties:
 
     additionalProperties: false
 
+  "^ldo[1-7]$":
+    type: object
+    $ref: "../regulator/regulator.yaml#"
+    description: PM8008 regulator peripherals of PM8008 regulator device
+
 required:
   - compatible
   - reg
-  - interrupts
   - "#address-cells"
   - "#size-cells"
-  - "#interrupt-cells"
 
 additionalProperties: false
 
@@ -102,13 +122,11 @@ examples:
     qupv3_se13_i2c {
       #address-cells = <1>;
       #size-cells = <0>;
-      pm8008i@8 {
+      pm8008_infra: pm8008@8 {
         compatible = "qcom,pm8008";
         reg = <0x8>;
         #address-cells = <1>;
         #size-cells = <0>;
-        interrupt-controller;
-        #interrupt-cells = <2>;
 
         interrupt-parent = <&tlmm>;
         interrupts = <32 IRQ_TYPE_EDGE_RISING>;
@@ -123,6 +141,24 @@ examples:
           #interrupt-cells = <2>;
         };
       };
-    };
 
+      pm8008_regulators: pm8008@9 {
+        compatible = "qcom,pm8008-regulators";
+        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>;
+
+        pm8008_l1: ldo1 {
+          regulator-name = "pm8008_l1";
+          regulator-min-microvolt = <950000>;
+          regulator-max-microvolt = <1300000>;
+        };
+      };
+    };
 ...
-- 
2.7.4

[PATCH V7 2/5] mfd: pm8008: Add mfd cell struct to register LDOs

[Satya Priya]

Add mfd cell struct with regulator driver name to match
with the pm8008 regulator driver and probe the LDOs.

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.

Changes in V6:
 - Changed the mfd_cell struct to have only name of the regulator driver.
 - Using device_get_match_data() instead of of_match_node() to match data.
 - Fixed few nits.

Changes in V7:
 - Fixed minor errors.

 drivers/mfd/qcom-pm8008.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f..6cfb267 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,10 @@
 #define INT_EN_CLR_OFFSET		0x16
 #define INT_LATCHED_STS_OFFSET		0x18
 
+static const struct mfd_cell pm8008_regulator_devs[] = {
+	MFD_CELL_NAME("qcom,pm8008-regulators"),
+};
+
 enum {
 	PM8008_MISC,
 	PM8008_TEMP_ALARM,
@@ -35,6 +40,11 @@ enum {
 	PM8008_NUM_PERIPHS,
 };
 
+enum pm8008_type {
+	PM8008_INFRA,
+	PM8008_REGULATORS,
+};
+
 #define PM8008_PERIPH_0_BASE	0x900
 #define PM8008_PERIPH_1_BASE	0x2400
 #define PM8008_PERIPH_2_BASE	0xC000
@@ -221,6 +231,7 @@ static int pm8008_probe(struct i2c_client *client)
 {
 	int rc;
 	struct pm8008_data *chip;
+	enum pm8008_type type;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -239,13 +250,25 @@ static int pm8008_probe(struct i2c_client *client)
 			dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
 	}
 
+	type = (uintptr_t) device_get_match_data(chip->dev);
+	if (type == PM8008_REGULATORS) {
+		rc = devm_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", },
-	{ },
+	{ .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA },
+	{ .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS },
+	{ }
 };
+MODULE_DEVICE_TABLE(of, pm8008_match);
 
 static struct i2c_driver pm8008_mfd_driver = {
 	.driver = {
-- 
2.7.4

[PATCH V7 3/5] 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.

Changes in V6:
 - Using loop to probe the regulators instead of creating separate platform device for
   each regulator.
 - Removed the of_parse_cb API as we are not parsing any DT properties. Moved slewrate
   configuration to probe.
 - Fixed other nits.

Changes in V7:
 - Removed unused Macros and headers.

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

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 00559c2..067013b 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 4b8794a..6462fd4 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..1c52864
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
+
+#include <linux/device.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 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_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)
+
+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)
+{
+	__le16 vset_raw;
+
+	vset_raw = cpu_to_le16(mV);
+
+	return regmap_bulk_write(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base),
+			(const void *)&vset_raw, sizeof(vset_raw));
+}
+
+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_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regulator_dev    *rdev;
+	struct pm8008_regulator *pm8008_reg;
+	struct regmap *regmap;
+	struct regulator_config reg_config = {};
+	int rc, i;
+	unsigned int reg;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "parent regmap is missing\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
+		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[i].base;
+
+		/* get slew rate */
+		rc = regmap_bulk_read(pm8008_reg->regmap,
+				LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
+		if (rc < 0) {
+			dev_err(dev, "failed to read step rate configuration rc=%d\n", rc);
+			return rc;
+		}
+		reg &= STEP_RATE_MASK;
+		pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
+
+		pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+		pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+		pm8008_reg->rdesc.name = reg_data[i].name;
+		pm8008_reg->rdesc.supply_name = reg_data[i].supply_name;
+		pm8008_reg->rdesc.of_match = reg_data[i].name;
+		pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
+		pm8008_reg->rdesc.min_uV = reg_data[i].min_uv;
+		pm8008_reg->rdesc.n_voltages
+			= ((reg_data[i].max_uv - reg_data[i].min_uv)
+				/ pm8008_reg->rdesc.uV_step) + 1;
+		pm8008_reg->rdesc.linear_ranges = reg_data[i].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[i].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[i].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

[PATCH V7 4/5] 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.

Changes in V6:
 - Changed node names to small letters.

Changes in V7:
 - Removed intermediate regulators node.

 arch/arm64/boot/dts/qcom/pm8008.dtsi | 44 ++++++++++++++++++++++++++++++++++++
 1 file changed, 44 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..0f48572
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2022, 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>;
+
+	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

[PATCH V7 5/5] 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.

Changes in V6:
 - No changes.

Changes in V7:
 - No Changes.

 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

Re: [PATCH V7 1/5] dt-bindings: mfd: pm8008: Add pm8008 regulators

[Stephen Boyd]

Quoting Satya Priya (2022-02-18 03:00:59)
> Add regulators 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>
> ---

Is the register layout compatible with SPMI regulators? The gpio node
seems to be fully compatible and the same driver probes there for SPMI
and i2c, so I wonder why we can't extend the existing SPMI gpio and
regulator bindings to have the new compatible strings for pm8008. Is
anything really different, or do we have the same device talking i2c
instead of SPMI now? Possibly it's exposing the different hardware
blocks inside the PMIC at different i2c addresses. It looks like the i2c
address is 0x8 and then there's 16-bits of address space inside the i2c
device to do things. 0x9 is the i2c address for the regulators and then
each ldo is at some offset in there?

> 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.
>
> Changes in V6:
>  - No changes.
>
> Changes in V7:
>  - Removed the intermediate regulators node and added ldos
>    directly under mfd node.
>
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 50 +++++++++++++++++++---
>  1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index ec3138c..6b3b53e 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,21 @@ 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.
> +
>  patternProperties:
>    "^gpio@[0-9a-f]+$":
>      type: object
> @@ -85,13 +102,16 @@ patternProperties:
>
>      additionalProperties: false
>
> +  "^ldo[1-7]$":
> +    type: object
> +    $ref: "../regulator/regulator.yaml#"
> +    description: PM8008 regulator peripherals of PM8008 regulator device
> +
>  required:
>    - compatible
>    - reg
> -  - interrupts
>    - "#address-cells"
>    - "#size-cells"
> -  - "#interrupt-cells"
>
>  additionalProperties: false
>
> @@ -102,13 +122,11 @@ examples:
>      qupv3_se13_i2c {
>        #address-cells = <1>;
>        #size-cells = <0>;
> -      pm8008i@8 {
> +      pm8008_infra: pm8008@8 {
>          compatible = "qcom,pm8008";
>          reg = <0x8>;
>          #address-cells = <1>;
>          #size-cells = <0>;
> -        interrupt-controller;
> -        #interrupt-cells = <2>;
>
>          interrupt-parent = <&tlmm>;
>          interrupts = <32 IRQ_TYPE_EDGE_RISING>;

I still fail to see what this part of the diff has to do with
regulators. Can it be split off to a different patch with a clear
description of why interrupt-controller and #interrupt-cells is no
longer required for qcom,pm8008?

It really looks like we're combining the binding for qcom,pm8008 and
qcom,pm8008-regulators at the same level, which looks wrong. We don't
want to describe the least common denominator between the two bindings.
Why not make two different bindings and files? One for the interrupty
gpio/interrupt controller device (at 0x8) and one for the regulator one
(at 0x9)?

> @@ -123,6 +141,24 @@ examples:
>            #interrupt-cells = <2>;
>          };
>        };
> -    };
>
> +      pm8008_regulators: pm8008@9 {

pmic@9, or regulators@9? The node name should be generic.

> +        compatible = "qcom,pm8008-regulators";
> +        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>;
> +
> +        pm8008_l1: ldo1 {
> +          regulator-name = "pm8008_l1";
> +          regulator-min-microvolt = <950000>;
> +          regulator-max-microvolt = <1300000>;
> +        };
> +      };

For some i2c devices that appear on multiple i2c addresses we make an
i2c client for each address in the driver that attaches to the node we
put in DT. I suppose that won't work easily here. Either way, it would
make it much clearer if this existing binding was left alone. Is there
other functionality inside the i2c address 0x9 register space that isn't
regulators?

Re: [PATCH V7 3/5] regulator: Add a regulator driver for the PM8008 PMIC

[Stephen Boyd]

Quoting Satya Priya (2022-02-18 03:01:01)
> 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>

> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..1c52864
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/device.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 VSET_STEP_MV                   8
> +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
> +#define ENABLE_BIT                     BIT(7)

This is SPMI_COMMON_REG_ENABLE and SPMI_COMMON_ENABLE_MASK

> +
> +#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)
> +
> +struct regulator_data {

Please use a name like spmi_regulator_data :) Or pm8008_regulator_data?
Something that isn't so generic.

It seems similar to qcom_spmi-regulator.c so is there some reason we
can't shove this into there? Less code for things that aren't relevant I
guess?

> +       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;

Re: [PATCH V7 4/5] arm64: dts: qcom: pm8008: Add base dts file

[Stephen Boyd]

Quoting Satya Priya (2022-02-18 03:01:02)
> 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.
>
> Changes in V6:
>  - Changed node names to small letters.
>
> Changes in V7:
>  - Removed intermediate regulators node.
>
>  arch/arm64/boot/dts/qcom/pm8008.dtsi | 44 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 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..0f48572
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +// Copyright (c) 2022, The Linux Foundation. All rights reserved.
> +
> +pm8008_infra: pm8008@8 {

Node name should be generic, pmic@8

> +       compatible = "qcom,pm8008";
> +       reg = <0x8>;
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +};
> +
> +pm8008_regulators: pm8008@9 {

Node name should be generic, pmic@9

> +       compatible = "qcom,pm8008-regulators";
> +       reg = <0x9>;
> +       #address-cells = <1>;

Address cells is 0 too?

> +       #size-cells = <0>;
> +
> +       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";
> +       };
>

Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Stephen Boyd]

Quoting Satya Priya (2022-02-18 03:01:03)
> 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.
>
> Changes in V6:
>  - No changes.
>
> Changes in V7:
>  - No Changes.
>
>  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 {

Can we add another phandle?

&pm8008_bus: &i2c1 {

> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       status = "okay";
> +
> +       #include "pm8008.dtsi"
> +};

And then

#include "pm8008.dtsi"

and have the pm8008.dtsi file add itself as a child of &pm8008_bus? Then
we can easily see that pm8008 is a child of pm8008_bus without having to
figure out where the file is included. It also helps avoid polluting the
i2c node with things that shouldn't be there in case we want to include
configuration bits in the pm8008.dtsi file that aren't directly related
to the bus node.

> +
> +&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 {

No underscore in node names. pm8008_active: pm8008-active {

> +               pins = "gpio4";
> +               function = "normal";
> +               bias-disable;
> +               output-high;

Is this a reset signal? Should the driver be deasserting the reset when
it is ready? That could be the same time the gpio is acquired.

> +               power-source = <0>;
> +       };
> +};

Re: [PATCH V7 1/5] dt-bindings: mfd: pm8008: Add pm8008 regulators

[Satya Priya Kakitapalli (Temp)]

On 2/19/2022 7:09 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-18 03:00:59)
>> Add regulators 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>
>> ---
> Is the register layout compatible with SPMI regulators? The gpio node
> seems to be fully compatible and the same driver probes there for SPMI
> and i2c, so I wonder why we can't extend the existing SPMI gpio and
> regulator bindings to have the new compatible strings for pm8008. Is
> anything really different, or do we have the same device talking i2c
> instead of SPMI now? Possibly it's exposing the different hardware
> blocks inside the PMIC at different i2c addresses. It looks like the i2c
> address is 0x8 and then there's 16-bits of address space inside the i2c
> device to do things. 0x9 is the i2c address for the regulators and then
> each ldo is at some offset in there?


The register layout is not compatible with spmi regulators, I see some 
differences w.r.t VOLTAGE_CTL, EN_CTL, MODE_CTL registers. Also, there 
is no headroom related stuff in the spmi driver.


>> 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.
>>
>> Changes in V6:
>>   - No changes.
>>
>> Changes in V7:
>>   - Removed the intermediate regulators node and added ldos
>>     directly under mfd node.
>>
>>   .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 50 +++++++++++++++++++---
>>   1 file changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> index ec3138c..6b3b53e 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,21 @@ 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.
>> +
>>   patternProperties:
>>     "^gpio@[0-9a-f]+$":
>>       type: object
>> @@ -85,13 +102,16 @@ patternProperties:
>>
>>       additionalProperties: false
>>
>> +  "^ldo[1-7]$":
>> +    type: object
>> +    $ref: "../regulator/regulator.yaml#"
>> +    description: PM8008 regulator peripherals of PM8008 regulator device
>> +
>>   required:
>>     - compatible
>>     - reg
>> -  - interrupts
>>     - "#address-cells"
>>     - "#size-cells"
>> -  - "#interrupt-cells"
>>
>>   additionalProperties: false
>>
>> @@ -102,13 +122,11 @@ examples:
>>       qupv3_se13_i2c {
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>> -      pm8008i@8 {
>> +      pm8008_infra: pm8008@8 {
>>           compatible = "qcom,pm8008";
>>           reg = <0x8>;
>>           #address-cells = <1>;
>>           #size-cells = <0>;
>> -        interrupt-controller;
>> -        #interrupt-cells = <2>;
>>
>>           interrupt-parent = <&tlmm>;
>>           interrupts = <32 IRQ_TYPE_EDGE_RISING>;
> I still fail to see what this part of the diff has to do with
> regulators. Can it be split off to a different patch with a clear
> description of why interrupt-controller and #interrupt-cells is no
> longer required for qcom,pm8008?


This diff has nothing to do with regulators, I removed it to avoid yaml 
errors during dtbs check.

I'll move this to a separate patch.


> It really looks like we're combining the binding for qcom,pm8008 and
> qcom,pm8008-regulators at the same level, which looks wrong. We don't
> want to describe the least common denominator between the two bindings.
> Why not make two different bindings and files? One for the interrupty
> gpio/interrupt controller device (at 0x8) and one for the regulator one
> (at 0x9)?


Okay, I'll add a different binding for regulators 
(mfd/qcom,pm8008-regulators.yaml), leave this binding as it is.. and 
also add separate DT files for pm8008-infra and pm8008-regulators.


>> @@ -123,6 +141,24 @@ examples:
>>             #interrupt-cells = <2>;
>>           };
>>         };
>> -    };
>>
>> +      pm8008_regulators: pm8008@9 {
> pmic@9, or regulators@9? The node name should be generic.
>
>> +        compatible = "qcom,pm8008-regulators";
>> +        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>;
>> +
>> +        pm8008_l1: ldo1 {
>> +          regulator-name = "pm8008_l1";
>> +          regulator-min-microvolt = <950000>;
>> +          regulator-max-microvolt = <1300000>;
>> +        };
>> +      };
> For some i2c devices that appear on multiple i2c addresses we make an
> i2c client for each address in the driver that attaches to the node we
> put in DT. I suppose that won't work easily here. Either way, it would
> make it much clearer if this existing binding was left alone. Is there
> other functionality inside the i2c address 0x9 register space that isn't
> regulators?


As mentioned above, I'll make a separate binding for regulators. There 
is no other functionality apart from regulators in the i2c 0x9 register 
space.

Re: [PATCH V7 3/5] regulator: Add a regulator driver for the PM8008 PMIC

[Satya Priya Kakitapalli (Temp)]

On 2/19/2022 7:20 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-18 03:01:01)
>> 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>
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
>> new file mode 100644
>> index 0000000..1c52864
>> --- /dev/null
>> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> @@ -0,0 +1,205 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
>> +
>> +#include <linux/device.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 VSET_STEP_MV                   8
>> +#define VSET_STEP_UV                   (VSET_STEP_MV * 1000)
>> +
>> +#define LDO_ENABLE_REG(base)           ((base) + 0x46)
>> +#define ENABLE_BIT                     BIT(7)
> This is SPMI_COMMON_REG_ENABLE and SPMI_COMMON_ENABLE_MASK
>
>> +
>> +#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)
>> +
>> +struct regulator_data {
> Please use a name like spmi_regulator_data :) Or pm8008_regulator_data?
> Something that isn't so generic.


Okay.


> It seems similar to qcom_spmi-regulator.c so is there some reason we
> can't shove this into there? Less code for things that aren't relevant I
> guess?


As mentioned earlier, the register layout is not compatible and some 
other differences w.r.t to code like headrooms, using mfd driver to 
register ldos etc..


>> +       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;

Re: [PATCH V7 4/5] arm64: dts: qcom: pm8008: Add base dts file

[Satya Priya Kakitapalli (Temp)]

On 2/19/2022 7:27 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-18 03:01:02)
>> 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.
>>
>> Changes in V6:
>>   - Changed node names to small letters.
>>
>> Changes in V7:
>>   - Removed intermediate regulators node.
>>
>>   arch/arm64/boot/dts/qcom/pm8008.dtsi | 44 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 44 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..0f48572
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
>> @@ -0,0 +1,44 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +// Copyright (c) 2022, The Linux Foundation. All rights reserved.
>> +
>> +pm8008_infra: pm8008@8 {
> Node name should be generic, pmic@8


Okay.


>> +       compatible = "qcom,pm8008";
>> +       reg = <0x8>;
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +};
>> +
>> +pm8008_regulators: pm8008@9 {
> Node name should be generic, pmic@9


Okay.


>> +       compatible = "qcom,pm8008-regulators";
>> +       reg = <0x9>;
>> +       #address-cells = <1>;
> Address cells is 0 too?


Okay.


>> +       #size-cells = <0>;
>> +
>> +       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";
>> +       };
>>

Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Satya Priya Kakitapalli (Temp)]

On 2/19/2022 7:31 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-18 03:01:03)
>> 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.
>>
>> Changes in V6:
>>   - No changes.
>>
>> Changes in V7:
>>   - No Changes.
>>
>>   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 {
> Can we add another phandle?
>
> &pm8008_bus: &i2c1 {

Okay.


>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       status = "okay";
>> +
>> +       #include "pm8008.dtsi"
>> +};
> And then
>
> #include "pm8008.dtsi"


Okay.


> and have the pm8008.dtsi file add itself as a child of &pm8008_bus? Then
> we can easily see that pm8008 is a child of pm8008_bus without having to
> figure out where the file is included. It also helps avoid polluting the
> i2c node with things that shouldn't be there in case we want to include
> configuration bits in the pm8008.dtsi file that aren't directly related
> to the bus node.
>
>> +
>> +&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 {
> No underscore in node names. pm8008_active: pm8008-active {


Okay.


>> +               pins = "gpio4";
>> +               function = "normal";
>> +               bias-disable;
>> +               output-high;
> Is this a reset signal? Should the driver be deasserting the reset when
> it is ready? That could be the same time the gpio is acquired.


I didn't get your question exactly.. hope this answers your query

The pm8008 chip needs this gpio to be toggled , in order to come out of 
reset and start any transactions..

Please let me know if you have more queries


>> +               power-source = <0>;
>> +       };
>> +};

Re: [PATCH V7 1/5] dt-bindings: mfd: pm8008: Add pm8008 regulators

[Stephen Boyd]

Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:14:56)
>
> On 2/19/2022 7:09 AM, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-02-18 03:00:59)
> >> Add regulators 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>
> >> ---
> > Is the register layout compatible with SPMI regulators? The gpio node
> > seems to be fully compatible and the same driver probes there for SPMI
> > and i2c, so I wonder why we can't extend the existing SPMI gpio and
> > regulator bindings to have the new compatible strings for pm8008. Is
> > anything really different, or do we have the same device talking i2c
> > instead of SPMI now? Possibly it's exposing the different hardware
> > blocks inside the PMIC at different i2c addresses. It looks like the i2c
> > address is 0x8 and then there's 16-bits of address space inside the i2c
> > device to do things. 0x9 is the i2c address for the regulators and then
> > each ldo is at some offset in there?
>
>
> The register layout is not compatible with spmi regulators, I see some
> differences w.r.t VOLTAGE_CTL, EN_CTL, MODE_CTL registers. Also, there
> is no headroom related stuff in the spmi driver.

It sounds like minor differences and/or improvements to the existing
SPMI regulator hardware.

> >>           interrupt-parent = <&tlmm>;
> >>           interrupts = <32 IRQ_TYPE_EDGE_RISING>;
> > I still fail to see what this part of the diff has to do with
> > regulators. Can it be split off to a different patch with a clear
> > description of why interrupt-controller and #interrupt-cells is no
> > longer required for qcom,pm8008?
>
>
> This diff has nothing to do with regulators, I removed it to avoid yaml
> errors during dtbs check.
>
> I'll move this to a separate patch.

Ok, thanks!

Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Stephen Boyd]

Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06)
>
> On 2/19/2022 7:31 AM, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-02-18 03:01:03)
>
> >> +               pins = "gpio4";
> >> +               function = "normal";
> >> +               bias-disable;
> >> +               output-high;
> > Is this a reset signal? Should the driver be deasserting the reset when
> > it is ready? That could be the same time the gpio is acquired.
>
>
> I didn't get your question exactly.. hope this answers your query
>
> The pm8008 chip needs this gpio to be toggled , in order to come out of
> reset and start any transactions..
>
> Please let me know if you have more queries

Yes that answers it for me. Thanks.

This is a reset gpio and should be a DT property like

	reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH>

in the pm8008 node. When the driver probes it should get the gpio and
do any toggling to take it out of reset. It shouldn't be done through
pinconf settings.

Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Satya Priya Kakitapalli (Temp)]

On 3/1/2022 2:06 AM, Stephen Boyd wrote:
> Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06)
>> On 2/19/2022 7:31 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-02-18 03:01:03)
>>>> +               pins = "gpio4";
>>>> +               function = "normal";
>>>> +               bias-disable;
>>>> +               output-high;
>>> Is this a reset signal? Should the driver be deasserting the reset when
>>> it is ready? That could be the same time the gpio is acquired.
>>
>> I didn't get your question exactly.. hope this answers your query
>>
>> The pm8008 chip needs this gpio to be toggled , in order to come out of
>> reset and start any transactions..
>>
>> Please let me know if you have more queries
> Yes that answers it for me. Thanks.
>
> This is a reset gpio and should be a DT property like
>
> 	reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH>
>
> in the pm8008 node. When the driver probes it should get the gpio and
> do any toggling to take it out of reset. It shouldn't be done through
> pinconf settings.


Okay, IIUC,  I have to remove the output-high here and add reset-gpios 
in pm8008 DT node. And then add below code in pm8008 mfd driver probe

+               chip->reset_gpio = devm_gpiod_get(chip->dev, "reset", 
GPIOD_OUT_HIGH);
+               if (IS_ERR(chip->reset_gpio)) {
+                       dev_err(chip->dev, "failed to acquire reset 
gpio\n");
+                       return PTR_ERR(chip->reset_gpio);
+               }
+               gpiod_set_value(chip->reset_gpio, 1);

This is working for me, Please let me know if I'm  missing something.

Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Stephen Boyd]

Quoting Satya Priya Kakitapalli (Temp) (2022-03-07 06:49:31)
>
> On 3/1/2022 2:06 AM, Stephen Boyd wrote:
> > Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:25:06)
> >> On 2/19/2022 7:31 AM, Stephen Boyd wrote:
> >>> Quoting Satya Priya (2022-02-18 03:01:03)
> >>>> +               pins = "gpio4";
> >>>> +               function = "normal";
> >>>> +               bias-disable;
> >>>> +               output-high;
> >>> Is this a reset signal? Should the driver be deasserting the reset when
> >>> it is ready? That could be the same time the gpio is acquired.
> >>
> >> I didn't get your question exactly.. hope this answers your query
> >>
> >> The pm8008 chip needs this gpio to be toggled , in order to come out of
> >> reset and start any transactions..
> >>
> >> Please let me know if you have more queries
> > Yes that answers it for me. Thanks.
> >
> > This is a reset gpio and should be a DT property like
> >
> >       reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH>
> >
> > in the pm8008 node. When the driver probes it should get the gpio and
> > do any toggling to take it out of reset. It shouldn't be done through
> > pinconf settings.
>
>
> Okay, IIUC,  I have to remove the output-high here and add reset-gpios
> in pm8008 DT node. And then add below code in pm8008 mfd driver probe
>
> +               chip->reset_gpio = devm_gpiod_get(chip->dev, "reset",
> GPIOD_OUT_HIGH);
> +               if (IS_ERR(chip->reset_gpio)) {
> +                       dev_err(chip->dev, "failed to acquire reset
> gpio\n");
> +                       return PTR_ERR(chip->reset_gpio);
> +               }
> +               gpiod_set_value(chip->reset_gpio, 1);
>
> This is working for me, Please let me know if I'm  missing something.
>

Yep looks good to me.