Add Qualcomm Technologies, Inc. PM8008 regulator driver

Add bindings for pm8008 pmic regulators.

Signed-off-by Satya Priya <[email protected]>

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

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

Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml new file +41 -0

new file mode 100644
index 0000000..31ac1eb
@@ -0,0 +1,72 @@
+# 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 <[email protected]>
+
+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:

Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml new file +16 -0

+          supply regulator must output, above the output of this
+          regulator.
+
+    required:
+      - reg
+      - regulator-name
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+...

Signature · 2 lines

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

Add pm8008-regulator node and example.

Signed-off-by Satya Priya <[email protected]>

---
Changes in V2:
- As per Rob's comments changed "pm8008[a-z]?-regulator" to "^pm8008[a-z]?-regulators".

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

Add pm8008-regulator node and example.

Signed-off-by Satya Priya <[email protected]>

---
Changes in V2:
- As per Rob's comments changed "pm8008[a-z]?-regulator" to "^pm8008[a-z]?-regulators".

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

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +0 -0

index ec3138c..0c9665e 100644

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +18 -0

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

Quoting Satya Priya (2021-09-30 21:00:57)

Quoted 17 lines

Add pm8008-regulator node and example.

Signed-off-by Satya Priya <[email protected]>

---
Changes in V2:
- As per Rob's comments changed "pm8008[a-z]?-regulator" to "^pm8008[a-z]?-regulators".

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

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +0 -0

index ec3138c..0c9665e 100644

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml properties: · line 48

Quoted 4 lines

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +1 -0

@@ -45,6 +45,10 @@ properties:
     const: 0
 
 patternProperties:
+  "^pm8008[a-z]?-regulators$":

Please just call it 'regulators'

Quoted 6 lines

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +3 -0

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

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml examples: · line 130

Quoted 6 lines

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +2 -0

@@ -122,6 +126,26 @@ examples:
           interrupt-controller;
           #interrupt-cells = <2>;
         };
+
+        pm8008-regulators {

Please just call it 'regulators'

Quoted 18 lines

Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +18 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+
+#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)

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       int             min_uv;
+       int             max_uv;
+       int             min_dropout_uv;
+};
+
+struct pm8008_regulator {

drivers/regulator/qcom-pm8008-regulator.c new file +8 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +14 -0

+}
+
+static inline 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).
+        */

drivers/regulator/qcom-pm8008-regulator.c new file +25 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +11 -0

+       .disable                = regulator_disable_regmap,
+       .is_enabled             = regulator_is_enabled_regmap,
+       .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)
+{

drivers/regulator/qcom-pm8008-regulator.c new file +14 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +15 -0

+               dev_err(dev, "%s: failed to get regulator data\n", name);
+               return -ENODATA;
+       }
+
+       init_data->constraints.input_uV = init_data->constraints.max_uV;
+       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;

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +7 -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);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+};
+
+module_platform_driver(pm8008_regulator_driver);
+

Quoting Satya Priya (2021-09-30 21:00:58)

drivers/regulator/qcom-pm8008-regulator.c line 4

Quoted 10 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

new file mode 100644
index 0000000..5dacaa4
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/delay.h>

Is this include used?

drivers/regulator/qcom-pm8008-regulator.c line 8

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>

Is this include used?

drivers/regulator/qcom-pm8008-regulator.c line 11

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>

Is this include used?

drivers/regulator/qcom-pm8008-regulator.c line 12

Quoted 1 line

drivers/regulator/qcom-pm8008-regulator.c new file +1 -0

+#include <linux/pm.h>

Is this include used?

drivers/regulator/qcom-pm8008-regulator.c line 15

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>

Is this include used? Probably should just be kernel.h?

drivers/regulator/qcom-pm8008-regulator.c line 18

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>

Is this include used?

Quoted 11 lines

drivers/regulator/qcom-pm8008-regulator.c new file +11 -0

+
+#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)
+

drivers/regulator/qcom-pm8008-regulator.c line 35

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

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

Drop define.

drivers/regulator/qcom-pm8008-regulator.c line 38

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+
+struct regulator_data {
+       char            *name;

const?

drivers/regulator/qcom-pm8008-regulator.c line 39

Quoted 1 line

drivers/regulator/qcom-pm8008-regulator.c new file +1 -0

+       char            *supply_name;

const?

Quoted 10 lines

drivers/regulator/qcom-pm8008-regulator.c new file +10 -0

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

drivers/regulator/qcom-pm8008-regulator.c line 55

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       struct device_node      *of_node;
+       u16                     base;
+       int                     step_rate;
+};
+
+static const struct regulator_data reg_data[PM8008_MAX_LDO] = {

Use [] instead of PM8008_MAX_LDO.

drivers/regulator/qcom-pm8008-regulator.c line 63

Quoted 8 lines

drivers/regulator/qcom-pm8008-regulator.c new file +8 -0

+       /* 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},

Nitpick: Put a space after { and before } to match kernel style.

Quoted 20 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+};
+
+static int pm8008_read(struct regmap *regmap,  u16 reg, u8 *val, int 

count)

drivers/regulator/qcom-pm8008-regulator.c new file +11 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+{
+       int rc;
+
+       pr_debug("Writing [%*ph] from address %#x\n", count, val, 

Quoted 1 line

reg);

Don't we already have regmap debugging facilities for this? Why duplicate it in this driver?

drivers/regulator/qcom-pm8008-regulator.c line 87

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       rc = regmap_bulk_write(regmap, reg, val, count);
+       if (rc < 0)
+               pr_err("failed to write %#x rc=%d\n", reg, rc);
+
+       return rc;
+}

The above two functions should just be inlined.

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);

drivers/regulator/qcom-pm8008-regulator.c line 97

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       u8 vset_raw[2];
+       int rc;
+
+       rc = pm8008_read(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base),
+                       vset_raw, 2);

Can this be an __le16 mV?

drivers/regulator/qcom-pm8008-regulator.c line 103

Quoted 7 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       if (rc < 0) {
+               dev_err(pm8008_reg->dev, "failed to read regulator 

voltage rc=%d\n", rc);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               return rc;
+       }
+
+       return (vset_raw[1] << 8 | vset_raw[0]) * 1000;

And then return le16_to_cpu(mV) * 1000;

Quoted 20 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator 

*pm8008_reg, int min_uv,

drivers/regulator/qcom-pm8008-regulator.c new file +15 -0

+                               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) {
+               dev_err(pm8008_reg->dev,
+                       "requested voltage (%d uV) above maximum limit 

(%d uV)\n",

drivers/regulator/qcom-pm8008-regulator.c line 127

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+                               mv*1000, max_uv);
+               return -EINVAL;
+       }
+
+       vset_raw[0] = mv & 0xff;
+       vset_raw[1] = (mv & 0xff00) >> 8;

Make vset_raw a u16?

	vset = mv;

And then use cpu_to_le16() below?

Quoted 3 lines

+ rc = pm8008_write(pm8008_reg->regmap,
LDO_VSET_LB_REG(pm8008_reg->base),
+ vset_raw, 2);

	regmap_bulk_write(pm8008_reg->regmap, 
LDO_VSET_LB_REG(pm8008_reg->base),
			  cpu_to_le16(vset), sizeof(vset));

does it work?

Quoted 33 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       if (rc < 0) {
+               dev_err(pm8008_reg->dev, "failed to write voltage 

rc=%d\n", rc);

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+               return rc;
+       }
+
+       return 0;
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev 

*rdev,

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+                               int min_uv, int max_uv, unsigned int 

*selector)

drivers/regulator/qcom-pm8008-regulator.c new file +11 -0

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

drivers/regulator/qcom-pm8008-regulator.c line 164

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       dev_dbg(pm8008_reg->dev, "voltage set to %d\n", min_uv);
+       return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+       .enable         = regulator_enable_regmap,

Weird tabbing.

Quoted 15 lines

drivers/regulator/qcom-pm8008-regulator.c new file +15 -0

+       .disable                = regulator_disable_regmap,
+       .is_enabled             = regulator_is_enabled_regmap,
+       .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;

drivers/regulator/qcom-pm8008-regulator.c line 185

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       int rc, i;
+       u32 base = 0;
+       u8 reg;
+
+       /* get regulator data */
+       for (i = 0; i < PM8008_MAX_LDO; i++)

Use ARRAY_SIZE()

Quoted 22 lines

drivers/regulator/qcom-pm8008-regulator.c new file +11 -0

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

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+               return rc;
+       }
+       pm8008_reg->base = base;
+
+       /* get slew rate */
+       rc = pm8008_read(pm8008_reg->regmap,
+                       LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 

1);
+ if (rc < 0) {

drivers/regulator/qcom-pm8008-regulator.c line 209

Quoted 6 lines

+ dev_err(dev, "%s: failed to read step rate
configuration rc=%d\n",

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+                               name, rc);
+               return rc;
+       }
+       pm8008_reg->step_rate = 38400 >> (reg & STEP_RATE_MASK);

Where does 38400 come from? Is that a frequency?

drivers/regulator/qcom-pm8008-regulator.c line 213

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+
+       init_data = of_get_regulator_init_data(dev, reg_node,
+                                               &pm8008_reg->rdesc);
+       if (init_data == NULL) {

	if (!init_data)

is more kernel style.

Quoted 24 lines

+ dev_err(dev, "%s: failed to get regulator data\n",
name);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               return -ENODATA;
+       }
+
+       init_data->constraints.input_uV = 

init_data->constraints.max_uV;

drivers/regulator/qcom-pm8008-regulator.c new file +16 -0

+       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.enable_reg = LDO_ENABLE_REG(base);

drivers/regulator/qcom-pm8008-regulator.c line 238

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+       pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+       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);

Why do we allow DT to override this? Isn't it a property of the hardware that doesn't change? So the driver can hardcode the knowledge about the dropout.

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+
+       pm8008_reg->rdev = devm_regulator_register(dev, 

&pm8008_reg->rdesc,

Is this assignment ever used? Seems like it would be better to merely

	return PTR_ERR_OR_ZERO(devm_regulator_register(dev, ...));

Quoted 12 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+                                               &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",

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+                               pm8008_reg->rdesc.name, rc);
+               return rc;
+       }
+
+       dev_dbg(dev, "%s regulator registered\n", name);
+
+       return 0;

drivers/regulator/qcom-pm8008-regulator.c

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+}
+
+static int pm8008_parse_regulator(struct regmap *regmap, struct 

device *dev)

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+{
+       int rc = 0;

Drop initialization.

Quoted 14 lines

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+       const char *name;
+       struct device_node *child;
+       struct pm8008_regulator *pm8008_reg;
+
+       /* parse each subnode and register regulator for regulator 

child */

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       for_each_available_child_of_node(dev->of_node, child) {
+               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), 

GFP_KERNEL);

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+
+               pm8008_reg->regmap = regmap;
+               pm8008_reg->of_node = child;
+               pm8008_reg->dev = dev;
+

drivers/regulator/qcom-pm8008-regulator.c line 273

Quoted 6 lines

+ rc = of_property_read_string(child, "regulator-name",
&name);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               if (rc)
+                       continue;
+
+               rc = pm8008_register_ldo(pm8008_reg, name);

Can we use the of_parse_cb similar to qcom_spmi-regulator.c?

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+               if (rc < 0) {
+                       dev_err(dev, "failed to register regulator %s 

rc=%d\n",

drivers/regulator/qcom-pm8008-regulator.c new file +1 -0

+                                       name, rc);

drivers/regulator/qcom-pm8008-regulator.c line 286

Quoted 10 lines

drivers/regulator/qcom-pm8008-regulator.c new file +10 -0

+                       return rc;
+               }
+       }
+
+       return 0;
+}
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+       int rc = 0;

Please don't initialize locals and then overwrite them before testing them.

drivers/regulator/qcom-pm8008-regulator.c line 295

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c new file +9 -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);

Just inline this code. It's basically the entire probe function so splitting it away to yet another function just makes it harder to read.

Quoted 7 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       if (rc < 0) {
+               dev_err(&pdev->dev, "failed to parse device tree 

rc=%d\n", rc);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               return rc;
+       }
+
+       return 0;

drivers/regulator/qcom-pm8008-regulator.c line 306

Quoted 5 lines

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+}
+
+static const struct of_device_id pm8008_regulator_match_table[] = {
+       { .compatible = "qcom,pm8008-regulator", },
+       { },

Nitpick: Drop comma on sentinel so nothing can come after without causing a compilation error.

Quoted 1 line

+};

Add a MODULE_DEVICE_TABLE please. Same comment applies to the mfd driver.

drivers/regulator/qcom-pm8008-regulator.c line 314

Quoted 7 lines

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+
+static struct platform_driver pm8008_regulator_driver = {
+       .driver = {
+               .name           = "qcom,pm8008-regulator",
+               .of_match_table = pm8008_regulator_match_table,
+       },
+       .probe          = pm8008_regulator_probe,

I have no idea what's going on with this tabbing.

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+};
+
+module_platform_driver(pm8008_regulator_driver);
+

Quoted 12 lines

Quoting Satya Priya (2021-09-30 21:00:58)

Quoted 11 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

new file mode 100644
index 0000000..5dacaa4
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/delay.h>

Quoted 4 lines

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c line 97

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       u8 vset_raw[2];
+       int rc;
+
+       rc = pm8008_read(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base),
+                       vset_raw, 2);

Can this be an __le16 mV?

Below is the diff after changing as per your suggestion, Please correct me if wrong.

(quoted diff) +3 -4

-       u8 vset_raw[2];
+       __le16 mV;
         int rc;
 
-       rc = pm8008_read(pm8008_reg->regmap,
-                       LDO_VSET_LB_REG(pm8008_reg->base),
-                       vset_raw, 2);
+       rc = regmap_bulk_read(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base), &mV, 2);
         if (rc < 0) {
                 dev_err(pm8008_reg->dev, "failed to read regulator

voltage rc=%d\n", rc);
                 return rc;
         }

Below is the diff:

(quoted diff) +2 -5

-       int rc = 0, mv;
-       u8 vset_raw[2];
+       int rc, mv;
+       u16 vset_raw;
[...]
-       vset_raw[0] = mv & 0xff;
-       vset_raw[1] = (mv & 0xff00) >> 8;
-       rc = pm8008_write(pm8008_reg->regmap,

LDO_VSET_LB_REG(pm8008_reg->base),

Quoted 24 lines

Quoted 24 lines

+ dev_err(dev, "%s: failed to get regulator data\n",
name);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               return -ENODATA;
+       }
+
+       init_data->constraints.input_uV =

init_data->constraints.max_uV;

drivers/regulator/qcom-pm8008-regulator.c new file +17 -0

+       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.enable_reg = LDO_ENABLE_REG(base);
+       pm8008_reg->rdesc.enable_mask = ENABLE_BIT;

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c line 238

Quoted 3 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

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

Why do we allow DT to override this? Isn't it a property of the hardware
that doesn't change? So the driver can hardcode the knowledge about the dropout.

The headroom values change with targets. We are adding some default headroom values in the driver and later overwriting them with the actual values specified in the DT.

Quoted 9 lines

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+
+       pm8008_reg->rdev = devm_regulator_register(dev,

&pm8008_reg->rdesc,

Is this assignment ever used? Seems like it would be better to merely

      return PTR_ERR_OR_ZERO(devm_regulator_register(dev, ...));

Okay.

Quoted 12 lines

Quoted 12 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+                                               &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",

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+                               pm8008_reg->rdesc.name, rc);
+               return rc;
+       }
+
+       dev_dbg(dev, "%s regulator registered\n", name);
+
+       return 0;

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+}
+
+static int pm8008_parse_regulator(struct regmap *regmap, struct

device *dev)

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+{
+       int rc = 0;

Drop initialization.

Okay.

Quoted 14 lines

Quoted 14 lines

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+       const char *name;
+       struct device_node *child;
+       struct pm8008_regulator *pm8008_reg;
+
+       /* parse each subnode and register regulator for regulator

child */

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       for_each_available_child_of_node(dev->of_node, child) {
+               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg),

GFP_KERNEL);

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+
+               pm8008_reg->regmap = regmap;
+               pm8008_reg->of_node = child;
+               pm8008_reg->dev = dev;
+

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c line 273

Quoted 6 lines

+ rc = of_property_read_string(child, "regulator-name",
&name);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               if (rc)
+                       continue;
+
+               rc = pm8008_register_ldo(pm8008_reg, name);

Can we use the of_parse_cb similar to qcom_spmi-regulator.c?

Are you suggesting to remove the pm8008_register_ldo API and add its contents in probe itself and then use of_parse_cb callback like in qcom_spmi-regulator.c?

Do we have any advantage using that here? Also I am not exactly sure what all contents to put in that. Seems like we can put the step rate and min-dropout-voltage configurations in there.

Quoting [email protected] (2021-10-22 05:28:34)

Quoted 13 lines · Stephen Boyd

Quoted 12 lines

Quoting Satya Priya (2021-09-30 21:00:58)

Quoted 11 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

new file mode 100644
index 0000000..5dacaa4
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/delay.h>

[...]

Quoted 32 lines

Quoted 4 lines

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c line 97

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+       u8 vset_raw[2];
+       int rc;
+
+       rc = pm8008_read(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base),
+                       vset_raw, 2);

Can this be an __le16 mV?

Below is the diff after changing as per your suggestion, Please correct me if wrong.

(quoted diff) +3 -4

-       u8 vset_raw[2];
+       __le16 mV;
         int rc;
 
-       rc = pm8008_read(pm8008_reg->regmap,
-                       LDO_VSET_LB_REG(pm8008_reg->base),
-                       vset_raw, 2);
+       rc = regmap_bulk_read(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base), &mV, 2);
         if (rc < 0) {
                 dev_err(pm8008_reg->dev, "failed to read regulator

voltage rc=%d\n", rc);
                 return rc;
         }

Review

Quoted 2 lines

(quoted diff) +1 -1

-       return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
+       return le16_to_cpu(mV) * 1000;

Looks good. Does mV need to be casted when passed to regmap_bulk_read()?

Quoted 13 lines

Below is the diff:

(quoted diff) +2 -5

-       int rc = 0, mv;
-       u8 vset_raw[2];
+       int rc, mv;
+       u16 vset_raw;
[...]
-       vset_raw[0] = mv & 0xff;
-       vset_raw[1] = (mv & 0xff00) >> 8;
-       rc = pm8008_write(pm8008_reg->regmap,

LDO_VSET_LB_REG(pm8008_reg->base),

Review

Quoted 6 lines

(quoted diff) +5 -1

-                       vset_raw, 2);
+       vset_raw = cpu_to_le16(mv);
+
+       rc = regmap_bulk_write(pm8008_reg->regmap,
+                       LDO_VSET_LB_REG(pm8008_reg->base), &vset_raw,
+                       sizeof(vset_raw));

Ok, thanks

Quoted 34 lines

Quoted 33 lines

Quoted 17 lines

+ dev_err(dev, "%s: failed to get regulator data\n",
name);

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               return -ENODATA;
+       }
+
+       init_data->constraints.input_uV =

init_data->constraints.max_uV;

drivers/regulator/qcom-pm8008-regulator.c new file +10 -0

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

drivers/regulator/qcom-pm8008-regulator.c line 238

Quoted 10 lines

drivers/regulator/qcom-pm8008-regulator.c new file +10 -0

+       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.enable_reg = LDO_ENABLE_REG(base);
+       pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+       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);

Why do we allow DT to override this? Isn't it a property of the hardware
that doesn't change? So the driver can hardcode the knowledge about the dropout.

Quoted 3 lines

The headroom values change with targets. We are adding some default headroom values in the driver and later overwriting them with the actual values specified in the DT.

What do you mean by "targets"? Is that the SoC the PMIC is paired with?

I'd prefer it be a standard regulator property instead of qcom specific if it actually needs to be different based on different devices.

Quoted 60 lines

Quoted 9 lines

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+
+       pm8008_reg->rdev = devm_regulator_register(dev,

&pm8008_reg->rdesc,

Is this assignment ever used? Seems like it would be better to merely

      return PTR_ERR_OR_ZERO(devm_regulator_register(dev, ...));

Okay.

Quoted 12 lines

Quoted 12 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+                                               &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",

drivers/regulator/qcom-pm8008-regulator.c new file +7 -0

+                               pm8008_reg->rdesc.name, rc);
+               return rc;
+       }
+
+       dev_dbg(dev, "%s regulator registered\n", name);
+
+       return 0;

Quoted 9 lines

drivers/regulator/qcom-pm8008-regulator.c

Quoted 6 lines

drivers/regulator/qcom-pm8008-regulator.c new file +3 -0

+}
+
+static int pm8008_parse_regulator(struct regmap *regmap, struct

device *dev)

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+{
+       int rc = 0;

Drop initialization.

Okay.

Quoted 23 lines

Quoted 16 lines

drivers/regulator/qcom-pm8008-regulator.c new file +5 -0

+       const char *name;
+       struct device_node *child;
+       struct pm8008_regulator *pm8008_reg;
+
+       /* parse each subnode and register regulator for regulator

child */

drivers/regulator/qcom-pm8008-regulator.c new file +2 -0

+       for_each_available_child_of_node(dev->of_node, child) {
+               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg),

GFP_KERNEL);

drivers/regulator/qcom-pm8008-regulator.c new file +6 -0

+
+               pm8008_reg->regmap = regmap;
+               pm8008_reg->of_node = child;
+               pm8008_reg->dev = dev;
+
+               rc = of_property_read_string(child, "regulator-name",

&name);

drivers/regulator/qcom-pm8008-regulator.c line 273

Quoted 4 lines

drivers/regulator/qcom-pm8008-regulator.c new file +4 -0

+               if (rc)
+                       continue;
+
+               rc = pm8008_register_ldo(pm8008_reg, name);

Can we use the of_parse_cb similar to qcom_spmi-regulator.c?

Quoted 3 lines

Are you suggesting to remove the pm8008_register_ldo API and add its contents in probe itself and then use of_parse_cb callback like in qcom_spmi-regulator.c?

Yes

Quoted 4 lines

Do we have any advantage using that here? Also I am not exactly sure what all contents to put in that. Seems like we can put the step rate and min-dropout-voltage configurations in there.

Right. The regulator code is setup to do "DT parsing stuff" for each regulator node already, so you don't need to duplicate that logic in this driver. That's the main goal, consolidate regulator matching and iteration into the core. Maybe Mark has more info.