* [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver
@ 2022-02-08 14:52 Satya Priya
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
` (5 more replies)
0 siblings, 6 replies; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Satya Priya (6):
dt-bindings: regulator: Add pm8008 regulator bindings
dt-bindings: mfd: pm8008: Add regulators node
mfd: pm8008: Add mfd cell struct to register LDOs
regulator: Add a regulator driver for the PM8008 PMIC
arm64: dts: qcom: pm8008: Add base dts file
arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp
.../devicetree/bindings/mfd/qcom,pm8008.yaml | 49 ++++-
.../bindings/regulator/qcom,pm8008-regulator.yaml | 31 +++
arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++
drivers/mfd/qcom-pm8008.c | 59 +++++-
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 234 +++++++++++++++++++++
8 files changed, 485 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
--
2.7.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
2022-02-09 18:49 ` Rob Herring
` (2 more replies)
2022-02-08 14:52 ` [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node Satya Priya
` (4 subsequent siblings)
5 siblings, 3 replies; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Add bindings for pm8008 pmic regulators.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
- Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
resolve dtschema errors. Removed regulator-min-microvolt and
regulator-max-microvolt properties.
Changes in V3:
- As per Rob's comments added standard unit suffix for mindropout property,
added blank lines where required and added description for reg property.
Changes in V4:
- Changed compatible string to "com,pm8008-regulators"
- Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
separate patch.
Changes in V5:
- Removed the separate compatible for pm8008 regulator driver.
- Moved the supply nodes to chip level.
- Removed min-dropout property.
.../bindings/regulator/qcom,pm8008-regulator.yaml | 31 ++++++++++++++++++++++
1 file changed, 31 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..0098845
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
@@ -0,0 +1,31 @@
+# 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.
+
+patternProperties:
+ "^LDO[1-7]$":
+ type: object
+ $ref: "regulator.yaml#"
+ description: PM8008 regulator peripherals of PM8008 regulator device
+
+ properties:
+ regulator-name: true
+
+ required:
+ - regulator-name
+
+ unevaluatedProperties: false
+
+additionalProperties: false
+...
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
2022-02-09 18:51 ` Rob Herring
2022-02-08 14:52 ` [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs Satya Priya
` (3 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Add regulators node and their supply nodes. Add separate compatible
"qcom,pm8008-regulators" to differentiate between pm8008 infra
and pm8008 regulators mfd devices.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Changes in V2:
- As per Rob's comments changed "pm8008[a-z]?-regulator" to
"^pm8008[a-z]?-regulators".
Changes in V3:
- Fixed bot errors.
- As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to
"regulators".
Changes in V4:
- Changed compatible string to "qcom,pm8008-regulators"
Changes in V5:
- Remove compatible for regulators node.
- Move supply nodes of the regulators to chip level.
.../devicetree/bindings/mfd/qcom,pm8008.yaml | 49 +++++++++++++++++++---
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
index ec3138c..fbe84e4 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
@@ -16,7 +16,9 @@ description: |
properties:
compatible:
- const: qcom,pm8008
+ enum:
+ - qcom,pm8008
+ - qcom,pm8008-regulators
reg:
description:
@@ -44,6 +46,25 @@ properties:
"#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.
+
+ regulators:
+ type: object
+ $ref: "../regulator/qcom,pm8008-regulator.yaml#"
+
patternProperties:
"^gpio@[0-9a-f]+$":
type: object
@@ -88,10 +109,8 @@ patternProperties:
required:
- compatible
- reg
- - interrupts
- "#address-cells"
- "#size-cells"
- - "#interrupt-cells"
additionalProperties: false
@@ -102,7 +121,7 @@ examples:
qupv3_se13_i2c {
#address-cells = <1>;
#size-cells = <0>;
- pm8008i@8 {
+ pm8008_infra: pm8008@8 {
compatible = "qcom,pm8008";
reg = <0x8>;
#address-cells = <1>;
@@ -123,6 +142,26 @@ examples:
#interrupt-cells = <2>;
};
};
- };
+ pm8008_regulators: pm8008@9 {
+ compatible = "qcom,pm8008";
+ reg = <0x9>;
+ #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>;
+
+ regulators {
+ pm8008_l1: LDO1 {
+ regulator-name = "pm8008_l1";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+ };
+ };
+ };
+ };
...
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
2022-02-08 14:52 ` [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
2022-02-10 1:32 ` Stephen Boyd
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
` (2 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Register mfd cell ID and name for each of the 7 pm8008
LDOs to probe them through mfd driver without needing
a separate compatible for regulator driver.
Also, add a different compatible for the mfd node that
contains regulators to make sure that the LDOs are
registered with the correct mfd device.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V5:
- Changes newly added from V5.
drivers/mfd/qcom-pm8008.c | 59 +++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index c472d7f..e8569cc 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -8,6 +8,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/mfd/core.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/of_platform.h>
@@ -27,6 +28,37 @@
#define INT_EN_CLR_OFFSET 0x16
#define INT_LATCHED_STS_OFFSET 0x18
+static const struct mfd_cell pm8008_regulator_devs[] = {
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 0,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 1,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 2,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 3,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 4,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 5,
+ },
+ {
+ .name = "qcom,pm8008-regulators",
+ .id = 6,
+ },
+};
+
enum {
PM8008_MISC,
PM8008_TEMP_ALARM,
@@ -35,6 +67,17 @@ enum {
PM8008_NUM_PERIPHS,
};
+enum {
+ PM8008_INFRA,
+ PM8008_REGULATORS,
+};
+
+static const struct of_device_id pm8008_match[] = {
+ { .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
+ { .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
+ { },
+};
+
#define PM8008_PERIPH_0_BASE 0x900
#define PM8008_PERIPH_1_BASE 0x2400
#define PM8008_PERIPH_2_BASE 0xC000
@@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
{
int rc;
struct pm8008_data *chip;
+ const struct of_device_id *id;
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
@@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
}
+ id = of_match_node(pm8008_match, chip->dev->of_node);
+ if (id->data == (void *)PM8008_REGULATORS) {
+ rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,
+ ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
+ if (rc) {
+ dev_err(chip->dev, "Failed to add children: %d\n", rc);
+ return rc;
+ }
+ }
+
return devm_of_platform_populate(chip->dev);
}
-static const struct of_device_id pm8008_match[] = {
- { .compatible = "qcom,pm8008", },
- { },
-};
-
static struct i2c_driver pm8008_mfd_driver = {
.driver = {
.name = "pm8008",
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (2 preceding siblings ...)
2022-02-08 14:52 ` [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
2022-02-09 14:18 ` Mark Brown
` (3 more replies)
2022-02-08 14:52 ` [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
2022-02-08 14:52 ` [PATCH V5 6/6] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp Satya Priya
5 siblings, 4 replies; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
containing 7 LDO regulators. Add a PM8008 regulator driver to
support PMIC regulator management via the regulator framework.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
As per Mark's comments
- Using regmap helpers for regulator enable/disable and is_enabled APIs
- Changed pr_err to dev_err wherever possible.
- Removed init_voltage property as it is not used.
- Removed if check for registering LDOs
- Other minor changes.
Changes in V3:
As per Stephen's comments,
- Removed unused includes
- Removed PM8008_MAX_LDO macro.
- Removed pm8008_read/write APIs, using regmap_bulk_read/write APIs
- Using le16_to_cpu/cpu_to_le16 APIs in pm8008_regulator_get/set_voltage
- Consolidated all probe related functions into single probe function.
- Added of_parse_cb call back and removed regulator-name matching loop.
- Fixed other minor nits.
Changes in V4:
- Removed unused members like rdev and of_node from pm8008_regulator struct.
- Replaced set_voltage with set_voltage_sel
- Removed init_data configuration as it is not needed.
- Removed few other unused assignments from probe
Changes in V5:
- Removed Compatible string.
- Changed the probe function accordingly to probe LDOs using mfd driver.
- Added max headrooms for LDOs and removed the part reading min-dropout from DT.
- Added base reg values in the regulator_data struct instead of reading it from DT.
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-pm8008-regulator.c | 234 ++++++++++++++++++++++++++++++
3 files changed, 244 insertions(+)
create mode 100644 drivers/regulator/qcom-pm8008-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 22503e4..4372ad7 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -925,6 +925,15 @@ config REGULATOR_PWM
This driver supports PWM controlled voltage regulators. PWM
duty cycle can increase or decrease the voltage.
+config REGULATOR_QCOM_PM8008
+ tristate "Qualcomm Technologies, Inc. PM8008 PMIC regulators"
+ depends on MFD_QCOM_PM8008
+ help
+ Select this option to get support for the voltage regulators
+ of Qualcomm Technologies, Inc. PM8008 PMIC chip. PM8008 has 7 LDO
+ regulators. This driver provides support for basic operations like
+ set/get voltage and enable/disable.
+
config REGULATOR_QCOM_RPM
tristate "Qualcomm RPM regulator driver"
depends on MFD_QCOM_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 2e1b087..9ab3ad7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
new file mode 100644
index 0000000..86043b4
--- /dev/null
+++ b/drivers/regulator/qcom-pm8008-regulator.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define STARTUP_DELAY_USEC 20
+#define VSET_STEP_MV 8
+#define VSET_STEP_UV (VSET_STEP_MV * 1000)
+
+#define LDO_ENABLE_REG(base) ((base) + 0x46)
+#define ENABLE_BIT BIT(7)
+
+#define LDO_STATUS1_REG(base) ((base) + 0x08)
+#define VREG_READY_BIT BIT(7)
+
+#define LDO_VSET_LB_REG(base) ((base) + 0x40)
+
+#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b)
+#define DEFAULT_VOLTAGE_STEPPER_RATE 38400
+#define STEP_RATE_MASK GENMASK(1, 0)
+
+#define PM8008_NUM_LDOS 7
+
+struct regulator_data {
+ const char *name;
+ const char *supply_name;
+ u16 base;
+ int min_uv;
+ int max_uv;
+ int min_dropout_uv;
+ const struct linear_range *voltage_range;
+};
+
+struct pm8008_regulator {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator_desc rdesc;
+ u16 base;
+ int step_rate;
+};
+
+static const struct linear_range nldo_ranges[] = {
+ REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
+};
+
+static const struct linear_range pldo_ranges[] = {
+ REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
+};
+
+static const struct regulator_data reg_data[] = {
+ /* name parent base min_uv max_uv headroom_uv voltage_range */
+ { "LDO1", "vdd_l1_l2", 0x4000, 528000, 1504000, 225000, nldo_ranges, },
+ { "LDO2", "vdd_l1_l2", 0x4100, 528000, 1504000, 225000, nldo_ranges, },
+ { "LDO3", "vdd_l3_l4", 0x4200, 1504000, 3400000, 300000, pldo_ranges, },
+ { "LDO4", "vdd_l3_l4", 0x4300, 1504000, 3400000, 300000, pldo_ranges, },
+ { "LDO5", "vdd_l5", 0x4400, 1504000, 3400000, 200000, pldo_ranges, },
+ { "LDO6", "vdd_l6", 0x4500, 1504000, 3400000, 200000, pldo_ranges, },
+ { "LDO7", "vdd_l7", 0x4600, 1504000, 3400000, 200000, pldo_ranges, },
+};
+
+static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+ __le16 mV;
+ int rc;
+
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
+ if (rc < 0) {
+ dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
+ return rc;
+ }
+
+ return le16_to_cpu(mV) * 1000;
+}
+
+static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
+ int mV)
+{
+ int rc;
+ u16 vset_raw;
+
+ vset_raw = cpu_to_le16(mV);
+
+ rc = regmap_bulk_write(pm8008_reg->regmap,
+ LDO_VSET_LB_REG(pm8008_reg->base),
+ (const void *)&vset_raw, sizeof(vset_raw));
+ if (rc < 0) {
+ dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
+ int old_uV, int new_uv)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+
+ return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
+}
+
+static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
+ unsigned int selector)
+{
+ struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
+ int rc, mV;
+
+ /* voltage control register is set with voltage in millivolts */
+ mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
+ 1000);
+ if (mV < 0)
+ return mV;
+
+ rc = pm8008_write_voltage(pm8008_reg, mV);
+ if (rc < 0)
+ return rc;
+
+ dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
+ return 0;
+}
+
+static const struct regulator_ops pm8008_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_voltage_sel = pm8008_regulator_set_voltage,
+ .get_voltage = pm8008_regulator_get_voltage,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_voltage_time = pm8008_regulator_set_voltage_time,
+};
+
+static int pm8008_regulator_of_parse(struct device_node *node,
+ const struct regulator_desc *desc,
+ struct regulator_config *config)
+{
+ struct pm8008_regulator *pm8008_reg = config->driver_data;
+ int rc;
+ unsigned int reg;
+
+ /* get slew rate */
+ rc = regmap_bulk_read(pm8008_reg->regmap,
+ LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
+ if (rc < 0) {
+ dev_err(pm8008_reg->dev,
+ "%s: failed to read step rate configuration rc=%d\n",
+ pm8008_reg->rdesc.name, rc);
+ return rc;
+ }
+ reg &= STEP_RATE_MASK;
+ pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
+
+ return 0;
+}
+
+static int pm8008_regulator_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ int id = pdev->id % PM8008_NUM_LDOS;
+ struct regulator_dev *rdev;
+ struct pm8008_regulator *pm8008_reg;
+ struct regmap *regmap;
+ struct regulator_config reg_config = {};
+ int rc;
+
+ dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
+
+ regmap = dev_get_regmap(dev->parent, NULL);
+ if (!regmap) {
+ dev_err(dev, "parent regmap is missing\n");
+ return -EINVAL;
+ }
+
+ pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
+ if (!pm8008_reg)
+ return -ENOMEM;
+
+ pm8008_reg->regmap = regmap;
+ pm8008_reg->dev = dev;
+ pm8008_reg->base = reg_data[id].base;
+
+ pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
+ pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
+ pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
+ pm8008_reg->rdesc.name = reg_data[id].name;
+ pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
+ pm8008_reg->rdesc.of_match = reg_data[id].name;
+ pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
+ pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
+ pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
+ pm8008_reg->rdesc.n_voltages
+ = ((reg_data[id].max_uv - reg_data[id].min_uv)
+ / pm8008_reg->rdesc.uV_step) + 1;
+ pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
+ pm8008_reg->rdesc.n_linear_ranges = 1;
+ pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
+ pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
+ pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
+
+ reg_config.dev = dev->parent;
+ reg_config.driver_data = pm8008_reg;
+
+ rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config);
+ if (IS_ERR(rdev)) {
+ rc = PTR_ERR(rdev);
+ dev_err(dev, "%s: failed to register regulator rc=%d\n",
+ reg_data[id].name, rc);
+ return rc;
+ }
+
+ return 0;
+}
+
+static struct platform_driver pm8008_regulator_driver = {
+ .driver = {
+ .name = "qcom,pm8008-regulators",
+ },
+ .probe = pm8008_regulator_probe,
+};
+
+module_platform_driver(pm8008_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (3 preceding siblings ...)
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
2022-02-10 1:41 ` Stephen Boyd
2022-02-08 14:52 ` [PATCH V5 6/6] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp Satya Priya
5 siblings, 1 reply; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Add base DTS file for pm8008 with infra and regulator nodes.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V4:
- This is newly added in V4, to add all the pm8008 common stuff.
Changes in V5:
- Changed the mfd node names from pm8008_chip to pm8008_infra and
pm8008_ldo to pm8008_regulators as they re more appropriate.
- Changed the compatible for pm8008@9 mfd node to differentiate from
pm8008@8 node in driver.
- Removed compatible for regulators node.
- Removed reg property for LDOs and added in driver.
arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
new file mode 100644
index 0000000..8e04983
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2021, The Linux Foundation. All rights reserved.
+
+pm8008_infra: pm8008@8 {
+ compatible = "qcom,pm8008";
+ reg = <0x8>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+};
+
+pm8008_regulators: pm8008@9 {
+ compatible = "qcom,pm8008-regulators";
+ reg = <0x9>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ regulators {
+ pm8008_l1: LDO1 {
+ regulator-name = "pm8008_l1";
+ };
+
+ pm8008_l2: LDO2 {
+ regulator-name = "pm8008_l2";
+ };
+
+ pm8008_l3: LDO3 {
+ regulator-name = "pm8008_l3";
+ };
+
+ pm8008_l4: LDO4 {
+ regulator-name = "pm8008_l4";
+ };
+
+ pm8008_l5: LDO5 {
+ regulator-name = "pm8008_l5";
+ };
+
+ pm8008_l6: LDO6 {
+ regulator-name = "pm8008_l6";
+ };
+
+ pm8008_l7: LDO7 {
+ regulator-name = "pm8008_l7";
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH V5 6/6] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
` (4 preceding siblings ...)
2022-02-08 14:52 ` [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
@ 2022-02-08 14:52 ` Satya Priya
5 siblings, 0 replies; 26+ messages in thread
From: Satya Priya @ 2022-02-08 14:52 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash, Satya Priya
Add pm8008_infra and pm8008_regulators support for sc7280 idp.
Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
---
Changes in V2:
- As per Stephen's comments, replaced '_' with '-' for node names.
Changes in V3:
- Changed the regulator node names as l1, l2 etc
- Changed "pm8008-regulators" to "regulators"
- Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt"
Changes in V4:
- Moved all common stuff to pm8008.dtsi and added board specific configurations here.
Changes in V5:
- Changed the node names as per pm8008.dtsi
- Moved supply nodes to chip level (mfd node).
- Removed the regulator-mindropout property.
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index ecbf2b8..371ad19 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -263,6 +263,62 @@
};
};
+&i2c1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "okay";
+
+ #include "pm8008.dtsi"
+};
+
+&pm8008_infra {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pm8008_active>;
+};
+
+&pm8008_regulators {
+ vdd_l1_l2-supply = <&vreg_s8b_1p2>;
+ vdd_l3_l4-supply = <&vreg_s1b_1p8>;
+ vdd_l5-supply = <&vreg_bob>;
+ vdd_l6-supply = <&vreg_bob>;
+ vdd_l7-supply = <&vreg_bob>;
+};
+
+&pm8008_l1 {
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+};
+
+&pm8008_l2 {
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1250000>;
+};
+
+&pm8008_l3 {
+ regulator-min-microvolt = <1650000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l4 {
+ regulator-min-microvolt = <1504000>;
+ regulator-max-microvolt = <1600000>;
+};
+
+&pm8008_l5 {
+ regulator-min-microvolt = <2600000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l6 {
+ regulator-min-microvolt = <2600000>;
+ regulator-max-microvolt = <3000000>;
+};
+
+&pm8008_l7 {
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3544000>;
+};
+
&qfprom {
vcc-supply = <&vreg_l1c_1p8>;
};
@@ -375,6 +431,16 @@
drive-strength = <2>;
};
+&pm8350c_gpios {
+ pm8008_active: pm8008_active {
+ pins = "gpio4";
+ function = "normal";
+ bias-disable;
+ output-high;
+ power-source = <0>;
+ };
+};
+
&qspi_cs0 {
bias-disable;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
@ 2022-02-09 14:18 ` Mark Brown
2022-02-11 10:00 ` Satya Priya Kakitapalli (Temp)
2022-02-10 1:40 ` Stephen Boyd
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2022-02-09 14:18 UTC (permalink / raw)
To: Satya Priya
Cc: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Lee Jones,
Liam Girdwood, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]
On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:
> +static int pm8008_regulator_of_parse(struct device_node *node,
> + const struct regulator_desc *desc,
> + struct regulator_config *config)
> +{
> + struct pm8008_regulator *pm8008_reg = config->driver_data;
> + int rc;
> + unsigned int reg;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev,
> + "%s: failed to read step rate configuration rc=%d\n",
> + pm8008_reg->rdesc.name, rc);
> + return rc;
> + }
> + reg &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> + return 0;
This is not doing any parsing of any DT properties at all, it is just
reading a default value back from the hardware. This shouldn't be in
the of_parse() callback, it should be done on probe() or something so
that if someone adds ACPI support or whatever there's no surprise
breakage, and so that we've got this configured even if there's no DT
bindings for the specific regulator.
> +}
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int id = pdev->id % PM8008_NUM_LDOS;
> + struct regulator_dev *rdev;
> + struct pm8008_regulator *pm8008_reg;
> + struct regmap *regmap;
> + struct regulator_config reg_config = {};
> + int rc;
> +
> + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
> +
> + regmap = dev_get_regmap(dev->parent, NULL);
> + if (!regmap) {
> + dev_err(dev, "parent regmap is missing\n");
> + return -EINVAL;
> + }
> +
> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> + if (!pm8008_reg)
> + return -ENOMEM;
> +
> + pm8008_reg->regmap = regmap;
> + pm8008_reg->dev = dev;
> + pm8008_reg->base = reg_data[id].base;
> +
> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
> + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
> + pm8008_reg->rdesc.name = reg_data[id].name;
> + pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
> + pm8008_reg->rdesc.of_match = reg_data[id].name;
> + pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
> + pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
> + pm8008_reg->rdesc.n_voltages
> + = ((reg_data[id].max_uv - reg_data[id].min_uv)
> + / pm8008_reg->rdesc.uV_step) + 1;
> + pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
> + pm8008_reg->rdesc.n_linear_ranges = 1;
> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
> + pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
> +
> + reg_config.dev = dev->parent;
> + reg_config.driver_data = pm8008_reg;
> +
> + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + rc = PTR_ERR(rdev);
> + dev_err(dev, "%s: failed to register regulator rc=%d\n",
> + reg_data[id].name, rc);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver pm8008_regulator_driver = {
> + .driver = {
> + .name = "qcom,pm8008-regulators",
> + },
> + .probe = pm8008_regulator_probe,
> +};
> +
> +module_platform_driver(pm8008_regulator_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
@ 2022-02-09 18:49 ` Rob Herring
2022-02-10 1:24 ` Stephen Boyd
2022-02-11 0:43 ` Bjorn Andersson
2 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2022-02-09 18:49 UTC (permalink / raw)
To: Satya Priya
Cc: Lee Jones, Bjorn Andersson, swboyd, Das Srinagesh, linux-arm-msm,
quic_subbaram, devicetree, Krzysztof Kozlowski, Liam Girdwood,
quic_jprakash, linux-kernel, quic_collinsd, Mark Brown,
Rob Herring
On Tue, 08 Feb 2022 20:22:15 +0530, Satya Priya wrote:
> Add bindings for pm8008 pmic regulators.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V2:
> - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
> resolve dtschema errors. Removed regulator-min-microvolt and
> regulator-max-microvolt properties.
>
> Changes in V3:
> - As per Rob's comments added standard unit suffix for mindropout property,
> added blank lines where required and added description for reg property.
>
> Changes in V4:
> - Changed compatible string to "com,pm8008-regulators"
> - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
> separate patch.
>
> Changes in V5:
> - Removed the separate compatible for pm8008 regulator driver.
> - Moved the supply nodes to chip level.
> - Removed min-dropout property.
>
> .../bindings/regulator/qcom,pm8008-regulator.yaml | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node
2022-02-08 14:52 ` [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node Satya Priya
@ 2022-02-09 18:51 ` Rob Herring
0 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2022-02-09 18:51 UTC (permalink / raw)
To: Satya Priya
Cc: Rob Herring, Das Srinagesh, linux-arm-msm, quic_collinsd,
linux-kernel, devicetree, Liam Girdwood, Mark Brown,
Bjorn Andersson, Krzysztof Kozlowski, Lee Jones, quic_subbaram,
quic_jprakash, swboyd
On Tue, 08 Feb 2022 20:22:16 +0530, Satya Priya wrote:
> Add regulators node and their supply nodes. Add separate compatible
> "qcom,pm8008-regulators" to differentiate between pm8008 infra
> and pm8008 regulators mfd devices.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> ---
> Changes in V2:
> - As per Rob's comments changed "pm8008[a-z]?-regulator" to
> "^pm8008[a-z]?-regulators".
>
> Changes in V3:
> - Fixed bot errors.
> - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to
> "regulators".
>
> Changes in V4:
> - Changed compatible string to "qcom,pm8008-regulators"
>
> Changes in V5:
> - Remove compatible for regulators node.
> - Move supply nodes of the regulators to chip level.
>
> .../devicetree/bindings/mfd/qcom,pm8008.yaml | 49 +++++++++++++++++++---
> 1 file changed, 44 insertions(+), 5 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
2022-02-09 18:49 ` Rob Herring
@ 2022-02-10 1:24 ` Stephen Boyd
2022-02-11 10:01 ` Satya Priya Kakitapalli (Temp)
2022-02-11 0:43 ` Bjorn Andersson
2 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2022-02-10 1:24 UTC (permalink / raw)
To: Bjorn Andersson, Krzysztof Kozlowski, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
Quoting Satya Priya (2022-02-08 06:52:15)
> 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..0098845
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> @@ -0,0 +1,31 @@
> +# 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.
> +
> +patternProperties:
> + "^LDO[1-7]$":
Any reason it needs to be capitalized vs. lowercase ldo?
> + type: object
> + $ref: "regulator.yaml#"
> + description: PM8008 regulator peripherals of PM8008 regulator device
> +
> + properties:
> + regulator-name: true
> +
> + required:
> + - regulator-name
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs
2022-02-08 14:52 ` [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs Satya Priya
@ 2022-02-10 1:32 ` Stephen Boyd
2022-02-11 10:12 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2022-02-10 1:32 UTC (permalink / raw)
To: Bjorn Andersson, Krzysztof Kozlowski, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
Quoting Satya Priya (2022-02-08 06:52:17)
> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> index c472d7f..e8569cc 100644
> --- a/drivers/mfd/qcom-pm8008.c
> +++ b/drivers/mfd/qcom-pm8008.c
> @@ -8,6 +8,7 @@
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/mfd/core.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/of_platform.h>
> @@ -27,6 +28,37 @@
> #define INT_EN_CLR_OFFSET 0x16
> #define INT_LATCHED_STS_OFFSET 0x18
>
> +static const struct mfd_cell pm8008_regulator_devs[] = {
Is there some way to not allocate this structure statically forever?
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 0,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 1,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 2,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 3,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 4,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 5,
> + },
> + {
> + .name = "qcom,pm8008-regulators",
> + .id = 6,
> + },
> +};
> +
> enum {
> PM8008_MISC,
> pm8008_temp_alarm,
> @@ -35,6 +67,17 @@ enum {
> PM8008_NUM_PERIPHS,
> };
>
> +enum {
> + PM8008_INFRA,
> + PM8008_REGULATORS,
> +};
> +
> +static const struct of_device_id pm8008_match[] = {
> + { .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
> + { .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
> + { },
Nitpick: Drop , on {} so nothing can come after without causing compile
error.
> +};
> +
> #define PM8008_PERIPH_0_BASE 0x900
> #define PM8008_PERIPH_1_BASE 0x2400
> #define PM8008_PERIPH_2_BASE 0xC000
> @@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
> {
> int rc;
> struct pm8008_data *chip;
> + const struct of_device_id *id;
>
> chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> if (!chip)
> @@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
> dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
> }
>
> + id = of_match_node(pm8008_match, chip->dev->of_node);
Use device_get_match_data()? And then use (uintptr_t) casts to check for
the enum? Using device_get_match_data() allows us to avoid moving the
pm8008_match table.
> + if (id->data == (void *)PM8008_REGULATORS) {
enum <your_name_here> dev_type = device_get_match_data(dev);
if (dev_type == PM8008_REGULATORS)
> + rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,
Why not devm_mfd_add_devices()?
> + ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
> + if (rc) {
> + dev_err(chip->dev, "Failed to add children: %d\n", rc);
> + return rc;
> + }
> + }
> +
> return devm_of_platform_populate(chip->dev);
> }
>
> -static const struct of_device_id pm8008_match[] = {
> - { .compatible = "qcom,pm8008", },
> - { },
> -};
This should have a MODULE_DEVICE_TABLE(of, pm8008_match) here.
> -
> static struct i2c_driver pm8008_mfd_driver = {
> .driver = {
> .name = "pm8008",
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
2022-02-09 14:18 ` Mark Brown
@ 2022-02-10 1:40 ` Stephen Boyd
2022-02-11 0:57 ` Bjorn Andersson
2022-02-11 11:01 ` Matti Vaittinen
3 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2022-02-10 1:40 UTC (permalink / raw)
To: Bjorn Andersson, Krzysztof Kozlowski, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
Quoting Satya Priya (2022-02-08 06:52:18)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..86043b4
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +
> +#define STARTUP_DELAY_USEC 20
> +#define VSET_STEP_MV 8
> +#define VSET_STEP_UV (VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base) ((base) + 0x46)
> +#define ENABLE_BIT BIT(7)
> +
> +#define LDO_STATUS1_REG(base) ((base) + 0x08)
> +#define VREG_READY_BIT BIT(7)
> +
> +#define LDO_VSET_LB_REG(base) ((base) + 0x40)
> +
> +#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b)
> +#define DEFAULT_VOLTAGE_STEPPER_RATE 38400
> +#define STEP_RATE_MASK GENMASK(1, 0)
> +
> +#define PM8008_NUM_LDOS 7
> +
> +struct regulator_data {
> + const char *name;
> + const char *supply_name;
> + u16 base;
> + int min_uv;
> + int max_uv;
> + int min_dropout_uv;
> + const struct linear_range *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regulator_desc rdesc;
> + u16 base;
> + int step_rate;
> +};
> +
> +static const struct linear_range nldo_ranges[] = {
> + REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
> +};
> +
> +static const struct linear_range pldo_ranges[] = {
> + REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
> +};
> +
> +static const struct regulator_data reg_data[] = {
> + /* name parent base min_uv max_uv headroom_uv voltage_range */
> + { "LDO1", "vdd_l1_l2", 0x4000, 528000, 1504000, 225000, nldo_ranges, },
> + { "LDO2", "vdd_l1_l2", 0x4100, 528000, 1504000, 225000, nldo_ranges, },
> + { "LDO3", "vdd_l3_l4", 0x4200, 1504000, 3400000, 300000, pldo_ranges, },
> + { "LDO4", "vdd_l3_l4", 0x4300, 1504000, 3400000, 300000, pldo_ranges, },
> + { "LDO5", "vdd_l5", 0x4400, 1504000, 3400000, 200000, pldo_ranges, },
> + { "LDO6", "vdd_l6", 0x4500, 1504000, 3400000, 200000, pldo_ranges, },
> + { "LDO7", "vdd_l7", 0x4600, 1504000, 3400000, 200000, pldo_ranges, },
> +};
> +
> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> + __le16 mV;
> + int rc;
> +
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
> + if (rc < 0) {
> + dev_err(&rdev->dev, "failed to read regulator voltage rc=%d\n", rc);
> + return rc;
> + }
> +
> + return le16_to_cpu(mV) * 1000;
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> + int mV)
> +{
> + int rc;
> + u16 vset_raw;
> +
> + vset_raw = cpu_to_le16(mV);
sparse should complain here that an le16 is degrading to a u16. Please
make vset_raw an __le16 as well.
> +
> + rc = regmap_bulk_write(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base),
> + (const void *)&vset_raw, sizeof(vset_raw));
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev, "failed to write voltage rc=%d\n", rc);
Do we really need this? It could spam the logs in theory. We have
tracepoints on regmap that could be used to figure out that some
read/write failed. I'd like to see a plain
return regmap_bulk_write(...)
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
> + int old_uV, int new_uv)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> + return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
> +}
> +
> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> + int rc, mV;
> +
> + /* voltage control register is set with voltage in millivolts */
> + mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
> + 1000);
Aren't there linear range APIs that can avoid any div roundups here?
> + if (mV < 0)
> + return mV;
> +
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
> + return 0;
> +}
> +
> +static const struct regulator_ops pm8008_regulator_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_voltage_sel = pm8008_regulator_set_voltage,
> + .get_voltage = pm8008_regulator_get_voltage,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_voltage_time = pm8008_regulator_set_voltage_time,
> +};
> +
> +static int pm8008_regulator_of_parse(struct device_node *node,
> + const struct regulator_desc *desc,
> + struct regulator_config *config)
> +{
> + struct pm8008_regulator *pm8008_reg = config->driver_data;
> + int rc;
> + unsigned int reg;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev,
> + "%s: failed to read step rate configuration rc=%d\n",
> + pm8008_reg->rdesc.name, rc);
> + return rc;
> + }
> + reg &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> + return 0;
> +}
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int id = pdev->id % PM8008_NUM_LDOS;
> + struct regulator_dev *rdev;
> + struct pm8008_regulator *pm8008_reg;
> + struct regmap *regmap;
> + struct regulator_config reg_config = {};
> + int rc;
> +
> + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
Why can't we probe one regulators (plural) platform device instead of 8
regulator platform devices? A 'struct device' isn't exactly small and
it would be simpler to probe all the regulators in a single loop.
> +
> + regmap = dev_get_regmap(dev->parent, NULL);
> + if (!regmap) {
> + dev_err(dev, "parent regmap is missing\n");
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file
2022-02-08 14:52 ` [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
@ 2022-02-10 1:41 ` Stephen Boyd
2022-02-11 10:22 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2022-02-10 1:41 UTC (permalink / raw)
To: Bjorn Andersson, Krzysztof Kozlowski, Rob Herring, Satya Priya
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
Quoting Satya Priya (2022-02-08 06:52:19)
> Add base DTS file for pm8008 with infra and regulator nodes.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V4:
> - This is newly added in V4, to add all the pm8008 common stuff.
>
> Changes in V5:
> - Changed the mfd node names from pm8008_chip to pm8008_infra and
> pm8008_ldo to pm8008_regulators as they re more appropriate.
> - Changed the compatible for pm8008@9 mfd node to differentiate from
> pm8008@8 node in driver.
> - Removed compatible for regulators node.
> - Removed reg property for LDOs and added in driver.
>
> arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> new file mode 100644
> index 0000000..8e04983
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +// Copyright (c) 2021, The Linux Foundation. All rights reserved.
> +
> +pm8008_infra: pm8008@8 {
> + compatible = "qcom,pm8008";
> + reg = <0x8>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +};
> +
> +pm8008_regulators: pm8008@9 {
> + compatible = "qcom,pm8008-regulators";
> + reg = <0x9>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + regulators {
What is the point of the 'regulators' node? Why can't we just put LDO1
directly underneath the node that has the "qcom,pm8008-regulators"
compatible?
> + pm8008_l1: LDO1 {
> + regulator-name = "pm8008_l1";
> + };
> +
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
2022-02-09 18:49 ` Rob Herring
2022-02-10 1:24 ` Stephen Boyd
@ 2022-02-11 0:43 ` Bjorn Andersson
2022-02-11 10:23 ` Satya Priya Kakitapalli (Temp)
2 siblings, 1 reply; 26+ messages in thread
From: Bjorn Andersson @ 2022-02-11 0:43 UTC (permalink / raw)
To: Satya Priya
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
> Add bindings for pm8008 pmic regulators.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
> Changes in V2:
> - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
> resolve dtschema errors. Removed regulator-min-microvolt and
> regulator-max-microvolt properties.
>
> Changes in V3:
> - As per Rob's comments added standard unit suffix for mindropout property,
> added blank lines where required and added description for reg property.
>
> Changes in V4:
> - Changed compatible string to "com,pm8008-regulators"
> - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
> separate patch.
>
> Changes in V5:
> - Removed the separate compatible for pm8008 regulator driver.
> - Moved the supply nodes to chip level.
> - Removed min-dropout property.
>
> .../bindings/regulator/qcom,pm8008-regulator.yaml | 31 ++++++++++++++++++++++
> 1 file changed, 31 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..0098845
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
> @@ -0,0 +1,31 @@
> +# 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.
> +
> +patternProperties:
> + "^LDO[1-7]$":
Please make this lower case, to match all other regulator bindings.
> + type: object
> + $ref: "regulator.yaml#"
> + description: PM8008 regulator peripherals of PM8008 regulator device
> +
> + properties:
> + regulator-name: true
> +
> + required:
> + - regulator-name
Why is regulator-name a (and the only) required property?
Regards,
Bjorn
> +
> + unevaluatedProperties: false
> +
> +additionalProperties: false
> +...
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
2022-02-09 14:18 ` Mark Brown
2022-02-10 1:40 ` Stephen Boyd
@ 2022-02-11 0:57 ` Bjorn Andersson
2022-02-11 10:35 ` Satya Priya Kakitapalli (Temp)
2022-02-11 11:01 ` Matti Vaittinen
3 siblings, 1 reply; 26+ messages in thread
From: Bjorn Andersson @ 2022-02-11 0:57 UTC (permalink / raw)
To: Satya Priya
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
[..]
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int id = pdev->id % PM8008_NUM_LDOS;
Why does this driver look completely different from all the other
Qualcomm regulator drivers that we already have, and why do you register
one platform_device per regulator?
The fundamental difference in design makes it hard to maintain and
you're wasting quite a bit of memory with the unnecessary
platfrom_device objects.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-09 14:18 ` Mark Brown
@ 2022-02-11 10:00 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:00 UTC (permalink / raw)
To: Mark Brown
Cc: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Lee Jones,
Liam Girdwood, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On 2/9/2022 7:48 PM, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:
>
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> + const struct regulator_desc *desc,
>> + struct regulator_config *config)
>> +{
>> + struct pm8008_regulator *pm8008_reg = config->driver_data;
>> + int rc;
>> + unsigned int reg;
>> +
>> + /* get slew rate */
>> + rc = regmap_bulk_read(pm8008_reg->regmap,
>> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
>> + if (rc < 0) {
>> + dev_err(pm8008_reg->dev,
>> + "%s: failed to read step rate configuration rc=%d\n",
>> + pm8008_reg->rdesc.name, rc);
>> + return rc;
>> + }
>> + reg &= STEP_RATE_MASK;
>> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> + return 0;
> This is not doing any parsing of any DT properties at all, it is just
> reading a default value back from the hardware. This shouldn't be in
> the of_parse() callback, it should be done on probe() or something so
> that if someone adds ACPI support or whatever there's no surprise
> breakage, and so that we've got this configured even if there's no DT
> bindings for the specific regulator.
Okay, I'll move it to probe.
>
>> +}
>> +
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int id = pdev->id % PM8008_NUM_LDOS;
>> + struct regulator_dev *rdev;
>> + struct pm8008_regulator *pm8008_reg;
>> + struct regmap *regmap;
>> + struct regulator_config reg_config = {};
>> + int rc;
>> +
>> + dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
>> +
>> + regmap = dev_get_regmap(dev->parent, NULL);
>> + if (!regmap) {
>> + dev_err(dev, "parent regmap is missing\n");
>> + return -EINVAL;
>> + }
>> +
>> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
>> + if (!pm8008_reg)
>> + return -ENOMEM;
>> +
>> + pm8008_reg->regmap = regmap;
>> + pm8008_reg->dev = dev;
>> + pm8008_reg->base = reg_data[id].base;
>> +
>> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> + pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
>> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> + pm8008_reg->rdesc.name = reg_data[id].name;
>> + pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
>> + pm8008_reg->rdesc.of_match = reg_data[id].name;
>> + pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
>> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
>> + pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
>> + pm8008_reg->rdesc.n_voltages
>> + = ((reg_data[id].max_uv - reg_data[id].min_uv)
>> + / pm8008_reg->rdesc.uV_step) + 1;
>> + pm8008_reg->rdesc.linear_ranges = reg_data[id].voltage_range;
>> + pm8008_reg->rdesc.n_linear_ranges = 1;
>> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
>> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
>> + pm8008_reg->rdesc.min_dropout_uV = reg_data[id].min_dropout_uv;
>> +
>> + reg_config.dev = dev->parent;
>> + reg_config.driver_data = pm8008_reg;
>> +
>> + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config);
>> + if (IS_ERR(rdev)) {
>> + rc = PTR_ERR(rdev);
>> + dev_err(dev, "%s: failed to register regulator rc=%d\n",
>> + reg_data[id].name, rc);
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver pm8008_regulator_driver = {
>> + .driver = {
>> + .name = "qcom,pm8008-regulators",
>> + },
>> + .probe = pm8008_regulator_probe,
>> +};
>> +
>> +module_platform_driver(pm8008_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-10 1:24 ` Stephen Boyd
@ 2022-02-11 10:01 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:01 UTC (permalink / raw)
To: Stephen Boyd, Bjorn Andersson, Krzysztof Kozlowski, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
On 2/10/2022 6:54 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:15)
>> 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..0098845
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> @@ -0,0 +1,31 @@
>> +# 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.
>> +
>> +patternProperties:
>> + "^LDO[1-7]$":
> Any reason it needs to be capitalized vs. lowercase ldo?
I'll change this to lowercase.
>> + type: object
>> + $ref: "regulator.yaml#"
>> + description: PM8008 regulator peripherals of PM8008 regulator device
>> +
>> + properties:
>> + regulator-name: true
>> +
>> + required:
>> + - regulator-name
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs
2022-02-10 1:32 ` Stephen Boyd
@ 2022-02-11 10:12 ` Satya Priya Kakitapalli (Temp)
2022-02-11 13:34 ` Lee Jones
0 siblings, 1 reply; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:12 UTC (permalink / raw)
To: Stephen Boyd, Bjorn Andersson, Krzysztof Kozlowski, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
On 2/10/2022 7:02 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:17)
>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>> index c472d7f..e8569cc 100644
>> --- a/drivers/mfd/qcom-pm8008.c
>> +++ b/drivers/mfd/qcom-pm8008.c
>> @@ -8,6 +8,7 @@
>> #include <linux/interrupt.h>
>> #include <linux/irq.h>
>> #include <linux/irqdomain.h>
>> +#include <linux/mfd/core.h>
>> #include <linux/module.h>
>> #include <linux/of_device.h>
>> #include <linux/of_platform.h>
>> @@ -27,6 +28,37 @@
>> #define INT_EN_CLR_OFFSET 0x16
>> #define INT_LATCHED_STS_OFFSET 0x18
>>
>> +static const struct mfd_cell pm8008_regulator_devs[] = {
> Is there some way to not allocate this structure statically forever?
I think No.
I found that some of the drivers are just using one cell with .name to
match with regulator driver and then probing regulators using a loop.
I'll do that too.
static const struct mfd_cell pm8008_regulator_devs[] = {
{
.name = "qcom,pm8008-regulators",
},
};
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 0,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 1,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 2,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 3,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 4,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 5,
>> + },
>> + {
>> + .name = "qcom,pm8008-regulators",
>> + .id = 6,
>> + },
>> +};
>> +
>> enum {
>> PM8008_MISC,
>> pm8008_temp_alarm,
>> @@ -35,6 +67,17 @@ enum {
>> PM8008_NUM_PERIPHS,
>> };
>>
>> +enum {
>> + PM8008_INFRA,
>> + PM8008_REGULATORS,
>> +};
>> +
>> +static const struct of_device_id pm8008_match[] = {
>> + { .compatible = "qcom,pm8008", .data = (void *)PM8008_INFRA},
>> + { .compatible = "qcom,pm8008-regulators", .data = (void *)PM8008_REGULATORS},
>> + { },
> Nitpick: Drop , on {} so nothing can come after without causing compile
> error.
Okay.
>> +};
>> +
>> #define PM8008_PERIPH_0_BASE 0x900
>> #define PM8008_PERIPH_1_BASE 0x2400
>> #define PM8008_PERIPH_2_BASE 0xC000
>> @@ -221,6 +264,7 @@ static int pm8008_probe(struct i2c_client *client)
>> {
>> int rc;
>> struct pm8008_data *chip;
>> + const struct of_device_id *id;
>>
>> chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>> if (!chip)
>> @@ -239,14 +283,19 @@ static int pm8008_probe(struct i2c_client *client)
>> dev_err(chip->dev, "Failed to probe irq periphs: %d\n", rc);
>> }
>>
>> + id = of_match_node(pm8008_match, chip->dev->of_node);
> Use device_get_match_data()? And then use (uintptr_t) casts to check for
> the enum? Using device_get_match_data() allows us to avoid moving the
> pm8008_match table.
Okay.
>> + if (id->data == (void *)PM8008_REGULATORS) {
> enum <your_name_here> dev_type = device_get_match_data(dev);
>
> if (dev_type == PM8008_REGULATORS)
>
>> + rc = mfd_add_devices(chip->dev, 0, pm8008_regulator_devs,
> Why not devm_mfd_add_devices()?
Okay.
>> + ARRAY_SIZE(pm8008_regulator_devs), NULL, 0, NULL);
>> + if (rc) {
>> + dev_err(chip->dev, "Failed to add children: %d\n", rc);
>> + return rc;
>> + }
>> + }
>> +
>> return devm_of_platform_populate(chip->dev);
>> }
>>
>> -static const struct of_device_id pm8008_match[] = {
>> - { .compatible = "qcom,pm8008", },
>> - { },
>> -};
> This should have a MODULE_DEVICE_TABLE(of, pm8008_match) here.
Okay.
>> -
>> static struct i2c_driver pm8008_mfd_driver = {
>> .driver = {
>> .name = "pm8008",
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file
2022-02-10 1:41 ` Stephen Boyd
@ 2022-02-11 10:22 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:22 UTC (permalink / raw)
To: Stephen Boyd, Bjorn Andersson, Krzysztof Kozlowski, Rob Herring
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, quic_collinsd,
quic_subbaram, quic_jprakash
On 2/10/2022 7:11 AM, Stephen Boyd wrote:
> Quoting Satya Priya (2022-02-08 06:52:19)
>> Add base DTS file for pm8008 with infra and regulator nodes.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>> Changes in V4:
>> - This is newly added in V4, to add all the pm8008 common stuff.
>>
>> Changes in V5:
>> - Changed the mfd node names from pm8008_chip to pm8008_infra and
>> pm8008_ldo to pm8008_regulators as they re more appropriate.
>> - Changed the compatible for pm8008@9 mfd node to differentiate from
>> pm8008@8 node in driver.
>> - Removed compatible for regulators node.
>> - Removed reg property for LDOs and added in driver.
>>
>> arch/arm64/boot/dts/qcom/pm8008.dtsi | 46 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 46 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/qcom/pm8008.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/pm8008.dtsi b/arch/arm64/boot/dts/qcom/pm8008.dtsi
>> new file mode 100644
>> index 0000000..8e04983
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/pm8008.dtsi
>> @@ -0,0 +1,46 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +// Copyright (c) 2021, The Linux Foundation. All rights reserved.
>> +
>> +pm8008_infra: pm8008@8 {
>> + compatible = "qcom,pm8008";
>> + reg = <0x8>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +};
>> +
>> +pm8008_regulators: pm8008@9 {
>> + compatible = "qcom,pm8008-regulators";
>> + reg = <0x9>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + regulators {
> What is the point of the 'regulators' node? Why can't we just put LDO1
> directly underneath the node that has the "qcom,pm8008-regulators"
> compatible?
It is not mandatory to have it but I think it makes the layout look a
little better. And it seems to be commonly used upstream.
>> + pm8008_l1: LDO1 {
>> + regulator-name = "pm8008_l1";
>> + };
>> +
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings
2022-02-11 0:43 ` Bjorn Andersson
@ 2022-02-11 10:23 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:23 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On 2/11/2022 6:13 AM, Bjorn Andersson wrote:
> On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
>
>> Add bindings for pm8008 pmic regulators.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>> Changes in V2:
>> - Moved this patch before "mfd: pm8008: Add pm8008 regulator node" to
>> resolve dtschema errors. Removed regulator-min-microvolt and
>> regulator-max-microvolt properties.
>>
>> Changes in V3:
>> - As per Rob's comments added standard unit suffix for mindropout property,
>> added blank lines where required and added description for reg property.
>>
>> Changes in V4:
>> - Changed compatible string to "com,pm8008-regulators"
>> - Moved "regulator-min-dropout-voltage-microvolt" to regulator.yaml as
>> separate patch.
>>
>> Changes in V5:
>> - Removed the separate compatible for pm8008 regulator driver.
>> - Moved the supply nodes to chip level.
>> - Removed min-dropout property.
>>
>> .../bindings/regulator/qcom,pm8008-regulator.yaml | 31 ++++++++++++++++++++++
>> 1 file changed, 31 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..0098845
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,pm8008-regulator.yaml
>> @@ -0,0 +1,31 @@
>> +# 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.
>> +
>> +patternProperties:
>> + "^LDO[1-7]$":
> Please make this lower case, to match all other regulator bindings.
Okay.
>> + type: object
>> + $ref: "regulator.yaml#"
>> + description: PM8008 regulator peripherals of PM8008 regulator device
>> +
>> + properties:
>> + regulator-name: true
>> +
>> + required:
>> + - regulator-name
> Why is regulator-name a (and the only) required property?
It is not a required property, I'll correct this.
> Regards,
> Bjorn
>
>> +
>> + unevaluatedProperties: false
>> +
>> +additionalProperties: false
>> +...
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-11 0:57 ` Bjorn Andersson
@ 2022-02-11 10:35 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 10:35 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Krzysztof Kozlowski, Lee Jones, Liam Girdwood,
Mark Brown, Das Srinagesh, linux-arm-msm, devicetree,
linux-kernel, swboyd, quic_collinsd, quic_subbaram,
quic_jprakash
On 2/11/2022 6:27 AM, Bjorn Andersson wrote:
> On Tue 08 Feb 08:52 CST 2022, Satya Priya wrote:
>> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> [..]
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int id = pdev->id % PM8008_NUM_LDOS;
> Why does this driver look completely different from all the other
> Qualcomm regulator drivers that we already have, and why do you register
> one platform_device per regulator?
Based on Mark's suggestions and the discussion happened here [1], I've
changed the design like this.
[1]
https://patchwork.kernel.org/project/linux-arm-msm/patch/1637314953-4215-3-git-send-email-quic_c_skakit@quicinc.com/
> The fundamental difference in design makes it hard to maintain and
> you're wasting quite a bit of memory with the unnecessary
> platfrom_device objects.
I'm going to change this. I will add only one cell with .name to match
with the regulator driver and probe all the regulators using a single
loop. That way we don't waste lot of memory by registering multiple
regulator platform devices.
> Regards,
> Bjorn
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
` (2 preceding siblings ...)
2022-02-11 0:57 ` Bjorn Andersson
@ 2022-02-11 11:01 ` Matti Vaittinen
2022-02-11 14:48 ` Satya Priya Kakitapalli (Temp)
3 siblings, 1 reply; 26+ messages in thread
From: Matti Vaittinen @ 2022-02-11 11:01 UTC (permalink / raw)
To: Satya Priya, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
[-- Attachment #1.1.1: Type: text/plain, Size: 1771 bytes --]
Hi Satya,
It's always nice to see new PMIC drivers :) I just one question after
reading your patch - please ignore it if it has already been discussed
before - for some reason this version was caught by my filters where the
previous versions didn't. It means I do not know the full history.
On 2/8/22 16:52, Satya Priya wrote:
> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
> containing 7 LDO regulators. Add a PM8008 regulator driver to
> support PMIC regulator management via the regulator framework.
>
> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
> ---
snip
> +
> +static int pm8008_regulator_of_parse(struct device_node *node,
> + const struct regulator_desc *desc,
> + struct regulator_config *config)
> +{
> + struct pm8008_regulator *pm8008_reg = config->driver_data;
> + int rc;
> + unsigned int reg;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
> + if (rc < 0) {
> + dev_err(pm8008_reg->dev,
> + "%s: failed to read step rate configuration rc=%d\n",
> + pm8008_reg->rdesc.name, rc);
> + return rc;
> + }
> + reg &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> + return 0;
> +}
I wonder why this is done in the of_parse_cb? Could this perhaps be done
directly in probe - I don't think this is actually parsing the
device_node properties, right?
Overall this looks pretty nice to me.
Best Regards
-- Matti
--
The Linux Kernel guy at ROHM Semiconductors
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~ this year is the year of a signature writers block ~~
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 5335 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs
2022-02-11 10:12 ` Satya Priya Kakitapalli (Temp)
@ 2022-02-11 13:34 ` Lee Jones
2022-02-11 15:11 ` Satya Priya Kakitapalli (Temp)
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2022-02-11 13:34 UTC (permalink / raw)
To: Satya Priya Kakitapalli (Temp)
Cc: Stephen Boyd, Bjorn Andersson, Krzysztof Kozlowski, Rob Herring,
Liam Girdwood, Mark Brown, Das Srinagesh, linux-arm-msm,
devicetree, linux-kernel, quic_collinsd, quic_subbaram,
quic_jprakash
On Fri, 11 Feb 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> On 2/10/2022 7:02 AM, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-02-08 06:52:17)
> > > diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
> > > index c472d7f..e8569cc 100644
> > > --- a/drivers/mfd/qcom-pm8008.c
> > > +++ b/drivers/mfd/qcom-pm8008.c
> > > @@ -8,6 +8,7 @@
> > > #include <linux/interrupt.h>
> > > #include <linux/irq.h>
> > > #include <linux/irqdomain.h>
> > > +#include <linux/mfd/core.h>
> > > #include <linux/module.h>
> > > #include <linux/of_device.h>
> > > #include <linux/of_platform.h>
> > > @@ -27,6 +28,37 @@
> > > #define INT_EN_CLR_OFFSET 0x16
> > > #define INT_LATCHED_STS_OFFSET 0x18
> > >
> > > +static const struct mfd_cell pm8008_regulator_devs[] = {
> > Is there some way to not allocate this structure statically forever?
>
>
> I think No.
>
> I found that some of the drivers are just using one cell with .name to match
> with regulator driver and then probing regulators using a loop. I'll do that
> too.
>
> static const struct mfd_cell pm8008_regulator_devs[] = {
> {
> .name = "qcom,pm8008-regulators",
> },
> };
Please use MFD_CELL_NAME() for these.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC
2022-02-11 11:01 ` Matti Vaittinen
@ 2022-02-11 14:48 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 14:48 UTC (permalink / raw)
To: Matti Vaittinen, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski
Cc: Lee Jones, Liam Girdwood, Mark Brown, Das Srinagesh,
linux-arm-msm, devicetree, linux-kernel, swboyd, quic_collinsd,
quic_subbaram, quic_jprakash
Hi Matti,
Thanks for reviewing the patches!
On 2/11/2022 4:31 PM, Matti Vaittinen wrote:
> Hi Satya,
>
> It's always nice to see new PMIC drivers :) I just one question after
> reading your patch - please ignore it if it has already been discussed
> before - for some reason this version was caught by my filters where
> the previous versions didn't. It means I do not know the full history.
> On 2/8/22 16:52, Satya Priya wrote:
>> Qualcomm Technologies, Inc. PM8008 is an I2C controlled PMIC
>> containing 7 LDO regulators. Add a PM8008 regulator driver to
>> support PMIC regulator management via the regulator framework.
>>
>> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com>
>> ---
>
> snip
>
>> +
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> + const struct regulator_desc *desc,
>> + struct regulator_config *config)
>> +{
>> + struct pm8008_regulator *pm8008_reg = config->driver_data;
>> + int rc;
>> + unsigned int reg;
>> +
>> + /* get slew rate */
>> + rc = regmap_bulk_read(pm8008_reg->regmap,
>> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
>> + if (rc < 0) {
>> + dev_err(pm8008_reg->dev,
>> + "%s: failed to read step rate configuration rc=%d\n",
>> + pm8008_reg->rdesc.name, rc);
>> + return rc;
>> + }
>> + reg &= STEP_RATE_MASK;
>> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> + return 0;
>> +}
>
> I wonder why this is done in the of_parse_cb? Could this perhaps be
> done directly in probe - I don't think this is actually parsing the
> device_node properties, right?
>
Right, I will move this part to probe. In the previous version there was
some code here which did the DT parsing, now that I removed all that, I
should move this to probe.
Thanks,
Satya Priya
> Overall this looks pretty nice to me.
>
> Best Regards
> -- Matti
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs
2022-02-11 13:34 ` Lee Jones
@ 2022-02-11 15:11 ` Satya Priya Kakitapalli (Temp)
0 siblings, 0 replies; 26+ messages in thread
From: Satya Priya Kakitapalli (Temp) @ 2022-02-11 15:11 UTC (permalink / raw)
To: Lee Jones
Cc: Stephen Boyd, Bjorn Andersson, Krzysztof Kozlowski, Rob Herring,
Liam Girdwood, Mark Brown, Das Srinagesh, linux-arm-msm,
devicetree, linux-kernel, quic_collinsd, quic_subbaram,
quic_jprakash
Hi,
On 2/11/2022 7:04 PM, Lee Jones wrote:
> On Fri, 11 Feb 2022, Satya Priya Kakitapalli (Temp) wrote:
>
>> On 2/10/2022 7:02 AM, Stephen Boyd wrote:
>>> Quoting Satya Priya (2022-02-08 06:52:17)
>>>> diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
>>>> index c472d7f..e8569cc 100644
>>>> --- a/drivers/mfd/qcom-pm8008.c
>>>> +++ b/drivers/mfd/qcom-pm8008.c
>>>> @@ -8,6 +8,7 @@
>>>> #include <linux/interrupt.h>
>>>> #include <linux/irq.h>
>>>> #include <linux/irqdomain.h>
>>>> +#include <linux/mfd/core.h>
>>>> #include <linux/module.h>
>>>> #include <linux/of_device.h>
>>>> #include <linux/of_platform.h>
>>>> @@ -27,6 +28,37 @@
>>>> #define INT_EN_CLR_OFFSET 0x16
>>>> #define INT_LATCHED_STS_OFFSET 0x18
>>>>
>>>> +static const struct mfd_cell pm8008_regulator_devs[] = {
>>> Is there some way to not allocate this structure statically forever?
>>
>> I think No.
>>
>> I found that some of the drivers are just using one cell with .name to match
>> with regulator driver and then probing regulators using a loop. I'll do that
>> too.
>>
>> static const struct mfd_cell pm8008_regulator_devs[] = {
>> {
>> .name = "qcom,pm8008-regulators",
>> },
>> };
> Please use MFD_CELL_NAME() for these.
Okay.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-02-11 15:11 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 14:52 [PATCH V5 0/6] Add Qualcomm Technologies, Inc. PM8008 regulator driver Satya Priya
2022-02-08 14:52 ` [PATCH V5 1/6] dt-bindings: regulator: Add pm8008 regulator bindings Satya Priya
2022-02-09 18:49 ` Rob Herring
2022-02-10 1:24 ` Stephen Boyd
2022-02-11 10:01 ` Satya Priya Kakitapalli (Temp)
2022-02-11 0:43 ` Bjorn Andersson
2022-02-11 10:23 ` Satya Priya Kakitapalli (Temp)
2022-02-08 14:52 ` [PATCH V5 2/6] dt-bindings: mfd: pm8008: Add regulators node Satya Priya
2022-02-09 18:51 ` Rob Herring
2022-02-08 14:52 ` [PATCH V5 3/6] mfd: pm8008: Add mfd cell struct to register LDOs Satya Priya
2022-02-10 1:32 ` Stephen Boyd
2022-02-11 10:12 ` Satya Priya Kakitapalli (Temp)
2022-02-11 13:34 ` Lee Jones
2022-02-11 15:11 ` Satya Priya Kakitapalli (Temp)
2022-02-08 14:52 ` [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008 PMIC Satya Priya
2022-02-09 14:18 ` Mark Brown
2022-02-11 10:00 ` Satya Priya Kakitapalli (Temp)
2022-02-10 1:40 ` Stephen Boyd
2022-02-11 0:57 ` Bjorn Andersson
2022-02-11 10:35 ` Satya Priya Kakitapalli (Temp)
2022-02-11 11:01 ` Matti Vaittinen
2022-02-11 14:48 ` Satya Priya Kakitapalli (Temp)
2022-02-08 14:52 ` [PATCH V5 5/6] arm64: dts: qcom: pm8008: Add base dts file Satya Priya
2022-02-10 1:41 ` Stephen Boyd
2022-02-11 10:22 ` Satya Priya Kakitapalli (Temp)
2022-02-08 14:52 ` [PATCH V5 6/6] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp Satya Priya
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.