[PATCH 0/3] qcom: add PMS405 SPMI regulator

[PATCH 0/3] qcom: add PMS405 SPMI regulator

[Jorge Ramirez-Ortiz]

The following patchset applies cleanly on linux 5.0-rc2.

It adds support for the pms405 spmi regulators and configures s3 as a
supply.

The dts modifications required to enable voltage scaling will be
posted after the currently pending cpufreq patches are merged.

Jorge Ramirez-Ortiz (3):
  dt-bindings: qcom_spmi: Document pms405 support
  drivers: regulator: qcom: add PMS405 SPMI regulator
  arm64: dts: qcom: pms405: add spmi regulators

 .../bindings/regulator/qcom,spmi-regulator.txt     |  24 +++
 arch/arm64/boot/dts/qcom/pms405.dtsi               |  20 +++
 drivers/regulator/qcom_spmi-regulator.c            | 197 +++++++++++++++++++--
 3 files changed, 224 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 0/3] qcom: add PMS405 SPMI regulator

[Jorge Ramirez-Ortiz]

The following patchset applies cleanly on linux 5.0-rc2.

It adds support for the pms405 spmi regulators and configures s3 as a
supply.

The dts modifications required to enable voltage scaling will be
posted after the currently pending cpufreq patches are merged.

Jorge Ramirez-Ortiz (3):
  dt-bindings: qcom_spmi: Document pms405 support
  drivers: regulator: qcom: add PMS405 SPMI regulator
  arm64: dts: qcom: pms405: add spmi regulators

 .../bindings/regulator/qcom,spmi-regulator.txt     |  24 +++
 arch/arm64/boot/dts/qcom/pms405.dtsi               |  20 +++
 drivers/regulator/qcom_spmi-regulator.c            | 197 +++++++++++++++++++--
 3 files changed, 224 insertions(+), 17 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support

[Jorge Ramirez-Ortiz]

The PMS405 supports 5 SMPS and 13 LDO regulators.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 .../bindings/regulator/qcom,spmi-regulator.txt     | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
index 406f2e5..8ee7aac 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
@@ -9,6 +9,7 @@ Qualcomm SPMI Regulators
 			"qcom,pm8941-regulators"
 			"qcom,pm8994-regulators"
 			"qcom,pmi8994-regulators"
+			"qcom,pms405-regulators"
 
 - interrupts:
 	Usage: optional
@@ -110,6 +111,29 @@ Qualcomm SPMI Regulators
 	Definition: Reference to regulator supplying the input pin, as
 		    described in the data sheet.
 
+- vdd_s1-supply:
+- vdd_s2-supply:
+- vdd_s3-supply:
+- vdd_s4-supply:
+- vdd_s5-supply:
+- vdd_l1-supply:
+- vdd_l2-supply:
+- vdd_l3-supply:
+- vdd_l4-supply:
+- vdd_l5-supply:
+- vdd_l6-supply:
+- vdd_l7-supply:
+- vdd_l8-supply:
+- vdd_l9-supply:
+- vdd_l10-supply:
+- vdd_l11-supply:
+- vdd_l12-supply:
+- vdd_l13-supply:
+	Usage: optional (pms405 only)
+	Value type: <phandle>
+	Definition: Reference to regulator supplying the input pin, as
+		    described in the data sheet.
+
 - qcom,saw-reg:
 	Usage: optional
 	Value type: <phandle>
-- 
2.7.4

[PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support

[Jorge Ramirez-Ortiz]

The PMS405 supports 5 SMPS and 13 LDO regulators.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 .../bindings/regulator/qcom,spmi-regulator.txt     | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
index 406f2e5..8ee7aac 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt
@@ -9,6 +9,7 @@ Qualcomm SPMI Regulators
 			"qcom,pm8941-regulators"
 			"qcom,pm8994-regulators"
 			"qcom,pmi8994-regulators"
+			"qcom,pms405-regulators"
 
 - interrupts:
 	Usage: optional
@@ -110,6 +111,29 @@ Qualcomm SPMI Regulators
 	Definition: Reference to regulator supplying the input pin, as
 		    described in the data sheet.
 
+- vdd_s1-supply:
+- vdd_s2-supply:
+- vdd_s3-supply:
+- vdd_s4-supply:
+- vdd_s5-supply:
+- vdd_l1-supply:
+- vdd_l2-supply:
+- vdd_l3-supply:
+- vdd_l4-supply:
+- vdd_l5-supply:
+- vdd_l6-supply:
+- vdd_l7-supply:
+- vdd_l8-supply:
+- vdd_l9-supply:
+- vdd_l10-supply:
+- vdd_l11-supply:
+- vdd_l12-supply:
+- vdd_l13-supply:
+	Usage: optional (pms405 only)
+	Value type: <phandle>
+	Definition: Reference to regulator supplying the input pin, as
+		    described in the data sheet.
+
 - qcom,saw-reg:
 	Usage: optional
 	Value type: <phandle>
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez-Ortiz]

The PMS405 has 5 HFSMPS and 13 LDO regulators,

This commit adds support for one of the 5 HFSMPS regulators (s3) to
the spmi regulator driver.

The PMIC HFSMPS 430 regulators have 8 mV step size and a voltage
control scheme consisting of two  8-bit registers defining a 16-bit
voltage set point in units of millivolts

S3 controls the cpu voltages (s3 is a buck regulator of type HFS430);
it is therefore required so we can enable voltage scaling for safely
running cpufreq.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 197 +++++++++++++++++++++++++++++---
 1 file changed, 180 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 53a61fb..6b8dc9c 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -99,6 +99,7 @@ enum spmi_regulator_logical_type {
 	SPMI_REGULATOR_LOGICAL_TYPE_VS,
 	SPMI_REGULATOR_LOGICAL_TYPE_BOOST,
 	SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS,
+	SPMI_REGULATOR_LOGICAL_TYPE_HFS430,
 	SPMI_REGULATOR_LOGICAL_TYPE_BOOST_BYP,
 	SPMI_REGULATOR_LOGICAL_TYPE_LN_LDO,
 	SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS,
@@ -120,6 +121,7 @@ enum spmi_regulator_type {
 enum spmi_regulator_subtype {
 	SPMI_REGULATOR_SUBTYPE_GP_CTL		= 0x08,
 	SPMI_REGULATOR_SUBTYPE_RF_CTL		= 0x09,
+	SPMI_REGULATOR_SUBTYPE_HFS430		= 0x0a,
 	SPMI_REGULATOR_SUBTYPE_N50		= 0x01,
 	SPMI_REGULATOR_SUBTYPE_N150		= 0x02,
 	SPMI_REGULATOR_SUBTYPE_N300		= 0x03,
@@ -183,6 +185,12 @@ enum spmi_boost_byp_registers {
 	SPMI_BOOST_BYP_REG_CURRENT_LIMIT	= 0x4b,
 };
 
+enum spmi_hfs430_registers {
+	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40,
+	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
+	SPMI_HFS430_REG_MODE			= 0x45,
+};
+
 enum spmi_saw3_registers {
 	SAW3_SECURE				= 0x00,
 	SAW3_ID					= 0x04,
@@ -260,20 +268,61 @@ enum spmi_common_control_register_index {
 #define SPMI_FTSMPS_STEP_CTRL_DELAY_MASK	0x07
 #define SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT	0
 
-/* Clock rate in kHz of the FTSMPS regulator reference clock. */
+#define SPMI_HFS430_STEP_CTRL_STEP_MASK		0
+#define SPMI_HFS430_STEP_CTRL_STEP_SHIFT	0
+#define SPMI_HFS430_STEP_CTRL_DELAY_MASK	0x3
+#define SPMI_HFS430_STEP_CTRL_DELAY_SHIFT	0
+
+/* Clock rate in kHz of the FTSMPS and HFS430 regulator reference clocks. */
 #define SPMI_FTSMPS_CLOCK_RATE		19200
+#define SPMI_HFS430_CLOCK_RATE		1600
 
 /* Minimum voltage stepper delay for each step. */
 #define SPMI_FTSMPS_STEP_DELAY		8
+#define SPMI_HFS430_STEP_DELAY		2
 #define SPMI_DEFAULT_STEP_DELAY		20
 
 /*
- * The ratio SPMI_FTSMPS_STEP_MARGIN_NUM/SPMI_FTSMPS_STEP_MARGIN_DEN is used to
+ * The ratio SPMI_xxxxx_STEP_MARGIN_NUM/SPMI_xxxxx_STEP_MARGIN_DEN is used to
  * adjust the step rate in order to account for oscillator variance.
  */
 #define SPMI_FTSMPS_STEP_MARGIN_NUM	4
 #define SPMI_FTSMPS_STEP_MARGIN_DEN	5
 
+#define SPMI_HFS430_STEP_MARGIN_NUM	10
+#define SPMI_HFS430_STEP_MARGIN_DEN	11
+
+#define SPMI_STEP_MASK(__x)	SPMI_##__x##_STEP_CTRL_STEP_MASK
+#define SPMI_STEP_SHIFT(__x)	SPMI_##__x##_STEP_CTRL_STEP_SHIFT
+#define SPMI_DELAY_MASK(__x)	SPMI_##__x##_STEP_CTRL_DELAY_MASK
+#define SPMI_DELAY_SHIFT(__x)	SPMI_##__x##_STEP_CTRL_DELAY_SHIFT
+#define SPMI_CLOCK_RATE(__x)	SPMI_##__x##_CLOCK_RATE
+#define SPMI_STEP_DELAY(__x)	SPMI_##__x##_STEP_DELAY
+#define SPMI_MARGIN_NUM(__x)	SPMI_##__x##_STEP_MARGIN_NUM
+#define SPMI_MARGIN_DEN(__x)	SPMI_##__x##_STEP_MARGIN_DEN
+
+struct slew_rate_config {
+	unsigned int delay_mask;
+	unsigned int delay_shift;
+	unsigned int step_mask;
+	unsigned int step_shift;
+	unsigned int margin_num;
+	unsigned int margin_den;
+	unsigned int step_delay;
+	unsigned int clock_rate;
+};
+
+#define SLEW_RATE_CONFIG(x)  {			\
+	.delay_shift = SPMI_DELAY_SHIFT(x),	\
+	.delay_mask = SPMI_DELAY_MASK(x),	\
+	.step_shift = SPMI_STEP_SHIFT(x),	\
+	.step_mask = SPMI_STEP_MASK(x),		\
+	.margin_num = SPMI_MARGIN_NUM(x),	\
+	.margin_den = SPMI_MARGIN_DEN(x),	\
+	.step_delay = SPMI_STEP_DELAY(x),	\
+	.clock_rate = SPMI_CLOCK_RATE(x),	\
+}
+
 /* VSET value to decide the range of ULT SMPS */
 #define ULT_SMPS_RANGE_SPLIT 0x60
 
@@ -472,6 +521,11 @@ static struct spmi_voltage_range ult_pldo_ranges[] = {
 	SPMI_VOLTAGE_RANGE(0, 1750000, 1750000, 3337500, 3337500, 12500),
 };
 
+/* must be only one range */
+static struct spmi_voltage_range hfs430_ranges[] = {
+	SPMI_VOLTAGE_RANGE(0, 320000, 320000, 2040000, 2040000, 8000),
+};
+
 static DEFINE_SPMI_SET_POINTS(pldo);
 static DEFINE_SPMI_SET_POINTS(nldo1);
 static DEFINE_SPMI_SET_POINTS(nldo2);
@@ -486,6 +540,7 @@ static DEFINE_SPMI_SET_POINTS(ult_lo_smps);
 static DEFINE_SPMI_SET_POINTS(ult_ho_smps);
 static DEFINE_SPMI_SET_POINTS(ult_nldo);
 static DEFINE_SPMI_SET_POINTS(ult_pldo);
+static DEFINE_SPMI_SET_POINTS(hfs430);
 
 static inline int spmi_vreg_read(struct spmi_regulator *vreg, u16 addr, u8 *buf,
 				 int len)
@@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
 	range = vreg->set_points->range;
 	end = range + vreg->set_points->count;
 
+	/* we know we only have one range for this type */
+	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
+		return range;
+
 	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
 
 	for (; range < end; range++)
@@ -1135,6 +1194,82 @@ spmi_regulator_saw_set_voltage(struct regulator_dev *rdev, unsigned selector)
 					&voltage_sel, true);
 }
 
+#define SPMI_HFS430_MODE_PWM	0x07
+#define SPMI_HFS430_MODE_AUTO	0x06
+
+static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 reg;
+	int ret;
+
+	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
+	if (ret) {
+		dev_err(&rdev->dev, "failed to get mode");
+		return ret;
+	}
+
+	if (reg == SPMI_HFS430_MODE_PWM)
+		return REGULATOR_MODE_NORMAL;
+
+	return REGULATOR_MODE_IDLE;
+}
+
+static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
+					  unsigned int mode)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
+						 SPMI_HFS430_MODE_AUTO;
+
+	return spmi_vreg_write(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
+}
+
+int spmi_regulator_hfs430_get_voltage(struct regulator_dev *rdev)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 val[2];
+	int ret, uV;
+
+	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_VOLTAGE_VALID_LB, val, 2);
+	if (ret) {
+		dev_err(&rdev->dev, "failed to get voltage");
+		return ret;
+	}
+
+	uV = 1000 * (((unsigned int) val[1] << 8) | val[0]);
+	dev_dbg(&rdev->dev, "read = %d", uV);
+
+	return uV;
+}
+
+static int spmi_regulator_hfs430_set_voltage(struct regulator_dev *rdev,
+					     unsigned selector)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	const struct spmi_voltage_range *range;
+	int uV, vlevel;
+	u8 val[2];
+
+	range = spmi_regulator_find_range(vreg);
+	if (!range)
+		return -EINVAL;
+
+	uV = spmi_regulator_common_list_voltage(rdev, selector);
+	if (uV <= 0)
+		return uV;
+
+	vlevel = roundup(uV, range->step_uV) / 1000;
+
+	dev_dbg(&rdev->dev, "write (%d, %d), mode (%d)", uV, vlevel,
+		spmi_regulator_hfs430_get_mode(rdev));
+
+	val[0] = vlevel & 0xFF;
+	val[1] = (vlevel >> 8) & 0xFF;
+
+	return spmi_vreg_write(vreg, SPMI_HFS430_REG_VOLTAGE_LB, val, 2);
+}
+
 static struct regulator_ops spmi_saw_ops = {};
 
 static struct regulator_ops spmi_smps_ops = {
@@ -1264,12 +1399,24 @@ static struct regulator_ops spmi_ult_ldo_ops = {
 	.set_soft_start		= spmi_regulator_common_set_soft_start,
 };
 
+static struct regulator_ops spmi_hfs430_ops = {
+	/* always on regulators */
+	.set_voltage_sel	= spmi_regulator_hfs430_set_voltage,
+	.set_voltage_time_sel	= spmi_regulator_set_voltage_time_sel,
+	.get_voltage		= spmi_regulator_hfs430_get_voltage,
+	.map_voltage		= spmi_regulator_common_map_voltage,
+	.list_voltage		= spmi_regulator_common_list_voltage,
+	.get_mode		= spmi_regulator_hfs430_get_mode,
+	.set_mode		= spmi_regulator_hfs430_set_mode,
+};
+
 /* Maximum possible digital major revision value */
 #define INF 0xFF
 
 static const struct spmi_regulator_mapping supported_regulators[] = {
-	/*           type subtype dig_min dig_max ltype ops setpoints hpm_min */
+	/*        type subtype dig_min dig_max ltype ops setpoints hpm_min */
 	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
+	SPMI_VREG(BUCK,  HFS430,   0, INF, 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),
@@ -1396,8 +1543,12 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg)
 {
 	int ret;
 	u8 reg = 0;
-	int step, delay, slew_rate, step_delay;
+	int step, delay, slew_rate;
 	const struct spmi_voltage_range *range;
+	struct slew_rate_config *config, configs[] = {
+		[0] = SLEW_RATE_CONFIG(HFS430),
+		[1] = SLEW_RATE_CONFIG(FTSMPS),
+	};
 
 	ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_STEP_CTRL, &reg, 1);
 	if (ret) {
@@ -1410,25 +1561,26 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg)
 		return -EINVAL;
 
 	switch (vreg->logical_type) {
-	case SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS:
-		step_delay = SPMI_FTSMPS_STEP_DELAY;
+	case SPMI_REGULATOR_LOGICAL_TYPE_HFS430:
+		config = &configs[0];
 		break;
 	default:
-		step_delay = SPMI_DEFAULT_STEP_DELAY;
-		break;
-	}
+		config = &configs[1];
+		if (vreg->logical_type != SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS)
+			config->step_delay =  SPMI_STEP_DELAY(DEFAULT);
+	};
 
-	step = reg & SPMI_FTSMPS_STEP_CTRL_STEP_MASK;
-	step >>= SPMI_FTSMPS_STEP_CTRL_STEP_SHIFT;
+	step = reg & config->step_mask;
+	step >>= config->step_shift;
 
-	delay = reg & SPMI_FTSMPS_STEP_CTRL_DELAY_MASK;
-	delay >>= SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT;
+	delay = reg & config->delay_mask;
+	delay >>= config->delay_shift;
 
 	/* slew_rate has units of uV/us */
-	slew_rate = SPMI_FTSMPS_CLOCK_RATE * range->step_uV * (1 << step);
-	slew_rate /= 1000 * (step_delay << delay);
-	slew_rate *= SPMI_FTSMPS_STEP_MARGIN_NUM;
-	slew_rate /= SPMI_FTSMPS_STEP_MARGIN_DEN;
+	slew_rate = config->clock_rate * range->step_uV * (1 << step);
+	slew_rate /= 1000 * (config->step_delay << delay);
+	slew_rate *= config->margin_num;
+	slew_rate /= config->margin_den;
 
 	/* Ensure that the slew rate is greater than 0 */
 	vreg->slew_rate = max(slew_rate, 1);
@@ -1445,6 +1597,9 @@ static int spmi_regulator_init_registers(struct spmi_regulator *vreg,
 
 	type = vreg->logical_type;
 
+	if (type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
+		return 0;
+
 	ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, ctrl_reg, 8);
 	if (ret)
 		return ret;
@@ -1572,9 +1727,11 @@ static int spmi_regulator_of_parse(struct device_node *node,
 	case SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS:
 	case SPMI_REGULATOR_LOGICAL_TYPE_ULT_HO_SMPS:
 	case SPMI_REGULATOR_LOGICAL_TYPE_SMPS:
+	case SPMI_REGULATOR_LOGICAL_TYPE_HFS430:
 		ret = spmi_regulator_init_slew_rate(vreg);
 		if (ret)
 			return ret;
+
 	default:
 		break;
 	}
@@ -1731,12 +1888,18 @@ static const struct spmi_regulator_data pmi8994_regulators[] = {
 	{ }
 };
 
+static const struct spmi_regulator_data pms405_regulators[] = {
+	{ "s3", 0x1a00, }, /* supply name in the dts only */
+	{ }
+};
+
 static const struct of_device_id qcom_spmi_regulator_match[] = {
 	{ .compatible = "qcom,pm8841-regulators", .data = &pm8841_regulators },
 	{ .compatible = "qcom,pm8916-regulators", .data = &pm8916_regulators },
 	{ .compatible = "qcom,pm8941-regulators", .data = &pm8941_regulators },
 	{ .compatible = "qcom,pm8994-regulators", .data = &pm8994_regulators },
 	{ .compatible = "qcom,pmi8994-regulators", .data = &pmi8994_regulators },
+	{ .compatible = "qcom,pms405-regulators", .data = &pms405_regulators },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_spmi_regulator_match);
-- 
2.7.4

[PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez-Ortiz]

The PMS405 has 5 HFSMPS and 13 LDO regulators,

This commit adds support for one of the 5 HFSMPS regulators (s3) to
the spmi regulator driver.

The PMIC HFSMPS 430 regulators have 8 mV step size and a voltage
control scheme consisting of two  8-bit registers defining a 16-bit
voltage set point in units of millivolts

S3 controls the cpu voltages (s3 is a buck regulator of type HFS430);
it is therefore required so we can enable voltage scaling for safely
running cpufreq.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 197 +++++++++++++++++++++++++++++---
 1 file changed, 180 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 53a61fb..6b8dc9c 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -99,6 +99,7 @@ enum spmi_regulator_logical_type {
 	SPMI_REGULATOR_LOGICAL_TYPE_VS,
 	SPMI_REGULATOR_LOGICAL_TYPE_BOOST,
 	SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS,
+	SPMI_REGULATOR_LOGICAL_TYPE_HFS430,
 	SPMI_REGULATOR_LOGICAL_TYPE_BOOST_BYP,
 	SPMI_REGULATOR_LOGICAL_TYPE_LN_LDO,
 	SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS,
@@ -120,6 +121,7 @@ enum spmi_regulator_type {
 enum spmi_regulator_subtype {
 	SPMI_REGULATOR_SUBTYPE_GP_CTL		= 0x08,
 	SPMI_REGULATOR_SUBTYPE_RF_CTL		= 0x09,
+	SPMI_REGULATOR_SUBTYPE_HFS430		= 0x0a,
 	SPMI_REGULATOR_SUBTYPE_N50		= 0x01,
 	SPMI_REGULATOR_SUBTYPE_N150		= 0x02,
 	SPMI_REGULATOR_SUBTYPE_N300		= 0x03,
@@ -183,6 +185,12 @@ enum spmi_boost_byp_registers {
 	SPMI_BOOST_BYP_REG_CURRENT_LIMIT	= 0x4b,
 };
 
+enum spmi_hfs430_registers {
+	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40,
+	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
+	SPMI_HFS430_REG_MODE			= 0x45,
+};
+
 enum spmi_saw3_registers {
 	SAW3_SECURE				= 0x00,
 	SAW3_ID					= 0x04,
@@ -260,20 +268,61 @@ enum spmi_common_control_register_index {
 #define SPMI_FTSMPS_STEP_CTRL_DELAY_MASK	0x07
 #define SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT	0
 
-/* Clock rate in kHz of the FTSMPS regulator reference clock. */
+#define SPMI_HFS430_STEP_CTRL_STEP_MASK		0
+#define SPMI_HFS430_STEP_CTRL_STEP_SHIFT	0
+#define SPMI_HFS430_STEP_CTRL_DELAY_MASK	0x3
+#define SPMI_HFS430_STEP_CTRL_DELAY_SHIFT	0
+
+/* Clock rate in kHz of the FTSMPS and HFS430 regulator reference clocks. */
 #define SPMI_FTSMPS_CLOCK_RATE		19200
+#define SPMI_HFS430_CLOCK_RATE		1600
 
 /* Minimum voltage stepper delay for each step. */
 #define SPMI_FTSMPS_STEP_DELAY		8
+#define SPMI_HFS430_STEP_DELAY		2
 #define SPMI_DEFAULT_STEP_DELAY		20
 
 /*
- * The ratio SPMI_FTSMPS_STEP_MARGIN_NUM/SPMI_FTSMPS_STEP_MARGIN_DEN is used to
+ * The ratio SPMI_xxxxx_STEP_MARGIN_NUM/SPMI_xxxxx_STEP_MARGIN_DEN is used to
  * adjust the step rate in order to account for oscillator variance.
  */
 #define SPMI_FTSMPS_STEP_MARGIN_NUM	4
 #define SPMI_FTSMPS_STEP_MARGIN_DEN	5
 
+#define SPMI_HFS430_STEP_MARGIN_NUM	10
+#define SPMI_HFS430_STEP_MARGIN_DEN	11
+
+#define SPMI_STEP_MASK(__x)	SPMI_##__x##_STEP_CTRL_STEP_MASK
+#define SPMI_STEP_SHIFT(__x)	SPMI_##__x##_STEP_CTRL_STEP_SHIFT
+#define SPMI_DELAY_MASK(__x)	SPMI_##__x##_STEP_CTRL_DELAY_MASK
+#define SPMI_DELAY_SHIFT(__x)	SPMI_##__x##_STEP_CTRL_DELAY_SHIFT
+#define SPMI_CLOCK_RATE(__x)	SPMI_##__x##_CLOCK_RATE
+#define SPMI_STEP_DELAY(__x)	SPMI_##__x##_STEP_DELAY
+#define SPMI_MARGIN_NUM(__x)	SPMI_##__x##_STEP_MARGIN_NUM
+#define SPMI_MARGIN_DEN(__x)	SPMI_##__x##_STEP_MARGIN_DEN
+
+struct slew_rate_config {
+	unsigned int delay_mask;
+	unsigned int delay_shift;
+	unsigned int step_mask;
+	unsigned int step_shift;
+	unsigned int margin_num;
+	unsigned int margin_den;
+	unsigned int step_delay;
+	unsigned int clock_rate;
+};
+
+#define SLEW_RATE_CONFIG(x)  {			\
+	.delay_shift = SPMI_DELAY_SHIFT(x),	\
+	.delay_mask = SPMI_DELAY_MASK(x),	\
+	.step_shift = SPMI_STEP_SHIFT(x),	\
+	.step_mask = SPMI_STEP_MASK(x),		\
+	.margin_num = SPMI_MARGIN_NUM(x),	\
+	.margin_den = SPMI_MARGIN_DEN(x),	\
+	.step_delay = SPMI_STEP_DELAY(x),	\
+	.clock_rate = SPMI_CLOCK_RATE(x),	\
+}
+
 /* VSET value to decide the range of ULT SMPS */
 #define ULT_SMPS_RANGE_SPLIT 0x60
 
@@ -472,6 +521,11 @@ static struct spmi_voltage_range ult_pldo_ranges[] = {
 	SPMI_VOLTAGE_RANGE(0, 1750000, 1750000, 3337500, 3337500, 12500),
 };
 
+/* must be only one range */
+static struct spmi_voltage_range hfs430_ranges[] = {
+	SPMI_VOLTAGE_RANGE(0, 320000, 320000, 2040000, 2040000, 8000),
+};
+
 static DEFINE_SPMI_SET_POINTS(pldo);
 static DEFINE_SPMI_SET_POINTS(nldo1);
 static DEFINE_SPMI_SET_POINTS(nldo2);
@@ -486,6 +540,7 @@ static DEFINE_SPMI_SET_POINTS(ult_lo_smps);
 static DEFINE_SPMI_SET_POINTS(ult_ho_smps);
 static DEFINE_SPMI_SET_POINTS(ult_nldo);
 static DEFINE_SPMI_SET_POINTS(ult_pldo);
+static DEFINE_SPMI_SET_POINTS(hfs430);
 
 static inline int spmi_vreg_read(struct spmi_regulator *vreg, u16 addr, u8 *buf,
 				 int len)
@@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
 	range = vreg->set_points->range;
 	end = range + vreg->set_points->count;
 
+	/* we know we only have one range for this type */
+	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
+		return range;
+
 	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
 
 	for (; range < end; range++)
@@ -1135,6 +1194,82 @@ spmi_regulator_saw_set_voltage(struct regulator_dev *rdev, unsigned selector)
 					&voltage_sel, true);
 }
 
+#define SPMI_HFS430_MODE_PWM	0x07
+#define SPMI_HFS430_MODE_AUTO	0x06
+
+static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 reg;
+	int ret;
+
+	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
+	if (ret) {
+		dev_err(&rdev->dev, "failed to get mode");
+		return ret;
+	}
+
+	if (reg == SPMI_HFS430_MODE_PWM)
+		return REGULATOR_MODE_NORMAL;
+
+	return REGULATOR_MODE_IDLE;
+}
+
+static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
+					  unsigned int mode)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
+						 SPMI_HFS430_MODE_AUTO;
+
+	return spmi_vreg_write(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
+}
+
+int spmi_regulator_hfs430_get_voltage(struct regulator_dev *rdev)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	u8 val[2];
+	int ret, uV;
+
+	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_VOLTAGE_VALID_LB, val, 2);
+	if (ret) {
+		dev_err(&rdev->dev, "failed to get voltage");
+		return ret;
+	}
+
+	uV = 1000 * (((unsigned int) val[1] << 8) | val[0]);
+	dev_dbg(&rdev->dev, "read = %d", uV);
+
+	return uV;
+}
+
+static int spmi_regulator_hfs430_set_voltage(struct regulator_dev *rdev,
+					     unsigned selector)
+{
+	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
+	const struct spmi_voltage_range *range;
+	int uV, vlevel;
+	u8 val[2];
+
+	range = spmi_regulator_find_range(vreg);
+	if (!range)
+		return -EINVAL;
+
+	uV = spmi_regulator_common_list_voltage(rdev, selector);
+	if (uV <= 0)
+		return uV;
+
+	vlevel = roundup(uV, range->step_uV) / 1000;
+
+	dev_dbg(&rdev->dev, "write (%d, %d), mode (%d)", uV, vlevel,
+		spmi_regulator_hfs430_get_mode(rdev));
+
+	val[0] = vlevel & 0xFF;
+	val[1] = (vlevel >> 8) & 0xFF;
+
+	return spmi_vreg_write(vreg, SPMI_HFS430_REG_VOLTAGE_LB, val, 2);
+}
+
 static struct regulator_ops spmi_saw_ops = {};
 
 static struct regulator_ops spmi_smps_ops = {
@@ -1264,12 +1399,24 @@ static struct regulator_ops spmi_ult_ldo_ops = {
 	.set_soft_start		= spmi_regulator_common_set_soft_start,
 };
 
+static struct regulator_ops spmi_hfs430_ops = {
+	/* always on regulators */
+	.set_voltage_sel	= spmi_regulator_hfs430_set_voltage,
+	.set_voltage_time_sel	= spmi_regulator_set_voltage_time_sel,
+	.get_voltage		= spmi_regulator_hfs430_get_voltage,
+	.map_voltage		= spmi_regulator_common_map_voltage,
+	.list_voltage		= spmi_regulator_common_list_voltage,
+	.get_mode		= spmi_regulator_hfs430_get_mode,
+	.set_mode		= spmi_regulator_hfs430_set_mode,
+};
+
 /* Maximum possible digital major revision value */
 #define INF 0xFF
 
 static const struct spmi_regulator_mapping supported_regulators[] = {
-	/*           type subtype dig_min dig_max ltype ops setpoints hpm_min */
+	/*        type subtype dig_min dig_max ltype ops setpoints hpm_min */
 	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
+	SPMI_VREG(BUCK,  HFS430,   0, INF, 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),
@@ -1396,8 +1543,12 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg)
 {
 	int ret;
 	u8 reg = 0;
-	int step, delay, slew_rate, step_delay;
+	int step, delay, slew_rate;
 	const struct spmi_voltage_range *range;
+	struct slew_rate_config *config, configs[] = {
+		[0] = SLEW_RATE_CONFIG(HFS430),
+		[1] = SLEW_RATE_CONFIG(FTSMPS),
+	};
 
 	ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_STEP_CTRL, &reg, 1);
 	if (ret) {
@@ -1410,25 +1561,26 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg)
 		return -EINVAL;
 
 	switch (vreg->logical_type) {
-	case SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS:
-		step_delay = SPMI_FTSMPS_STEP_DELAY;
+	case SPMI_REGULATOR_LOGICAL_TYPE_HFS430:
+		config = &configs[0];
 		break;
 	default:
-		step_delay = SPMI_DEFAULT_STEP_DELAY;
-		break;
-	}
+		config = &configs[1];
+		if (vreg->logical_type != SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS)
+			config->step_delay =  SPMI_STEP_DELAY(DEFAULT);
+	};
 
-	step = reg & SPMI_FTSMPS_STEP_CTRL_STEP_MASK;
-	step >>= SPMI_FTSMPS_STEP_CTRL_STEP_SHIFT;
+	step = reg & config->step_mask;
+	step >>= config->step_shift;
 
-	delay = reg & SPMI_FTSMPS_STEP_CTRL_DELAY_MASK;
-	delay >>= SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT;
+	delay = reg & config->delay_mask;
+	delay >>= config->delay_shift;
 
 	/* slew_rate has units of uV/us */
-	slew_rate = SPMI_FTSMPS_CLOCK_RATE * range->step_uV * (1 << step);
-	slew_rate /= 1000 * (step_delay << delay);
-	slew_rate *= SPMI_FTSMPS_STEP_MARGIN_NUM;
-	slew_rate /= SPMI_FTSMPS_STEP_MARGIN_DEN;
+	slew_rate = config->clock_rate * range->step_uV * (1 << step);
+	slew_rate /= 1000 * (config->step_delay << delay);
+	slew_rate *= config->margin_num;
+	slew_rate /= config->margin_den;
 
 	/* Ensure that the slew rate is greater than 0 */
 	vreg->slew_rate = max(slew_rate, 1);
@@ -1445,6 +1597,9 @@ static int spmi_regulator_init_registers(struct spmi_regulator *vreg,
 
 	type = vreg->logical_type;
 
+	if (type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
+		return 0;
+
 	ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, ctrl_reg, 8);
 	if (ret)
 		return ret;
@@ -1572,9 +1727,11 @@ static int spmi_regulator_of_parse(struct device_node *node,
 	case SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS:
 	case SPMI_REGULATOR_LOGICAL_TYPE_ULT_HO_SMPS:
 	case SPMI_REGULATOR_LOGICAL_TYPE_SMPS:
+	case SPMI_REGULATOR_LOGICAL_TYPE_HFS430:
 		ret = spmi_regulator_init_slew_rate(vreg);
 		if (ret)
 			return ret;
+
 	default:
 		break;
 	}
@@ -1731,12 +1888,18 @@ static const struct spmi_regulator_data pmi8994_regulators[] = {
 	{ }
 };
 
+static const struct spmi_regulator_data pms405_regulators[] = {
+	{ "s3", 0x1a00, }, /* supply name in the dts only */
+	{ }
+};
+
 static const struct of_device_id qcom_spmi_regulator_match[] = {
 	{ .compatible = "qcom,pm8841-regulators", .data = &pm8841_regulators },
 	{ .compatible = "qcom,pm8916-regulators", .data = &pm8916_regulators },
 	{ .compatible = "qcom,pm8941-regulators", .data = &pm8941_regulators },
 	{ .compatible = "qcom,pm8994-regulators", .data = &pm8994_regulators },
 	{ .compatible = "qcom,pmi8994-regulators", .data = &pmi8994_regulators },
+	{ .compatible = "qcom,pms405-regulators", .data = &pms405_regulators },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_spmi_regulator_match);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: dts: qcom: pms405: add spmi regulators

[Jorge Ramirez-Ortiz]

The PMS405 sports 5 SMPS and 13 LDO regulators.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 arch/arm64/boot/dts/qcom/pms405.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pms405.dtsi b/arch/arm64/boot/dts/qcom/pms405.dtsi
index ad2b62d..6fbdb1e 100644
--- a/arch/arm64/boot/dts/qcom/pms405.dtsi
+++ b/arch/arm64/boot/dts/qcom/pms405.dtsi
@@ -52,4 +52,24 @@
 			interrupts = <0x0 0x61 0x1 IRQ_TYPE_NONE>;
 		};
 	};
+
+	pms405_1: pms405@1 {
+		compatible = "qcom,spmi-pmic";
+		reg = <0x1 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		regulators {
+			compatible = "qcom,pms405-regulators";
+			vdd_s3-supply = <&pms405_s3>;
+
+			pms405_s3: s3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-name = "vdd_cpu";
+				regulator-min-microvolt = <1048000>;
+				regulator-max-microvolt = <1352000>;
+			};
+		};
+	};
 };
-- 
2.7.4

[PATCH 3/3] arm64: dts: qcom: pms405: add spmi regulators

[Jorge Ramirez-Ortiz]

The PMS405 sports 5 SMPS and 13 LDO regulators.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 arch/arm64/boot/dts/qcom/pms405.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pms405.dtsi b/arch/arm64/boot/dts/qcom/pms405.dtsi
index ad2b62d..6fbdb1e 100644
--- a/arch/arm64/boot/dts/qcom/pms405.dtsi
+++ b/arch/arm64/boot/dts/qcom/pms405.dtsi
@@ -52,4 +52,24 @@
 			interrupts = <0x0 0x61 0x1 IRQ_TYPE_NONE>;
 		};
 	};
+
+	pms405_1: pms405@1 {
+		compatible = "qcom,spmi-pmic";
+		reg = <0x1 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		regulators {
+			compatible = "qcom,pms405-regulators";
+			vdd_s3-supply = <&pms405_s3>;
+
+			pms405_s3: s3 {
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-name = "vdd_cpu";
+				regulator-min-microvolt = <1048000>;
+				regulator-max-microvolt = <1352000>;
+			};
+		};
+	};
 };
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote:

> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
>  	range = vreg->set_points->range;
>  	end = range + vreg->set_points->count;
>  
> +	/* we know we only have one range for this type */
> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
> +		return range;
> +
>  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
>  
>  	for (; range < end; range++)

Rather than have special casing for the logical type in here it seems
better to just provide a specific op for this logical type, you could
always make _find_range() call into that one if you really want code
reuse here.  You already have separate ops for this regulator type
anyway.

> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
> +{
> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> +	u8 reg;
> +	int ret;
> +
> +	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
> +	if (ret) {
> +		dev_err(&rdev->dev, "failed to get mode");
> +		return ret;
> +	}
> +
> +	if (reg == SPMI_HFS430_MODE_PWM)
> +		return REGULATOR_MODE_NORMAL;
> +
> +	return REGULATOR_MODE_IDLE;
> +}

I'd have expected a switch statement here, ideally flagging a warning or
error if we get a surprising value in there.

> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
> +					  unsigned int mode)
> +{
> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> +	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
> +						 SPMI_HFS430_MODE_AUTO;

Please write a normal if statement (or switch statement) to help
legibility.

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1725 bytes --]

On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote:

> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
>  	range = vreg->set_points->range;
>  	end = range + vreg->set_points->count;
>  
> +	/* we know we only have one range for this type */
> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
> +		return range;
> +
>  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
>  
>  	for (; range < end; range++)

Rather than have special casing for the logical type in here it seems
better to just provide a specific op for this logical type, you could
always make _find_range() call into that one if you really want code
reuse here.  You already have separate ops for this regulator type
anyway.

> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
> +{
> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> +	u8 reg;
> +	int ret;
> +
> +	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
> +	if (ret) {
> +		dev_err(&rdev->dev, "failed to get mode");
> +		return ret;
> +	}
> +
> +	if (reg == SPMI_HFS430_MODE_PWM)
> +		return REGULATOR_MODE_NORMAL;
> +
> +	return REGULATOR_MODE_IDLE;
> +}

I'd have expected a switch statement here, ideally flagging a warning or
error if we get a surprising value in there.

> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
> +					  unsigned int mode)
> +{
> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> +	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
> +						 SPMI_HFS430_MODE_AUTO;

Please write a normal if statement (or switch statement) to help
legibility.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support

[Rob Herring]

On Mon, 28 Jan 2019 12:45:02 +0100, Jorge Ramirez-Ortiz wrote:
> The PMS405 supports 5 SMPS and 13 LDO regulators.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  .../bindings/regulator/qcom,spmi-regulator.txt     | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support

[Rob Herring]

On Mon, 28 Jan 2019 12:45:02 +0100, Jorge Ramirez-Ortiz wrote:
> The PMS405 supports 5 SMPS and 13 LDO regulators.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  .../bindings/regulator/qcom,spmi-regulator.txt     | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support

[Rob Herring]

On Mon, 28 Jan 2019 12:45:02 +0100, Jorge Ramirez-Ortiz wrote:
> The PMS405 supports 5 SMPS and 13 LDO regulators.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  .../bindings/regulator/qcom,spmi-regulator.txt     | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 2/4/19 10:03, Mark Brown wrote:
> On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote:
> 
>> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
>>  	range = vreg->set_points->range;
>>  	end = range + vreg->set_points->count;
>>  
>> +	/* we know we only have one range for this type */
>> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
>> +		return range;
>> +
>>  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
>>  
>>  	for (; range < end; range++)
> 
> Rather than have special casing for the logical type in here it seems
> better to just provide a specific op for this logical type, you could
> always make _find_range() call into that one if you really want code
> reuse here.  You already have separate ops for this regulator type
> anyway.

sorry I dont quite understand your point.

static struct regulator_ops spmi_hfs430_ops = {
	/* always on regulators */
	.set_voltage_sel	= spmi_regulator_hfs430_set_voltage, *
	.set_voltage_time_sel	= spmi_regulator_set_voltage_time_sel, *
	.get_voltage		= spmi_regulator_hfs430_get_voltage,
	.map_voltage		= spmi_regulator_common_map_voltage, *
	.list_voltage		= spmi_regulator_common_list_voltage,
	.get_mode		= spmi_regulator_hfs430_get_mode,
	.set_mode		= spmi_regulator_hfs430_set_mode,
};

find_range affects the functions above with *

You are right and I can easily adjust the private
spmi_regulator_hfs430_set_voltage. And since it is quite small I can
also _duplicate_ the common function spmi_regulator_set_voltage_time_sel
with a small change for hfs430.

But spmi_regulator_common_map_voltage ends up being a large function and
I dont see the point in replicating it to save the "if" statement above.

why cant different logical_types extend spmi_regulator_find_range(..)?

Or maybe are you saying that I should add a new interface to struct
spmi_regulator that implements priv_find_range(..) for the logical types
that dont want to use the common implementation?

But also I am not sure I see the benefits with respect to the proposed
change...


> 
>> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg;
>> +	int ret;
>> +
>> +	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
>> +	if (ret) {
>> +		dev_err(&rdev->dev, "failed to get mode");
>> +		return ret;
>> +	}
>> +
>> +	if (reg == SPMI_HFS430_MODE_PWM)
>> +		return REGULATOR_MODE_NORMAL;
>> +
>> +	return REGULATOR_MODE_IDLE;
>> +}
> 
> I'd have expected a switch statement here, ideally flagging a warning or
> error if we get a surprising value in there.

this implementation follows what the common function
spmi_regulator_common_get_mode implements (ie, checks for a case and
defaults if that is not the one; and when defaulting,  there is no
reporting that it is actually defaulting: ie, defaulting is not being
interpreted as an error..should it?)

> 
>> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
>> +					  unsigned int mode)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
>> +						 SPMI_HFS430_MODE_AUTO;
> 
> Please write a normal if statement (or switch statement) to help
> legibility.
> 

ok.

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 2/4/19 10:03, Mark Brown wrote:
> On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote:
> 
>> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
>>  	range = vreg->set_points->range;
>>  	end = range + vreg->set_points->count;
>>  
>> +	/* we know we only have one range for this type */
>> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
>> +		return range;
>> +
>>  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
>>  
>>  	for (; range < end; range++)
> 
> Rather than have special casing for the logical type in here it seems
> better to just provide a specific op for this logical type, you could
> always make _find_range() call into that one if you really want code
> reuse here.  You already have separate ops for this regulator type
> anyway.

sorry I dont quite understand your point.

static struct regulator_ops spmi_hfs430_ops = {
	/* always on regulators */
	.set_voltage_sel	= spmi_regulator_hfs430_set_voltage, *
	.set_voltage_time_sel	= spmi_regulator_set_voltage_time_sel, *
	.get_voltage		= spmi_regulator_hfs430_get_voltage,
	.map_voltage		= spmi_regulator_common_map_voltage, *
	.list_voltage		= spmi_regulator_common_list_voltage,
	.get_mode		= spmi_regulator_hfs430_get_mode,
	.set_mode		= spmi_regulator_hfs430_set_mode,
};

find_range affects the functions above with *

You are right and I can easily adjust the private
spmi_regulator_hfs430_set_voltage. And since it is quite small I can
also _duplicate_ the common function spmi_regulator_set_voltage_time_sel
with a small change for hfs430.

But spmi_regulator_common_map_voltage ends up being a large function and
I dont see the point in replicating it to save the "if" statement above.

why cant different logical_types extend spmi_regulator_find_range(..)?

Or maybe are you saying that I should add a new interface to struct
spmi_regulator that implements priv_find_range(..) for the logical types
that dont want to use the common implementation?

But also I am not sure I see the benefits with respect to the proposed
change...


> 
>> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg;
>> +	int ret;
>> +
>> +	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
>> +	if (ret) {
>> +		dev_err(&rdev->dev, "failed to get mode");
>> +		return ret;
>> +	}
>> +
>> +	if (reg == SPMI_HFS430_MODE_PWM)
>> +		return REGULATOR_MODE_NORMAL;
>> +
>> +	return REGULATOR_MODE_IDLE;
>> +}
> 
> I'd have expected a switch statement here, ideally flagging a warning or
> error if we get a surprising value in there.

this implementation follows what the common function
spmi_regulator_common_get_mode implements (ie, checks for a case and
defaults if that is not the one; and when defaulting,  there is no
reporting that it is actually defaulting: ie, defaulting is not being
interpreted as an error..should it?)

> 
>> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
>> +					  unsigned int mode)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
>> +						 SPMI_HFS430_MODE_AUTO;
> 
> Please write a normal if statement (or switch statement) to help
> legibility.
> 

ok.

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 2/4/19 10:03, Mark Brown wrote:
> On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote:
> 
>> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
>>  	range = vreg->set_points->range;
>>  	end = range + vreg->set_points->count;
>>  
>> +	/* we know we only have one range for this type */
>> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
>> +		return range;
>> +
>>  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);
>>  
>>  	for (; range < end; range++)
> 
> Rather than have special casing for the logical type in here it seems
> better to just provide a specific op for this logical type, you could
> always make _find_range() call into that one if you really want code
> reuse here.  You already have separate ops for this regulator type
> anyway.

sorry I dont quite understand your point.

static struct regulator_ops spmi_hfs430_ops = {
	/* always on regulators */
	.set_voltage_sel	= spmi_regulator_hfs430_set_voltage, *
	.set_voltage_time_sel	= spmi_regulator_set_voltage_time_sel, *
	.get_voltage		= spmi_regulator_hfs430_get_voltage,
	.map_voltage		= spmi_regulator_common_map_voltage, *
	.list_voltage		= spmi_regulator_common_list_voltage,
	.get_mode		= spmi_regulator_hfs430_get_mode,
	.set_mode		= spmi_regulator_hfs430_set_mode,
};

find_range affects the functions above with *

You are right and I can easily adjust the private
spmi_regulator_hfs430_set_voltage. And since it is quite small I can
also _duplicate_ the common function spmi_regulator_set_voltage_time_sel
with a small change for hfs430.

But spmi_regulator_common_map_voltage ends up being a large function and
I dont see the point in replicating it to save the "if" statement above.

why cant different logical_types extend spmi_regulator_find_range(..)?

Or maybe are you saying that I should add a new interface to struct
spmi_regulator that implements priv_find_range(..) for the logical types
that dont want to use the common implementation?

But also I am not sure I see the benefits with respect to the proposed
change...


> 
>> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg;
>> +	int ret;
>> +
>> +	ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, &reg, 1);
>> +	if (ret) {
>> +		dev_err(&rdev->dev, "failed to get mode");
>> +		return ret;
>> +	}
>> +
>> +	if (reg == SPMI_HFS430_MODE_PWM)
>> +		return REGULATOR_MODE_NORMAL;
>> +
>> +	return REGULATOR_MODE_IDLE;
>> +}
> 
> I'd have expected a switch statement here, ideally flagging a warning or
> error if we get a surprising value in there.

this implementation follows what the common function
spmi_regulator_common_get_mode implements (ie, checks for a case and
defaults if that is not the one; and when defaulting,  there is no
reporting that it is actually defaulting: ie, defaulting is not being
interpreted as an error..should it?)

> 
>> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev,
>> +					  unsigned int mode)
>> +{
>> +	struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
>> +	u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM :
>> +						 SPMI_HFS430_MODE_AUTO;
> 
> Please write a normal if statement (or switch statement) to help
> legibility.
> 

ok.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote:
> On 2/4/19 10:03, Mark Brown wrote:

> >> +	/* we know we only have one range for this type */
> >> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
> >> +		return range;

> > Rather than have special casing for the logical type in here it seems
> > better to just provide a specific op for this logical type, you could
> > always make _find_range() call into that one if you really want code
> > reuse here.  You already have separate ops for this regulator type
> > anyway.

> sorry I dont quite understand your point.

If you need to skip the majority of the contents of the function for
some regulators just define a separate function for those regulators and
give them different ops structures rather than using the same ops
structure and handling it in the functions.

> But also I am not sure I see the benefits with respect to the proposed
> change...

The benefit is that the selection of which operations to use is done in
only one place (the selection of the ops structure) rather than in
multiple places (the selection of the ops structure and the contents of
the operations).

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1168 bytes --]

On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote:
> On 2/4/19 10:03, Mark Brown wrote:

> >> +	/* we know we only have one range for this type */
> >> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
> >> +		return range;

> > Rather than have special casing for the logical type in here it seems
> > better to just provide a specific op for this logical type, you could
> > always make _find_range() call into that one if you really want code
> > reuse here.  You already have separate ops for this regulator type
> > anyway.

> sorry I dont quite understand your point.

If you need to skip the majority of the contents of the function for
some regulators just define a separate function for those regulators and
give them different ops structures rather than using the same ops
structure and handling it in the functions.

> But also I am not sure I see the benefits with respect to the proposed
> change...

The benefit is that the selection of which operations to use is done in
only one place (the selection of the ops structure) rather than in
multiple places (the selection of the ops structure and the contents of
the operations).

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 4/25/19 20:37, Mark Brown wrote:
> On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote:
>> On 2/4/19 10:03, Mark Brown wrote:
> 
>>>> +	/* we know we only have one range for this type */
>>>> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
>>>> +		return range;
> 
>>> Rather than have special casing for the logical type in here it seems
>>> better to just provide a specific op for this logical type, you could
>>> always make _find_range() call into that one if you really want code
>>> reuse here.  You already have separate ops for this regulator type
>>> anyway.
> 
>> sorry I dont quite understand your point.
> 
> If you need to skip the majority of the contents of the function for
> some regulators just define a separate function for those regulators and
> give them different ops structures rather than using the same ops
> structure and handling it in the functions.

sure this is 101 as a general rule: but this is not applicable to the
situation that I described in my original note, so I dont think you read
my points.

> 
>> But also I am not sure I see the benefits with respect to the proposed
>> change...
> 
> The benefit is that the selection of which operations to use is done in
> only one place (the selection of the ops structure) rather than in
> multiple places (the selection of the ops structure and the contents of
> the operations).

all right, how do you propose that we handle
spmi_regulator_select_voltage_same_range() then?

the way I see it, if I follow your suggestion and since we are not
allowed to extend spmi_regulator_find_range(), the only options are:

1) duplicate verbatim this whole function
(spmi_regulator_select_voltage_same_range) with a minor change (this
amount of code duplication in the kernel seems rather unnecessary to me)

2) modify the struct spmi_regulator definition with a new operation that
calls a different implementation of find range (seems a massive overkill)

because both seem wrong to me, can you confirm that you are ok with one
of those two options? or if not give an alternative?

But I still would like to understand why you think it is wrong extending
spmi_regulator_find_range() to support HFS430.

Are you saying that this function should not exist for this regulator?

Sure the HFS430 doesnt use the register SPMI_COMMON_REG_VOLTAGE_RANGE
and therefore doesnt need to use it to find its range....but that doesnt
mean that the semantics of spmi_regulator_find_range are invalid.

The way I understand your concern, you seem to be assuming that
spmi_regulator_find_range means something like
spmi_regulator_find_range_from_reg_voltage but that is not the case or
if it is maybe it should be renamed.....

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 4/25/19 20:37, Mark Brown wrote:
> On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote:
>> On 2/4/19 10:03, Mark Brown wrote:
> 
>>>> +	/* we know we only have one range for this type */
>>>> +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
>>>> +		return range;
> 
>>> Rather than have special casing for the logical type in here it seems
>>> better to just provide a specific op for this logical type, you could
>>> always make _find_range() call into that one if you really want code
>>> reuse here.  You already have separate ops for this regulator type
>>> anyway.
> 
>> sorry I dont quite understand your point.
> 
> If you need to skip the majority of the contents of the function for
> some regulators just define a separate function for those regulators and
> give them different ops structures rather than using the same ops
> structure and handling it in the functions.

sure this is 101 as a general rule: but this is not applicable to the
situation that I described in my original note, so I dont think you read
my points.

> 
>> But also I am not sure I see the benefits with respect to the proposed
>> change...
> 
> The benefit is that the selection of which operations to use is done in
> only one place (the selection of the ops structure) rather than in
> multiple places (the selection of the ops structure and the contents of
> the operations).

all right, how do you propose that we handle
spmi_regulator_select_voltage_same_range() then?

the way I see it, if I follow your suggestion and since we are not
allowed to extend spmi_regulator_find_range(), the only options are:

1) duplicate verbatim this whole function
(spmi_regulator_select_voltage_same_range) with a minor change (this
amount of code duplication in the kernel seems rather unnecessary to me)

2) modify the struct spmi_regulator definition with a new operation that
calls a different implementation of find range (seems a massive overkill)

because both seem wrong to me, can you confirm that you are ok with one
of those two options? or if not give an alternative?

But I still would like to understand why you think it is wrong extending
spmi_regulator_find_range() to support HFS430.

Are you saying that this function should not exist for this regulator?

Sure the HFS430 doesnt use the register SPMI_COMMON_REG_VOLTAGE_RANGE
and therefore doesnt need to use it to find its range....but that doesnt
mean that the semantics of spmi_regulator_find_range are invalid.

The way I understand your concern, you seem to be assuming that
spmi_regulator_find_range means something like
spmi_regulator_find_range_from_reg_voltage but that is not the case or
if it is maybe it should be renamed.....










_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote:

> the way I see it, if I follow your suggestion and since we are not
> allowed to extend spmi_regulator_find_range(), the only options are:

> 1) duplicate verbatim this whole function
> (spmi_regulator_select_voltage_same_range) with a minor change (this
> amount of code duplication in the kernel seems rather unnecessary to me)

> 2) modify the struct spmi_regulator definition with a new operation that
> calls a different implementation of find range (seems a massive overkill)

Since the point of this change is AFAICT that this regulator only has a
single linear range it seems like it should just be able to use the
existing generic functions shouldn't it?  It just needs it's own
set/get_voltage_sel() operations.  As far as I can see the main thing
the driver is doing with the custom stuff is handling the fact that
there's multiple ranges but that's not an issue for this regulator.
It's possible I'm missing something there but that was the main thing
(and we do have some generic support for multiple linear ranges in the
helper code already, can't remember why this driver isn't using that -
the ranges overlap IIRC?).

TBH looking at the uses of find_range() I'm not sure they're 100%
sensible as they are - the existing _time_sel() is assuming we only need
to work out the ramp time between voltages in the same range which is
going to have trouble.

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1431 bytes --]

On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote:

> the way I see it, if I follow your suggestion and since we are not
> allowed to extend spmi_regulator_find_range(), the only options are:

> 1) duplicate verbatim this whole function
> (spmi_regulator_select_voltage_same_range) with a minor change (this
> amount of code duplication in the kernel seems rather unnecessary to me)

> 2) modify the struct spmi_regulator definition with a new operation that
> calls a different implementation of find range (seems a massive overkill)

Since the point of this change is AFAICT that this regulator only has a
single linear range it seems like it should just be able to use the
existing generic functions shouldn't it?  It just needs it's own
set/get_voltage_sel() operations.  As far as I can see the main thing
the driver is doing with the custom stuff is handling the fact that
there's multiple ranges but that's not an issue for this regulator.
It's possible I'm missing something there but that was the main thing
(and we do have some generic support for multiple linear ranges in the
helper code already, can't remember why this driver isn't using that -
the ranges overlap IIRC?).

TBH looking at the uses of find_range() I'm not sure they're 100%
sensible as they are - the existing _time_sel() is assuming we only need
to work out the ramp time between voltages in the same range which is
going to have trouble.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 4/27/19 20:21, Mark Brown wrote:
> On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote:
> 
>> the way I see it, if I follow your suggestion and since we are not
>> allowed to extend spmi_regulator_find_range(), the only options are:
> 
>> 1) duplicate verbatim this whole function
>> (spmi_regulator_select_voltage_same_range) with a minor change (this
>> amount of code duplication in the kernel seems rather unnecessary to me)
> 
>> 2) modify the struct spmi_regulator definition with a new operation that
>> calls a different implementation of find range (seems a massive overkill)
> 
> Since the point of this change is AFAICT that this regulator only has a
> single linear range it seems like it should just be able to use the
> existing generic functions shouldn't it?  

yes that would have been ideal but it does not seem to be the case for
this hardware.

The register that stores the voltage range for all other SPMI regulators
(SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
bytes 0x40 and 0x41;

This overlap really what is creating the pain: HFS430 cant use 0x40 to
store the range (even if it is only one)

so yeah, most of the changes in the patch are working around this fact.

enum spmi_common_regulator_registers {
	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
	SPMI_COMMON_REG_TYPE			= 0x04,
	SPMI_COMMON_REG_SUBTYPE			= 0x05,
	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
	SPMI_COMMON_REG_MODE			= 0x45,
	SPMI_COMMON_REG_ENABLE			= 0x46,
	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
	SPMI_COMMON_REG_SOFT_START		= 0x4c,
	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
};

enum spmi_hfs430_registers {
	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
	SPMI_HFS430_REG_MODE			= 0x45,
};

It just needs it's own
> set/get_voltage_sel() operations.  As far as I can see the main thing
> the driver is doing with the custom stuff is handling the fact that
> there's multiple ranges but that's not an issue for this regulator.
> It's possible I'm missing something there but that was the main thing
> (and we do have some generic support for multiple linear ranges in the
> helper code already, can't remember why this driver isn't using that -
> the ranges overlap IIRC?).
> 
> TBH looking at the uses of find_range() I'm not sure they're 100%
> sensible as they are - the existing _time_sel() is assuming we only need
> to work out the ramp time between voltages in the same range which is
> going to have trouble.
> 

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 4/27/19 20:21, Mark Brown wrote:
> On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote:
> 
>> the way I see it, if I follow your suggestion and since we are not
>> allowed to extend spmi_regulator_find_range(), the only options are:
> 
>> 1) duplicate verbatim this whole function
>> (spmi_regulator_select_voltage_same_range) with a minor change (this
>> amount of code duplication in the kernel seems rather unnecessary to me)
> 
>> 2) modify the struct spmi_regulator definition with a new operation that
>> calls a different implementation of find range (seems a massive overkill)
> 
> Since the point of this change is AFAICT that this regulator only has a
> single linear range it seems like it should just be able to use the
> existing generic functions shouldn't it?  

yes that would have been ideal but it does not seem to be the case for
this hardware.

The register that stores the voltage range for all other SPMI regulators
(SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
bytes 0x40 and 0x41;

This overlap really what is creating the pain: HFS430 cant use 0x40 to
store the range (even if it is only one)

so yeah, most of the changes in the patch are working around this fact.

enum spmi_common_regulator_registers {
	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
	SPMI_COMMON_REG_TYPE			= 0x04,
	SPMI_COMMON_REG_SUBTYPE			= 0x05,
	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
	SPMI_COMMON_REG_MODE			= 0x45,
	SPMI_COMMON_REG_ENABLE			= 0x46,
	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
	SPMI_COMMON_REG_SOFT_START		= 0x4c,
	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
};

enum spmi_hfs430_registers {
	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
	SPMI_HFS430_REG_MODE			= 0x45,
};

It just needs it's own
> set/get_voltage_sel() operations.  As far as I can see the main thing
> the driver is doing with the custom stuff is handling the fact that
> there's multiple ranges but that's not an issue for this regulator.
> It's possible I'm missing something there but that was the main thing
> (and we do have some generic support for multiple linear ranges in the
> helper code already, can't remember why this driver isn't using that -
> the ranges overlap IIRC?).
> 
> TBH looking at the uses of find_range() I'm not sure they're 100%
> sensible as they are - the existing _time_sel() is assuming we only need
> to work out the ramp time between voltages in the same range which is
> going to have trouble.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote:
> On 4/27/19 20:21, Mark Brown wrote:

> > Since the point of this change is AFAICT that this regulator only has a
> > single linear range it seems like it should just be able to use the
> > existing generic functions shouldn't it?  

> yes that would have been ideal but it does not seem to be the case for
> this hardware.

> The register that stores the voltage range for all other SPMI regulators
> (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
> HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
> bytes 0x40 and 0x41;

> This overlap really what is creating the pain: HFS430 cant use 0x40 to
> store the range (even if it is only one)

> so yeah, most of the changes in the patch are working around this fact.

I'm not sure I follow here, sorry - I can see that the driver needs a
custom get/set selector operation but shouldn't it be able to use the
standard list and map operations for linear ranges?

> 
> enum spmi_common_regulator_registers {
> 	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
> 	SPMI_COMMON_REG_TYPE			= 0x04,
> 	SPMI_COMMON_REG_SUBTYPE			= 0x05,
> 	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
> 	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
> 	SPMI_COMMON_REG_MODE			= 0x45,
> 	SPMI_COMMON_REG_ENABLE			= 0x46,
> 	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
> 	SPMI_COMMON_REG_SOFT_START		= 0x4c,
> 	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
> };
> 
> enum spmi_hfs430_registers {
> 	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
> 	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
> 	SPMI_HFS430_REG_MODE			= 0x45,
> };
> 
> It just needs it's own
> > set/get_voltage_sel() operations.  As far as I can see the main thing
> > the driver is doing with the custom stuff is handling the fact that
> > there's multiple ranges but that's not an issue for this regulator.
> > It's possible I'm missing something there but that was the main thing
> > (and we do have some generic support for multiple linear ranges in the
> > helper code already, can't remember why this driver isn't using that -
> > the ranges overlap IIRC?).
> > 
> > TBH looking at the uses of find_range() I'm not sure they're 100%
> > sensible as they are - the existing _time_sel() is assuming we only need
> > to work out the ramp time between voltages in the same range which is
> > going to have trouble.
> > 
> 

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2440 bytes --]

On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote:
> On 4/27/19 20:21, Mark Brown wrote:

> > Since the point of this change is AFAICT that this regulator only has a
> > single linear range it seems like it should just be able to use the
> > existing generic functions shouldn't it?  

> yes that would have been ideal but it does not seem to be the case for
> this hardware.

> The register that stores the voltage range for all other SPMI regulators
> (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
> HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
> bytes 0x40 and 0x41;

> This overlap really what is creating the pain: HFS430 cant use 0x40 to
> store the range (even if it is only one)

> so yeah, most of the changes in the patch are working around this fact.

I'm not sure I follow here, sorry - I can see that the driver needs a
custom get/set selector operation but shouldn't it be able to use the
standard list and map operations for linear ranges?

> 
> enum spmi_common_regulator_registers {
> 	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
> 	SPMI_COMMON_REG_TYPE			= 0x04,
> 	SPMI_COMMON_REG_SUBTYPE			= 0x05,
> 	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
> 	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
> 	SPMI_COMMON_REG_MODE			= 0x45,
> 	SPMI_COMMON_REG_ENABLE			= 0x46,
> 	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
> 	SPMI_COMMON_REG_SOFT_START		= 0x4c,
> 	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
> };
> 
> enum spmi_hfs430_registers {
> 	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
> 	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,
> 	SPMI_HFS430_REG_MODE			= 0x45,
> };
> 
> It just needs it's own
> > set/get_voltage_sel() operations.  As far as I can see the main thing
> > the driver is doing with the custom stuff is handling the fact that
> > there's multiple ranges but that's not an issue for this regulator.
> > It's possible I'm missing something there but that was the main thing
> > (and we do have some generic support for multiple linear ranges in the
> > helper code already, can't remember why this driver isn't using that -
> > the ranges overlap IIRC?).
> > 
> > TBH looking at the uses of find_range() I'm not sure they're 100%
> > sensible as they are - the existing _time_sel() is assuming we only need
> > to work out the ramp time between voltages in the same range which is
> > going to have trouble.
> > 
> 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/2/19 04:33, Mark Brown wrote:
> On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote:
>> On 4/27/19 20:21, Mark Brown wrote:
> 
>>> Since the point of this change is AFAICT that this regulator only has a
>>> single linear range it seems like it should just be able to use the
>>> existing generic functions shouldn't it?  
> 
>> yes that would have been ideal but it does not seem to be the case for
>> this hardware.
> 
>> The register that stores the voltage range for all other SPMI regulators
>> (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
>> HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
>> bytes 0x40 and 0x41;
> 
>> This overlap really what is creating the pain: HFS430 cant use 0x40 to
>> store the range (even if it is only one)
> 
>> so yeah, most of the changes in the patch are working around this fact.
> 
> I'm not sure I follow here, sorry - I can see that the driver needs a
> custom get/set selector operation but shouldn't it be able to use the
> standard list and map operations for linear ranges?

I agree it should, but unfortunately that is not the case; when I first
posted the patch I was concerned that for a regulator to be supported by
this driver it should obey to the driver's internals (ie: comply with
all of the spmi_common_regulator_registers definitions).

However, since there was just a single range to support, the
modifications I had to do to support this SPMI regulator were minimal -
hence why I opted for the changes under discussion instead of writing a
new driver (which IMO it is an overkill).

what do you think?

> 
>>
>> enum spmi_common_regulator_registers {
>> 	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
>> 	SPMI_COMMON_REG_TYPE			= 0x04,
>> 	SPMI_COMMON_REG_SUBTYPE			= 0x05,
>> 	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
>> 	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
>> 	SPMI_COMMON_REG_MODE			= 0x45,
>> 	SPMI_COMMON_REG_ENABLE			= 0x46,
>> 	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
>> 	SPMI_COMMON_REG_SOFT_START		= 0x4c,
>> 	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
>> };
>>
>> enum spmi_hfs430_registers {
>> 	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
>> 	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,

ah, this definition I can remove and use the common one above. I'll do that.
>> 	SPMI_HFS430_REG_MODE			= 0x45,


>> };
>>
>> It just needs it's own
>>> set/get_voltage_sel() operations.  As far as I can see the main thing
>>> the driver is doing with the custom stuff is handling the fact that
>>> there's multiple ranges but that's not an issue for this regulator.
>>> It's possible I'm missing something there but that was the main thing
>>> (and we do have some generic support for multiple linear ranges in the
>>> helper code already, can't remember why this driver isn't using that -
>>> the ranges overlap IIRC?).
>>>
>>> TBH looking at the uses of find_range() I'm not sure they're 100%
>>> sensible as they are - the existing _time_sel() is assuming we only need
>>> to work out the ramp time between voltages in the same range which is
>>> going to have trouble.
>>>
>>

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/2/19 04:33, Mark Brown wrote:
> On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote:
>> On 4/27/19 20:21, Mark Brown wrote:
> 
>>> Since the point of this change is AFAICT that this regulator only has a
>>> single linear range it seems like it should just be able to use the
>>> existing generic functions shouldn't it?  
> 
>> yes that would have been ideal but it does not seem to be the case for
>> this hardware.
> 
>> The register that stores the voltage range for all other SPMI regulators
>> (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the
>> HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two
>> bytes 0x40 and 0x41;
> 
>> This overlap really what is creating the pain: HFS430 cant use 0x40 to
>> store the range (even if it is only one)
> 
>> so yeah, most of the changes in the patch are working around this fact.
> 
> I'm not sure I follow here, sorry - I can see that the driver needs a
> custom get/set selector operation but shouldn't it be able to use the
> standard list and map operations for linear ranges?

I agree it should, but unfortunately that is not the case; when I first
posted the patch I was concerned that for a regulator to be supported by
this driver it should obey to the driver's internals (ie: comply with
all of the spmi_common_regulator_registers definitions).

However, since there was just a single range to support, the
modifications I had to do to support this SPMI regulator were minimal -
hence why I opted for the changes under discussion instead of writing a
new driver (which IMO it is an overkill).

what do you think?

> 
>>
>> enum spmi_common_regulator_registers {
>> 	SPMI_COMMON_REG_DIG_MAJOR_REV		= 0x01,
>> 	SPMI_COMMON_REG_TYPE			= 0x04,
>> 	SPMI_COMMON_REG_SUBTYPE			= 0x05,
>> 	SPMI_COMMON_REG_VOLTAGE_RANGE		= 0x40, ******
>> 	SPMI_COMMON_REG_VOLTAGE_SET		= 0x41,
>> 	SPMI_COMMON_REG_MODE			= 0x45,
>> 	SPMI_COMMON_REG_ENABLE			= 0x46,
>> 	SPMI_COMMON_REG_PULL_DOWN		= 0x48,
>> 	SPMI_COMMON_REG_SOFT_START		= 0x4c,
>> 	SPMI_COMMON_REG_STEP_CTRL		= 0x61,
>> };
>>
>> enum spmi_hfs430_registers {
>> 	SPMI_HFS430_REG_VOLTAGE_LB		= 0x40, *******
>> 	SPMI_HFS430_REG_VOLTAGE_VALID_LB	= 0x42,

ah, this definition I can remove and use the common one above. I'll do that.
>> 	SPMI_HFS430_REG_MODE			= 0x45,


>> };
>>
>> It just needs it's own
>>> set/get_voltage_sel() operations.  As far as I can see the main thing
>>> the driver is doing with the custom stuff is handling the fact that
>>> there's multiple ranges but that's not an issue for this regulator.
>>> It's possible I'm missing something there but that was the main thing
>>> (and we do have some generic support for multiple linear ranges in the
>>> helper code already, can't remember why this driver isn't using that -
>>> the ranges overlap IIRC?).
>>>
>>> TBH looking at the uses of find_range() I'm not sure they're 100%
>>> sensible as they are - the existing _time_sel() is assuming we only need
>>> to work out the ramp time between voltages in the same range which is
>>> going to have trouble.
>>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:
> On 5/2/19 04:33, Mark Brown wrote:

> > I'm not sure I follow here, sorry - I can see that the driver needs a
> > custom get/set selector operation but shouldn't it be able to use the
> > standard list and map operations for linear ranges?

> I agree it should, but unfortunately that is not the case; when I first
> posted the patch I was concerned that for a regulator to be supported by
> this driver it should obey to the driver's internals (ie: comply with
> all of the spmi_common_regulator_registers definitions).

That's not a requirement that I'd particularly expect - it's not unusual
for devices to have multiple different styles of regulators in a single
chip (eg, DCDCs often have quite different register maps to LDOs).

> However, since there was just a single range to support, the
> modifications I had to do to support this SPMI regulator were minimal -
> hence why I opted for the changes under discussion instead of writing a
> new driver (which IMO it is an overkill).

> what do you think?

It seems a bit of a jump to add a new driver - it's just another
descriptor and ops structure isn't it?  Though as ever with the Qualcomm
stuff this driver is pretty baroque which doesn't entirely help though I
think it's just another regulator type which there's already some
handling for.

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/3/19 08:26, Mark Brown wrote:
> On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:
>> On 5/2/19 04:33, Mark Brown wrote:
> 
>>> I'm not sure I follow here, sorry - I can see that the driver needs a
>>> custom get/set selector operation but shouldn't it be able to use the
>>> standard list and map operations for linear ranges?
> 
>> I agree it should, but unfortunately that is not the case; when I first
>> posted the patch I was concerned that for a regulator to be supported by
>> this driver it should obey to the driver's internals (ie: comply with
>> all of the spmi_common_regulator_registers definitions).
> 
> That's not a requirement that I'd particularly expect - it's not unusual
> for devices to have multiple different styles of regulators in a single
> chip (eg, DCDCs often have quite different register maps to LDOs).
> 
>> However, since there was just a single range to support, the
>> modifications I had to do to support this SPMI regulator were minimal -
>> hence why I opted for the changes under discussion instead of writing a
>> new driver (which IMO it is an overkill).
> 
>> what do you think?
> 
> It seems a bit of a jump to add a new driver - it's just another
> descriptor and ops structure isn't it?  Though as ever with the Qualcomm
> stuff this driver is pretty baroque which doesn't entirely help though I
> think it's just another regulator type which there's already some
> handling for.
> 

So how do we move this forward?

To sum up his regulator needs to be able to bypass accesses to
SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way
hence the change below

I can't find a simpler solution than this since the function does now
what is supposed to do for all the regulator types supported in the driver

 @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
  	range = vreg->set_points->range;
  	end = range + vreg->set_points->count;

 +	/* we know we only have one range for this type */
 +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
 +		return range;
 +
  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);

  	for (; range < end; range++)

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/3/19 08:26, Mark Brown wrote:
> On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:
>> On 5/2/19 04:33, Mark Brown wrote:
> 
>>> I'm not sure I follow here, sorry - I can see that the driver needs a
>>> custom get/set selector operation but shouldn't it be able to use the
>>> standard list and map operations for linear ranges?
> 
>> I agree it should, but unfortunately that is not the case; when I first
>> posted the patch I was concerned that for a regulator to be supported by
>> this driver it should obey to the driver's internals (ie: comply with
>> all of the spmi_common_regulator_registers definitions).
> 
> That's not a requirement that I'd particularly expect - it's not unusual
> for devices to have multiple different styles of regulators in a single
> chip (eg, DCDCs often have quite different register maps to LDOs).
> 
>> However, since there was just a single range to support, the
>> modifications I had to do to support this SPMI regulator were minimal -
>> hence why I opted for the changes under discussion instead of writing a
>> new driver (which IMO it is an overkill).
> 
>> what do you think?
> 
> It seems a bit of a jump to add a new driver - it's just another
> descriptor and ops structure isn't it?  Though as ever with the Qualcomm
> stuff this driver is pretty baroque which doesn't entirely help though I
> think it's just another regulator type which there's already some
> handling for.
> 

So how do we move this forward?

To sum up his regulator needs to be able to bypass accesses to
SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way
hence the change below

I can't find a simpler solution than this since the function does now
what is supposed to do for all the regulator types supported in the driver

 @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg)
  	range = vreg->set_points->range;
  	end = range + vreg->set_points->count;

 +	/* we know we only have one range for this type */
 +	if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430)
 +		return range;
 +
  	spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1);

  	for (; range < end; range++)




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Fri, May 03, 2019 at 10:29:42AM +0200, Jorge Ramirez wrote:
> On 5/3/19 08:26, Mark Brown wrote:
> > On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:

> > It seems a bit of a jump to add a new driver - it's just another
> > descriptor and ops structure isn't it?  Though as ever with the Qualcomm
> > stuff this driver is pretty baroque which doesn't entirely help though I
> > think it's just another regulator type which there's already some
> > handling for.

> So how do we move this forward?

> To sum up his regulator needs to be able to bypass accesses to
> SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way
> hence the change below

> I can't find a simpler solution than this since the function does now
> what is supposed to do for all the regulator types supported in the driver

The assumption that you need to have this regulator use functions that
use and provide ranges is the very thing I'm trying to get you to
change.  It looks like these regulators just need their own
set_voltage_sel() and get_voltage_sel() then they can use the standard
linear range mapping functions (and pobably the set_voltage_time_sel()
needs fixing anyway for all the other regulators).

There's already some conditional code in the probe function for handling
different operations for the over current protection and SAW stuff, this
looks like it should fit in reasonably well.  Usually this would be even
easier as probe functions are just data driven but for some reason more
than usual of this driver's data initializaiton is done dynamically.

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/6/19 06:38, Mark Brown wrote:
> On Fri, May 03, 2019 at 10:29:42AM +0200, Jorge Ramirez wrote:
>> On 5/3/19 08:26, Mark Brown wrote:
>>> On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:
> 
>>> It seems a bit of a jump to add a new driver - it's just another
>>> descriptor and ops structure isn't it?  Though as ever with the Qualcomm
>>> stuff this driver is pretty baroque which doesn't entirely help though I
>>> think it's just another regulator type which there's already some
>>> handling for.
> 
>> So how do we move this forward?
> 
>> To sum up his regulator needs to be able to bypass accesses to
>> SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way
>> hence the change below
> 
>> I can't find a simpler solution than this since the function does now
>> what is supposed to do for all the regulator types supported in the driver
> 
> The assumption that you need to have this regulator use functions that
> use and provide ranges is the very thing I'm trying to get you to
> change.  It looks like these regulators just need their own
> set_voltage_sel() and get_voltage_sel() then they can use the standard
> linear range mapping functions (and pobably the set_voltage_time_sel()
> needs fixing anyway for all the other regulators).

Right, and I understand what you are asking, is just that I completely
disagree with you. But moving on.

Would you accept if I wrote a separate driver specific to pms405 or do
you want me to integrate in qcom-spmi_regulator.c?

I am asking because none of the ops will use the common functions (I
wont be reusing much code from this qcom-spmi_regulator.c file)

> 
> There's already some conditional code in the probe function for handling
> different operations for the over current protection and SAW stuff, this
> looks like it should fit in reasonably well.  Usually this would be even
> easier as probe functions are just data driven but for some reason more
> than usual of this driver's data initializaiton is done dynamically.
> 

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Jorge Ramirez]

On 5/6/19 06:38, Mark Brown wrote:
> On Fri, May 03, 2019 at 10:29:42AM +0200, Jorge Ramirez wrote:
>> On 5/3/19 08:26, Mark Brown wrote:
>>> On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote:
> 
>>> It seems a bit of a jump to add a new driver - it's just another
>>> descriptor and ops structure isn't it?  Though as ever with the Qualcomm
>>> stuff this driver is pretty baroque which doesn't entirely help though I
>>> think it's just another regulator type which there's already some
>>> handling for.
> 
>> So how do we move this forward?
> 
>> To sum up his regulator needs to be able to bypass accesses to
>> SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way
>> hence the change below
> 
>> I can't find a simpler solution than this since the function does now
>> what is supposed to do for all the regulator types supported in the driver
> 
> The assumption that you need to have this regulator use functions that
> use and provide ranges is the very thing I'm trying to get you to
> change.  It looks like these regulators just need their own
> set_voltage_sel() and get_voltage_sel() then they can use the standard
> linear range mapping functions (and pobably the set_voltage_time_sel()
> needs fixing anyway for all the other regulators).

Right, and I understand what you are asking, is just that I completely
disagree with you. But moving on.

Would you accept if I wrote a separate driver specific to pms405 or do
you want me to integrate in qcom-spmi_regulator.c?

I am asking because none of the ops will use the common functions (I
wont be reusing much code from this qcom-spmi_regulator.c file)

> 
> There's already some conditional code in the probe function for handling
> different operations for the over current protection and SAW stuff, this
> looks like it should fit in reasonably well.  Usually this would be even
> easier as probe functions are just data driven but for some reason more
> than usual of this driver's data initializaiton is done dynamically.
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

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

On Thu, May 23, 2019 at 10:35:46AM +0200, Jorge Ramirez wrote:

> Would you accept if I wrote a separate driver specific to pms405 or do
> you want me to integrate in qcom-spmi_regulator.c?

> I am asking because none of the ops will use the common functions (I
> wont be reusing much code from this qcom-spmi_regulator.c file)

I don't really mind, if there's nothing really shared then making it a
separate driver is probably best but it's not a strong opinion either
way.

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

Re: [PATCH 2/3] drivers: regulator: qcom: add PMS405 SPMI regulator

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 475 bytes --]

On Thu, May 23, 2019 at 10:35:46AM +0200, Jorge Ramirez wrote:

> Would you accept if I wrote a separate driver specific to pms405 or do
> you want me to integrate in qcom-spmi_regulator.c?

> I am asking because none of the ops will use the common functions (I
> wont be reusing much code from this qcom-spmi_regulator.c file)

I don't really mind, if there's nothing really shared then making it a
separate driver is probably best but it's not a strong opinion either
way.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel