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

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

[Satya Priya]

Satya Priya (2):
  dt-bindings: mfd: pm8008: Add pm8008-regulator binding
  regulator: dt-bindings: Add pm8008 regulator bindings

satya priya (2):
  regulator: Add a regulator driver for the PM8008 PMIC
  arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp

 .../devicetree/bindings/mfd/qcom,pm8008.yaml       |  24 ++
 .../bindings/regulator/qcom,pm8008-regulator.yaml  |  76 ++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           | 103 +++++
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-pm8008-regulator.c          | 441 +++++++++++++++++++++
 6 files changed, 654 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[Satya Priya]

Add pm8008-regulator node and example.

Signed-off-by: Satya Priya <skakit@codeaurora.org>
---
 .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..de182f8 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -45,6 +45,10 @@ properties:
     const: 0
 
 patternProperties:
+  "^pm8008[a-z]?-regulator$":
+    type: object
+    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
+
   "^gpio@[0-9a-f]+$":
     type: object
 
@@ -122,6 +126,26 @@ examples:
           interrupt-controller;
           #interrupt-cells = <2>;
         };
+
+        pm8008-regulator {
+          compatible = "qcom,pm8008-regulator";
+          #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: regulator@4000 {
+            reg = <0x4000>;
+            regulator-name = "pm8008_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+            qcom,min-dropout-voltage = <96000>;
+          };
+        };
       };
     };
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

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

[Satya Priya]

Add bindings for pm8008 pmic regulators.

Signed-off-by: Satya Priya <skakit@codeaurora.org>
---
 .../bindings/regulator/qcom,pm8008-regulator.yaml  | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
new file mode 100644
index 0000000..59518c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom,pm8008-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. PM8008 Regulator bindings
+
+maintainers:
+  - Satya Priya <skakit@codeaurora.org>
+
+description:
+  Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
+  containing 7 LDO regulators.
+
+properties:
+  compatible:
+    const: qcom,pm8008-regulator
+
+  "#address-cells":
+    const: 1
+
+  "#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:
+  "^regulator@[0-9a-f]+$":
+    type: object
+
+    $ref: "regulator.yaml#"
+
+    description: PM8008 regulator peripherals of PM8008 regulator device
+
+    properties:
+      reg:
+        maxItems: 1
+      regulator-name: true
+      regulator-min-microvolt: true
+      regulator-max-microvolt: true
+      qcom,min-dropout-voltage:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description:
+          Specifies the minimum voltage in microvolts that the parent
+          supply regulator must output, above the output of this
+          regulator.
+
+    required:
+      - reg
+      - regulator-name
+      - regulator-min-microvolt
+      - regulator-max-microvolt
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+...
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

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

[Satya Priya]

From: satya priya <skakit@codeaurora.org>

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 <skakit@codeaurora.org>
---
 drivers/regulator/Kconfig                 |   9 +
 drivers/regulator/Makefile                |   1 +
 drivers/regulator/qcom-pm8008-regulator.c | 441 ++++++++++++++++++++++++++++++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index e35cca5..888ce30 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -916,6 +916,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 9e382b5..5e935ff 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -100,6 +100,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..fdc2f16
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,441 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#define STARTUP_DELAY_USEC		20
+#define VSET_STEP_MV			8
+#define VSET_STEP_UV			(VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base)		(base + 0x46)
+#define ENABLE_BIT			BIT(7)
+
+#define LDO_STATUS1_REG(base)		(base + 0x08)
+#define VREG_READY_BIT			BIT(7)
+
+#define LDO_VSET_LB_REG(base)		(base + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base)	(base + 0x3b)
+#define STEP_RATE_MASK			GENMASK(1, 0)
+
+#define PM8008_MAX_LDO			7
+
+struct regulator_data {
+	char		*name;
+	char		*supply_name;
+	int		min_uv;
+	int		max_uv;
+	int		min_dropout_uv;
+};
+
+struct pm8008_regulator {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct regulator_desc	rdesc;
+	struct regulator_dev	*rdev;
+	struct device_node	*of_node;
+	u16			base;
+	int			step_rate;
+};
+
+static const struct regulator_data reg_data[PM8008_MAX_LDO] = {
+	/* name  parent      min_uv  max_uv  headroom_uv */
+	{"l1", "vdd_l1_l2",  528000, 1504000, 225000},
+	{"l2", "vdd_l1_l2",  528000, 1504000, 225000},
+	{"l3", "vdd_l3_l4", 1504000, 3400000, 200000},
+	{"l4", "vdd_l3_l4", 1504000, 3400000, 200000},
+	{"l5", "vdd_l5",    1504000, 3400000, 300000},
+	{"l6", "vdd_l6",    1504000, 3400000, 300000},
+	{"l7", "vdd_l7",    1504000, 3400000, 300000},
+};
+
+static int pm8008_read(struct regmap *regmap,  u16 reg, u8 *val, int count)
+{
+	int rc;
+
+	rc = regmap_bulk_read(regmap, reg, val, count);
+	if (rc < 0)
+		pr_err("failed to read %#x, rc=%d\n", reg, rc);
+
+	return rc;
+}
+
+static int pm8008_write(struct regmap *regmap, u16 reg, u8 *val, int count)
+{
+	int rc;
+
+	pr_debug("Writing [%*ph] from address %#x\n", count, val, reg);
+	rc = regmap_bulk_write(regmap, reg, val, count);
+	if (rc < 0)
+		pr_err("failed to write %#x rc=%d\n", reg, rc);
+
+	return rc;
+}
+
+static int pm8008_masked_write(struct regmap *regmap, u16 reg, u8 mask,
+				u8 val)
+{
+	int rc;
+
+	pr_debug("Writing %#x to %#x with mask %#x\n", val, reg, mask);
+	rc = regmap_update_bits(regmap, reg, mask, val);
+	if (rc < 0)
+		pr_err("failed to write %#x to %#x with mask %#x rc=%d\n",
+				val, reg, mask, rc);
+
+	return rc;
+}
+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	u8 vset_raw[2];
+	int rc;
+
+	rc = pm8008_read(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base),
+			vset_raw, 2);
+	if (rc < 0) {
+		pr_err("failed to read regulator voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
+}
+
+static int pm8008_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc;
+	u8 reg;
+
+	rc = pm8008_read(pm8008_reg->regmap,
+			LDO_ENABLE_REG(pm8008_reg->base), &reg, 1);
+	if (rc < 0) {
+		pr_err("failed to read enable reg rc=%d\n", rc);
+		return rc;
+	}
+
+	return !!(reg & ENABLE_BIT);
+}
+
+static int pm8008_regulator_enable(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc, current_uv, delay_us, delay_ms, retry_count = 10;
+	u8 reg;
+
+	current_uv = pm8008_regulator_get_voltage(rdev);
+	if (current_uv < 0) {
+		pr_err("failed to get current voltage rc=%d\n", current_uv);
+		return current_uv;
+	}
+
+	rc = pm8008_masked_write(pm8008_reg->regmap,
+				LDO_ENABLE_REG(pm8008_reg->base),
+				ENABLE_BIT, ENABLE_BIT);
+	if (rc < 0) {
+		pr_err("failed to enable regulator rc=%d\n", rc);
+		return rc;
+	}
+
+	/*
+	 * Wait for the VREG_READY status bit to be set using a timeout delay
+	 * calculated from the current commanded voltage.
+	 */
+	delay_us = STARTUP_DELAY_USEC
+			+ DIV_ROUND_UP(current_uv, pm8008_reg->step_rate);
+	delay_ms = DIV_ROUND_UP(delay_us, 1000);
+
+	/* Retry 10 times for VREG_READY before bailing out */
+	while (retry_count--) {
+		if (delay_ms > 20)
+			msleep(delay_ms);
+		else
+			usleep_range(delay_us, delay_us + 100);
+
+		rc = pm8008_read(pm8008_reg->regmap,
+				LDO_STATUS1_REG(pm8008_reg->base), &reg, 1);
+		if (rc < 0) {
+			pr_err("failed to read regulator status rc=%d\n", rc);
+			goto disable_ldo;
+		}
+		if (reg & VREG_READY_BIT) {
+			pr_debug("regulator enabled\n");
+			return 0;
+		}
+	}
+
+	pr_err("failed to enable regulator, VREG_READY not set\n");
+	rc = -ETIME;
+
+disable_ldo:
+	pm8008_masked_write(pm8008_reg->regmap,
+			LDO_ENABLE_REG(pm8008_reg->base), ENABLE_BIT, 0);
+
+	return rc;
+}
+
+static int pm8008_regulator_disable(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc;
+
+	rc = pm8008_masked_write(pm8008_reg->regmap,
+				LDO_ENABLE_REG(pm8008_reg->base),
+				ENABLE_BIT, 0);
+	if (rc < 0) {
+		pr_err("failed to disable regulator rc=%d\n", rc);
+		return rc;
+	}
+
+	pr_debug("regulator disabled\n");
+	return 0;
+}
+
+static int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg, int min_uv,
+				int max_uv)
+{
+	int rc = 0, mv;
+	u8 vset_raw[2];
+
+	mv = DIV_ROUND_UP(min_uv, 1000);
+
+	/*
+	 * Each LSB of regulator is 1mV and the voltage setpoint
+	 * should be multiple of 8mV(step).
+	 */
+	mv = DIV_ROUND_UP(mv, VSET_STEP_MV) * VSET_STEP_MV;
+	if (mv * 1000 > max_uv) {
+		pr_err("requested voltage (%d uV) above maximum limit (%d uV)\n",
+				mv*1000, max_uv);
+		return -EINVAL;
+	}
+
+	vset_raw[0] = mv & 0xff;
+	vset_raw[1] = (mv & 0xff00) >> 8;
+	rc = pm8008_write(pm8008_reg->regmap, LDO_VSET_LB_REG(pm8008_reg->base),
+			vset_raw, 2);
+	if (rc < 0) {
+		pr_err("failed to write voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+				int old_uV, int new_uv)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+	return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+				int min_uv, int max_uv, unsigned int *selector)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc;
+
+	rc = pm8008_write_voltage(pm8008_reg, min_uv, max_uv);
+	if (rc < 0)
+		return rc;
+
+	*selector = DIV_ROUND_UP(min_uv - pm8008_reg->rdesc.min_uV,
+				VSET_STEP_UV);
+
+	pr_debug("voltage set to %d\n", min_uv);
+	return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+	.enable		= pm8008_regulator_enable,
+	.disable		= pm8008_regulator_disable,
+	.is_enabled		= pm8008_regulator_is_enabled,
+	.set_voltage		= 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_register_ldo(struct pm8008_regulator *pm8008_reg,
+						const char *name)
+{
+	struct regulator_config reg_config = {};
+	struct regulator_init_data *init_data;
+	struct device *dev = pm8008_reg->dev;
+	struct device_node *reg_node = pm8008_reg->of_node;
+	int rc, i, init_voltage;
+	u32 base = 0;
+	u8 reg;
+
+	/* get regulator data */
+	for (i = 0; i < PM8008_MAX_LDO; i++)
+		if (strstr(name, reg_data[i].name))
+			break;
+
+	if (i == PM8008_MAX_LDO) {
+		dev_err(dev, "Invalid regulator name %s\n", name);
+		return -EINVAL;
+	}
+
+	rc = of_property_read_u32(reg_node, "reg", &base);
+	if (rc < 0) {
+		dev_err(dev, "%s: failed to get regulator base rc=%d\n", name, rc);
+		return rc;
+	}
+	pm8008_reg->base = base;
+
+	init_voltage = -EINVAL;
+	of_property_read_u32(reg_node, "qcom,init-voltage", &init_voltage);
+
+	/* get slew rate */
+	rc = pm8008_read(pm8008_reg->regmap,
+			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
+	if (rc < 0) {
+		dev_err(dev, "%s: failed to read step rate configuration rc=%d\n",
+				name, rc);
+		return rc;
+	}
+	pm8008_reg->step_rate = 38400 >> (reg & STEP_RATE_MASK);
+
+	init_data = of_get_regulator_init_data(dev, reg_node,
+						&pm8008_reg->rdesc);
+	if (init_data == NULL) {
+		dev_err(dev, "%s: failed to get regulator data\n", name);
+		return -ENODATA;
+	}
+	if (!init_data->constraints.name) {
+		dev_err(dev, "%s: regulator name missing\n", name);
+		return -EINVAL;
+	}
+
+	/* configure the initial voltage for the regulator */
+	if (init_voltage > 0) {
+		rc = pm8008_write_voltage(pm8008_reg, init_voltage,
+					init_data->constraints.max_uV);
+		if (rc < 0)
+			dev_err(dev, "%s: failed to set initial voltage rc=%d\n",
+					name, rc);
+	}
+
+	init_data->constraints.input_uV = init_data->constraints.max_uV;
+	init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS
+						| REGULATOR_CHANGE_VOLTAGE;
+	reg_config.dev = dev;
+	reg_config.init_data = init_data;
+	reg_config.driver_data = pm8008_reg;
+	reg_config.of_node = reg_node;
+
+	pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+	pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+	pm8008_reg->rdesc.name = init_data->constraints.name;
+	pm8008_reg->rdesc.supply_name = reg_data[i].supply_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.min_dropout_uV = reg_data[i].min_dropout_uv;
+	of_property_read_u32(reg_node, "qcom,min-dropout-voltage",
+			     &pm8008_reg->rdesc.min_dropout_uV);
+
+	pm8008_reg->rdev = devm_regulator_register(dev, &pm8008_reg->rdesc,
+						&reg_config);
+	if (IS_ERR(pm8008_reg->rdev)) {
+		rc = PTR_ERR(pm8008_reg->rdev);
+		dev_err(dev, "%s: failed to register regulator rc=%d\n",
+				pm8008_reg->rdesc.name, rc);
+		return rc;
+	}
+
+	pr_debug("%s regulator registered\n", name);
+
+	return 0;
+}
+
+static int pm8008_parse_regulator(struct regmap *regmap, struct device *dev)
+{
+	int rc = 0;
+	const char *name;
+	struct device_node *child;
+	struct pm8008_regulator *pm8008_reg;
+
+	/* parse each subnode and register regulator for regulator child */
+	for_each_available_child_of_node(dev->of_node, child) {
+		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+		if (!pm8008_reg)
+			return -ENOMEM;
+
+		pm8008_reg->regmap = regmap;
+		pm8008_reg->of_node = child;
+		pm8008_reg->dev = dev;
+
+		rc = of_property_read_string(child, "regulator-name", &name);
+		if (rc)
+			continue;
+
+		rc = pm8008_register_ldo(pm8008_reg, name);
+		if (rc < 0) {
+			dev_err(dev, "failed to register regulator %s rc=%d\n",
+					name, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+	int rc = 0;
+	struct regmap *regmap;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap) {
+		dev_err(&pdev->dev, "parent regmap is missing\n");
+		return -EINVAL;
+	}
+
+	rc = pm8008_parse_regulator(regmap, &pdev->dev);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "failed to parse device tree rc=%d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id pm8008_regulator_match_table[] = {
+	{ .compatible = "qcom,pm8008-regulator", },
+	{ },
+};
+
+static struct platform_driver pm8008_regulator_driver = {
+	.driver	= {
+		.name		= "qcom,pm8008-regulator",
+		.of_match_table	= pm8008_regulator_match_table,
+	},
+	.probe		= pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL v2");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 4/4] arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp

[Satya Priya]

From: satya priya <skakit@codeaurora.org>

Add pm8008 regulators support for sc7280 idp.

Signed-off-by: satya priya <skakit@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 103 +++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 52638e2..3b3af49 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -207,6 +207,97 @@
 	};
 };
 
+&i2c1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	pm8008_chip: pm8008@8 {
+		compatible = "qcom,pm8008";
+		reg = <0x8>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pm8008_active>;
+	};
+
+	pm8008_ldo: pm8008@9 {
+		compatible = "qcom,pm8008";
+		reg = <0x9>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pm8008-regulator {
+			compatible = "qcom,pm8008-regulator";
+			#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: regulator@4000 {
+				reg = <0x4000>;
+				regulator-name = "pm8008_l1";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1300000>;
+				qcom,min-dropout-voltage = <96000>;
+			};
+
+			pm8008_l2: regulator@4100 {
+				reg = <0x4100>;
+				regulator-name = "pm8008_l2";
+				regulator-min-microvolt = <950000>;
+				regulator-max-microvolt = <1250000>;
+				qcom,min-dropout-voltage = <24000>;
+			};
+
+			pm8008_l3: regulator@4200 {
+				reg = <0x4200>;
+				regulator-name = "pm8008_l3";
+				regulator-min-microvolt = <1650000>;
+				regulator-max-microvolt = <3000000>;
+				qcom,min-dropout-voltage = <224000>;
+			};
+
+			pm8008_l4: regulator@4300 {
+				reg = <0x4300>;
+				regulator-name = "pm8008_l4";
+				regulator-min-microvolt = <1504000>;
+				regulator-max-microvolt = <1600000>;
+				qcom,min-dropout-voltage = <0>;
+			};
+
+			pm8008_l5: regulator@4400 {
+				reg = <0x4400>;
+				regulator-name = "pm8008_l5";
+				regulator-min-microvolt = <2600000>;
+				regulator-max-microvolt = <3000000>;
+				qcom,min-dropout-voltage = <104000>;
+			};
+
+			pm8008_l6: regulator@4500 {
+				reg = <0x4500>;
+				regulator-name = "pm8008_l6";
+				regulator-min-microvolt = <2600000>;
+				regulator-max-microvolt = <3000000>;
+				qcom,min-dropout-voltage = <112000>;
+			};
+
+			pm8008_l7: regulator@4600 {
+				reg = <0x4600>;
+				regulator-name = "pm8008_l7";
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3544000>;
+				qcom,min-dropout-voltage = <96000>;
+			};
+		};
+	};
+};
+
 &qfprom {
 	vcc-supply = <&vreg_l1c_1p8>;
 };
@@ -313,6 +404,18 @@
 
 /* PINCTRL - additions to nodes defined in sc7280.dtsi */
 
+&pm8350c_gpios {
+	pm8008_reset {
+		pm8008_active: pm8008_active {
+			pins = "gpio4";
+			function = "normal";
+			bias-disable;
+			output-high;
+			power-source = <0>;
+		};
+	};
+};
+
 &qspi_cs0 {
 	bias-disable;
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver

[skakit]

On 2021-09-17 16:15, Satya Priya wrote:
> Satya Priya (2):
>   dt-bindings: mfd: pm8008: Add pm8008-regulator binding
>   regulator: dt-bindings: Add pm8008 regulator bindings
> 
> satya priya (2):
>   regulator: Add a regulator driver for the PM8008 PMIC
>   arm64: dts: qcom: sc7280: Add pm8008 regulators support for 
> sc7280-idp

This dt change depends on [1] which adds i2c1 DT node
[1] 
https://patchwork.kernel.org/project/linux-arm-msm/patch/1631872087-24416-5-git-send-email-rajpat@codeaurora.org/

>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       |  24 ++
>  .../bindings/regulator/qcom,pm8008-regulator.yaml  |  76 ++++
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           | 103 +++++
>  drivers/regulator/Kconfig                          |   9 +
>  drivers/regulator/Makefile                         |   1 +
>  drivers/regulator/qcom-pm8008-regulator.c          | 441 
> +++++++++++++++++++++
>  6 files changed, 654 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>  create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

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

[Mark Brown]

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

On Fri, Sep 17, 2021 at 04:15:37PM +0530, Satya Priya wrote:

> +static int pm8008_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc;
> +	u8 reg;
> +
> +	rc = pm8008_read(pm8008_reg->regmap,
> +			LDO_ENABLE_REG(pm8008_reg->base), &reg, 1);
> +	if (rc < 0) {
> +		pr_err("failed to read enable reg rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	return !!(reg & ENABLE_BIT);
> +}

This could just be regulator_is_enabled_regmap().  There's also a lot of
instances in the driver where it's using pr_err() not dev_err() (and
similarly for the debug prints).

> +
> +static int pm8008_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc, current_uv, delay_us, delay_ms, retry_count = 10;
> +	u8 reg;

This is the regmap helper.

> +	/*
> +	 * Wait for the VREG_READY status bit to be set using a timeout delay
> +	 * calculated from the current commanded voltage.
> +	 */
> +	delay_us = STARTUP_DELAY_USEC
> +			+ DIV_ROUND_UP(current_uv, pm8008_reg->step_rate);
> +	delay_ms = DIV_ROUND_UP(delay_us, 1000);

Set poll_enable_time and implement get_status() then this will be
handled by the core.

> +static int pm8008_regulator_disable(struct regulator_dev *rdev)
> +{

Use the regmap helper.

> +	rc = pm8008_write_voltage(pm8008_reg, min_uv, max_uv);
> +	if (rc < 0)
> +		return rc;

This is the only place where write_voltage() is called, may as well just
inline it.

> +	init_voltage = -EINVAL;
> +	of_property_read_u32(reg_node, "qcom,init-voltage", &init_voltage);

Why does this property exist and if it's needed why is it specific to
this device?  It looks like the device allows you to read the voltage on
startup from the regmap.

> +	init_data = of_get_regulator_init_data(dev, reg_node,
> +						&pm8008_reg->rdesc);
> +	if (init_data == NULL) {
> +		dev_err(dev, "%s: failed to get regulator data\n", name);
> +		return -ENODATA;
> +	}
> +	if (!init_data->constraints.name) {
> +		dev_err(dev, "%s: regulator name missing\n", name);
> +		return -EINVAL;
> +	}

Just let the core find the init data for you, there is no reason to
insist on a system provided name - that is an entirely optional property
for systems to use, there is no reason for a regulator driver to care.

> +	init_data->constraints.input_uV = init_data->constraints.max_uV;
> +	init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS
> +						| REGULATOR_CHANGE_VOLTAGE;

This is absolutely not something that a regulator driver should be
doing, the whole point with constraints is that they come from the
machine.

> +static int pm8008_parse_regulator(struct regmap *regmap, struct device *dev)
> +{
> +	int rc = 0;
> +	const char *name;
> +	struct device_node *child;
> +	struct pm8008_regulator *pm8008_reg;
> +
> +	/* parse each subnode and register regulator for regulator child */
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +		if (!pm8008_reg)

You shouldn't be doing this, just unconditionally register all the
regulators supported by the chip.  If they don't appear in the DT that's
totally fine - it gives read only access which can be useful for
diagnostics.

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

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

[Mark Brown]

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

On Fri, Sep 17, 2021 at 04:15:36PM +0530, Satya Priya wrote:

> +    properties:
> +      reg:
> +        maxItems: 1
> +      regulator-name: true
> +      regulator-min-microvolt: true
> +      regulator-max-microvolt: true

You shouldn't be forcing these properties, it should be perfectly OK for
boards to have fixed voltages especially for example during bringup or
for debugging.

> +      qcom,min-dropout-voltage:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description:
> +          Specifies the minimum voltage in microvolts that the parent
> +          supply regulator must output, above the output of this
> +          regulator.

If this is needed in DT it should be a generic property since most
regulators have some requirement here however usually it's a fixed
property of the silicon and should therefore just gets set in the
regulator_desc as min_dropout_uV - I'd strongly recommend having a
default there even if there's some requirement for it to be set per
board.

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

Re: [PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[Rob Herring]

On Fri, 17 Sep 2021 16:15:35 +0530, Satya Priya wrote:
> Add pm8008-regulator node and example.
> 
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/qcom,pm8008-regulator.yaml'
xargs: dt-doc-validate: exited with status 255; aborting
make[1]: *** Deleting file 'Documentation/devicetree/bindings/mfd/qcom,pm8008.example.dt.yaml'
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/qcom,pm8008-regulator.yaml'
make[1]: *** [scripts/Makefile.lib:385: Documentation/devicetree/bindings/mfd/qcom,pm8008.example.dt.yaml] Error 255
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1441: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1529335

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[Stephen Boyd]

Quoting Satya Priya (2021-09-17 03:45:35)
> Add pm8008-regulator node and example.
>
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index ec3138c..de182f8 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -45,6 +45,10 @@ properties:
>      const: 0
>
>  patternProperties:
> +  "^pm8008[a-z]?-regulator$":

Shouldn't it be ^pm8008-regulator$ without the a-z optional letter?

> +    type: object
> +    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
> +
>    "^gpio@[0-9a-f]+$":
>      type: object
>

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

[Stephen Boyd]

Quoting Satya Priya (2021-09-17 03:45:38)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index 52638e2..3b3af49 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -207,6 +207,97 @@
>         };
>  };
>
> +&i2c1 {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       status = "okay";
> +
> +       pm8008_chip: pm8008@8 {
> +               compatible = "qcom,pm8008";
> +               reg = <0x8>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pm8008_active>;
> +       };
> +
> +       pm8008_ldo: pm8008@9 {
[...]
> +
> +                       pm8008_l7: regulator@4600 {
> +                               reg = <0x4600>;
> +                               regulator-name = "pm8008_l7";
> +                               regulator-min-microvolt = <3000000>;
> +                               regulator-max-microvolt = <3544000>;
> +                               qcom,min-dropout-voltage = <96000>;

Is this headroom? Is it actually configurable or is it merely a property
of the hardware? If it's the latter then it should be in the driver and
not in the DTS.

> +                       };
> +               };
> +       };
> +};
> +
>  &qfprom {
>         vcc-supply = <&vreg_l1c_1p8>;
>  };
> @@ -313,6 +404,18 @@
>
>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>
> +&pm8350c_gpios {
> +       pm8008_reset {

Is this a pinctrl node?

> +               pm8008_active: pm8008_active {

Please use dashes in node names wherever an underscore goes.

> +                       pins = "gpio4";
> +                       function = "normal";
> +                       bias-disable;
> +                       output-high;
> +                       power-source = <0>;
> +               };
> +       };
> +};

Re: [PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[Rob Herring]

On Fri, Sep 17, 2021 at 04:15:35PM +0530, Satya Priya wrote:
> Add pm8008-regulator node and example.
> 
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index ec3138c..de182f8 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> @@ -45,6 +45,10 @@ properties:
>      const: 0
>  
>  patternProperties:
> +  "^pm8008[a-z]?-regulator$":

Is more than 1 node possible for a given PMIC? If not use 'regulators' 
for the node name.

> +    type: object
> +    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
> +
>    "^gpio@[0-9a-f]+$":
>      type: object
>  
> @@ -122,6 +126,26 @@ examples:
>            interrupt-controller;
>            #interrupt-cells = <2>;
>          };
> +
> +        pm8008-regulator {
> +          compatible = "qcom,pm8008-regulator";
> +          #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: regulator@4000 {
> +            reg = <0x4000>;
> +            regulator-name = "pm8008_l1";
> +            regulator-min-microvolt = <950000>;
> +            regulator-max-microvolt = <1300000>;
> +            qcom,min-dropout-voltage = <96000>;
> +          };
> +        };
>        };
>      };
>  
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> of Code Aurora Forum, hosted by The Linux Foundation
> 
> 

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

[skakit]

Hi Mark,

Thanks for reviewing the changes!

On 2021-09-17 21:08, Mark Brown wrote:
> On Fri, Sep 17, 2021 at 04:15:37PM +0530, Satya Priya wrote:
> 
>> +static int pm8008_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
>> +	int rc;
>> +	u8 reg;
>> +
>> +	rc = pm8008_read(pm8008_reg->regmap,
>> +			LDO_ENABLE_REG(pm8008_reg->base), &reg, 1);
>> +	if (rc < 0) {
>> +		pr_err("failed to read enable reg rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	return !!(reg & ENABLE_BIT);
>> +}
> 
> This could just be regulator_is_enabled_regmap().  There's also a lot 
> of
> instances in the driver where it's using pr_err() not dev_err() (and
> similarly for the debug prints).
> 

Okay, I'll use the helper regulator_is_enabled_regmap() here and remove 
this completely.

>> +
>> +static int pm8008_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
>> +	int rc, current_uv, delay_us, delay_ms, retry_count = 10;
>> +	u8 reg;
> 
> This is the regmap helper.
> 

Okay, I'll use regulator_enable_regmap().

>> +	/*
>> +	 * Wait for the VREG_READY status bit to be set using a timeout 
>> delay
>> +	 * calculated from the current commanded voltage.
>> +	 */
>> +	delay_us = STARTUP_DELAY_USEC
>> +			+ DIV_ROUND_UP(current_uv, pm8008_reg->step_rate);
>> +	delay_ms = DIV_ROUND_UP(delay_us, 1000);
> 
> Set poll_enable_time and implement get_status() then this will be
> handled by the core.
> 

Anyway I will be removing this API.

>> +static int pm8008_regulator_disable(struct regulator_dev *rdev)
>> +{
> 
> Use the regmap helper.
> 

Ok, I'll use regulator_disable_regmap.

>> +	rc = pm8008_write_voltage(pm8008_reg, min_uv, max_uv);
>> +	if (rc < 0)
>> +		return rc;
> 
> This is the only place where write_voltage() is called, may as well 
> just
> inline it.
> 

Okay.

>> +	init_voltage = -EINVAL;
>> +	of_property_read_u32(reg_node, "qcom,init-voltage", &init_voltage);
> 
> Why does this property exist and if it's needed why is it specific to
> this device?  It looks like the device allows you to read the voltage 
> on
> startup from the regmap.
> 

I think it is not necessary, will remove it.

>> +	init_data = of_get_regulator_init_data(dev, reg_node,
>> +						&pm8008_reg->rdesc);
>> +	if (init_data == NULL) {
>> +		dev_err(dev, "%s: failed to get regulator data\n", name);
>> +		return -ENODATA;
>> +	}
>> +	if (!init_data->constraints.name) {
>> +		dev_err(dev, "%s: regulator name missing\n", name);
>> +		return -EINVAL;
>> +	}
> 
> Just let the core find the init data for you, there is no reason to
> insist on a system provided name - that is an entirely optional 
> property
> for systems to use, there is no reason for a regulator driver to care.
> 

OK, I will remove this if check.

>> +	init_data->constraints.input_uV = init_data->constraints.max_uV;
>> +	init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS
>> +						| REGULATOR_CHANGE_VOLTAGE;
> 
> This is absolutely not something that a regulator driver should be
> doing, the whole point with constraints is that they come from the
> machine.
> 

Okay I will remove this.

>> +static int pm8008_parse_regulator(struct regmap *regmap, struct 
>> device *dev)
>> +{
>> +	int rc = 0;
>> +	const char *name;
>> +	struct device_node *child;
>> +	struct pm8008_regulator *pm8008_reg;
>> +
>> +	/* parse each subnode and register regulator for regulator child */
>> +	for_each_available_child_of_node(dev->of_node, child) {
>> +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
>> +		if (!pm8008_reg)
> 
> You shouldn't be doing this, just unconditionally register all the
> regulators supported by the chip.  If they don't appear in the DT 
> that's
> totally fine - it gives read only access which can be useful for
> diagnostics.

Okay will remove this check as well.

-Satya Priya.

Re: [PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[skakit]

On 2021-09-21 01:02, Stephen Boyd wrote:
> Quoting Satya Priya (2021-09-17 03:45:35)
>> Add pm8008-regulator node and example.
>> 
>> Signed-off-by: Satya Priya <skakit@codeaurora.org>
>> ---
>>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 
>> ++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml 
>> b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> index ec3138c..de182f8 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> @@ -45,6 +45,10 @@ properties:
>>      const: 0
>> 
>>  patternProperties:
>> +  "^pm8008[a-z]?-regulator$":
> 
> Shouldn't it be ^pm8008-regulator$ without the a-z optional letter?
> 

Some platforms use 2 PM8008 PMICS, in that case we need suffixing like 
pm8008i pm8008j etc. So, I mentioned this way.

>> +    type: object
>> +    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
>> +
>>    "^gpio@[0-9a-f]+$":
>>      type: object
>> 

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

[skakit]

On 2021-09-21 01:07, Stephen Boyd wrote:
> Quoting Satya Priya (2021-09-17 03:45:38)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index 52638e2..3b3af49 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -207,6 +207,97 @@
>>         };
>>  };
>> 
>> +&i2c1 {
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       status = "okay";
>> +
>> +       pm8008_chip: pm8008@8 {
>> +               compatible = "qcom,pm8008";
>> +               reg = <0x8>;
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&pm8008_active>;
>> +       };
>> +
>> +       pm8008_ldo: pm8008@9 {
> [...]
>> +
>> +                       pm8008_l7: regulator@4600 {
>> +                               reg = <0x4600>;
>> +                               regulator-name = "pm8008_l7";
>> +                               regulator-min-microvolt = <3000000>;
>> +                               regulator-max-microvolt = <3544000>;
>> +                               qcom,min-dropout-voltage = <96000>;
> 
> Is this headroom? Is it actually configurable or is it merely a 
> property
> of the hardware? If it's the latter then it should be in the driver and
> not in the DTS.
> 

Yes this is a headroom and its configurable.

>> +                       };
>> +               };
>> +       };
>> +};
>> +
>>  &qfprom {
>>         vcc-supply = <&vreg_l1c_1p8>;
>>  };
>> @@ -313,6 +404,18 @@
>> 
>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> 
>> +&pm8350c_gpios {
>> +       pm8008_reset {
> 
> Is this a pinctrl node?
> 

No.

>> +               pm8008_active: pm8008_active {
> 
> Please use dashes in node names wherever an underscore goes.
> 

Okay.

>> +                       pins = "gpio4";
>> +                       function = "normal";
>> +                       bias-disable;
>> +                       output-high;
>> +                       power-source = <0>;
>> +               };
>> +       };
>> +};

Re: [PATCH 1/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[skakit]

On 2021-09-21 02:10, Rob Herring wrote:
> On Fri, Sep 17, 2021 at 04:15:35PM +0530, Satya Priya wrote:
>> Add pm8008-regulator node and example.
>> 
>> Signed-off-by: Satya Priya <skakit@codeaurora.org>
>> ---
>>  .../devicetree/bindings/mfd/qcom,pm8008.yaml       | 24 
>> ++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml 
>> b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> index ec3138c..de182f8 100644
>> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> @@ -45,6 +45,10 @@ properties:
>>      const: 0
>> 
>>  patternProperties:
>> +  "^pm8008[a-z]?-regulator$":
> 
> Is more than 1 node possible for a given PMIC? If not use 'regulators'
> for the node name.
> 

Not possible, will change the node name.

>> +    type: object
>> +    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
>> +
>>    "^gpio@[0-9a-f]+$":
>>      type: object
>> 
>> @@ -122,6 +126,26 @@ examples:
>>            interrupt-controller;
>>            #interrupt-cells = <2>;
>>          };
>> +
>> +        pm8008-regulator {
>> +          compatible = "qcom,pm8008-regulator";
>> +          #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: regulator@4000 {
>> +            reg = <0x4000>;
>> +            regulator-name = "pm8008_l1";
>> +            regulator-min-microvolt = <950000>;
>> +            regulator-max-microvolt = <1300000>;
>> +            qcom,min-dropout-voltage = <96000>;
>> +          };
>> +        };
>>        };
>>      };
>> 
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>> 
>> 

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

[skakit]

On 2021-09-17 21:18, Mark Brown wrote:
> On Fri, Sep 17, 2021 at 04:15:36PM +0530, Satya Priya wrote:
> 
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +      regulator-name: true
>> +      regulator-min-microvolt: true
>> +      regulator-max-microvolt: true
> 
> You shouldn't be forcing these properties, it should be perfectly OK 
> for
> boards to have fixed voltages especially for example during bringup or
> for debugging.
> 

Okay. I will remove these.

>> +      qcom,min-dropout-voltage:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description:
>> +          Specifies the minimum voltage in microvolts that the parent
>> +          supply regulator must output, above the output of this
>> +          regulator.
> 
> If this is needed in DT it should be a generic property since most
> regulators have some requirement here however usually it's a fixed
> property of the silicon and should therefore just gets set in the
> regulator_desc as min_dropout_uV - I'd strongly recommend having a
> default there even if there's some requirement for it to be set per
> board.

Yeah, we are setting the default values for this(headroom_uv) from 
driver. Please see below

struct regulator_data {
      char        *name;
      char        *supply_name;
      int     min_uv;
      int     max_uv;
      int     min_dropout_uv;
};

static const struct regulator_data reg_data[PM8008_MAX_LDO] = {
      /* name  parent      min_uv  max_uv  headroom_uv */
     {"l1", "vdd_l1_l2",  528000, 1504000, 225000},
     {"l2", "vdd_l1_l2",  528000, 1504000, 225000},
     {"l3", "vdd_l3_l4", 1504000, 3400000, 200000},
     {"l4", "vdd_l3_l4", 1504000, 3400000, 200000},
     {"l5", "vdd_l5",    1504000, 3400000, 300000},
     {"l6", "vdd_l6",    1504000, 3400000, 300000},
     {"l7", "vdd_l7",    1504000, 3400000, 300000},
};

Inside Register LDO API:

pm8008_reg->rdesc.min_dropout_uV = reg_data[i].min_dropout_uv;
of_property_read_u32(reg_node, "qcom,min-dropout-voltage",
                   &pm8008_reg->rdesc.min_dropout_uV);