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

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

[Satya Priya]

Satya Priya (6):
  dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"
  dt-bindings: regulator: Add pm8008 regulator bindings
  dt-bindings: mfd: pm8008: Add pm8008 regulator node
  regulator: Add a regulator driver for the PM8008 PMIC
  arm64: dts: qcom: pm8008: Add base dts file
  arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp

 .../devicetree/bindings/mfd/qcom,pm8008.yaml       |  24 ++
 .../bindings/regulator/qcom,pm8008-regulator.yaml  |  68 ++++++
 .../devicetree/bindings/regulator/regulator.yaml   |   4 +
 arch/arm64/boot/dts/qcom/pm8008.dtsi               |  57 +++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi           |  73 ++++++
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-pm8008-regulator.c          | 258 +++++++++++++++++++++
 8 files changed, 494 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
 create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
 create mode 100644 drivers/regulator/qcom-pm8008-regulator.c

-- 
2.7.4

[PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Satya Priya]

Add "regulator-min-dropout-voltage-microvolt" property.

Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V4:
 - This is newly added in V4.

 Documentation/devicetree/bindings/regulator/regulator.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.yaml b/Documentation/devicetree/bindings/regulator/regulator.yaml
index a6ae9ec..fdb37d3 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/regulator.yaml
@@ -224,6 +224,10 @@ properties:
     description: Maximum difference between current and target voltages
       that can be changed safely in a single step.
 
+  regulator-min-dropout-voltage-microvolt:
+    description: Specifies the minimum voltage in microvolts that the
+      parent supply regulator must output, above the output of this regulator.
+
 patternProperties:
   ".*-supply$":
     description: Input supply phandle(s) for this node
-- 
2.7.4

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

[Satya Priya]

Add bindings for pm8008 pmic regulators.

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

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

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

 .../bindings/regulator/qcom,pm8008-regulator.yaml  | 68 ++++++++++++++++++++++
 1 file changed, 68 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..38ee970
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
@@ -0,0 +1,68 @@
+# 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-regulators
+
+  "#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:
+  "^l[1-7]@[0-9a-f]+$":
+    type: object
+
+    $ref: "regulator.yaml#"
+
+    description: PM8008 regulator peripherals of PM8008 regulator device
+
+    properties:
+      reg:
+        maxItems: 1
+        description: Base address of the regulator.
+
+      regulator-name: true
+
+    required:
+      - reg
+      - regulator-name
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+...
-- 
2.7.4

[PATCH V4 3/6] dt-bindings: mfd: pm8008: Add pm8008 regulator node

[Satya Priya]

Add pm8008-regulator node and example.

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

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

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

 .../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..ab6166d 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -44,6 +44,10 @@ properties:
   "#size-cells":
     const: 0
 
+  regulators:
+    type: object
+    $ref: "../regulator/qcom,pm8008-regulator.yaml#"
+
 patternProperties:
   "^gpio@[0-9a-f]+$":
     type: object
@@ -122,6 +126,26 @@ examples:
           interrupt-controller;
           #interrupt-cells = <2>;
         };
+
+        regulators {
+          compatible = "qcom,pm8008-regulators";
+          #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: l1@4000 {
+            reg = <0x4000>;
+            regulator-name = "pm8008_l1";
+            regulator-min-microvolt = <950000>;
+            regulator-max-microvolt = <1300000>;
+            regulator-min-dropout-voltage-microvolt = <96000>;
+          };
+        };
       };
     };
 
-- 
2.7.4

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

[Satya Priya]

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

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

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

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

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

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 6be9b1c..61e3e98 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 b07d2a2..4fac16b6 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..38d258e
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define STARTUP_DELAY_USEC		20
+#define VSET_STEP_MV			8
+#define VSET_STEP_UV			(VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base)		((base) + 0x46)
+#define ENABLE_BIT			BIT(7)
+
+#define LDO_STATUS1_REG(base)		((base) + 0x08)
+#define VREG_READY_BIT			BIT(7)
+
+#define LDO_VSET_LB_REG(base)		((base) + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base)	((base) + 0x3b)
+#define DEFAULT_VOLTAGE_STEPPER_RATE	38400
+#define STEP_RATE_MASK			GENMASK(1, 0)
+
+struct regulator_data {
+	const char			*name;
+	const char			*supply_name;
+	int				min_uv;
+	int				max_uv;
+	int				min_dropout_uv;
+	const struct linear_range	*voltage_range;
+};
+
+struct pm8008_regulator {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct regulator_desc	rdesc;
+	u16			base;
+	int			step_rate;
+};
+
+static const struct linear_range nldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
+};
+
+static const struct linear_range pldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
+};
+
+static const struct regulator_data reg_data[] = {
+	/* name  parent      min_uv  max_uv  headroom_uv */
+	{ "l1", "vdd_l1_l2",  528000, 1504000, 225000, nldo_ranges },
+	{ "l2", "vdd_l1_l2",  528000, 1504000, 225000, nldo_ranges },
+	{ "l3", "vdd_l3_l4", 1504000, 3400000, 200000, pldo_ranges },
+	{ "l4", "vdd_l3_l4", 1504000, 3400000, 200000, pldo_ranges },
+	{ "l5", "vdd_l5",    1504000, 3400000, 300000, pldo_ranges },
+	{ "l6", "vdd_l6",    1504000, 3400000, 300000, pldo_ranges },
+	{ "l7", "vdd_l7",    1504000, 3400000, 300000, pldo_ranges },
+	{ }
+};
+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	__le16 mV;
+	int rc;
+
+	rc = regmap_bulk_read(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+	if (rc < 0) {
+		dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return le16_to_cpu(mV) * 1000;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+							int mV)
+{
+	int rc;
+	u16 vset_raw;
+
+	vset_raw = cpu_to_le16(mV);
+
+	rc = regmap_bulk_write(pm8008_reg->regmap,
+			LDO_VSET_LB_REG(pm8008_reg->base),
+			(const void *)&vset_raw, sizeof(vset_raw));
+	if (rc < 0) {
+		dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+				int old_uV, int new_uv)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+	return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+					unsigned int selector)
+{
+	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+	int rc, mV;
+
+	/* voltage control register is set with voltage in millivolts */
+	mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
+						1000);
+	if (mV < 0)
+		return mV;
+
+	rc = pm8008_write_voltage(pm8008_reg, mV);
+	if (rc < 0)
+		return rc;
+
+	dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
+	return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.set_voltage_sel	= pm8008_regulator_set_voltage,
+	.get_voltage		= pm8008_regulator_get_voltage,
+	.list_voltage		= regulator_list_voltage_linear,
+	.set_voltage_time	= pm8008_regulator_set_voltage_time,
+};
+
+static int pm8008_regulator_of_parse(struct device_node *node,
+			const struct regulator_desc *desc,
+			struct regulator_config *config)
+{
+	struct pm8008_regulator *pm8008_reg = config->driver_data;
+	struct device *dev = config->dev;
+	int rc;
+	unsigned int reg;
+
+	rc = of_property_read_u32(node, "regulator-min-dropout-voltage-microvolt",
+			&pm8008_reg->rdesc.min_dropout_uV);
+	if (rc) {
+		dev_err(dev, "failed to read min-dropout voltage rc=%d\n", rc);
+		return rc;
+	}
+
+	/* 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, "%s: failed to read step rate configuration rc=%d\n",
+				pm8008_reg->rdesc.name, rc);
+		return rc;
+	}
+	reg &= STEP_RATE_MASK;
+	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
+
+	return 0;
+}
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent_node = pdev->dev.of_node;
+	struct device_node *child_node;
+	struct regulator_dev    *rdev;
+	struct pm8008_regulator *pm8008_reg;
+	struct regmap *regmap;
+	struct regulator_config reg_config = {};
+	const struct regulator_data *reg;
+	int rc;
+	u32 base;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "parent regmap is missing\n");
+		return -EINVAL;
+	}
+
+	for (reg = &reg_data[0]; reg->name; reg++) {
+		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+
+		pm8008_reg->regmap = regmap;
+
+		child_node = of_get_child_by_name(parent_node, reg->name);
+		if (!child_node) {
+			dev_err(dev, "child node %s not found\n", reg->name);
+			return -ENODEV;
+		}
+
+		pm8008_reg->dev = dev;
+
+		rc = of_property_read_u32(child_node, "reg", &base);
+		if (rc < 0) {
+			dev_err(dev, "%s: failed to get regulator base rc=%d\n",
+						reg->name, rc);
+			return rc;
+		}
+		pm8008_reg->base = base;
+
+		pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+		pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+		pm8008_reg->rdesc.name = reg->name;
+		pm8008_reg->rdesc.supply_name = reg->supply_name;
+		pm8008_reg->rdesc.of_match = reg->name;
+		pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
+		pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
+		pm8008_reg->rdesc.min_uV = reg->min_uv;
+		pm8008_reg->rdesc.n_voltages
+			= ((reg->max_uv - reg->min_uv)
+				/ pm8008_reg->rdesc.uV_step) + 1;
+		pm8008_reg->rdesc.linear_ranges = reg->voltage_range;
+		pm8008_reg->rdesc.n_linear_ranges = 1;
+		pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(base);
+		pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+		pm8008_reg->rdesc.min_dropout_uV = reg->min_dropout_uv;
+
+		reg_config.dev = dev;
+		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->name, rc);
+			return rc;
+		}
+	}
+	return 0;
+}
+
+static const struct of_device_id pm8008_regulator_match_table[] = {
+	{ .compatible = "qcom,pm8008-regulators", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pm8008_regulator_match_table);
+
+static struct platform_driver pm8008_regulator_driver = {
+	.driver	= {
+		.name		= "qcom,pm8008-regulators",
+		.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");
-- 
2.7.4

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

[Satya Priya]

Add base DTS file for pm8008 with chip and ldo nodes.

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

 arch/arm64/boot/dts/qcom/pm8008.dtsi | 57 ++++++++++++++++++++++++++++++++++++
 1 file changed, 57 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..e23478c
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2021, The Linux Foundation. All rights reserved.
+
+pm8008_chip: pm8008@8 {
+	compatible = "qcom,pm8008";
+	reg = <0x8>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+};
+
+pm8008_ldo: pm8008@9 {
+	compatible = "qcom,pm8008";
+	reg = <0x9>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	pm8008_regulators: regulators {
+		compatible = "qcom,pm8008-regulators";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pm8008_l1: l1@4000 {
+			reg = <0x4000>;
+			regulator-name = "pm8008_l1";
+		};
+
+		pm8008_l2: l2@4100 {
+			reg = <0x4100>;
+			regulator-name = "pm8008_l2";
+		};
+
+		pm8008_l3: l3@4200 {
+			reg = <0x4200>;
+			regulator-name = "pm8008_l3";
+		};
+
+		pm8008_l4: l4@4300 {
+			reg = <0x4300>;
+			regulator-name = "pm8008_l4";
+		};
+
+		pm8008_l5: l5@4400 {
+			reg = <0x4400>;
+			regulator-name = "pm8008_l5";
+		};
+
+		pm8008_l6: l6@4500 {
+			reg = <0x4500>;
+			regulator-name = "pm8008_l6";
+		};
+
+		pm8008_l7: l7@4600 {
+			reg = <0x4600>;
+			regulator-name = "pm8008_l7";
+		};
+	};
+};
-- 
2.7.4

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

[Satya Priya]

Add pm8008 regulators support for sc7280 idp.

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

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

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

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

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index d623d71..f86368d 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -309,6 +309,69 @@
 	};
 };
 
+&i2c1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	#include "pm8008.dtsi"
+};
+
+&pm8008_chip {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pm8008_active>;
+};
+
+&pm8008_regulators {
+	vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+	vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+	vdd_l5-supply = <&vreg_bob>;
+	vdd_l6-supply = <&vreg_bob>;
+	vdd_l7-supply = <&vreg_bob>;
+};
+
+&pm8008_l1 {
+	regulator-min-microvolt = <950000>;
+	regulator-max-microvolt = <1300000>;
+	regulator-min-dropout-voltage-microvolt = <96000>;
+};
+
+&pm8008_l2 {
+	regulator-min-microvolt = <950000>;
+	regulator-max-microvolt = <1250000>;
+	regulator-min-dropout-voltage-microvolt = <24000>;
+};
+
+&pm8008_l3 {
+	regulator-min-microvolt = <1650000>;
+	regulator-max-microvolt = <3000000>;
+	regulator-min-dropout-voltage-microvolt = <224000>;
+};
+
+&pm8008_l4 {
+	regulator-min-microvolt = <1504000>;
+	regulator-max-microvolt = <1600000>;
+	regulator-min-dropout-voltage-microvolt = <0>;
+};
+
+&pm8008_l5 {
+	regulator-min-microvolt = <2600000>;
+	regulator-max-microvolt = <3000000>;
+	regulator-min-dropout-voltage-microvolt = <104000>;
+};
+
+&pm8008_l6 {
+	regulator-min-microvolt = <2600000>;
+	regulator-max-microvolt = <3000000>;
+	regulator-min-dropout-voltage-microvolt = <112000>;
+};
+
+&pm8008_l7 {
+	regulator-min-microvolt = <3000000>;
+	regulator-max-microvolt = <3544000>;
+	regulator-min-dropout-voltage-microvolt = <96000>;
+};
+
 &qfprom {
 	vcc-supply = <&vreg_l1c_1p8>;
 };
@@ -437,6 +500,16 @@
 	};
 };
 
+&pm8350c_gpios {
+	pm8008_active: pm8008_active {
+		pins = "gpio4";
+		function = "normal";
+		bias-disable;
+		output-high;
+		power-source = <0>;
+	};
+};
+
 &qspi_cs0 {
 	bias-disable;
 };
-- 
2.7.4

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Mark Brown]

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

On Fri, Nov 19, 2021 at 03:12:28PM +0530, Satya Priya wrote:

> +  regulator-min-dropout-voltage-microvolt:
> +    description: Specifies the minimum voltage in microvolts that the
> +      parent supply regulator must output, above the output of this regulator.

Usually this is a fixed property of the regulator rather than something
that varies per board - why have a DT property?

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

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

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

[Mark Brown]

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

On Fri, Nov 19, 2021 at 03:12:29PM +0530, Satya Priya wrote:

> +properties:
> +  compatible:
> +    const: qcom,pm8008-regulators

Why are we adding a separate compatible for this when we already know
that this is a pm8008 based on the parent?

> +  vdd_l1_l2-supply:
> +    description: Input supply phandle of ldo1 and ldo2 regulators.

These supply nodes should be chip level, they're going into the chip and
in general the expectation is that you should be able to describe the
supplies going into a device without worrying about how or if any
particular OS splits things up.

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

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

[Mark Brown]

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

On Fri, Nov 19, 2021 at 03:12:31PM +0530, Satya Priya wrote:

> +	for (reg = &reg_data[0]; reg->name; reg++) {

Why is this not just iterating from 0 to ARRAY_SIZE() - that's the more
normal way to write things and doesn't require a terminator on the
array.

> +		child_node = of_get_child_by_name(parent_node, reg->name);
> +		if (!child_node) {
> +			dev_err(dev, "child node %s not found\n", reg->name);
> +			return -ENODEV;
> +		}

This could be pulled out of the array.  I think you're also missing an
of_node_put() on the child_node.

> +		rc = of_property_read_u32(child_node, "reg", &base);
> +		if (rc < 0) {
> +			dev_err(dev, "%s: failed to get regulator base rc=%d\n",
> +						reg->name, rc);
> +			return rc;
> +		}

It's not clear to me why this in particular is being read out of the DT
binding, I'd expect this to be in the array describing the regulator the
same as everything else?

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

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Satya Priya Kakitapalli (Temp)]

On 11/25/2021 8:47 PM, Mark Brown wrote:
> On Fri, Nov 19, 2021 at 03:12:28PM +0530, Satya Priya wrote:
>
>> +  regulator-min-dropout-voltage-microvolt:
>> +    description: Specifies the minimum voltage in microvolts that the
>> +      parent supply regulator must output, above the output of this regulator.
> Usually this is a fixed property of the regulator rather than something
> that varies per board - why have a DT property?


The min-dropout value (headroom) varies with boards, that's why we have 
a DT property for it. We overwrite the default value in driver with 
actual value read from DT

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

[Satya Priya Kakitapalli (Temp)]

On 11/25/2021 8:54 PM, Mark Brown wrote:
> On Fri, Nov 19, 2021 at 03:12:29PM +0530, Satya Priya wrote:
>
>> +properties:
>> +  compatible:
>> +    const: qcom,pm8008-regulators
> Why are we adding a separate compatible for this when we already know
> that this is a pm8008 based on the parent?


For the regulator driver to be probed we do need a separate compatible 
right? may be I didn't get your question..

My understanding is we should have a separate compatible for each 
peripheral under the parent mfd node.. like gpios, temp alarm, 
regulators etc..


>> +  vdd_l1_l2-supply:
>> +    description: Input supply phandle of ldo1 and ldo2 regulators.
> These supply nodes should be chip level, they're going into the chip and
> in general the expectation is that you should be able to describe the
> supplies going into a device without worrying about how or if any
> particular OS splits things up.

So, if i understand correctly, we don't have to mention these in the 
documentation as these are handled at framework level?

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

[Mark Brown]

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

On Mon, Dec 06, 2021 at 07:13:02PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> 
> On 11/25/2021 8:54 PM, Mark Brown wrote:
> > On Fri, Nov 19, 2021 at 03:12:29PM +0530, Satya Priya wrote:

> > > +properties:
> > > +  compatible:
> > > +    const: qcom,pm8008-regulators
> > Why are we adding a separate compatible for this when we already know
> > that this is a pm8008 based on the parent?

> For the regulator driver to be probed we do need a separate compatible
> right? may be I didn't get your question..

> My understanding is we should have a separate compatible for each peripheral
> under the parent mfd node.. like gpios, temp alarm, regulators etc..

No, the MFD can register whatever children it likes without needing any
help from the DT.

> > > +  vdd_l1_l2-supply:
> > > +    description: Input supply phandle of ldo1 and ldo2 regulators.

> > These supply nodes should be chip level, they're going into the chip and
> > in general the expectation is that you should be able to describe the
> > supplies going into a device without worrying about how or if any
> > particular OS splits things up.

> So, if i understand correctly, we don't have to mention these in the
> documentation as these are handled at framework level?

No.  I'm saying you should document these at the chip level, they do
need to be documented though.

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

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

[Satya Priya Kakitapalli (Temp)]

On 11/25/2021 9:15 PM, Mark Brown wrote:
> On Fri, Nov 19, 2021 at 03:12:31PM +0530, Satya Priya wrote:
>
>> +	for (reg = &reg_data[0]; reg->name; reg++) {
> Why is this not just iterating from 0 to ARRAY_SIZE() - that's the more
> normal way to write things and doesn't require a terminator on the
> array.


While changing some other things in the probe I made this change, but I 
think this is not necessary, I'll change it to iterate from 0 to 
ARRAY_SIZE()


>> +		child_node = of_get_child_by_name(parent_node, reg->name);
>> +		if (!child_node) {
>> +			dev_err(dev, "child node %s not found\n", reg->name);
>> +			return -ENODEV;
>> +		}
> This could be pulled out of the array.

Not sure what you meant here. could you elaborate a bit?


>    I think you're also missing an
> of_node_put() on the child_node.


I'll add it in the next version.


>> +		rc = of_property_read_u32(child_node, "reg", &base);
>> +		if (rc < 0) {
>> +			dev_err(dev, "%s: failed to get regulator base rc=%d\n",
>> +						reg->name, rc);
>> +			return rc;
>> +		}
> It's not clear to me why this in particular is being read out of the DT
> binding, I'd expect this to be in the array describing the regulator the
> same as everything else?

I thought that the base address would change with boards, but seems it 
doesn't change. I'll add this in the array.

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

[Mark Brown]

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

On Mon, Dec 06, 2021 at 08:13:57PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> On 11/25/2021 9:15 PM, Mark Brown wrote:
> > On Fri, Nov 19, 2021 at 03:12:31PM +0530, Satya Priya wrote:

> > > +		child_node = of_get_child_by_name(parent_node, reg->name);
> > > +		if (!child_node) {
> > > +			dev_err(dev, "child node %s not found\n", reg->name);
> > > +			return -ENODEV;
> > > +		}

> > This could be pulled out of the array.

> Not sure what you meant here. could you elaborate a bit?

Why is this in every iteration of the loop?

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

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Mark Brown]

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

On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> On 11/25/2021 8:47 PM, Mark Brown wrote:

> > Usually this is a fixed property of the regulator rather than something
> > that varies per board - why have a DT property?

> The min-dropout value (headroom) varies with boards, that's why we have a DT
> property for it. We overwrite the default value in driver with actual value
> read from DT

Interesting.  How exactly does that end up happening - presumably other
systems are going to run into it?

If you do have board designs which somehow managed to introduce
additional dropouts (seems pretty concerning TBH) then I think the best
way to handle that is to add a generic property for it and have that
either added on to or override the requirements of the regulator itself
which should continue to be defined in the driver.  That way only boards
with issues need to do anything which will avoid bugs with the property
being omitted in what should be the common case.

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

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Satya Priya Kakitapalli (Temp)]

On 12/6/2021 11:55 PM, Mark Brown wrote:
> On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>> On 11/25/2021 8:47 PM, Mark Brown wrote:
>>> Usually this is a fixed property of the regulator rather than something
>>> that varies per board - why have a DT property?
>> The min-dropout value (headroom) varies with boards, that's why we have a DT
>> property for it. We overwrite the default value in driver with actual value
>> read from DT
> Interesting.  How exactly does that end up happening - presumably other
> systems are going to run into it?


The parent supplies such as "vdd-l1-l2" are coming from other pmic 
regulators, which are shared supplies with other subsystems like BT, 
Display etc, they vary between boards as per requirements, so we cannot 
expect these to be fixed and so are the headroom values. We get the 
headroom values from PMIC systems team for every target.


> If you do have board designs which somehow managed to introduce
> additional dropouts (seems pretty concerning TBH) then I think the best
> way to handle that is to add a generic property for it and have that
> either added on to or override the requirements of the regulator itself
> which should continue to be defined in the driver.  That way only boards
> with issues need to do anything which will avoid bugs with the property
> being omitted in what should be the common case.

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Mark Brown]

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

On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> On 12/6/2021 11:55 PM, Mark Brown wrote:
> > On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote:

> > > The min-dropout value (headroom) varies with boards, that's why we have a DT
> > > property for it. We overwrite the default value in driver with actual value
> > > read from DT

> > Interesting.  How exactly does that end up happening - presumably other
> > systems are going to run into it?

> The parent supplies such as "vdd-l1-l2" are coming from other pmic
> regulators, which are shared supplies with other subsystems like BT, Display
> etc, they vary between boards as per requirements, so we cannot expect these
> to be fixed and so are the headroom values. We get the headroom values from
> PMIC systems team for every target.

I don't think you're talking about the thing the code is saying it's
describing here.  The regulator API is referring to the minimum droput
voltage that individual regulators require, that is how much higher the
input to a single regulator must be than the voltage being output by
that regulator.  We absolutely can and do expect this to be board
independent, it's a function of the design of the regulator.  Sharing
the input supply has no impact on this, the input voltage that the
regulator needs just get fed into the requiremnts on the supply voltage.

If there is a board specific constraint on the minimum voltage that a
given supply can have then that should be expressed using the normal
constraint mechanism, that's nothing to do with the headroom that the
regulators require to operate though.

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

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[David Collins]

On 12/7/21 7:19 AM, Mark Brown wrote:
> On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>> On 12/6/2021 11:55 PM, Mark Brown wrote:
>>> On Mon, Dec 06, 2021 at 06:33:26PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> 
>>>> The min-dropout value (headroom) varies with boards, that's why we have a DT
>>>> property for it. We overwrite the default value in driver with actual value
>>>> read from DT
> 
>>> Interesting.  How exactly does that end up happening - presumably other
>>> systems are going to run into it?
> 
>> The parent supplies such as "vdd-l1-l2" are coming from other pmic
>> regulators, which are shared supplies with other subsystems like BT, Display
>> etc, they vary between boards as per requirements, so we cannot expect these
>> to be fixed and so are the headroom values. We get the headroom values from
>> PMIC systems team for every target.
> 
> I don't think you're talking about the thing the code is saying it's
> describing here.  The regulator API is referring to the minimum droput
> voltage that individual regulators require, that is how much higher the
> input to a single regulator must be than the voltage being output by
> that regulator.  We absolutely can and do expect this to be board
> independent, it's a function of the design of the regulator.  Sharing
> the input supply has no impact on this, the input voltage that the
> regulator needs just get fed into the requiremnts on the supply voltage.
> 
> If there is a board specific constraint on the minimum voltage that a
> given supply can have then that should be expressed using the normal
> constraint mechanism, that's nothing to do with the headroom that the
> regulators require to operate though.

The PM8008 LDOs are low noise LDOs intended to supply noise sensitive
camera sensor hardware.  They can maintain output regulation with a
fixed headroom voltage.  However, in order to guarantee high PSRR, the
headroom voltage must be scaled according to the peak load expected from
the each LDO on a given board.  Thus, we included support for a DT
property to specify the headroom per LDO to meet noise requirements
across boards.

As a minor note the PM8008 chip package has a single pin to supply LDOs
1 and 2 along with a single pin for LDOs 3 and 4.  That is why
vdd_l1_l2-supply is specified instead of vdd_l1-supply and vdd_l2-supply.

Take care,
David

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Mark Brown]

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

On Wed, Dec 08, 2021 at 04:56:48PM -0800, David Collins wrote:
> On 12/7/21 7:19 AM, Mark Brown wrote:
> > On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote:

> > that regulator.  We absolutely can and do expect this to be board
> > independent, it's a function of the design of the regulator.  Sharing
> > the input supply has no impact on this, the input voltage that the
> > regulator needs just get fed into the requiremnts on the supply voltage.

> The PM8008 LDOs are low noise LDOs intended to supply noise sensitive
> camera sensor hardware.  They can maintain output regulation with a
> fixed headroom voltage.  However, in order to guarantee high PSRR, the
> headroom voltage must be scaled according to the peak load expected from
> the each LDO on a given board.  Thus, we included support for a DT
> property to specify the headroom per LDO to meet noise requirements
> across boards.

Interesting...  how much extra headroom are we talking about here?  I'd
be unsurprised to see this usually just quoted as part of the standard
headroom requirement and this smells like the sort of thing that's going
to be frequently misused.  If the gains are something worth writing home
about I'd think we should consider if it's better to support this
dynamically at runtime based on load information and provide options for
configuring the peak load information through DT instead for static
configurations.  That would fit in with the stuff we have for managing
modes on DCDCs (which isn't really deployed but is there) and the API we
have for allowing client drivers to indicate their load requirements at
runtime that fits in with that.  That'd allow us to only boost the
headroom when it's really needed.

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

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

[Satya Priya Kakitapalli (Temp)]

On 12/6/2021 8:39 PM, Mark Brown wrote:
> On Mon, Dec 06, 2021 at 08:13:57PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>> On 11/25/2021 9:15 PM, Mark Brown wrote:
>>> On Fri, Nov 19, 2021 at 03:12:31PM +0530, Satya Priya wrote:
>>>> +		child_node = of_get_child_by_name(parent_node, reg->name);
>>>> +		if (!child_node) {
>>>> +			dev_err(dev, "child node %s not found\n", reg->name);
>>>> +			return -ENODEV;
>>>> +		}
>>> This could be pulled out of the array.
>> Not sure what you meant here. could you elaborate a bit?
> Why is this in every iteration of the loop?


Getting the child node here is not required anymore. This got carried 
from previous versions, I'll remove this.

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

[Satya Priya Kakitapalli (Temp)]

On 12/6/2021 7:17 PM, Mark Brown wrote:
> On Mon, Dec 06, 2021 at 07:13:02PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>> On 11/25/2021 8:54 PM, Mark Brown wrote:
>>> On Fri, Nov 19, 2021 at 03:12:29PM +0530, Satya Priya wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,pm8008-regulators
>>> Why are we adding a separate compatible for this when we already know
>>> that this is a pm8008 based on the parent?
>> For the regulator driver to be probed we do need a separate compatible
>> right? may be I didn't get your question..
>> My understanding is we should have a separate compatible for each peripheral
>> under the parent mfd node.. like gpios, temp alarm, regulators etc..
> No, the MFD can register whatever children it likes without needing any
> help from the DT.

I think this is possible by using of_platform_bus_probe() API. But, the 
mfd driver uses of_platform_populate() API, this needs all device nodes 
to have a 'compatible' property unlike the of_platform_bus_probe() API.

All other MFD upstream drivers are also using the same API and 
registering the child regulators by using separate compatible strings.


>>>> +  vdd_l1_l2-supply:
>>>> +    description: Input supply phandle of ldo1 and ldo2 regulators.
>>> These supply nodes should be chip level, they're going into the chip and
>>> in general the expectation is that you should be able to describe the
>>> supplies going into a device without worrying about how or if any
>>> particular OS splits things up.
>> So, if i understand correctly, we don't have to mention these in the
>> documentation as these are handled at framework level?
> No.  I'm saying you should document these at the chip level, they do
> need to be documented though.

By chip level do you mean "pm8008.yaml" documentation? If so, yes, I can 
move these to pm8008.yaml and change DT accordingly.

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Satya Priya Kakitapalli (Temp)]

On 12/11/2021 2:41 AM, Mark Brown wrote:
> On Wed, Dec 08, 2021 at 04:56:48PM -0800, David Collins wrote:
>> On 12/7/21 7:19 AM, Mark Brown wrote:
>>> On Tue, Dec 07, 2021 at 08:36:11PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>>> that regulator.  We absolutely can and do expect this to be board
>>> independent, it's a function of the design of the regulator.  Sharing
>>> the input supply has no impact on this, the input voltage that the
>>> regulator needs just get fed into the requiremnts on the supply voltage.
>> The PM8008 LDOs are low noise LDOs intended to supply noise sensitive
>> camera sensor hardware.  They can maintain output regulation with a
>> fixed headroom voltage.  However, in order to guarantee high PSRR, the
>> headroom voltage must be scaled according to the peak load expected from
>> the each LDO on a given board.  Thus, we included support for a DT
>> property to specify the headroom per LDO to meet noise requirements
>> across boards.
> Interesting...  how much extra headroom are we talking about here?  I'd
> be unsurprised to see this usually just quoted as part of the standard
> headroom requirement and this smells like the sort of thing that's going
> to be frequently misused.  If the gains are something worth writing home
> about


> I'd think we should consider if it's better to support this
> dynamically at runtime based on load information and provide options for
> configuring the peak load information through DT instead for static
> configurations.  That would fit in with the stuff we have for managing
> modes on DCDCs (which isn't really deployed but is there) and the API we
> have for allowing client drivers to indicate their load requirements at
> runtime that fits in with that.  That'd allow us to only boost the
> headroom when it's really needed.

This means Dynamic headroom control feature needs to be implemented. I 
need to explore more on this and gather info from team, Could we merge 
the present driver with "static headroom" for now? I'll do a follow up 
series to implement this feature.

Re: [PATCH V4 1/6] dt-bindings: regulator: Add "regulator-min-dropout-voltage-microvolt"

[Mark Brown]

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

On Mon, Jan 03, 2022 at 08:05:40PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> On 12/11/2021 2:41 AM, Mark Brown wrote:

> > I'd think we should consider if it's better to support this
> > dynamically at runtime based on load information and provide options for
> > configuring the peak load information through DT instead for static
> > configurations.  That would fit in with the stuff we have for managing
> > modes on DCDCs (which isn't really deployed but is there) and the API we
> > have for allowing client drivers to indicate their load requirements at
> > runtime that fits in with that.  That'd allow us to only boost the
> > headroom when it's really needed.

> This means Dynamic headroom control feature needs to be implemented. I need
> to explore more on this and gather info from team, Could we merge the
> present driver with "static headroom" for now? I'll do a follow up series to
> implement this feature.

I'd be happy to merge something with the headroom configured statically
in the driver like we do for other devices - I guess if you set the
highest headroom that should cover it.

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

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

[Mark Brown]

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

On Mon, Jan 10, 2022 at 06:42:08PM +0530, Satya Priya Kakitapalli (Temp) wrote:

> To understand how other upstream mfd drivers are handling this I've gone
> through some of them. Taking one example, mfd/stpmic1.c is a pmic  mfd
> device which has a regulators sub-node with separate compatible, and has the
> parent supplies listed under the regulators node.

There are some devices that did get merged doing this, that doesn't mean
it's a great idea though.

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

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

[Satya Priya Kakitapalli (Temp)]

On 1/10/2022 7:51 PM, Mark Brown wrote:
> On Mon, Jan 10, 2022 at 06:42:08PM +0530, Satya Priya Kakitapalli (Temp) wrote:
>
>> To understand how other upstream mfd drivers are handling this I've gone
>> through some of them. Taking one example, mfd/stpmic1.c is a pmic  mfd
>> device which has a regulators sub-node with separate compatible, and has the
>> parent supplies listed under the regulators node.
> There are some devices that did get merged doing this, that doesn't mean
> it's a great idea though.

In that case, it would be helpful if you could provide an example which 
has the design you suggested.

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

[Mark Brown]

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

On Tue, Jan 11, 2022 at 05:45:16PM +0530, Satya Priya Kakitapalli (Temp) wrote:
> On 1/10/2022 7:51 PM, Mark Brown wrote:

> > There are some devices that did get merged doing this, that doesn't mean
> > it's a great idea though.

> In that case, it would be helpful if you could provide an example which has
> the design you suggested.

There's plenty of mfds that register child devices without using DT -
off the top of my head wm8994 or arizona.

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