* [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver
@ 2021-10-01 4:00 Satya Priya
2021-10-01 4:00 ` [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings Satya Priya
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Satya Priya @ 2021-10-01 4:00 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, swboyd, collinsd,
subbaram, kgunda, Satya Priya
Satya Priya (2):
regulator: dt-bindings: Add pm8008 regulator bindings
dt-bindings: mfd: pm8008: Add pm8008 regulator node
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 | 72 +++++
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 103 +++++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 320 +++++++++++++++++++++
6 files changed, 529 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings
2021-10-01 4:00 [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
@ 2021-10-01 4:00 ` Satya Priya
2021-10-08 22:23 ` Rob Herring
2021-10-01 4:00 ` [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node Satya Priya
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Satya Priya @ 2021-10-01 4:00 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, swboyd, collinsd,
subbaram, kgunda, Satya Priya
Add bindings for pm8008 pmic regulators.
Signed-off-by: Satya Priya <skakit@codeaurora.org>
---
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
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..31ac1eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
@@ -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 <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
+ 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
+
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node
2021-10-01 4:00 [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2021-10-01 4:00 ` [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings Satya Priya
@ 2021-10-01 4:00 ` Satya Priya
2021-10-01 13:16 ` Rob Herring
2021-10-05 18:10 ` Stephen Boyd
2021-10-01 4:00 ` [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
2021-10-01 4:00 ` [PATCH V2 4/4] arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp Satya Priya
3 siblings, 2 replies; 13+ messages in thread
From: Satya Priya @ 2021-10-01 4:00 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, swboyd, collinsd,
subbaram, kgunda, Satya Priya
Add pm8008-regulator node and example.
Signed-off-by: Satya Priya <skakit@codeaurora.org>
---
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(+)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..0c9665e 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]?-regulators$":
+ 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-regulators {
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC
2021-10-01 4:00 [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2021-10-01 4:00 ` [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings Satya Priya
2021-10-01 4:00 ` [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node Satya Priya
@ 2021-10-01 4:00 ` Satya Priya
2021-10-05 18:35 ` Stephen Boyd
2021-10-01 4:00 ` [PATCH V2 4/4] arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp Satya Priya
3 siblings, 1 reply; 13+ messages in thread
From: Satya Priya @ 2021-10-01 4:00 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, swboyd, collinsd,
subbaram, kgunda, 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>
---
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.
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 320 ++++++++++++++++++++++++++++++
3 files changed, 330 insertions(+)
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 27578e9..95d234f 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..5dacaa4
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,320 @@
+// 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_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) {
+ dev_err(pm8008_reg->dev, "failed to read regulator voltage rc=%d\n", rc);
+ return rc;
+ }
+
+ return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
+}
+
+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).
+ */
+ 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",
+ 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) {
+ 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;
+
+ *selector = DIV_ROUND_UP(min_uv - pm8008_reg->rdesc.min_uV,
+ VSET_STEP_UV);
+
+ 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,
+ .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;
+ int rc, i;
+ 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;
+
+ /* get slew rate */
+ rc = pm8008_read(pm8008_reg->regmap,
+ LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 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;
+ }
+
+ 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;
+ 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);
+
+ pm8008_reg->rdev = devm_regulator_register(dev, &pm8008_reg->rdesc,
+ ®_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;
+ }
+
+ dev_dbg(dev, "%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);
+
+ 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH V2 4/4] arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp
2021-10-01 4:00 [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (2 preceding siblings ...)
2021-10-01 4:00 ` [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2021-10-01 4:00 ` Satya Priya
3 siblings, 0 replies; 13+ messages in thread
From: Satya Priya @ 2021-10-01 4:00 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, swboyd, collinsd,
subbaram, kgunda, satya priya
From: satya priya <skakit@codeaurora.org>
Add pm8008 regulators support for sc7280 idp.
Signed-off-by: satya priya <skakit@codeaurora.org>
---
Changes in V2:
- As per Stephen's comments, replaced '_' with '-' for node names.
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 272d5ca..b953261 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -280,6 +280,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-regulators {
+ 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>;
};
@@ -408,6 +499,18 @@
};
};
+&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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node
2021-10-01 4:00 ` [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node Satya Priya
@ 2021-10-01 13:16 ` Rob Herring
2021-10-05 18:10 ` Stephen Boyd
1 sibling, 0 replies; 13+ messages in thread
From: Rob Herring @ 2021-10-01 13:16 UTC (permalink / raw)
To: Satya Priya
Cc: Mark Brown, Rob Herring, Lee Jones, mka, Liam Girdwood, swboyd,
collinsd, linux-arm-msm, Bjorn Andersson, Das Srinagesh,
linux-kernel, kgunda, devicetree, subbaram
On Fri, 01 Oct 2021 09:30:57 +0530, Satya Priya wrote:
> Add pm8008-regulator node and example.
>
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
> 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(+)
>
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:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/qcom,pm8008.example.dt.yaml: pm8008i@8: pm8008-regulators:regulator@4000: 'regulator-max-microvolt', 'regulator-min-microvolt' do not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/qcom,pm8008.example.dt.yaml: pm8008-regulators: regulator@4000: 'regulator-max-microvolt', 'regulator-min-microvolt' do not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1535124
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node
2021-10-01 4:00 ` [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node Satya Priya
2021-10-01 13:16 ` Rob Herring
@ 2021-10-05 18:10 ` Stephen Boyd
2021-10-21 8:51 ` skakit
1 sibling, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-10-05 18:10 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, collinsd, subbaram,
kgunda
Quoting Satya Priya (2021-09-30 21:00:57)
> Add pm8008-regulator node and example.
>
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
> 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(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
> index ec3138c..0c9665e 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]?-regulators$":
Please just call it 'regulators'
> + 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-regulators {
Please just call it 'regulators'
> + 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>;
> + };
> + };
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC
2021-10-01 4:00 ` [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2021-10-05 18:35 ` Stephen Boyd
2021-10-22 12:28 ` skakit
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-10-05 18:35 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, mka, collinsd, subbaram,
kgunda
Quoting Satya Priya (2021-09-30 21:00:58)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..5dacaa4
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -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?
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
Is this include used?
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
Is this include used?
> +#include <linux/pm.h>
Is this include used?
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
Is this include used? Probably should just be kernel.h?
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
Is this include used?
> +
> +#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
Drop define.
> +
> +struct regulator_data {
> + char *name;
const?
> + char *supply_name;
const?
> + 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] = {
Use [] instead of 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},
Nitpick: Put a space after { and before } to match kernel style.
> +};
> +
> +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);
Don't we already have regmap debugging facilities for this? Why
duplicate it in this driver?
> + 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.
> +
> +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);
Can this be an __le16 mV?
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev, "failed to read regulator voltage rc=%d\n", rc);
> + return rc;
> + }
> +
> + return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
And then return le16_to_cpu(mV) * 1000;
> +}
> +
> +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).
> + */
> + 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",
> + 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?
> + 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?
> + 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;
> +
> + *selector = DIV_ROUND_UP(min_uv - pm8008_reg->rdesc.min_uV,
> + VSET_STEP_UV);
> +
> + 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.
> + .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;
> + int rc, i;
> + u32 base = 0;
> + u8 reg;
> +
> + /* get regulator data */
> + for (i = 0; i < PM8008_MAX_LDO; i++)
Use ARRAY_SIZE()
> + 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;
> +
> + /* get slew rate */
> + rc = pm8008_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 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);
Where does 38400 come from? Is that a frequency?
> +
> + init_data = of_get_regulator_init_data(dev, reg_node,
> + &pm8008_reg->rdesc);
> + if (init_data == NULL) {
if (!init_data)
is more kernel style.
> + 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;
> + 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.
> +
> + 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, ...));
> + ®_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;
> + }
> +
> + dev_dbg(dev, "%s regulator registered\n", name);
> +
> + return 0;
> +}
> +
> +static int pm8008_parse_regulator(struct regmap *regmap, struct device *dev)
> +{
> + int rc = 0;
Drop initialization.
> + 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);
> +
> + 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);
Can we use the of_parse_cb similar to qcom_spmi-regulator.c?
> + 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;
Please don't initialize locals and then overwrite them before testing
them.
> + 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.
> + 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", },
> + { },
Nitpick: Drop comma on sentinel so nothing can come after without
causing a compilation error.
> +};
Add a MODULE_DEVICE_TABLE please. Same comment applies to the mfd
driver.
> +
> +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.
> +};
> +
> +module_platform_driver(pm8008_regulator_driver);
> +
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings
2021-10-01 4:00 ` [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings Satya Priya
@ 2021-10-08 22:23 ` Rob Herring
0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2021-10-08 22:23 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Lee Jones, Liam Girdwood, Mark Brown,
Das Srinagesh, linux-arm-msm, devicetree, linux-kernel, mka,
swboyd, collinsd, subbaram, kgunda
On Fri, Oct 01, 2021 at 09:30:56AM +0530, Satya Priya wrote:
> Add bindings for pm8008 pmic regulators.
>
> Signed-off-by: Satya Priya <skakit@codeaurora.org>
> ---
> 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
>
> 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..31ac1eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> @@ -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 <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
blank line here
What do the reg values represent here? You need to define that since it
specific to this devices. Some constraints on the values would be nice
too.
> + regulator-name: true
blank line.
> + qcom,min-dropout-voltage:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Specifies the minimum voltage in microvolts that the parent
Use a standard unit suffix and you can drop the type ref.
> + supply regulator must output, above the output of this
> + regulator.
> +
> + required:
> + - reg
> + - regulator-name
> +
> + 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
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node
2021-10-05 18:10 ` Stephen Boyd
@ 2021-10-21 8:51 ` skakit
0 siblings, 0 replies; 13+ messages in thread
From: skakit @ 2021-10-21 8:51 UTC (permalink / raw)
To: Stephen Boyd
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, mka, collinsd, subbaram, kgunda
On 2021-10-05 23:40, Stephen Boyd wrote:
> Quoting Satya Priya (2021-09-30 21:00:57)
>> Add pm8008-regulator node and example.
>>
>> Signed-off-by: Satya Priya <skakit@codeaurora.org>
>> ---
>> 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(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
>> index ec3138c..0c9665e 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]?-regulators$":
>
> Please just call it 'regulators'
>
>> + 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-regulators {
>
> Please just call it 'regulators'
>
Okay
>> + 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>;
>> + };
>> + };
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC
2021-10-05 18:35 ` Stephen Boyd
@ 2021-10-22 12:28 ` skakit
2021-10-25 19:46 ` Stephen Boyd
0 siblings, 1 reply; 13+ messages in thread
From: skakit @ 2021-10-22 12:28 UTC (permalink / raw)
To: Stephen Boyd
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, mka, collinsd, subbaram, kgunda
On 2021-10-06 00:05, Stephen Boyd wrote:
> Quoting Satya Priya (2021-09-30 21:00:58)
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c
>> b/drivers/regulator/qcom-pm8008-regulator.c
>> new file mode 100644
>> index 0000000..5dacaa4
>> --- /dev/null
>> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> @@ -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?
>
No will remove.
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>
> Is this include used?
>
No
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>
> Is this include used?
>
No
>> +#include <linux/pm.h>
>
> Is this include used?
>
No
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/string.h>
>
> Is this include used? Probably should just be kernel.h?
>
string.h is not used , will change it as kernel.h
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +#include <linux/regulator/of_regulator.h>
>
> Is this include used?
>
Yes it is used. For of_get_regulator_init_data().
>> +
>> +#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
>
> Drop define.
>
ok.
>> +
>> +struct regulator_data {
>> + char *name;
>
> const?
>
ok
>> + char *supply_name;
>
> const?
>
ok
>> + 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] = {
>
> Use [] instead of PM8008_MAX_LDO.
>
Ok.
>> + /* 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.
>
Okay.
>> +};
>> +
>> +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);
>
> Don't we already have regmap debugging facilities for this? Why
> duplicate it in this driver?
>
>> + 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.
>
I am planning to remove these 2 APIs and use regmap_bulk_read/write
directly.
>> +
>> +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);
>
> Can this be an __le16 mV?
>
Below is the diff after changing as per your suggestion, Please correct
me if wrong.
- 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;
}
- return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
+ return le16_to_cpu(mV) * 1000;
>> + if (rc < 0) {
>> + dev_err(pm8008_reg->dev, "failed to read regulator
>> voltage rc=%d\n", rc);
>> + return rc;
>> + }
>> +
>> + return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
>
> And then return le16_to_cpu(mV) * 1000;
>
>
>> +}
>> +
>> +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).
>> + */
>> + 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",
>> + 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?
>
Below is the diff:
- 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),
- 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));
>> + 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?
>
It is working fine after modifying as above.
>> + 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;
>> +
>> + *selector = DIV_ROUND_UP(min_uv - pm8008_reg->rdesc.min_uV,
>> + VSET_STEP_UV);
>> +
>> + 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.
>
Will correct it.
>> + .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;
>> + int rc, i;
>> + u32 base = 0;
>> + u8 reg;
>> +
>> + /* get regulator data */
>> + for (i = 0; i < PM8008_MAX_LDO; i++)
>
> Use ARRAY_SIZE()
Ok.
>
>> + 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;
>> +
>> + /* get slew rate */
>> + rc = pm8008_read(pm8008_reg->regmap,
>> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®,
>> 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);
>
> Where does 38400 come from? Is that a frequency?
>
It is the default voltage step rate. I'll add a macro
DEFAULT_VOLTAGE_STEP_RATE for this to be clear.
>> +
>> + init_data = of_get_regulator_init_data(dev, reg_node,
>> + &pm8008_reg->rdesc);
>> + if (init_data == NULL) {
>
> if (!init_data)
>
> is more kernel style.
Okay.
>
>> + 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;
>> + 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.
>
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.
>> +
>> + 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.
>> + ®_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;
>> + }
>> +
>> + dev_dbg(dev, "%s regulator registered\n", name);
>> +
>> + return 0;
>> +}
>> +
>> +static int pm8008_parse_regulator(struct regmap *regmap, struct
>> device *dev)
>> +{
>> + int rc = 0;
>
> Drop initialization.
>
Okay.
>> + 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);
>> +
>> + 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);
>
> 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.
>> + 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;
>
> Please don't initialize locals and then overwrite them before testing
> them.
>
>> + 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.
>
Okay.
>> + 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", },
>> + { },
>
> Nitpick: Drop comma on sentinel so nothing can come after without
> causing a compilation error.
>
Okay
>> +};
>
> Add a MODULE_DEVICE_TABLE please. Same comment applies to the mfd
> driver.
>
Okay
>> +
>> +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.
>
>> +};
>> +
>> +module_platform_driver(pm8008_regulator_driver);
>> +
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC
2021-10-22 12:28 ` skakit
@ 2021-10-25 19:46 ` Stephen Boyd
2021-10-27 13:40 ` skakit
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2021-10-25 19:46 UTC (permalink / raw)
To: skakit
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, mka, collinsd, subbaram, kgunda
Quoting skakit@codeaurora.org (2021-10-22 05:28:34)
> On 2021-10-06 00:05, Stephen Boyd wrote:
> > Quoting Satya Priya (2021-09-30 21:00:58)
> >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c
> >> b/drivers/regulator/qcom-pm8008-regulator.c
> >> new file mode 100644
> >> index 0000000..5dacaa4
> >> --- /dev/null
> >> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> >> @@ -0,0 +1,320 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
> >> +
> >> +#include <linux/delay.h>
[...]
> >> +
> >> +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);
> >
> > Can this be an __le16 mV?
> >
>
> Below is the diff after changing as per your suggestion, Please correct
> me if wrong.
>
> - 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;
> }
>
> - 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()?
>
> Below is the diff:
>
> - 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),
> - 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
> >> + 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;
> >> + 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.
> >
>
> 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.
>
> >> +
> >> + 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.
>
> >> + ®_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;
> >> + }
> >> +
> >> + dev_dbg(dev, "%s regulator registered\n", name);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int pm8008_parse_regulator(struct regmap *regmap, struct
> >> device *dev)
> >> +{
> >> + int rc = 0;
> >
> > Drop initialization.
> >
>
> Okay.
>
> >> + 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);
> >> +
> >> + 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);
> >
> > 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?
Yes
>
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC
2021-10-25 19:46 ` Stephen Boyd
@ 2021-10-27 13:40 ` skakit
0 siblings, 0 replies; 13+ messages in thread
From: skakit @ 2021-10-27 13:40 UTC (permalink / raw)
To: Stephen Boyd
Cc: Bjorn Andersson, Rob Herring, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, mka, collinsd, subbaram, kgunda
On 2021-10-26 01:16, Stephen Boyd wrote:
> Quoting skakit@codeaurora.org (2021-10-22 05:28:34)
>> On 2021-10-06 00:05, Stephen Boyd wrote:
>> > Quoting Satya Priya (2021-09-30 21:00:58)
>> >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c
>> >> b/drivers/regulator/qcom-pm8008-regulator.c
>> >> new file mode 100644
>> >> index 0000000..5dacaa4
>> >> --- /dev/null
>> >> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> >> @@ -0,0 +1,320 @@
>> >> +// SPDX-License-Identifier: GPL-2.0-only
>> >> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
>> >> +
>> >> +#include <linux/delay.h>
> [...]
>> >> +
>> >> +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);
>> >
>> > Can this be an __le16 mV?
>> >
>>
>> Below is the diff after changing as per your suggestion, Please
>> correct
>> me if wrong.
>>
>> - 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;
>> }
>>
>> - 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()?
>
>>
>> Below is the diff:
>>
>> - 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),
>> - 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
>
>> >> + 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;
>> >> + 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.
>> >
>>
>> 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?
Yes I meant the SoC/board on which the pmic is present.
> I'd prefer it be a standard regulator property instead of qcom specific
> if it actually needs to be different based on different devices.
>
Ok, I'll drop the qcom prefix.
>>
>> >> +
>> >> + 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.
>>
>> >> + ®_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;
>> >> + }
>> >> +
>> >> + dev_dbg(dev, "%s regulator registered\n", name);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int pm8008_parse_regulator(struct regmap *regmap, struct
>> >> device *dev)
>> >> +{
>> >> + int rc = 0;
>> >
>> > Drop initialization.
>> >
>>
>> Okay.
>>
>> >> + 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);
>> >> +
>> >> + 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);
>> >
>> > 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?
>
> Yes
>
Okay.
>>
>> 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.
Okay.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-10-27 13:40 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 4:00 [PATCH V2 0/4] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2021-10-01 4:00 ` [PATCH V2 1/4] regulator: dt-bindings: Add pm8008 regulator bindings Satya Priya
2021-10-08 22:23 ` Rob Herring
2021-10-01 4:00 ` [PATCH V2 2/4] dt-bindings: mfd: pm8008: Add pm8008 regulator node Satya Priya
2021-10-01 13:16 ` Rob Herring
2021-10-05 18:10 ` Stephen Boyd
2021-10-21 8:51 ` skakit
2021-10-01 4:00 ` [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
2021-10-05 18:35 ` Stephen Boyd
2021-10-22 12:28 ` skakit
2021-10-25 19:46 ` Stephen Boyd
2021-10-27 13:40 ` skakit
2021-10-01 4:00 ` [PATCH V2 4/4] arm64: dts: qcom: sc7280: Add pm8008 regulators support for sc7280-idp Satya Priya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).