[PATCH V10 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[PATCH V10 0/9] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[Satya Priya]

Satya Priya (9):
  dt-bindings: mfd: pm8008: Add reset-gpios
  dt-bindings: regulator: pm8008: Add pm8008 regulator bindings
  dt-bindings: mfd: pm8008: Add regulators
  mfd: pm8008: Add reset-gpios
  mfd: pm8008: Use i2c_new_dummy_device() API
  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       |  24 +++
 .../bindings/regulator/qcom,pm8008-regulators.yaml |  40 ++++
 arch/arm64/boot/dts/qcom/pm8008.dtsi               |  42 +++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |  68 +++++++
 drivers/mfd/qcom-pm8008.c                          |  76 +++++++-
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-pm8008-regulator.c          | 201 +++++++++++++++++++++
 include/linux/mfd/qcom_pm8008.h                    |  13 ++
 9 files changed, 464 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulators.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
 create mode 100644 include/linux/mfd/qcom_pm8008.h

-- 
2.7.4

[PATCH V10 1/9] dt-bindings: mfd: pm8008: Add reset-gpios

[Satya Priya]

Add reset-gpios property for pm8008.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in V10:
 - None.

Changes in V9:
 - Undo the changes from V8 and only add reset-gpios. Leave interrupts
   as required properties and do not change compatible.

 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..3312784 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -44,6 +44,9 @@ properties:
   "#size-cells":
     const: 0
 
+  reset-gpios:
+    maxItems: 1
+
 patternProperties:
   "^gpio@[0-9a-f]+$":
     type: object
@@ -92,6 +95,7 @@ required:
   - "#address-cells"
   - "#size-cells"
   - "#interrupt-cells"
+  - reset-gpios
 
 additionalProperties: false
 
@@ -99,6 +103,7 @@ examples:
   - |
     #include <dt-bindings/mfd/qcom-pm8008.h>
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
     qupv3_se13_i2c {
       #address-cells = <1>;
       #size-cells = <0>;
@@ -113,6 +118,8 @@ examples:
         interrupt-parent = <&tlmm>;
         interrupts = <32 IRQ_TYPE_EDGE_RISING>;
 
+        reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH>;
+
         pm8008_gpios: gpio@c000 {
           compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
           reg = <0xc000>;
-- 
2.7.4

[PATCH V10 2/9] dt-bindings: regulator: pm8008: Add pm8008 regulator bindings

[Satya Priya]

Add bindings for pm8008 regulators.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V8:
 - This is split from pm8008.yaml binding.

Changes in V9:
  - Remove description for reg and drop unused phandle from example.

Changes in V10:
 - Regulators are added as a part of pm8008@8 device. Change bindings doc
   accordingly.

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

diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulators.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulators.yaml
new file mode 100644
index 0000000..f56cdcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulators.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulators.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
+
+maintainers:
+  - Satya Priya <quic_c_skakit@quicinc.com>
+
+description: |
+  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
+  containing 7 LDO regulators. This binding specifies the PM8008
+  regulator peripherals of PM8008 device.
+
+properties:
+  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:
+  "^ldo[1-7]$":
+    type: object
+    $ref: "/schemas/regulator/regulator.yaml#"
+    description: PM8008 regulator peripherals of PM8008 regulator device.
+
+additionalProperties: false
+...
-- 
2.7.4

[PATCH V10 3/9] dt-bindings: mfd: pm8008: Add regulators

[Satya Priya]

Add regulators for pm8008 with example.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V10:
 - Add regulators under main mfd node as in DT.

 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index 3312784..060cc35 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -44,6 +44,9 @@ properties:
   "#size-cells":
     const: 0
 
+  regulators:
+    $ref: "/schemas/regulator/qcom,pm8008-regulators.yaml#"
+
   reset-gpios:
     maxItems: 1
 
@@ -129,6 +132,20 @@ examples:
           interrupt-controller;
           #interrupt-cells = <2>;
         };
+
+        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>;
+
+          ldo1 {
+            regulator-name = "pm8008_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+          };
+        };
       };
     };
 
-- 
2.7.4

[PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Satya Priya]

Add the reset-gpio toggling in the pm8008_probe() to bring
pm8008 chip out of reset instead of doing it in DT node using
"output-high" property.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V10:
 - This has been split from [V9,3/6] as per comments here [1]
 [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1649166633-25872-4-git-send-email-quic_c_skakit@quicinc.com/#24803409

 drivers/mfd/qcom-pm8008.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f..97a72da 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -57,6 +58,7 @@ enum {
 struct pm8008_data {
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *reset_gpio;
 	int irq;
 	struct regmap_irq_chip_data *irq_data;
 };
@@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
 			dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
 	}
 
+	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);
+
 	return devm_of_platform_populate(chip->dev);
 }
 
-- 
2.7.4

[PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Satya Priya]

Use i2c_new_dummy_device() to register clients along with
the main mfd device.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V10:
 - Implement i2c_new_dummy_device to register extra clients.

 drivers/mfd/qcom-pm8008.c       | 54 +++++++++++++++++++++++++++++++++--------
 include/linux/mfd/qcom_pm8008.h | 13 ++++++++++
 2 files changed, 57 insertions(+), 10 deletions(-)
 create mode 100644 include/linux/mfd/qcom_pm8008.h

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index 97a72da..ca5240d 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/mfd/qcom_pm8008.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
@@ -56,8 +57,10 @@ enum {
 #define PM8008_PERIPH_OFFSET(paddr)	(paddr - PM8008_PERIPH_0_BASE)
 
 struct pm8008_data {
+	bool ready;
 	struct device *dev;
-	struct regmap *regmap;
+	struct i2c_client *clients[PM8008_NUM_CLIENTS];
+	struct regmap *regmap[PM8008_NUM_CLIENTS];
 	struct gpio_desc *reset_gpio;
 	int irq;
 	struct regmap_irq_chip_data *irq_data;
@@ -152,9 +155,20 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
 	.max_register	= 0xFFFF,
 };
 
+struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid)
+{
+	if (!chip || !chip->ready) {
+		pr_err("pm8008 chip not initialized\n");
+		return NULL;
+	}
+
+	return chip->regmap[sid];
+}
+
 static int pm8008_init(struct pm8008_data *chip)
 {
 	int rc;
+	struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
 
 	/*
 	 * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
@@ -162,19 +176,19 @@ static int pm8008_init(struct pm8008_data *chip)
 	 * This is required to enable the writing of TYPE registers in
 	 * regmap_irq_sync_unlock().
 	 */
-	rc = regmap_write(chip->regmap,
+	rc = regmap_write(regmap,
 			 (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
 			 BIT(0));
 	if (rc)
 		return rc;
 
 	/* Do the same for GPIO1 and GPIO2 peripherals */
-	rc = regmap_write(chip->regmap,
+	rc = regmap_write(regmap,
 			 (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
 	if (rc)
 		return rc;
 
-	rc = regmap_write(chip->regmap,
+	rc = regmap_write(regmap,
 			 (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
 
 	return rc;
@@ -186,6 +200,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
 	int rc, i;
 	struct regmap_irq_type *type;
 	struct regmap_irq_chip_data *irq_data;
+	struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
 
 	rc = pm8008_init(chip);
 	if (rc) {
@@ -209,7 +224,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
 				IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
 	}
 
-	rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
+	rc = devm_regmap_add_irq_chip(chip->dev, regmap, client_irq,
 			IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
 	if (rc) {
 		dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
@@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
 
 static int pm8008_probe(struct i2c_client *client)
 {
-	int rc;
+	int rc, i;
 	struct pm8008_data *chip;
+	struct device_node *node = client->dev.of_node;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
 	chip->dev = &client->dev;
-	chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
-	if (!chip->regmap)
-		return -ENODEV;
 
-	i2c_set_clientdata(client, chip);
+	for (i = 0; i < PM8008_NUM_CLIENTS; i++) {
+		if (i == 0) {
+			chip->clients[i] = client;
+		} else {
+			chip->clients[i] = i2c_new_dummy_device(client->adapter,
+								client->addr + i);
+			if (IS_ERR(chip->clients[i])) {
+				dev_err(&client->dev, "can't attach client %d\n", i);
+				return PTR_ERR(chip->clients[i]);
+			}
+			chip->clients[i]->dev.of_node = of_node_get(node);
+		}
+
+		chip->regmap[i] = devm_regmap_init_i2c(chip->clients[i],
+							&qcom_mfd_regmap_cfg);
+		if (!chip->regmap[i])
+			return -ENODEV;
+
+		i2c_set_clientdata(chip->clients[i], chip);
+	}
+
+	chip->ready = true;
 
 	if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
 		rc = pm8008_probe_irq_peripherals(chip, client->irq);
diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
new file mode 100644
index 0000000..bc64f01
--- /dev/null
+++ b/include/linux/mfd/qcom_pm8008.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_PM8008_H__
+#define __QCOM_PM8008_H__
+
+#define PM8008_INFRA_SID	0
+#define PM8008_REGULATORS_SID	1
+
+#define PM8008_NUM_CLIENTS	2
+
+struct pm8008_data;
+struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);
+
+#endif
-- 
2.7.4

[PATCH V10 6/9] mfd: pm8008: Add mfd_cell struct to register LDOs

[Satya Priya]

Add mfd_cell struct to probe LDO regulators using
devm_mfd_add_devices() API.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V8:
 - Split the probe for infra and regulator devices
 - Add the reset-gpio toggling in the infra driver probe

Changes in V9:
 - Fixed nits.

Changes in V10:
 - Removed the extra probe added for regulators as is is not needed now.

 drivers/mfd/qcom-pm8008.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index ca5240d..ab4ba55 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
 #include <linux/mfd/qcom_pm8008.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -29,6 +30,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-regulator"),
+};
+
 enum {
 	PM8008_MISC,
 	PM8008_TEMP_ALARM,
@@ -282,6 +287,14 @@ static int pm8008_probe(struct i2c_client *client)
 	}
 	gpiod_set_value(chip->reset_gpio, 1);
 
+	rc = devm_mfd_add_devices(&chip->clients[PM8008_REGULATORS_SID]->dev,
+			0, pm8008_regulator_devs, ARRAY_SIZE(pm8008_regulator_devs),
+			NULL, 0, NULL);
+	if (rc) {
+		dev_err(chip->dev, "Failed to add regulators: %d\n", rc);
+		return rc;
+	}
+
 	return devm_of_platform_populate(chip->dev);
 }
 
-- 
2.7.4

[PATCH V10 7/9] 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 V8:
 - Changed the regulators_data struct name to pm8008_regulator_data

Changes in V9:
 - Nothing has changed.

Changes in V10:
 - Changed the driver name.
 - Removed unused header.
 - Use get_voltage_sel.

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

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index a7effc5..ce8522d 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 69b5a19..fc045d9 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..4375ac5
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,201 @@
+// 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/mfd/qcom_pm8008.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>
+
+#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 pm8008_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;
+	int			voltage_selector;
+};
+
+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 pm8008_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);
+
+	return pm8008_reg->voltage_selector;
+}
+
+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;
+
+	pm8008_reg->voltage_selector = selector;
+	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_sel	= 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 pm8008_data *chip = dev_get_drvdata(pdev->dev.parent);
+	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 = pm8008_get_regmap(chip, PM8008_REGULATORS_SID);
+	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;
+		pm8008_reg->voltage_selector = -ENOTRECOVERABLE;
+		pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
+
+		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-regulator",
+	},
+	.probe	= pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH V10 8/9] arm64: dts: qcom: pm8008: Add base dts file

[Satya Priya]

Add base DTS file for pm8008.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V9:
 - Add single dt file for pm8008 instead of adding files like in V8.

Changes in V10:
 - Add regulators under pm8008@8 i.e main mfd node.

 arch/arm64/boot/dts/qcom/pm8008.dtsi | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 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..7b4fe68
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2022, The Linux Foundation. All rights reserved.
+
+&pm8008_bus {
+	pm8008: pmic@8 {
+		compatible = "qcom,pm8008";
+		reg = <0x8>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <2>;
+
+		pm8008_regulators: 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

[PATCH V10 9/9] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

[Satya Priya]

Add pm8008 infra and regulators support for sc7280 idp.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V8:
 - Add an extra phandle "pm8008_bus" and then include pm8008 dtsi files inside it.
 - Remove output-high from pm8008_active node.

Changes in V9:
 - Added interrupts properties.

Changes in V10:
 - None.

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

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 015a347..a834476 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -263,6 +263,65 @@
 	};
 };
 
+pm8008_bus: &i2c1 {
+	status = "okay";
+};
+
+#include "pm8008.dtsi"
+
+&pm8008 {
+	interrupt-parent = <&tlmm>;
+	interrupts = <24 IRQ_TYPE_EDGE_RISING>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pm8008_active>;
+
+	reset-gpios = <&pm8350c_gpios 4 GPIO_ACTIVE_HIGH>;
+};
+
+&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 +434,15 @@
 	drive-strength = <2>;
 };
 
+&pm8350c_gpios {
+	pm8008_active: pm8008-active {
+		pins = "gpio4";
+		function = "normal";
+		bias-disable;
+		power-source = <0>;
+	};
+};
+
 &qspi_cs0 {
 	bias-disable;
 };
-- 
2.7.4

Re: [PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Stephen Boyd]

Quoting Satya Priya (2022-04-14 05:30:13)
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index c472d7f..97a72da 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
>                         dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>         }
>
> +       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");

The API looks to print debug messages. This print doesn't look required.

> +               return PTR_ERR(chip->reset_gpio);
> +       }
> +       gpiod_set_value(chip->reset_gpio, 1);

Does this do anything? Does this work just as well?

	reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
	if (IS_ERR(reset_gpio))
		return PTR_ERR(reset_gpio);

Note that there's no point to store the reset gpio in the structure if
it won't be used outside of probe. This should work fine? I used
GPIOD_OUT_LOW to indicate that the reset should be returned to this
function deasserted, i.e. taking the PMIC out of reset.

> +
>         return devm_of_platform_populate(chip->dev);

Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Stephen Boyd]

Quoting Satya Priya (2022-04-14 05:30:14)
> Use i2c_new_dummy_device() to register clients along with
> the main mfd device.

Why?

>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V10:
>  - Implement i2c_new_dummy_device to register extra clients.
>
>  drivers/mfd/qcom-pm8008.c       | 54 +++++++++++++++++++++++++++++++++--------
>  include/linux/mfd/qcom_pm8008.h | 13 ++++++++++
>  2 files changed, 57 insertions(+), 10 deletions(-)
>  create mode 100644 include/linux/mfd/qcom_pm8008.h
>
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index 97a72da..ca5240d 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -56,8 +57,10 @@ enum {
>  #define PM8008_PERIPH_OFFSET(paddr)    (paddr - PM8008_PERIPH_0_BASE)
>
>  struct pm8008_data {
> +       bool ready;
>         struct device *dev;
> -       struct regmap *regmap;
> +       struct i2c_client *clients[PM8008_NUM_CLIENTS];
> +       struct regmap *regmap[PM8008_NUM_CLIENTS];
>         struct gpio_desc *reset_gpio;
>         int irq;
>         struct regmap_irq_chip_data *irq_data;
> @@ -152,9 +155,20 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>         .max_register   = 0xFFFF,
>  };
>
> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid)
> +{
> +       if (!chip || !chip->ready) {

Is it even possible?

> +               pr_err("pm8008 chip not initialized\n");
> +               return NULL;
> +       }
> +
> +       return chip->regmap[sid];
> +}
> +
>  static int pm8008_init(struct pm8008_data *chip)
>  {
>         int rc;
> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
>
>         /*
>          * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
> @@ -162,19 +176,19 @@ static int pm8008_init(struct pm8008_data *chip)
>          * This is required to enable the writing of TYPE registers in
>          * regmap_irq_sync_unlock().
>          */
> -       rc = regmap_write(chip->regmap,
> +       rc = regmap_write(regmap,
>                          (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>                          BIT(0));
>         if (rc)
>                 return rc;
>
>         /* Do the same for GPIO1 and GPIO2 peripherals */
> -       rc = regmap_write(chip->regmap,
> +       rc = regmap_write(regmap,
>                          (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>         if (rc)
>                 return rc;
>
> -       rc = regmap_write(chip->regmap,
> +       rc = regmap_write(regmap,
>                          (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>
>         return rc;
> @@ -186,6 +200,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>         int rc, i;
>         struct regmap_irq_type *type;
>         struct regmap_irq_chip_data *irq_data;
> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);

Instead of calling pm8008_get_regmap() many times why not pass the
regmap through from pm8008_probe_irq_peripherals() called in probe? At
that point we could remove the regmap pointer from struct pm8008_data?

>
>         rc = pm8008_init(chip);
>         if (rc) {
> @@ -209,7 +224,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>                                 IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>         }
>
> -       rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
> +       rc = devm_regmap_add_irq_chip(chip->dev, regmap, client_irq,
>                         IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>         if (rc) {
>                 dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
> @@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>
>  static int pm8008_probe(struct i2c_client *client)
>  {
> -       int rc;
> +       int rc, i;
>         struct pm8008_data *chip;
> +       struct device_node *node = client->dev.of_node;
>
>         chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>         if (!chip)
>                 return -ENOMEM;
>
>         chip->dev = &client->dev;
> -       chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> -       if (!chip->regmap)
> -               return -ENODEV;
>
> -       i2c_set_clientdata(client, chip);
> +       for (i = 0; i < PM8008_NUM_CLIENTS; i++) {

This is 2. Why do we have a loop? Just register the i2c client for
pm8008_infra first and then make a dummy for the second address without
the loop and the indentation. Are there going to be more i2c clients?

> +               if (i == 0) {
> +                       chip->clients[i] = client;
> +               } else {
> +                       chip->clients[i] = i2c_new_dummy_device(client->adapter,
> +                                                               client->addr + i);
> +                       if (IS_ERR(chip->clients[i])) {
> +                               dev_err(&client->dev, "can't attach client %d\n", i);
> +                               return PTR_ERR(chip->clients[i]);
> +                       }
> +                       chip->clients[i]->dev.of_node = of_node_get(node);
> +               }
> +
> +               chip->regmap[i] = devm_regmap_init_i2c(chip->clients[i],
> +                                                       &qcom_mfd_regmap_cfg);
> +               if (!chip->regmap[i])
> +                       return -ENODEV;
> +
> +               i2c_set_clientdata(chip->clients[i], chip);
> +       }
> +
> +       chip->ready = true;
>
>         if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
>                 rc = pm8008_probe_irq_peripherals(chip, client->irq);
> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
> new file mode 100644
> index 0000000..bc64f01
> --- /dev/null
> +++ b/include/linux/mfd/qcom_pm8008.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __QCOM_PM8008_H__
> +#define __QCOM_PM8008_H__
> +
> +#define PM8008_INFRA_SID       0
> +#define PM8008_REGULATORS_SID  1
> +
> +#define PM8008_NUM_CLIENTS     2
> +
> +struct pm8008_data;
> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);

Could this be avoided if the regulator driver used
dev_get_regmap(&pdev->dev.parent, "regulator") to find the regmap named
"regulator" of the parent device, i.e. pm8008-infra.

Re: [PATCH V10 6/9] mfd: pm8008: Add mfd_cell struct to register LDOs

[Stephen Boyd]

Quoting Satya Priya (2022-04-14 05:30:15)
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index ca5240d..ab4ba55 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -282,6 +287,14 @@ static int pm8008_probe(struct i2c_client *client)
>         }
>         gpiod_set_value(chip->reset_gpio, 1);
>
> +       rc = devm_mfd_add_devices(&chip->clients[PM8008_REGULATORS_SID]->dev,
> +                       0, pm8008_regulator_devs, ARRAY_SIZE(pm8008_regulator_devs),
> +                       NULL, 0, NULL);
> +       if (rc) {
> +               dev_err(chip->dev, "Failed to add regulators: %d\n", rc);
> +               return rc;
> +       }
> +
>         return devm_of_platform_populate(chip->dev);

Can we just use devm_of_platform_populate()? Then have a compatible =
"qcom,pm8008-regulators" that binds with the regulator platform driver
and searches for the named regmap for the second i2c client.

Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

[Stephen Boyd]

Quoting Satya Priya (2022-04-14 05:30:16)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..4375ac5
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
> +
[..]
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +       struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent);
> +       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 = pm8008_get_regmap(chip, PM8008_REGULATORS_SID);

Can we use dev_get_regmap(pdev->dev.parent, "regulators") here
instead? The benefit to that is we don't tightly couple this driver to
the pm8008_data structure or header file.

> +       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;
> +               pm8008_reg->voltage_selector = -ENOTRECOVERABLE;
> +               pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
> +
> +               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-regulator",

I'd prefer to use an of_device_id table here. That would let us populate
a "qcom,pm8008-regulators" node that had the ldo nodes as children and
avoid mfd cells.

Re: [PATCH V10 8/9] arm64: dts: qcom: pm8008: Add base dts file

[Stephen Boyd]

Quoting Satya Priya (2022-04-14 05:30:17)
> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> new file mode 100644
> index 0000000..7b4fe68
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +// Copyright (c) 2022, The Linux Foundation. All rights reserved.
> +
> +&pm8008_bus {
> +       pm8008: pmic@8 {
> +               compatible = "qcom,pm8008";
> +               reg = <0x8>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               #interrupt-cells = <2>;
> +
> +               pm8008_regulators: regulators {

Container nodes without a compatible string are frowned upon. How about
we add

			compatible = "qcom,pm8008-regulators"

and then this can populated by the driver in the
devm_of_platform_populate() call that's already there.


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

Re: [PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Satya Priya Kakitapalli (Temp)]

On 4/15/2022 5:40 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:13)
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index c472d7f..97a72da 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client *client)
>>                          dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>>          }
>>
>> +       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");
> The API looks to print debug messages. This print doesn't look required.


Okay.


>> +               return PTR_ERR(chip->reset_gpio);
>> +       }
>> +       gpiod_set_value(chip->reset_gpio, 1);
> Does this do anything? Does this work just as well?
>
> 	reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> 	if (IS_ERR(reset_gpio))
> 		return PTR_ERR(reset_gpio);
>
> Note that there's no point to store the reset gpio in the structure if
> it won't be used outside of probe.


Okay, I'll use a local variable.


> This should work fine? I used
> GPIOD_OUT_LOW to indicate that the reset should be returned to this
> function deasserted, i.e. taking the PMIC out of reset.


I'll try this out.

Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Satya Priya Kakitapalli (Temp)]

On 4/15/2022 5:51 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:14)
>> Use i2c_new_dummy_device() to register clients along with
>> the main mfd device.
> Why?


As discussed on V9 of this series, I've done these changes.

By using this API we can register other clients at different address 
space without having separate DT node.

This avoids calling the probe twice for the same chip, once for each 
address space 0x8 and 0x9. I'll add this description in commit text.


>> Signed-off-by: Satya Priya<quic_c_skakit@quicinc.com>
>> ---
>> Changes in V10:
>>   - Implement i2c_new_dummy_device to register extra clients.
>>
>>   drivers/mfd/qcom-pm8008.c       | 54 +++++++++++++++++++++++++++++++++--------
>>   include/linux/mfd/qcom_pm8008.h | 13 ++++++++++
>>   2 files changed, 57 insertions(+), 10 deletions(-)
>>   create mode 100644 include/linux/mfd/qcom_pm8008.h
>>
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index 97a72da..ca5240d 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -56,8 +57,10 @@ enum {
>>   #define PM8008_PERIPH_OFFSET(paddr)    (paddr - PM8008_PERIPH_0_BASE)
>>
>>   struct pm8008_data {
>> +       bool ready;
>>          struct device *dev;
>> -       struct regmap *regmap;
>> +       struct i2c_client *clients[PM8008_NUM_CLIENTS];
>> +       struct regmap *regmap[PM8008_NUM_CLIENTS];
>>          struct gpio_desc *reset_gpio;
>>          int irq;
>>          struct regmap_irq_chip_data *irq_data;
>> @@ -152,9 +155,20 @@ static struct regmap_config qcom_mfd_regmap_cfg = {
>>          .max_register   = 0xFFFF,
>>   };
>>
>> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid)
>> +{
>> +       if (!chip || !chip->ready) {
> Is it even possible?


I think no, I'll remove it.


>> +               pr_err("pm8008 chip not initialized\n");
>> +               return NULL;
>> +       }
>> +
>> +       return chip->regmap[sid];
>> +}
>> +
>>   static int pm8008_init(struct pm8008_data *chip)
>>   {
>>          int rc;
>> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
>>
>>          /*
>>           * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework
>> @@ -162,19 +176,19 @@ static int pm8008_init(struct pm8008_data *chip)
>>           * This is required to enable the writing of TYPE registers in
>>           * regmap_irq_sync_unlock().
>>           */
>> -       rc = regmap_write(chip->regmap,
>> +       rc = regmap_write(regmap,
>>                           (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET),
>>                           BIT(0));
>>          if (rc)
>>                  return rc;
>>
>>          /* Do the same for GPIO1 and GPIO2 peripherals */
>> -       rc = regmap_write(chip->regmap,
>> +       rc = regmap_write(regmap,
>>                           (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>          if (rc)
>>                  return rc;
>>
>> -       rc = regmap_write(chip->regmap,
>> +       rc = regmap_write(regmap,
>>                           (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0));
>>
>>          return rc;
>> @@ -186,6 +200,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>          int rc, i;
>>          struct regmap_irq_type *type;
>>          struct regmap_irq_chip_data *irq_data;
>> +       struct regmap *regmap = pm8008_get_regmap(chip, PM8008_INFRA_SID);
> Instead of calling pm8008_get_regmap() many times why not pass the
> regmap through from pm8008_probe_irq_peripherals() called in probe? At
> that point we could remove the regmap pointer from struct pm8008_data?


Okay.


>>          rc = pm8008_init(chip);
>>          if (rc) {
>> @@ -209,7 +224,7 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>                                  IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW);
>>          }
>>
>> -       rc = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client_irq,
>> +       rc = devm_regmap_add_irq_chip(chip->dev, regmap, client_irq,
>>                          IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);
>>          if (rc) {
>>                  dev_err(chip->dev, "Failed to add IRQ chip: %d\n", rc);
>> @@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>
>>   static int pm8008_probe(struct i2c_client *client)
>>   {
>> -       int rc;
>> +       int rc, i;
>>          struct pm8008_data *chip;
>> +       struct device_node *node = client->dev.of_node;
>>
>>          chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>          if (!chip)
>>                  return -ENOMEM;
>>
>>          chip->dev = &client->dev;
>> -       chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>> -       if (!chip->regmap)
>> -               return -ENODEV;
>>
>> -       i2c_set_clientdata(client, chip);
>> +       for (i = 0; i < PM8008_NUM_CLIENTS; i++) {
> This is 2. Why do we have a loop? Just register the i2c client for
> pm8008_infra first and then make a dummy for the second address without
> the loop and the indentation. Are there going to be more i2c clients?


There wont be more than 2 clients, I can remove the loop, but then we 
will have repetitive code.. something like below

      chip->dev = &client->dev;
      i2c_set_clientdata(client, chip);

      regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
      if (!regmap)
          return -ENODEV;


      pm8008_client = i2c_new_dummy_device(client->adapter,
                              client->addr + 1);
      if (IS_ERR(pm8008_client)) {
          dev_err(&pm8008_client->dev, "can't attach client\n");
          return PTR_ERR(pm8008_client);
      }
      pm8008_client->dev.of_node = of_node_get(client->dev.of_node);
      i2c_set_clientdata(pm8008_client, chip);

      regulators_regmap = devm_regmap_init_i2c(pm8008_client, 
&qcom_mfd_regmap_cfg[1]);
      if (!regmap)
          return -ENODEV;

>> +               if (i == 0) {
>> +                       chip->clients[i] = client;
>> +               } else {
>> +                       chip->clients[i] = i2c_new_dummy_device(client->adapter,
>> +                                                               client->addr + i);
>> +                       if (IS_ERR(chip->clients[i])) {
>> +                               dev_err(&client->dev, "can't attach client %d\n", i);
>> +                               return PTR_ERR(chip->clients[i]);
>> +                       }
>> +                       chip->clients[i]->dev.of_node = of_node_get(node);
>> +               }
>> +
>> +               chip->regmap[i] = devm_regmap_init_i2c(chip->clients[i],
>> +                                                       &qcom_mfd_regmap_cfg);
>> +               if (!chip->regmap[i])
>> +                       return -ENODEV;
>> +
>> +               i2c_set_clientdata(chip->clients[i], chip);
>> +       }
>> +
>> +       chip->ready = true;
>>
>>          if (of_property_read_bool(chip->dev->of_node, "interrupt-controller")) {
>>                  rc = pm8008_probe_irq_peripherals(chip, client->irq);
>> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
>> new file mode 100644
>> index 0000000..bc64f01
>> --- /dev/null
>> +++ b/include/linux/mfd/qcom_pm8008.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __QCOM_PM8008_H__
>> +#define __QCOM_PM8008_H__
>> +
>> +#define PM8008_INFRA_SID       0
>> +#define PM8008_REGULATORS_SID  1
>> +
>> +#define PM8008_NUM_CLIENTS     2
>> +
>> +struct pm8008_data;
>> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);
> Could this be avoided if the regulator driver used
> dev_get_regmap(&pdev->dev.parent, "regulator") to find the regmap named
> "regulator" of the parent device, i.e. pm8008-infra.

I gave it a try, it didn't work. I could not get the regmap for 
regulators using pm8008-infra i.e., 0x8 device pointer.

I checked the other drivers using i2c_new_dummy_device(), they are 
either getting the regmap by accessing the mfd struct through global 
variables or using headers.

Re: [PATCH V10 6/9] mfd: pm8008: Add mfd_cell struct to register LDOs

[Satya Priya Kakitapalli (Temp)]

On 4/15/2022 5:53 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:15)
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index ca5240d..ab4ba55 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -282,6 +287,14 @@ static int pm8008_probe(struct i2c_client *client)
>>          }
>>          gpiod_set_value(chip->reset_gpio, 1);
>>
>> +       rc = devm_mfd_add_devices(&chip->clients[PM8008_REGULATORS_SID]->dev,
>> +                       0, pm8008_regulator_devs, ARRAY_SIZE(pm8008_regulator_devs),
>> +                       NULL, 0, NULL);
>> +       if (rc) {
>> +               dev_err(chip->dev, "Failed to add regulators: %d\n", rc);
>> +               return rc;
>> +       }
>> +
>>          return devm_of_platform_populate(chip->dev);
> Can we just use devm_of_platform_populate()? Then have a compatible =
> "qcom,pm8008-regulators" that binds with the regulator platform driver
> and searches for the named regmap for the second i2c client.


On earlier versions we had a separate compatible for regulator driver, I 
had to drop it as per Mark's suggestions here [1], [2].


If Mark is OK with adding a separate compatible, I can add it back.


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

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

Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Stephen Boyd]

Quoting Satya Priya Kakitapalli (Temp) (2022-04-18 08:08:33)
>
> On 4/15/2022 5:51 AM, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-04-14 05:30:14)
> >> Use i2c_new_dummy_device() to register clients along with
> >> the main mfd device.
> > Why?
>
>
> As discussed on V9 of this series, I've done these changes.
>
> By using this API we can register other clients at different address
> space without having separate DT node.
>
> This avoids calling the probe twice for the same chip, once for each
> address space 0x8 and 0x9. I'll add this description in commit text.
>

Perfect, thanks.

>
> >> @@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
> >>
> >>   static int pm8008_probe(struct i2c_client *client)
> >>   {
> >> -       int rc;
> >> +       int rc, i;
> >>          struct pm8008_data *chip;
> >> +       struct device_node *node = client->dev.of_node;
> >>
> >>          chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> >>          if (!chip)
> >>                  return -ENOMEM;
> >>
> >>          chip->dev = &client->dev;
> >> -       chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
> >> -       if (!chip->regmap)
> >> -               return -ENODEV;
> >>
> >> -       i2c_set_clientdata(client, chip);
> >> +       for (i = 0; i < PM8008_NUM_CLIENTS; i++) {
> > This is 2. Why do we have a loop? Just register the i2c client for
> > pm8008_infra first and then make a dummy for the second address without
> > the loop and the indentation. Are there going to be more i2c clients?
>
>
> There wont be more than 2 clients, I can remove the loop, but then we
> will have repetitive code.. something like below

Repetitive code can be refactored into a subroutine.

>
>       chip->dev = &client->dev;
>       i2c_set_clientdata(client, chip);
>
>       regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
>       if (!regmap)
>           return -ENODEV;
>
>
>       pm8008_client = i2c_new_dummy_device(client->adapter,
>                               client->addr + 1);
>       if (IS_ERR(pm8008_client)) {
>           dev_err(&pm8008_client->dev, "can't attach client\n");
>           return PTR_ERR(pm8008_client);
>       }
>       pm8008_client->dev.of_node = of_node_get(client->dev.of_node);
>       i2c_set_clientdata(pm8008_client, chip);
>
>       regulators_regmap = devm_regmap_init_i2c(pm8008_client,
> &qcom_mfd_regmap_cfg[1]);
>       if (!regmap)
>           return -ENODEV;
>
> >> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
> >> new file mode 100644
> >> index 0000000..bc64f01
> >> --- /dev/null
> >> +++ b/include/linux/mfd/qcom_pm8008.h
> >> @@ -0,0 +1,13 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +#ifndef __QCOM_PM8008_H__
> >> +#define __QCOM_PM8008_H__
> >> +
> >> +#define PM8008_INFRA_SID       0
> >> +#define PM8008_REGULATORS_SID  1
> >> +
> >> +#define PM8008_NUM_CLIENTS     2
> >> +
> >> +struct pm8008_data;
> >> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);
> > Could this be avoided if the regulator driver used
> > dev_get_regmap(&pdev->dev.parent, "regulator") to find the regmap named
> > "regulator" of the parent device, i.e. pm8008-infra.
>
> I gave it a try, it didn't work. I could not get the regmap for
> regulators using pm8008-infra i.e., 0x8 device pointer.

Did it not work because the regmap config for the regulator config
didn't have a name?

Re: [PATCH V10 5/9] mfd: pm8008: Use i2c_new_dummy_device() API

[Satya Priya Kakitapalli (Temp)]

On 4/19/2022 12:53 AM, Stephen Boyd wrote:
> Quoting Satya Priya Kakitapalli (Temp) (2022-04-18 08:08:33)
>> On 4/15/2022 5:51 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-04-14 05:30:14)
>>>> Use i2c_new_dummy_device() to register clients along with
>>>> the main mfd device.
>>> Why?
>>
>> As discussed on V9 of this series, I've done these changes.
>>
>> By using this API we can register other clients at different address
>> space without having separate DT node.
>>
>> This avoids calling the probe twice for the same chip, once for each
>> address space 0x8 and 0x9. I'll add this description in commit text.
>>
> Perfect, thanks.
>
>>>> @@ -221,19 +236,38 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip,
>>>>
>>>>    static int pm8008_probe(struct i2c_client *client)
>>>>    {
>>>> -       int rc;
>>>> +       int rc, i;
>>>>           struct pm8008_data *chip;
>>>> +       struct device_node *node = client->dev.of_node;
>>>>
>>>>           chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>>>           if (!chip)
>>>>                   return -ENOMEM;
>>>>
>>>>           chip->dev = &client->dev;
>>>> -       chip->regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg);
>>>> -       if (!chip->regmap)
>>>> -               return -ENODEV;
>>>>
>>>> -       i2c_set_clientdata(client, chip);
>>>> +       for (i = 0; i < PM8008_NUM_CLIENTS; i++) {
>>> This is 2. Why do we have a loop? Just register the i2c client for
>>> pm8008_infra first and then make a dummy for the second address without
>>> the loop and the indentation. Are there going to be more i2c clients?
>>
>> There wont be more than 2 clients, I can remove the loop, but then we
>> will have repetitive code.. something like below
> Repetitive code can be refactored into a subroutine.
>
>>        chip->dev = &client->dev;
>>        i2c_set_clientdata(client, chip);
>>
>>        regmap = devm_regmap_init_i2c(client, &qcom_mfd_regmap_cfg[0]);
>>        if (!regmap)
>>            return -ENODEV;
>>
>>
>>        pm8008_client = i2c_new_dummy_device(client->adapter,
>>                                client->addr + 1);
>>        if (IS_ERR(pm8008_client)) {
>>            dev_err(&pm8008_client->dev, "can't attach client\n");
>>            return PTR_ERR(pm8008_client);
>>        }
>>        pm8008_client->dev.of_node = of_node_get(client->dev.of_node);
>>        i2c_set_clientdata(pm8008_client, chip);
>>
>>        regulators_regmap = devm_regmap_init_i2c(pm8008_client,
>> &qcom_mfd_regmap_cfg[1]);
>>        if (!regmap)
>>            return -ENODEV;
>>
>>>> diff --git a/include/linux/mfd/qcom_pm8008.h b/include/linux/mfd/qcom_pm8008.h
>>>> new file mode 100644
>>>> index 0000000..bc64f01
>>>> --- /dev/null
>>>> +++ b/include/linux/mfd/qcom_pm8008.h
>>>> @@ -0,0 +1,13 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef __QCOM_PM8008_H__
>>>> +#define __QCOM_PM8008_H__
>>>> +
>>>> +#define PM8008_INFRA_SID       0
>>>> +#define PM8008_REGULATORS_SID  1
>>>> +
>>>> +#define PM8008_NUM_CLIENTS     2
>>>> +
>>>> +struct pm8008_data;
>>>> +struct regmap *pm8008_get_regmap(struct pm8008_data *chip, u8 sid);
>>> Could this be avoided if the regulator driver used
>>> dev_get_regmap(&pdev->dev.parent, "regulator") to find the regmap named
>>> "regulator" of the parent device, i.e. pm8008-infra.
>> I gave it a try, it didn't work. I could not get the regmap for
>> regulators using pm8008-infra i.e., 0x8 device pointer.
> Did it not work because the regmap config for the regulator config
> didn't have a name?


No I specified the name "regulators" in the regmap_config struct for 
regulator config.

Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

[Mark Brown]

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

On Thu, Apr 14, 2022 at 05:25:49PM -0700, Stephen Boyd wrote:
> Quoting Satya Priya (2022-04-14 05:30:16)

> > +static struct platform_driver pm8008_regulator_driver = {
> > +       .driver = {
> > +               .name           = "qcom-pm8008-regulator",

> I'd prefer to use an of_device_id table here. That would let us populate
> a "qcom,pm8008-regulators" node that had the ldo nodes as children and
> avoid mfd cells.

That's encoding the current Linux way of splitting up drivers into the
DT rather than describing the hardware.

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

Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

[Stephen Boyd]

Quoting Mark Brown (2022-04-19 07:55:43)
> On Thu, Apr 14, 2022 at 05:25:49PM -0700, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-04-14 05:30:16)
>
> > > +static struct platform_driver pm8008_regulator_driver = {
> > > +       .driver = {
> > > +               .name           = "qcom-pm8008-regulator",
>
> > I'd prefer to use an of_device_id table here. That would let us populate
> > a "qcom,pm8008-regulators" node that had the ldo nodes as children and
> > avoid mfd cells.
>
> That's encoding the current Linux way of splitting up drivers into the
> DT rather than describing the hardware.

The DT binding has a subnode of the pm8008@8 node for 'regulators'.
There's also a subnode for gpios (see qcom,pm8008-gpio). The gpio node
has a reg property, so I'm confused how we can even have the regulators
container node at the same level as the gpio node with a reg property.
Every node that's a child of pm8008@8 either needs to have a reg
property or not.

What benefit does it have to not describe secondary i2c addresses as
nodes in DT? I think it's necessary because the reset gpio needs to be
deasserted before i2c read/write to either address, 8 or 9, will work.
Otherwise I don't understand. Having the reset puts us into a small bind
though because child nodes sometimes have a reg property and other times
don't.

This is the current example

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <1>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;
	    gpios {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0xc000>;
	      ...

	    };

	    regulators {
	      vdd_l1_l2-supply = <&vreg_s8b_1p2>;
	
	      ldo1 {
	        regulator-name = "pm8008_l1";
	      };
	      ldo2 {
	        regulator-name = "pm8008_l2";
	      };
	    };
	  };
	};

What should the final result be? Dropping the regulators node would end
up with the same problem where ldo1 has no reg property. Adding a reg
property to ldo1 might work, but it would be a register offset inside
i2c address 9 while the binding makes it look like a register offset
within 9. The binding for the container node could get two address
cells, so that the first cell describes the i2c address offset?

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <2>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;

	    vdd_l1_l2-supply = <&vreg_s8b_1p2>;

	    gpios@0,c000 {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0x0 0xc000>;
	      ...

	    };

	    ldo1@1,30 {
	      compatible = "qcom,pm8008-regulator";
	      reg = <0x1 0x30>;
	      regulator-name = "pm8008_l1";
	    };
	    ldo2@1,40 {
	      compatible = "qcom,pm8008-regulator";
	      reg = <0x1 0x40>;
	      regulator-name = "pm8008_l2";
	    };
	  };
	};

We don't make a node for each gpio so I don't know why we would make a
node for each regulator. The above could be changed to have the
regulators node and a reg property like this

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <2>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;

	    gpios@0,c000 {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0x0 0xc000>;
	      ...

	    };

	    regulators@1,0 {
	      compatible = "qcom,pm8008-regulators";
	      vdd_l1_l2-supply = <&vreg_s8b_1p2>;

	      reg = <0x1 0x0>;
	      ldo1 {
	      regulator-name = "pm8008_l1";
	      };
	      ldo2 {
	        regulator-name = "pm8008_l2";
	      };
	    };
	  };
	};

I wonder if there's a mapping table property like i2c-ranges or
something like that which could be used to map the i2c dummy to the
first reg property. That would be super awesome so that when the
platform bus is populated we could match up the regmap for the i2c
device to the platform device automatically.

By the way, Is there any document on "best practices" for i2c devicetree
bindings?  We should add details to the document to describe this
situation so this can be conveyed faster next time.

Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

[Mark Brown]

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

On Tue, Apr 19, 2022 at 08:45:47AM -0700, Stephen Boyd wrote:
> Quoting Mark Brown (2022-04-19 07:55:43)

> > > > +static struct platform_driver pm8008_regulator_driver = {
> > > > +       .driver = {
> > > > +               .name           = "qcom-pm8008-regulator",

> > > I'd prefer to use an of_device_id table here. That would let us populate
> > > a "qcom,pm8008-regulators" node that had the ldo nodes as children and
> > > avoid mfd cells.

> > That's encoding the current Linux way of splitting up drivers into the
> > DT rather than describing the hardware.

> The DT binding has a subnode of the pm8008@8 node for 'regulators'.
> There's also a subnode for gpios (see qcom,pm8008-gpio). The gpio node
> has a reg property, so I'm confused how we can even have the regulators
> container node at the same level as the gpio node with a reg property.
> Every node that's a child of pm8008@8 either needs to have a reg
> property or not.

That seems unfortunate if it's a limitation of DT :/

> What benefit does it have to not describe secondary i2c addresses as
> nodes in DT? I think it's necessary because the reset gpio needs to be

This is a platform device, not an I2C device.  Converting it to an I2C
device and describing the second I2C address as a separate device would
be much less of a confusion here (and I suspect may well reflect the
underlying physical implementation).  I'm mostly concerned about these
platform devices.

The other option would be to describe each regulator the device supports
as a separate IP which does have some hope of being reusable since it
reflects the actual IPs here.

> deasserted before i2c read/write to either address, 8 or 9, will work.
> Otherwise I don't understand. Having the reset puts us into a small bind
> though because child nodes sometimes have a reg property and other times
> don't.

Right, that's an issue and does tie the two I2C addresses of the device
into a single thing.

> What should the final result be? Dropping the regulators node would end
> up with the same problem where ldo1 has no reg property. Adding a reg
> property to ldo1 might work, but it would be a register offset inside
> i2c address 9 while the binding makes it look like a register offset
> within 9. The binding for the container node could get two address
> cells, so that the first cell describes the i2c address offset?

Yeah, I think we want reg properties on the LDOs.  

> 	i2c {
> 	  pm8008@8 {
> 	    compatible = "qcom,pm8008";
> 	    #address-cells = <2>;
> 	    #size-cells = <0>;
> 	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;
> 
> 	    vdd_l1_l2-supply = <&vreg_s8b_1p2>;
> 
> 	    gpios@0,c000 {
> 	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
> 	      reg = <0x0 0xc000>;
> 	      ...
> 
> 	    };
> 
> 	    ldo1@1,30 {

This looks sensible to me.

> We don't make a node for each gpio so I don't know why we would make a
> node for each regulator. The above could be changed to have the
> regulators node and a reg property like this

GPIOs tend to be one IP block with one set of registers that provides a
bank of GPIOs.  Regulators tend to be a separate IP block for each
individual regulator, typically repeated multiple times within a single
chip.  A binding that describes the regulators within the device should
generally be describing these individual regulator IPs rather than just
saying "there are some regulators" which doesn't add any information or
promote reuse with other PMICs sharing the same IP.

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

Re: [PATCH V10 2/9] dt-bindings: regulator: pm8008: Add pm8008 regulator bindings

[Rob Herring]

On Thu, 14 Apr 2022 18:00:11 +0530, Satya Priya wrote:
> Add bindings for pm8008 regulators.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V8:
>  - This is split from pm8008.yaml binding.
> 
> Changes in V9:
>   - Remove description for reg and drop unused phandle from example.
> 
> Changes in V10:
>  - Regulators are added as a part of pm8008@8 device. Change bindings doc
>    accordingly.
> 
>  .../bindings/regulator/qcom,pm8008-regulators.yaml | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulators.yaml
> 

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

Re: [PATCH V10 3/9] dt-bindings: mfd: pm8008: Add regulators

[Rob Herring]

On Thu, 14 Apr 2022 18:00:12 +0530, Satya Priya wrote:
> Add regulators for pm8008 with example.
> 
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V10:
>  - Add regulators under main mfd node as in DT.
> 
>  Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

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

Re: [PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Satya Priya Kakitapalli (Temp)]

On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
>
> On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
>>
>> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-04-14 05:30:13)
>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>> index c472d7f..97a72da 100644
>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>> @@ -239,6 +241,13 @@ static int pm8008_probe(struct i2c_client 
>>>> *client)
>>>>                          dev_err(chip->dev, "Failed to probe irq 
>>>> periphs: %d\n", rc);
>>>>          }
>>>>
>>>> +       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");
>>> The API looks to print debug messages. This print doesn't look 
>>> required.
>>
>>
>> Okay.
>>
>>
>>>> +               return PTR_ERR(chip->reset_gpio);
>>>> +       }
>>>> +       gpiod_set_value(chip->reset_gpio, 1);
>>> Does this do anything? Does this work just as well?
>>>
>>>     reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>>>     if (IS_ERR(reset_gpio))
>>>         return PTR_ERR(reset_gpio);
>>>
>
> This is not working as expected. We need to add 
> "gpiod_set_value(chip->reset_gpio, 1);"  to actually toggle the line.
>

I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.

reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
     if (IS_ERR(reset_gpio))
         return PTR_ERR(reset_gpio);


>
>>> Note that there's no point to store the reset gpio in the structure if
>>> it won't be used outside of probe.
>>
>>
>> Okay, I'll use a local variable.
>>
>>
>>> This should work fine? I used
>>> GPIOD_OUT_LOW to indicate that the reset should be returned to this
>>> function deasserted, i.e. taking the PMIC out of reset.
>>
>>
>> I'll try this out.
>>
>>

Re: [PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Stephen Boyd]

Quoting Satya Priya Kakitapalli (Temp) (2022-04-26 23:03:08)
>
> On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
> >
> > On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
> >>
> >> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
> >>> Quoting Satya Priya (2022-04-14 05:30:13)
> >>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> >>>> index c472d7f..97a72da 100644
> >>>> --- a/drivers/mfd/qcom-pm8008.c
> >>>> +++ b/drivers/mfd/qcom-pm8008.c
> >>
> >>>> +               return PTR_ERR(chip->reset_gpio);
> >>>> +       }
> >>>> +       gpiod_set_value(chip->reset_gpio, 1);
> >>> Does this do anything? Does this work just as well?
> >>>
> >>>     reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
> >>>     if (IS_ERR(reset_gpio))
> >>>         return PTR_ERR(reset_gpio);
> >>>
> >
> > This is not working as expected. We need to add
> > "gpiod_set_value(chip->reset_gpio, 1);"  to actually toggle the line.
> >
>
> I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.
>
> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
>      if (IS_ERR(reset_gpio))
>          return PTR_ERR(reset_gpio);
>

What do you have in DT for the 'reset-gpios' property? GPIOD_OUT_HIGH
means the reset line is asserted, which presumably you don't want to be
the case because you typically deassert a reset to "take it out of
reset". Using GPIOD_OUT_HIGH probably works because DT has the wrong
GPIO flag, i.e.  GPIO_ACTIVE_HIGH, for an active low reset, or vice
versa.

Re: [PATCH V10 4/9] mfd: pm8008: Add reset-gpios

[Satya Priya Kakitapalli (Temp)]

On 4/27/2022 12:00 PM, Stephen Boyd wrote:
> Quoting Satya Priya Kakitapalli (Temp) (2022-04-26 23:03:08)
>> On 4/27/2022 10:58 AM, Satya Priya Kakitapalli (Temp) wrote:
>>> On 4/18/2022 10:34 AM, Satya Priya Kakitapalli (Temp) wrote:
>>>> On 4/15/2022 5:40 AM, Stephen Boyd wrote:
>>>>> Quoting Satya Priya (2022-04-14 05:30:13)
>>>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>>>> index c472d7f..97a72da 100644
>>>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>>>> +               return PTR_ERR(chip->reset_gpio);
>>>>>> +       }
>>>>>> +       gpiod_set_value(chip->reset_gpio, 1);
>>>>> Does this do anything? Does this work just as well?
>>>>>
>>>>>      reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_LOW);
>>>>>      if (IS_ERR(reset_gpio))
>>>>>          return PTR_ERR(reset_gpio);
>>>>>
>>> This is not working as expected. We need to add
>>> "gpiod_set_value(chip->reset_gpio, 1);"  to actually toggle the line.
>>>
>> I checked again and it is working after using GPIOD_OUT_HIGH instead of LOW.
>>
>> reset_gpio = devm_gpiod_get(chip->dev, "reset", GPIOD_OUT_HIGH);
>>       if (IS_ERR(reset_gpio))
>>           return PTR_ERR(reset_gpio);
>>
> What do you have in DT for the 'reset-gpios' property? GPIOD_OUT_HIGH
> means the reset line is asserted, which presumably you don't want to be
> the case because you typically deassert a reset to "take it out of
> reset". Using GPIOD_OUT_HIGH probably works because DT has the wrong
> GPIO flag, i.e.  GPIO_ACTIVE_HIGH, for an active low reset, or vice
> versa.


Yeah, I had GPIOD_OUT_HIGH in DT, now I changed and it is working fine 
with GPIOD_OUT_LOW.