LinuxLists.cc - [PATCH v2 0/5] PM6125 regulator support

[permalink] [raw]

Subject: [PATCH v2 1/5] dt-bindings: regulator: Document the PM6125 SPMI PMIC

Add support for pm6125 compatible string and add relevant supplies in QCom SPMI
regulator documentation.

Signed-off-by: Iskren Chernev <[email protected]>
Signed-off-by: Adam Skladowski <[email protected]>
---
.../regulator/qcom,spmi-regulator.yaml | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
index 8b7c4af4b551..d8f18b441484 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
@@ -12,6 +12,7 @@ maintainers:
properties:
compatible:
enum:
+ - qcom,pm6125-regulators
- qcom,pm660-regulators
- qcom,pm660l-regulators
- qcom,pm8004-regulators
@@ -106,6 +107,24 @@ required:
- compatible

allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,pm6125-regulators
+ then:
+ properties:
+ vdd_l1_l7_l17_l18-supply: true
+ vdd_l2_l3_l4-supply: true
+ vdd_l5_l15_l19_l20_l21_l22-supply: true
+ vdd_l6_l8-supply: true
+ vdd_l9_l11-supply: true
+ vdd_l10_l13_l14-supply: true
+ vdd_l12_l16-supply: true
+ vdd_l23_l24-supply: true
+ patternProperties:
+ "^vdd_s[1-8]-supply$": true
- if:
properties:
compatible:
--
2.37.1

2022-07-26 18:15:12

[permalink] [raw]

Subject: [PATCH v2 5/5] regulator: qcom_smd: Add PM6125 regulators support

Add support for PM6125 PMIC which is found on SM4250/6115 SoCs.

Signed-off-by: Iskren Chernev <[email protected]>
Signed-off-by: Adam Skladowski <[email protected]>
---
drivers/regulator/qcom_smd-regulator.c | 46 ++++++++++++++++++++++++++
1 file changed, 46 insertions(+)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 59024c639141..2ce2941a032f 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -668,6 +668,15 @@ static const struct regulator_desc pm660l_bob = {
.ops = &rpm_bob_ops,
};

+static const struct regulator_desc pm6125_ftsmps = {
+ .linear_ranges = (struct linear_range[]) {
+ REGULATOR_LINEAR_RANGE(300000, 0, 268, 4000),
+ },
+ .n_linear_ranges = 1,
+ .n_voltages = 269,
+ .ops = &rpm_smps_ldo_ops,
+};
+
static const struct regulator_desc pms405_hfsmps3 = {
.linear_ranges = (struct linear_range[]) {
REGULATOR_LINEAR_RANGE(320000, 0, 215, 8000),
@@ -1190,6 +1199,42 @@ static const struct rpm_regulator_data rpm_pm660l_regulators[] = {
{ }
};

+static const struct rpm_regulator_data rpm_pm6125_regulators[] = {
+ { "s1", QCOM_SMD_RPM_SMPA, 1, &pm6125_ftsmps, "vdd_s1" },
+ { "s2", QCOM_SMD_RPM_SMPA, 2, &pm6125_ftsmps, "vdd_s2" },
+ { "s3", QCOM_SMD_RPM_SMPA, 3, &pm6125_ftsmps, "vdd_s3" },
+ { "s4", QCOM_SMD_RPM_SMPA, 4, &pm6125_ftsmps, "vdd_s4" },
+ { "s5", QCOM_SMD_RPM_SMPA, 5, &pm8998_hfsmps, "vdd_s5" },
+ { "s6", QCOM_SMD_RPM_SMPA, 6, &pm8998_hfsmps, "vdd_s6" },
+ { "s7", QCOM_SMD_RPM_SMPA, 7, &pm8998_hfsmps, "vdd_s7" },
+ { "s8", QCOM_SMD_RPM_SMPA, 8, &pm6125_ftsmps, "vdd_s8" },
+ { "l1", QCOM_SMD_RPM_LDOA, 1, &pm660_nldo660, "vdd_l1_l7_l17_l18" },
+ { "l2", QCOM_SMD_RPM_LDOA, 2, &pm660_nldo660, "vdd_l2_l3_l4" },
+ { "l3", QCOM_SMD_RPM_LDOA, 3, &pm660_nldo660, "vdd_l2_l3_l4" },
+ { "l4", QCOM_SMD_RPM_LDOA, 4, &pm660_nldo660, "vdd_l2_l3_l4" },
+ { "l5", QCOM_SMD_RPM_LDOA, 5, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l6", QCOM_SMD_RPM_LDOA, 6, &pm660_nldo660, "vdd_l6_l8" },
+ { "l7", QCOM_SMD_RPM_LDOA, 7, &pm660_nldo660, "vdd_l1_l7_l17_l18" },
+ { "l8", QCOM_SMD_RPM_LDOA, 8, &pm660_nldo660, "vdd_l6_l8" },
+ { "l9", QCOM_SMD_RPM_LDOA, 9, &pm660_ht_lvpldo, "vdd_l9_l11" },
+ { "l10", QCOM_SMD_RPM_LDOA, 10, &pm660_ht_lvpldo, "vdd_l10_l13_l14" },
+ { "l11", QCOM_SMD_RPM_LDOA, 11, &pm660_ht_lvpldo, "vdd_l9_l11" },
+ { "l12", QCOM_SMD_RPM_LDOA, 12, &pm660_ht_lvpldo, "vdd_l12_l16" },
+ { "l13", QCOM_SMD_RPM_LDOA, 13, &pm660_ht_lvpldo, "vdd_l10_l13_l14" },
+ { "l14", QCOM_SMD_RPM_LDOA, 14, &pm660_ht_lvpldo, "vdd_l10_l13_l14" },
+ { "l15", QCOM_SMD_RPM_LDOA, 15, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l16", QCOM_SMD_RPM_LDOA, 16, &pm660_ht_lvpldo, "vdd_l12_l16" },
+ { "l17", QCOM_SMD_RPM_LDOA, 17, &pm660_nldo660, "vdd_l1_l7_l17_l18" },
+ { "l18", QCOM_SMD_RPM_LDOA, 18, &pm660_nldo660, "vdd_l1_l7_l17_l18" },
+ { "l19", QCOM_SMD_RPM_LDOA, 19, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l20", QCOM_SMD_RPM_LDOA, 20, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l21", QCOM_SMD_RPM_LDOA, 21, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l22", QCOM_SMD_RPM_LDOA, 22, &pm660_pldo660, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l23", QCOM_SMD_RPM_LDOA, 23, &pm660_pldo660, "vdd_l23_l24" },
+ { "l24", QCOM_SMD_RPM_LDOA, 24, &pm660_pldo660, "vdd_l23_l24" },
+ { }
+};
+
static const struct rpm_regulator_data rpm_pms405_regulators[] = {
{ "s1", QCOM_SMD_RPM_SMPA, 1, &pms405_hfsmps3, "vdd_s1" },
{ "s2", QCOM_SMD_RPM_SMPA, 2, &pms405_hfsmps3, "vdd_s2" },
@@ -1255,6 +1300,7 @@ static const struct of_device_id rpm_of_match[] = {
{ .compatible = "qcom,rpm-pm8998-regulators", .data = &rpm_pm8998_regulators },
{ .compatible = "qcom,rpm-pm660-regulators", .data = &rpm_pm660_regulators },
{ .compatible = "qcom,rpm-pm660l-regulators", .data = &rpm_pm660l_regulators },
+ { .compatible = "qcom,rpm-pm6125-regulators", .data = &rpm_pm6125_regulators },
{ .compatible = "qcom,rpm-pma8084-regulators", .data = &rpm_pma8084_regulators },
{ .compatible = "qcom,rpm-pmi8994-regulators", .data = &rpm_pmi8994_regulators },
{ .compatible = "qcom,rpm-pmi8998-regulators", .data = &rpm_pmi8998_regulators },
--
2.37.1

2022-07-26 18:15:30

[permalink] [raw]

Subject: [PATCH v2 2/5] dt-bindings: regulator: Document the PM6125 RPM regulators

Document the pm6125 compatible string and available regulators in the QCom SMD
RPM regulator documentation.

Signed-off-by: Iskren Chernev <[email protected]>
Signed-off-by: Adam Skladowski <[email protected]>
---
.../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
index c233461cc980..1122a3a17f56 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
@@ -57,6 +57,9 @@ description:

For pm660l s1, s2, s3, s5, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, bob

+ For pm6125 s1, s2, s3, s4, s5, s6, s7, s8, l1, l2, l3, l5, l6, l7, l8, l9,
+ l10, l22, l12, l13, l14, l15, l16, l17, l18, l19, l20, l21, l22, l23, l24
+
For pma8084, s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, l1, l2, l3,
l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15, l16, l17, l18, l19,
l20, l21, l22, l23, l24, l25, l26, l27, lvs1, lvs2, lvs3, lvs4, 5vs1
@@ -90,6 +93,7 @@ properties:
- qcom,rpm-pm8998-regulators
- qcom,rpm-pm660-regulators
- qcom,rpm-pm660l-regulators
+ - qcom,rpm-pm6125-regulators
- qcom,rpm-pma8084-regulators
- qcom,rpm-pmi8994-regulators
- qcom,rpm-pmi8998-regulators
--
2.37.1

2022-07-26 18:26:26

[permalink] [raw]

Subject: [PATCH v2 4/5] regulator: qcom_spmi: Add PM6125 PMIC support

Add pm6125 regulators and their sources in the QCom SPMI driver.

Signed-off-by: Iskren Chernev <[email protected]>
Signed-off-by: Adam Skladowski <[email protected]>
---
drivers/regulator/qcom_spmi-regulator.c | 37 ++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index efb3f6fffb4f..ca19b4404ea9 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -1993,6 +1993,41 @@ static int spmi_regulator_of_parse(struct device_node *node,
return 0;
}

+static const struct spmi_regulator_data pm6125_regulators[] = {
+ { "s1", 0x1400, "vdd_s1" },
+ { "s2", 0x1700, "vdd_s2" },
+ { "s3", 0x1a00, "vdd_s3" },
+ { "s4", 0x1d00, "vdd_s4" },
+ { "s5", 0x2000, "vdd_s5" },
+ { "s6", 0x2300, "vdd_s6" },
+ { "s7", 0x2600, "vdd_s7" },
+ { "s8", 0x2900, "vdd_s8" },
+ { "l1", 0x4000, "vdd_l1_l7_l17_l18" },
+ { "l2", 0x4100, "vdd_l2_l3_l4" },
+ { "l3", 0x4200, "vdd_l2_l3_l4" },
+ { "l4", 0x4300, "vdd_l2_l3_l4" },
+ { "l5", 0x4400, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l6", 0x4500, "vdd_l6_l8" },
+ { "l7", 0x4600, "vdd_l1_l7_l17_l18" },
+ { "l8", 0x4700, "vdd_l6_l8" },
+ { "l9", 0x4800, "vdd_l9_l11" },
+ { "l10", 0x4900, "vdd_l10_l13_l14" },
+ { "l11", 0x4a00, "vdd_l9_l11" },
+ { "l12", 0x4b00, "vdd_l12_l16" },
+ { "l13", 0x4c00, "vdd_l10_l13_l14" },
+ { "l14", 0x4d00, "vdd_l10_l13_l14" },
+ { "l15", 0x4e00, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l16", 0x4f00, "vdd_l12_l16" },
+ { "l17", 0x5000, "vdd_l1_l7_l17_l18" },
+ { "l18", 0x5100, "vdd_l1_l7_l17_l18" },
+ { "l19", 0x5200, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l20", 0x5300, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l21", 0x5400, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l22", 0x5500, "vdd_l5_l15_l19_l20_l21_l22" },
+ { "l23", 0x5600, "vdd_l23_l24" },
+ { "l24", 0x5700, "vdd_l23_l24" },
+};
+
static const struct spmi_regulator_data pm8941_regulators[] = {
{ "s1", 0x1400, "vdd_s1", },
{ "s2", 0x1700, "vdd_s2", },
@@ -2245,7 +2280,6 @@ static const struct spmi_regulator_data pm660l_regulators[] = {
{ }
};

-
static const struct spmi_regulator_data pm8004_regulators[] = {
{ "s2", 0x1700, "vdd_s2", },
{ "s5", 0x2000, "vdd_s5", },
@@ -2288,6 +2322,7 @@ static const struct spmi_regulator_data pms405_regulators[] = {
};

static const struct of_device_id qcom_spmi_regulator_match[] = {
+ { .compatible = "qcom,pm6125-regulators", .data = &pm6125_regulators },
{ .compatible = "qcom,pm8004-regulators", .data = &pm8004_regulators },
{ .compatible = "qcom,pm8005-regulators", .data = &pm8005_regulators },
{ .compatible = "qcom,pm8226-regulators", .data = &pm8226_regulators },
--
2.37.1

2022-07-26 18:42:29

[permalink] [raw]

Subject: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

Add support for some regulator types that are missing in this driver, all
belonging to the FTSMPS426 register layout. This is done in preparation
for adding support for the PM6125 PMIC.

The inspiration for the magic constants was taken from [1]

[1]: https://source.codeaurora.org/quic/la/kernel/msm-5.4/commit/?h=kernel.lnx.5.4.r1-rel&id=d1220daeffaa440ffff0a8c47322eb0033bf54f5

Signed-off-by: Iskren Chernev <[email protected]>
Signed-off-by: Adam Skladowski <[email protected]>
---
drivers/regulator/qcom_spmi-regulator.c | 123 +++++++++++++++++++++++-
1 file changed, 122 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index a2d0292a92fd..efb3f6fffb4f 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -99,6 +99,9 @@ enum spmi_regulator_logical_type {
SPMI_REGULATOR_LOGICAL_TYPE_ULT_LDO,
SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS426,
SPMI_REGULATOR_LOGICAL_TYPE_HFS430,
+ SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS3,
+ SPMI_REGULATOR_LOGICAL_TYPE_LDO_510,
+ SPMI_REGULATOR_LOGICAL_TYPE_HFSMPS,
};

enum spmi_regulator_type {
@@ -166,6 +169,17 @@ enum spmi_regulator_subtype {
SPMI_REGULATOR_SUBTYPE_HFS430 = 0x0a,
SPMI_REGULATOR_SUBTYPE_HT_P150 = 0x35,
SPMI_REGULATOR_SUBTYPE_HT_P600 = 0x3d,
+ SPMI_REGULATOR_SUBTYPE_HFSMPS_510 = 0x0a,
+ SPMI_REGULATOR_SUBTYPE_FTSMPS_510 = 0x0b,
+ SPMI_REGULATOR_SUBTYPE_LV_P150_510 = 0x71,
+ SPMI_REGULATOR_SUBTYPE_LV_P300_510 = 0x72,
+ SPMI_REGULATOR_SUBTYPE_LV_P600_510 = 0x73,
+ SPMI_REGULATOR_SUBTYPE_N300_510 = 0x6a,
+ SPMI_REGULATOR_SUBTYPE_N600_510 = 0x6b,
+ SPMI_REGULATOR_SUBTYPE_N1200_510 = 0x6c,
+ SPMI_REGULATOR_SUBTYPE_MV_P50_510 = 0x7a,
+ SPMI_REGULATOR_SUBTYPE_MV_P150_510 = 0x7b,
+ SPMI_REGULATOR_SUBTYPE_MV_P600_510 = 0x7d,
};

enum spmi_common_regulator_registers {
@@ -193,6 +207,14 @@ enum spmi_ftsmps426_regulator_registers {
SPMI_FTSMPS426_REG_VOLTAGE_ULS_MSB = 0x69,
};

+/*
+ * Third common register layout
+ */
+enum spmi_ftsmps3_regulator_registers {
+ SPMI_FTSMPS3_REG_STEP_CTRL = 0x3c,
+};
+
+
enum spmi_vs_registers {
SPMI_VS_REG_OCP = 0x4a,
SPMI_VS_REG_SOFT_START = 0x4c,
@@ -260,6 +282,15 @@ enum spmi_common_control_register_index {

#define SPMI_FTSMPS426_MODE_MASK 0x07

+/* Third common regulator mode register values */
+#define SPMI_FTSMPS3_MODE_BYPASS_MASK 2
+#define SPMI_FTSMPS3_MODE_RETENTION_MASK 3
+#define SPMI_FTSMPS3_MODE_LPM_MASK 4
+#define SPMI_FTSMPS3_MODE_AUTO_MASK 6
+#define SPMI_FTSMPS3_MODE_HPM_MASK 7
+
+#define SPMI_FTSMPS3_MODE_MASK 0x07
+
/* Common regulator pull down control register layout */
#define SPMI_COMMON_PULL_DOWN_ENABLE_MASK 0x80

@@ -305,6 +336,9 @@ enum spmi_common_control_register_index {
#define SPMI_FTSMPS_STEP_MARGIN_NUM 4
#define SPMI_FTSMPS_STEP_MARGIN_DEN 5

+/* slew_rate has units of uV/us. */
+#define SPMI_FTSMPS3_SLEW_RATE_38p4 38400
+
#define SPMI_FTSMPS426_STEP_CTRL_DELAY_MASK 0x03
#define SPMI_FTSMPS426_STEP_CTRL_DELAY_SHIFT 0

@@ -554,6 +588,14 @@ static struct spmi_voltage_range ht_p600_ranges[] = {
SPMI_VOLTAGE_RANGE(0, 1704000, 1704000, 1896000, 1896000, 8000),
};

+static struct spmi_voltage_range nldo_510_ranges[] = {
+ SPMI_VOLTAGE_RANGE(0, 320000, 320000, 1304000, 1304000, 8000),
+};
+
+static struct spmi_voltage_range ftsmps510_ranges[] = {
+ SPMI_VOLTAGE_RANGE(0, 300000, 300000, 1372000, 1372000, 4000),
+};
+
static DEFINE_SPMI_SET_POINTS(pldo);
static DEFINE_SPMI_SET_POINTS(nldo1);
static DEFINE_SPMI_SET_POINTS(nldo2);
@@ -576,6 +618,8 @@ static DEFINE_SPMI_SET_POINTS(ht_nldo);
static DEFINE_SPMI_SET_POINTS(hfs430);
static DEFINE_SPMI_SET_POINTS(ht_p150);
static DEFINE_SPMI_SET_POINTS(ht_p600);
+static DEFINE_SPMI_SET_POINTS(nldo_510);
+static DEFINE_SPMI_SET_POINTS(ftsmps510);

static inline int spmi_vreg_read(struct spmi_regulator *vreg, u16 addr, u8 *buf,
int len)
@@ -1108,6 +1152,33 @@ spmi_regulator_ftsmps426_set_mode(struct regulator_dev *rdev, unsigned int mode)
return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
}

+static int
+spmi_regulator_ftsmps3_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+ u8 mask = SPMI_FTSMPS3_MODE_MASK;
+ u8 val;
+
+ switch (mode) {
+ case REGULATOR_MODE_NORMAL:
+ val = SPMI_FTSMPS3_MODE_HPM_MASK;
+ break;
+ case REGULATOR_MODE_FAST:
+ val = SPMI_FTSMPS3_MODE_AUTO_MASK;
+ break;
+ case REGULATOR_MODE_IDLE:
+ val = vreg->logical_type ==
+ SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS3 ?
+ SPMI_FTSMPS3_MODE_RETENTION_MASK :
+ SPMI_FTSMPS3_MODE_LPM_MASK;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
+}
+
static int
spmi_regulator_common_set_load(struct regulator_dev *rdev, int load_uA)
{
@@ -1465,6 +1536,19 @@ static const struct regulator_ops spmi_hfs430_ops = {
.get_mode = spmi_regulator_ftsmps426_get_mode,
};

+static const struct regulator_ops spmi_ftsmps3_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_voltage_sel = spmi_regulator_ftsmps426_set_voltage,
+ .set_voltage_time_sel = spmi_regulator_set_voltage_time_sel,
+ .get_voltage_sel = spmi_regulator_ftsmps426_get_voltage,
+ .map_voltage = spmi_regulator_single_map_voltage,
+ .list_voltage = spmi_regulator_common_list_voltage,
+ .set_mode = spmi_regulator_ftsmps3_set_mode,
+ .get_mode = spmi_regulator_ftsmps426_get_mode,
+};
+
/* Maximum possible digital major revision value */
#define INF 0xFF

@@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
SPMI_VREG(LDO, HT_P600, 0, INF, HFS430, hfs430, ht_p600, 10000),
SPMI_VREG(LDO, HT_P150, 0, INF, HFS430, hfs430, ht_p150, 10000),
SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000),
- SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000),
+ SPMI_VREG(BUCK, HFS430, 0, 3, HFS430, hfs430, hfs430, 10000),
SPMI_VREG(LDO, N300, 0, INF, LDO, ldo, nldo1, 10000),
SPMI_VREG(LDO, N600, 0, 0, LDO, ldo, nldo2, 10000),
SPMI_VREG(LDO, N1200, 0, 0, LDO, ldo, nldo2, 10000),
@@ -1549,6 +1633,17 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
SPMI_VREG(ULT_LDO, P300, 0, INF, ULT_LDO, ult_ldo, ult_pldo, 10000),
SPMI_VREG(ULT_LDO, P150, 0, INF, ULT_LDO, ult_ldo, ult_pldo, 10000),
SPMI_VREG(ULT_LDO, P50, 0, INF, ULT_LDO, ult_ldo, ult_pldo, 5000),
+ SPMI_VREG(LDO, LV_P150_510, 0, INF, LDO_510, ftsmps3, ht_lvpldo, 10000),
+ SPMI_VREG(LDO, LV_P300_510, 0, INF, LDO_510, ftsmps3, ht_lvpldo, 10000),
+ SPMI_VREG(LDO, LV_P600_510, 0, INF, LDO_510, ftsmps3, ht_lvpldo, 10000),
+ SPMI_VREG(LDO, MV_P50_510, 0, INF, LDO_510, ftsmps3, pldo660, 10000),
+ SPMI_VREG(LDO, MV_P150_510, 0, INF, LDO_510, ftsmps3, pldo660, 10000),
+ SPMI_VREG(LDO, MV_P600_510, 0, INF, LDO_510, ftsmps3, pldo660, 10000),
+ SPMI_VREG(LDO, N300_510, 0, INF, LDO_510, ftsmps3, nldo_510, 10000),
+ SPMI_VREG(LDO, N600_510, 0, INF, LDO_510, ftsmps3, nldo_510, 10000),
+ SPMI_VREG(LDO, N1200_510, 0, INF, LDO_510, ftsmps3, nldo_510, 10000),
+ SPMI_VREG(BUCK, HFSMPS_510, 4, INF, HFSMPS, ftsmps3, hfs430, 100000),
+ SPMI_VREG(FTS, FTSMPS_510, 0, INF, FTSMPS3, ftsmps3, ftsmps510, 100000),
};

static void spmi_calculate_num_voltages(struct spmi_voltage_set_points *points)
@@ -1696,6 +1791,27 @@ static int spmi_regulator_init_slew_rate_ftsmps426(struct spmi_regulator *vreg,
return ret;
}

+static int spmi_regulator_init_slew_rate_ftsmps3(struct spmi_regulator *vreg)
+{
+ int ret;
+ u8 reg = 0;
+ int delay;
+
+ ret = spmi_vreg_read(vreg, SPMI_FTSMPS3_REG_STEP_CTRL, &reg, 1);
+ if (ret) {
+ dev_err(vreg->dev, "spmi read failed, ret=%d\n", ret);
+ return ret;
+ }
+
+ delay = reg & SPMI_FTSMPS426_STEP_CTRL_DELAY_MASK;
+ delay >>= SPMI_FTSMPS426_STEP_CTRL_DELAY_SHIFT;
+
+
+ vreg->slew_rate = SPMI_FTSMPS3_SLEW_RATE_38p4 >> delay;
+
+ return ret;
+}
+
static int spmi_regulator_init_registers(struct spmi_regulator *vreg,
const struct spmi_regulator_init_data *data)
{
@@ -1846,6 +1962,11 @@ static int spmi_regulator_of_parse(struct device_node *node,
if (ret)
return ret;
break;
+ case SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS3:
+ ret = spmi_regulator_init_slew_rate_ftsmps3(vreg);
+ if (ret)
+ return ret;
+ break;
default:
break;
}
--
2.37.1

2022-07-26 20:43:36

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] dt-bindings: regulator: Document the PM6125 SPMI PMIC

On Tue, 26 Jul 2022 at 21:11, Iskren Chernev <[email protected]> wrote:
>
> Add support for pm6125 compatible string and add relevant supplies in QCom SPMI
> regulator documentation.
>
> Signed-off-by: Iskren Chernev <[email protected]>
> Signed-off-by: Adam Skladowski <[email protected]>

The order of sign-offs seems incorrect. The sender's signature should
be the last one.

> ---
> .../regulator/qcom,spmi-regulator.yaml | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> index 8b7c4af4b551..d8f18b441484 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> @@ -12,6 +12,7 @@ maintainers:
> properties:
> compatible:
> enum:
> + - qcom,pm6125-regulators
> - qcom,pm660-regulators
> - qcom,pm660l-regulators
> - qcom,pm8004-regulators
> @@ -106,6 +107,24 @@ required:
> - compatible
>
> allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,pm6125-regulators
> + then:
> + properties:
> + vdd_l1_l7_l17_l18-supply: true
> + vdd_l2_l3_l4-supply: true
> + vdd_l5_l15_l19_l20_l21_l22-supply: true
> + vdd_l6_l8-supply: true
> + vdd_l9_l11-supply: true
> + vdd_l10_l13_l14-supply: true
> + vdd_l12_l16-supply: true
> + vdd_l23_l24-supply: true
> + patternProperties:
> + "^vdd_s[1-8]-supply$": true

Add an empty line please.

> - if:
> properties:
> compatible:
> --
> 2.37.1
>

--
With best wishes
Dmitry

2022-07-26 21:00:25

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] regulator: qcom_smd: Add PM6125 regulators support

On Tue, 26 Jul 2022 at 21:12, Iskren Chernev <[email protected]> wrote:
>
> Add support for PM6125 PMIC which is found on SM4250/6115 SoCs.
>
> Signed-off-by: Iskren Chernev <[email protected]>
> Signed-off-by: Adam Skladowski <[email protected]>

Please fix the order of sign-offs.

With that fixed:
Reviewed-by: Dmitry Baryshkov <[email protected]>

> ---
> drivers/regulator/qcom_smd-regulator.c | 46 ++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)

--
With best wishes
Dmitry

2022-07-26 21:22:53

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] regulator: qcom_spmi: Add PM6125 PMIC support

On Tue, 26 Jul 2022 at 21:11, Iskren Chernev <[email protected]> wrote:
>
> Add pm6125 regulators and their sources in the QCom SPMI driver.
>
> Signed-off-by: Iskren Chernev <[email protected]>
> Signed-off-by: Adam Skladowski <[email protected]>

Please fix the order of sign-offs.

With that fixed:
Reviewed-by: Dmitry Baryshkov <[email protected]>

--
With best wishes
Dmitry

2022-07-27 07:52:57

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] dt-bindings: regulator: Document the PM6125 SPMI PMIC

On 7/26/22 23:36, Dmitry Baryshkov wrote:
> On Tue, 26 Jul 2022 at 21:11, Iskren Chernev <[email protected]> wrote:
>>
>> Add support for pm6125 compatible string and add relevant supplies in QCom SPMI
>> regulator documentation.
>>
>> Signed-off-by: Iskren Chernev <[email protected]>
>> Signed-off-by: Adam Skladowski <[email protected]>
>
> The order of sign-offs seems incorrect. The sender's signature should
> be the last one.

Sure, will do!

>> ---
>> .../regulator/qcom,spmi-regulator.yaml | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
>> index 8b7c4af4b551..d8f18b441484 100644
>> --- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
>> @@ -12,6 +12,7 @@ maintainers:
>> properties:
>> compatible:
>> enum:
>> + - qcom,pm6125-regulators
>> - qcom,pm660-regulators
>> - qcom,pm660l-regulators
>> - qcom,pm8004-regulators
>> @@ -106,6 +107,24 @@ required:
>> - compatible
>>
>> allOf:
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - qcom,pm6125-regulators
>> + then:
>> + properties:
>> + vdd_l1_l7_l17_l18-supply: true
>> + vdd_l2_l3_l4-supply: true
>> + vdd_l5_l15_l19_l20_l21_l22-supply: true
>> + vdd_l6_l8-supply: true
>> + vdd_l9_l11-supply: true
>> + vdd_l10_l13_l14-supply: true
>> + vdd_l12_l16-supply: true
>> + vdd_l23_l24-supply: true
>> + patternProperties:
>> + "^vdd_s[1-8]-supply$": true
>
> Add an empty line please.

All other if-then blocks don't have newlines, shall I add one between each as
well?

>> - if:
>> properties:
>> compatible:
>> --
>> 2.37.1
>>
>
>

2022-07-27 08:06:45

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] dt-bindings: regulator: Document the PM6125 RPM regulators

On 26/07/2022 20:11, Iskren Chernev wrote:
> Document the pm6125 compatible string and available regulators in the QCom SMD
> RPM regulator documentation.
>
> Signed-off-by: Iskren Chernev <[email protected]>
> Signed-off-by: Adam Skladowski <[email protected]>

Unusual SoB chain here as well.

> ---
> .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
> index c233461cc980..1122a3a17f56 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
> @@ -57,6 +57,9 @@ description:
>
> For pm660l s1, s2, s3, s5, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, bob
>
> + For pm6125 s1, s2, s3, s4, s5, s6, s7, s8, l1, l2, l3, l5, l6, l7, l8, l9,
> + l10, l22, l12, l13, l14, l15, l16, l17, l18, l19, l20, l21, l22, l23, l24
> +
> For pma8084, s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, l1, l2, l3,
> l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15, l16, l17, l18, l19,
> l20, l21, l22, l23, l24, l25, l26, l27, lvs1, lvs2, lvs3, lvs4, 5vs1
> @@ -90,6 +93,7 @@ properties:
> - qcom,rpm-pm8998-regulators
> - qcom,rpm-pm660-regulators
> - qcom,rpm-pm660l-regulators
> + - qcom,rpm-pm6125-regulators

Put new entry in alphabetical order.

Best regards,
Krzysztof

2022-07-27 09:06:23

[permalink] [raw]

Subject: Re: [PATCH v2 1/5] dt-bindings: regulator: Document the PM6125 SPMI PMIC

On Wed, 27 Jul 2022 at 10:49, Iskren Chernev <[email protected]> wrote:
>
>
>
> On 7/26/22 23:36, Dmitry Baryshkov wrote:
> > On Tue, 26 Jul 2022 at 21:11, Iskren Chernev <[email protected]> wrote:
> >>
> >> Add support for pm6125 compatible string and add relevant supplies in QCom SPMI
> >> regulator documentation.
> >>
> >> Signed-off-by: Iskren Chernev <[email protected]>
> >> Signed-off-by: Adam Skladowski <[email protected]>
> >
> > The order of sign-offs seems incorrect. The sender's signature should
> > be the last one.
>
> Sure, will do!
>
> >> ---
> >> .../regulator/qcom,spmi-regulator.yaml | 19 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> index 8b7c4af4b551..d8f18b441484 100644
> >> --- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.yaml
> >> @@ -12,6 +12,7 @@ maintainers:
> >> properties:
> >> compatible:
> >> enum:
> >> + - qcom,pm6125-regulators
> >> - qcom,pm660-regulators
> >> - qcom,pm660l-regulators
> >> - qcom,pm8004-regulators
> >> @@ -106,6 +107,24 @@ required:
> >> - compatible
> >>
> >> allOf:
> >> + - if:
> >> + properties:
> >> + compatible:
> >> + contains:
> >> + enum:
> >> + - qcom,pm6125-regulators
> >> + then:
> >> + properties:
> >> + vdd_l1_l7_l17_l18-supply: true
> >> + vdd_l2_l3_l4-supply: true
> >> + vdd_l5_l15_l19_l20_l21_l22-supply: true
> >> + vdd_l6_l8-supply: true
> >> + vdd_l9_l11-supply: true
> >> + vdd_l10_l13_l14-supply: true
> >> + vdd_l12_l16-supply: true
> >> + vdd_l23_l24-supply: true
> >> + patternProperties:
> >> + "^vdd_s[1-8]-supply$": true
> >
> > Add an empty line please.
>
> All other if-then blocks don't have newlines, shall I add one between each as
> well?

Yes, please, in a separate commit.

--
With best wishes
Dmitry

2022-07-27 10:56:24

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] dt-bindings: regulator: Document the PM6125 RPM regulators

On 7/27/22 10:26, Krzysztof Kozlowski wrote:
> On 26/07/2022 20:11, Iskren Chernev wrote:
>> Document the pm6125 compatible string and available regulators in the QCom SMD
>> RPM regulator documentation.
>>
>> Signed-off-by: Iskren Chernev <[email protected]>
>> Signed-off-by: Adam Skladowski <[email protected]>
>
> Unusual SoB chain here as well.

Will fix.

>> ---
>> .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>> index c233461cc980..1122a3a17f56 100644
>> --- a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>> @@ -57,6 +57,9 @@ description:
>>
>> For pm660l s1, s2, s3, s5, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, bob
>>
>> + For pm6125 s1, s2, s3, s4, s5, s6, s7, s8, l1, l2, l3, l5, l6, l7, l8, l9,
>> + l10, l22, l12, l13, l14, l15, l16, l17, l18, l19, l20, l21, l22, l23, l24
>> +
>> For pma8084, s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, l1, l2, l3,
>> l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15, l16, l17, l18, l19,
>> l20, l21, l22, l23, l24, l25, l26, l27, lvs1, lvs2, lvs3, lvs4, 5vs1
>> @@ -90,6 +93,7 @@ properties:
>> - qcom,rpm-pm8998-regulators
>> - qcom,rpm-pm660-regulators
>> - qcom,rpm-pm660l-regulators
>> + - qcom,rpm-pm6125-regulators
>
> Put new entry in alphabetical order.

Will sort first (they are currently not sorted), then add pm6125. Should I also
sort the driver code?

> Best regards,
> Krzysztof

2022-07-27 12:22:03

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:

> Add support for some regulator types that are missing in this driver, all
> belonging to the FTSMPS426 register layout. This is done in preparation
> for adding support for the PM6125 PMIC.

> + .set_mode = spmi_regulator_ftsmps3_set_mode,
> + .get_mode = spmi_regulator_ftsmps426_get_mode,

Why are set and get asymmetric?

> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> SPMI_VREG(LDO, HT_P600, 0, INF, HFS430, hfs430, ht_p600, 10000),
> SPMI_VREG(LDO, HT_P150, 0, INF, HFS430, hfs430, ht_p150, 10000),
> SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000),
> - SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000),
> + SPMI_VREG(BUCK, HFS430, 0, 3, HFS430, hfs430, hfs430, 10000),

The changelog said we were adding support for new types but this looks
like changing an existing type.

Attachments:

(No filename) (973.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-27 12:26:34

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] regulator: qcom_spmi: Add PM6125 PMIC support

On Tue, Jul 26, 2022 at 09:11:32PM +0300, Iskren Chernev wrote:

> @@ -2245,7 +2280,6 @@ static const struct spmi_regulator_data pm660l_regulators[] = {
> { }
> };
>
> -
> static const struct spmi_regulator_data pm8004_regulators[] = {
> { "s2", 0x1700, "vdd_s2", },
> { "s5", 0x2000, "vdd_s5", },

Unrelated indentation change (I think undoing a random extra blank line
added in the prior patch, at least there were some random extras in
that).

Attachments:

(No filename) (469.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-27 12:27:02

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v2 2/5] dt-bindings: regulator: Document the PM6125 RPM regulators

On 27/07/2022 12:32, Iskren Chernev wrote:
>
>>> ---
>>> .../devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>>> index c233461cc980..1122a3a17f56 100644
>>> --- a/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>>> +++ b/Documentation/devicetree/bindings/regulator/qcom,smd-rpm-regulator.yaml
>>> @@ -57,6 +57,9 @@ description:
>>>
>>> For pm660l s1, s2, s3, s5, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, bob
>>>
>>> + For pm6125 s1, s2, s3, s4, s5, s6, s7, s8, l1, l2, l3, l5, l6, l7, l8, l9,
>>> + l10, l22, l12, l13, l14, l15, l16, l17, l18, l19, l20, l21, l22, l23, l24
>>> +
>>> For pma8084, s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, s11, s12, l1, l2, l3,
>>> l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15, l16, l17, l18, l19,
>>> l20, l21, l22, l23, l24, l25, l26, l27, lvs1, lvs2, lvs3, lvs4, 5vs1
>>> @@ -90,6 +93,7 @@ properties:
>>> - qcom,rpm-pm8998-regulators
>>> - qcom,rpm-pm660-regulators
>>> - qcom,rpm-pm660l-regulators
>>> + - qcom,rpm-pm6125-regulators
>>
>> Put new entry in alphabetical order.
>
> Will sort first (they are currently not sorted),

Arh, indeed, they are not sorted.

> then add pm6125. Should I also
> sort the driver code?

You can, but don't have to.

Best regards,
Krzysztof

2022-07-27 23:22:33

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

On 7/27/22 14:57, Mark Brown wrote:
> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
>
>> Add support for some regulator types that are missing in this driver, all
>> belonging to the FTSMPS426 register layout. This is done in preparation
>> for adding support for the PM6125 PMIC.
>
>> + .set_mode = spmi_regulator_ftsmps3_set_mode,
>> + .get_mode = spmi_regulator_ftsmps426_get_mode,
>
> Why are set and get asymmetric?

Because the get method, only uses AUTO and HPM, which have the same value
for ftsmps3 and ftsmps426 (so there is no need for a new function).

>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
>> SPMI_VREG(LDO, HT_P600, 0, INF, HFS430, hfs430, ht_p600, 10000),
>> SPMI_VREG(LDO, HT_P150, 0, INF, HFS430, hfs430, ht_p150, 10000),
>> SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000),
>> - SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000),
>> + SPMI_VREG(BUCK, HFS430, 0, 3, HFS430, hfs430, hfs430, 10000),
>
> The changelog said we were adding support for new types but this looks
> like changing an existing type.

The code, as written now does a different thing for BUCK, HFS430 (on
mainline (ML) and downstream (DS) linked in the commit message). Since DS
only supports newer stuff, to be on safe side, I kept existing behavior for
rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
completely.

The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
might be right for the time being (i.e initial revisions had different
behavior). I'm CC-ing Jorge.

Question is is BUCK+HFS430 on common2 (ftsmps426) or common3 (ftsmps3) or
a mix (depending on revision).

[1] 0211f68e626f (regulator: qcom_spmi: add PMS405 SPMI regulator, 2019-06-17)

2022-07-28 11:13:05

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
> On 7/27/22 14:57, Mark Brown wrote:
> > On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:

> >> Add support for some regulator types that are missing in this driver, all
> >> belonging to the FTSMPS426 register layout. This is done in preparation
> >> for adding support for the PM6125 PMIC.

> >> + .set_mode = spmi_regulator_ftsmps3_set_mode,
> >> + .get_mode = spmi_regulator_ftsmps426_get_mode,

> > Why are set and get asymmetric?

> Because the get method, only uses AUTO and HPM, which have the same value
> for ftsmps3 and ftsmps426 (so there is no need for a new function).

This needs at least a comment.

> >> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> >> SPMI_VREG(LDO, HT_P600, 0, INF, HFS430, hfs430, ht_p600, 10000),
> >> SPMI_VREG(LDO, HT_P150, 0, INF, HFS430, hfs430, ht_p150, 10000),
> >> SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000),
> >> - SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000),
> >> + SPMI_VREG(BUCK, HFS430, 0, 3, HFS430, hfs430, hfs430, 10000),

> > The changelog said we were adding support for new types but this looks
> > like changing an existing type.

> The code, as written now does a different thing for BUCK, HFS430 (on
> mainline (ML) and downstream (DS) linked in the commit message). Since DS
> only supports newer stuff, to be on safe side, I kept existing behavior for
> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
> completely.

This needs describing in the changelog, probably you need multiple
paches here since you are making a number of different changes each of
which needs some explanation.

> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
> might be right for the time being (i.e initial revisions had different
> behavior). I'm CC-ing Jorge.

If that's the case perhaps part of this needs to be sent as a fix.

Attachments:

(No filename) (2.00 kB)
signature.asc (499.00 B)
Download all attachments

2022-07-28 21:02:03

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

On 7/28/22 14:11, Mark Brown wrote:
> On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
>> On 7/27/22 14:57, Mark Brown wrote:
>>> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
>
>>>> Add support for some regulator types that are missing in this driver, all
>>>> belonging to the FTSMPS426 register layout. This is done in preparation
>>>> for adding support for the PM6125 PMIC.
>
>>>> + .set_mode = spmi_regulator_ftsmps3_set_mode,
>>>> + .get_mode = spmi_regulator_ftsmps426_get_mode,
>
>>> Why are set and get asymmetric?
>
>> Because the get method, only uses AUTO and HPM, which have the same value
>> for ftsmps3 and ftsmps426 (so there is no need for a new function).
>
> This needs at least a comment.

I agree, I think to add the function with the right macros, and comment
that it is the same now but might change in the future if support for mode
modes is added.

>>>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
>>>> SPMI_VREG(LDO, HT_P600, 0, INF, HFS430, hfs430, ht_p600, 10000),
>>>> SPMI_VREG(LDO, HT_P150, 0, INF, HFS430, hfs430, ht_p150, 10000),
>>>> SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000),
>>>> - SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000),
>>>> + SPMI_VREG(BUCK, HFS430, 0, 3, HFS430, hfs430, hfs430, 10000),
>
>>> The changelog said we were adding support for new types but this looks
>>> like changing an existing type.
>
>> The code, as written now does a different thing for BUCK, HFS430 (on
>> mainline (ML) and downstream (DS) linked in the commit message). Since DS
>> only supports newer stuff, to be on safe side, I kept existing behavior for
>> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
>> completely.
>
> This needs describing in the changelog, probably you need multiple
> paches here since you are making a number of different changes each of
> which needs some explanation.
>
>> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
>> might be right for the time being (i.e initial revisions had different
>> behavior). I'm CC-ing Jorge.
>
> If that's the case perhaps part of this needs to be sent as a fix.

The Downstream patch is adding 3 logical types:
- LDO_510 -- these have new subtypes, so no existing PMICs are affected
- FTSMPS3 -- this has a new subtype (0xb), so no existing PMICs are
affected
- HFSMPS -- this has the same type and subtype (BUCK+HFS430) as an existing
mainline logical type (HFS430), both declaring 0-INF revisions.

So if we fully trust the downstream patch, I can make a fix for the
existing BUCK+HFS430+0-INF, so it uses the slighly modified mode values.

Currently the set mode fn differs in LPM mode (5 in the common2 case and
4 in the common3 case), so if indeed downstream is correct it would mean
this regulator (when turned off) was set to an invalid mode (5 has
undefined meaning in common3 map) from 2019 onward.

On the other hand, if we assume downstream is wrong, then their code sets
4, which actually means RETENTION (not LPM). I really don't know how this
could cause trouble. In fact downstream does a bunch of weird stuff, it
doesn't "just" set to LPM (like mainline), instead there is complex logic
per logical type and "initial mode". Or they're just masking this mistake
;-)

TL;DR Jorge's mail is gone, so we can't get info from the original author.

Another issue is I can't really test any other PMIC (and even my PMIC
I can't turn off most of the regs without loosing critical functionality,
and the BUCKs are kinda important :)).

So we can:
1. politely ask for somebody with access to the secret sauce to say what is
correct, at least according to the docs (with a timeout)
2. assume downstream patch is right, and fix the existing HFS430 regulator
3. maintain the current (patch) behavior, which likely won't affect older
PMICs, but is still adhering to DS patch, because it adds support for
this particular PMIC, so presumably it was tested and works with it
4. drop the pmic patch and rely on SMD

Please advice.

In any case if we go with 2 or 3, I can split out this particular (BUCK)
part in a separate patch with more information/comments.

2022-07-29 12:09:29