[PATCH v3 0/6] regulator: mcp16502: add support for ramp delay

[PATCH v3 0/6] regulator: mcp16502: add support for ramp delay

[Claudiu Beznea]

Hi,

This series adds support for ramp delay on mcp16502. It also adds
some cleanup on mcp16502.

Apart from that patches 1/6 fixes the selector validation in case
the regulator::desc::linear_min_sel is not zero.

Thank you,
Claudiu Beznea

Changes in v3:
- fix compilation error in patch 5/6
  Reported-by: kernel test robot <lkp@intel.com>

Changes in v2:
- rebase on top of regulator/for-next
- checked 1/6 and 3/6 applies on top of regulator/for-5.10

Claudiu Beznea (6):
  regulator: core: validate selector against linear_min_sel
  regulator: core: do not continue if selector match
  regulator: mcp16502: add linear_min_sel
  regulator: mcp16502: adapt for get/set on other registers
  regulator: mcp16502: add support for ramp delay
  regulator: mcp16502: remove void documentation of struct mcp16502

 drivers/regulator/core.c     |  12 +++-
 drivers/regulator/helpers.c  |   3 +-
 drivers/regulator/mcp16502.c | 135 ++++++++++++++++++++++++++++++++++++-------
 3 files changed, 127 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Claudiu Beznea]

There are regulators who's min selector is not zero. Selectors loops
(looping b/w zero and regulator::desc::n_voltages) might throw errors
because invalid selectors are used (lower than
regulator::desc::linear_min_sel). For this situations validate selectors
against regulator::desc::linear_min_sel.

Fixes: 3a40cfc36bb3d ("regulator: core: create unlocked version of regulator_list_voltage")
Fixes: 04eca28cde52c ("regulator: Add helpers for low-level register access")
Fixes: 88cd222b259d6 ("regulator: provide consumer interface for fall/rise time")
Fixes: d295f7670127e ("regulator: core: Move list_voltage_{linear,linear_range,table} to helpers.c")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/core.c    | 9 +++++++--
 drivers/regulator/helpers.c | 3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2e1ea18221ef..1d0d35f14f37 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2958,7 +2958,8 @@ static int _regulator_list_voltage(struct regulator_dev *rdev,
 		return rdev->desc->fixed_uV;
 
 	if (ops->list_voltage) {
-		if (selector >= rdev->desc->n_voltages)
+		if (selector >= rdev->desc->n_voltages ||
+		    selector < rdev->desc->linear_min_sel)
 			return -EINVAL;
 		if (lock)
 			regulator_lock(rdev);
@@ -3109,7 +3110,8 @@ int regulator_list_hardware_vsel(struct regulator *regulator,
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
 
-	if (selector >= rdev->desc->n_voltages)
+	if (selector >= rdev->desc->n_voltages ||
+	    selector < rdev->desc->linear_min_sel)
 		return -EINVAL;
 	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
 		return -EOPNOTSUPP;
@@ -4032,6 +4034,9 @@ int regulator_set_voltage_time(struct regulator *regulator,
 
 	for (i = 0; i < rdev->desc->n_voltages; i++) {
 		/* We only look for exact voltage matches here */
+		if (i < rdev->desc->linear_min_sel)
+			continue;
+
 		voltage = regulator_list_voltage(regulator, i);
 		if (voltage < 0)
 			return -EINVAL;
diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index e4bb09bbd3fa..974f1a63993d 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -647,7 +647,8 @@ int regulator_list_voltage_table(struct regulator_dev *rdev,
 		return -EINVAL;
 	}
 
-	if (selector >= rdev->desc->n_voltages)
+	if (selector >= rdev->desc->n_voltages ||
+	    selector < rdev->desc->linear_min_sel)
 		return -EINVAL;
 
 	return rdev->desc->volt_table[selector];
-- 
2.7.4

[PATCH v3 2/6] regulator: core: do not continue if selector match

[Claudiu Beznea]

Do not continue if selector has already been located.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1d0d35f14f37..6724324d372c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4037,6 +4037,9 @@ int regulator_set_voltage_time(struct regulator *regulator,
 		if (i < rdev->desc->linear_min_sel)
 			continue;
 
+		if (old_sel >= 0 && new_sel >= 0)
+			break;
+
 		voltage = regulator_list_voltage(regulator, i);
 		if (voltage < 0)
 			return -EINVAL;
-- 
2.7.4

[PATCH v3 3/6] regulator: mcp16502: add linear_min_sel

[Claudiu Beznea]

Selectors b/w zero and VDD_LOW_SEL are not valid. Use linear_min_sel.

Fixes: 919261c03e7ca ("regulator: mcp16502: add regulator driver for MCP16502")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/mcp16502.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
index 6d0ad74935b3..ab78f831f5bf 100644
--- a/drivers/regulator/mcp16502.c
+++ b/drivers/regulator/mcp16502.c
@@ -93,6 +93,7 @@ static unsigned int mcp16502_of_map_mode(unsigned int mode)
 		.owner			= THIS_MODULE,			\
 		.n_voltages		= MCP16502_VSEL + 1,		\
 		.linear_ranges		= _ranges,			\
+		.linear_min_sel		= VDD_LOW_SEL,			\
 		.n_linear_ranges	= ARRAY_SIZE(_ranges),		\
 		.of_match		= of_match_ptr(_name),		\
 		.of_map_mode		= mcp16502_of_map_mode,		\
-- 
2.7.4

[PATCH v3 4/6] regulator: mcp16502: adapt for get/set on other registers

[Claudiu Beznea]

MCP16502 have multiple registers for each regulator (as described
in enum mcp16502_reg). Adapt the code to be able to get/set all these
registers. This is necessary for the following commits.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/mcp16502.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
index ab78f831f5bf..48eb64bc4018 100644
--- a/drivers/regulator/mcp16502.c
+++ b/drivers/regulator/mcp16502.c
@@ -54,13 +54,9 @@
  * This function is useful for iterating over all regulators and accessing their
  * registers in a generic way or accessing a regulator device by its id.
  */
-#define MCP16502_BASE(i) (((i) + 1) << 4)
+#define MCP16502_REG_BASE(i, r) ((((i) + 1) << 4) + MCP16502_REG_##r)
 #define MCP16502_STAT_BASE(i) ((i) + 5)
 
-#define MCP16502_OFFSET_MODE_A 0
-#define MCP16502_OFFSET_MODE_LPM 1
-#define MCP16502_OFFSET_MODE_HIB 2
-
 #define MCP16502_OPMODE_ACTIVE REGULATOR_MODE_NORMAL
 #define MCP16502_OPMODE_LPM REGULATOR_MODE_IDLE
 #define MCP16502_OPMODE_HIB REGULATOR_MODE_STANDBY
@@ -75,6 +71,23 @@
 #define MCP16502_MIN_REG 0x0
 #define MCP16502_MAX_REG 0x65
 
+/**
+ * enum mcp16502_reg - MCP16502 regulators's registers
+ * @MCP16502_REG_A: active state register
+ * @MCP16502_REG_LPM: low power mode state register
+ * @MCP16502_REG_HIB: hibernate state register
+ * @MCP16502_REG_SEQ: startup sequence register
+ * @MCP16502_REG_CFG: configuration register
+ */
+enum mcp16502_reg {
+	MCP16502_REG_A,
+	MCP16502_REG_LPM,
+	MCP16502_REG_HIB,
+	MCP16502_REG_HPM,
+	MCP16502_REG_SEQ,
+	MCP16502_REG_CFG,
+};
+
 static unsigned int mcp16502_of_map_mode(unsigned int mode)
 {
 	if (mode == REGULATOR_MODE_NORMAL || mode == REGULATOR_MODE_IDLE)
@@ -144,22 +157,20 @@ static void mcp16502_gpio_set_mode(struct mcp16502 *mcp, int mode)
 }
 
 /*
- * mcp16502_get_reg() - get the PMIC's configuration register for opmode
+ * mcp16502_get_reg() - get the PMIC's state configuration register for opmode
  *
  * @rdev: the regulator whose register we are searching
  * @opmode: the PMIC's operating mode ACTIVE, Low-power, Hibernate
  */
-static int mcp16502_get_reg(struct regulator_dev *rdev, int opmode)
+static int mcp16502_get_state_reg(struct regulator_dev *rdev, int opmode)
 {
-	int reg = MCP16502_BASE(rdev_get_id(rdev));
-
 	switch (opmode) {
 	case MCP16502_OPMODE_ACTIVE:
-		return reg + MCP16502_OFFSET_MODE_A;
+		return MCP16502_REG_BASE(rdev_get_id(rdev), A);
 	case MCP16502_OPMODE_LPM:
-		return reg + MCP16502_OFFSET_MODE_LPM;
+		return MCP16502_REG_BASE(rdev_get_id(rdev), LPM);
 	case MCP16502_OPMODE_HIB:
-		return reg + MCP16502_OFFSET_MODE_HIB;
+		return MCP16502_REG_BASE(rdev_get_id(rdev), HIB);
 	default:
 		return -EINVAL;
 	}
@@ -179,7 +190,7 @@ static unsigned int mcp16502_get_mode(struct regulator_dev *rdev)
 	unsigned int val;
 	int ret, reg;
 
-	reg = mcp16502_get_reg(rdev, MCP16502_OPMODE_ACTIVE);
+	reg = mcp16502_get_state_reg(rdev, MCP16502_OPMODE_ACTIVE);
 	if (reg < 0)
 		return reg;
 
@@ -210,7 +221,7 @@ static int _mcp16502_set_mode(struct regulator_dev *rdev, unsigned int mode,
 	int val;
 	int reg;
 
-	reg = mcp16502_get_reg(rdev, op_mode);
+	reg = mcp16502_get_state_reg(rdev, op_mode);
 	if (reg < 0)
 		return reg;
 
@@ -269,10 +280,10 @@ static int mcp16502_suspend_get_target_reg(struct regulator_dev *rdev)
 {
 	switch (pm_suspend_target_state) {
 	case PM_SUSPEND_STANDBY:
-		return mcp16502_get_reg(rdev, MCP16502_OPMODE_LPM);
+		return mcp16502_get_state_reg(rdev, MCP16502_OPMODE_LPM);
 	case PM_SUSPEND_ON:
 	case PM_SUSPEND_MEM:
-		return mcp16502_get_reg(rdev, MCP16502_OPMODE_HIB);
+		return mcp16502_get_state_reg(rdev, MCP16502_OPMODE_HIB);
 	default:
 		dev_err(&rdev->dev, "invalid suspend target: %d\n",
 			pm_suspend_target_state);
-- 
2.7.4

[PATCH v3 5/6] regulator: mcp16502: add support for ramp delay

[Claudiu Beznea]

MCP16502 have configurable ramp delay support (via DVSR bits in
regulators' CFG register).

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/mcp16502.c | 89 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
index 48eb64bc4018..f81afeeddb19 100644
--- a/drivers/regulator/mcp16502.c
+++ b/drivers/regulator/mcp16502.c
@@ -22,8 +22,9 @@
 #define VDD_LOW_SEL 0x0D
 #define VDD_HIGH_SEL 0x3F
 
-#define MCP16502_FLT BIT(7)
-#define MCP16502_ENS BIT(0)
+#define MCP16502_FLT		BIT(7)
+#define MCP16502_DVSR		GENMASK(3, 2)
+#define MCP16502_ENS		BIT(0)
 
 /*
  * The PMIC has four sets of registers corresponding to four power modes:
@@ -88,6 +89,12 @@ enum mcp16502_reg {
 	MCP16502_REG_CFG,
 };
 
+/* Ramp delay (uV/us) for buck1, ldo1, ldo2. */
+static const int mcp16502_ramp_b1l12[] = { 6250, 3125, 2083, 1563 };
+
+/* Ramp delay (uV/us) for buck2, buck3, buck4. */
+static const int mcp16502_ramp_b234[] = { 3125, 1563, 1042, 781 };
+
 static unsigned int mcp16502_of_map_mode(unsigned int mode)
 {
 	if (mode == REGULATOR_MODE_NORMAL || mode == REGULATOR_MODE_IDLE)
@@ -271,6 +278,80 @@ static int mcp16502_get_status(struct regulator_dev *rdev)
 	return REGULATOR_STATUS_UNDEFINED;
 }
 
+static int mcp16502_set_voltage_time_sel(struct regulator_dev *rdev,
+					 unsigned int old_sel,
+					 unsigned int new_sel)
+{
+	static const u8 us_ramp[] = { 8, 16, 24, 32 };
+	int id = rdev_get_id(rdev);
+	unsigned int uV_delta, val;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, MCP16502_REG_BASE(id, CFG), &val);
+	if (ret)
+		return ret;
+
+	val = (val & MCP16502_DVSR) >> 2;
+	uV_delta = abs(new_sel * rdev->desc->linear_ranges->step -
+		       old_sel * rdev->desc->linear_ranges->step);
+	switch (id) {
+	case BUCK1:
+	case LDO1:
+	case LDO2:
+		ret = DIV_ROUND_CLOSEST(uV_delta * us_ramp[val],
+					mcp16502_ramp_b1l12[val]);
+		break;
+
+	case BUCK2:
+	case BUCK3:
+	case BUCK4:
+		ret = DIV_ROUND_CLOSEST(uV_delta * us_ramp[val],
+					mcp16502_ramp_b234[val]);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int mcp16502_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
+{
+	const int *ramp;
+	int id = rdev_get_id(rdev);
+	unsigned int i, size;
+
+	switch (id) {
+	case BUCK1:
+	case LDO1:
+	case LDO2:
+		ramp = mcp16502_ramp_b1l12;
+		size = ARRAY_SIZE(mcp16502_ramp_b1l12);
+		break;
+
+	case BUCK2:
+	case BUCK3:
+	case BUCK4:
+		ramp = mcp16502_ramp_b234;
+		size = ARRAY_SIZE(mcp16502_ramp_b234);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	for (i = 0; i < size; i++) {
+		if (ramp[i] == ramp_delay)
+			break;
+	}
+	if (i == size)
+		return -EINVAL;
+
+	return regmap_update_bits(rdev->regmap, MCP16502_REG_BASE(id, CFG),
+				  MCP16502_DVSR, (i << 2));
+}
+
 #ifdef CONFIG_SUSPEND
 /*
  * mcp16502_suspend_get_target_reg() - get the reg of the target suspend PMIC
@@ -365,6 +446,8 @@ static const struct regulator_ops mcp16502_buck_ops = {
 	.disable			= regulator_disable_regmap,
 	.is_enabled			= regulator_is_enabled_regmap,
 	.get_status			= mcp16502_get_status,
+	.set_voltage_time_sel		= mcp16502_set_voltage_time_sel,
+	.set_ramp_delay			= mcp16502_set_ramp_delay,
 
 	.set_mode			= mcp16502_set_mode,
 	.get_mode			= mcp16502_get_mode,
@@ -389,6 +472,8 @@ static const struct regulator_ops mcp16502_ldo_ops = {
 	.disable			= regulator_disable_regmap,
 	.is_enabled			= regulator_is_enabled_regmap,
 	.get_status			= mcp16502_get_status,
+	.set_voltage_time_sel		= mcp16502_set_voltage_time_sel,
+	.set_ramp_delay			= mcp16502_set_ramp_delay,
 
 #ifdef CONFIG_SUSPEND
 	.set_suspend_voltage		= mcp16502_set_suspend_voltage,
-- 
2.7.4

[PATCH v3 6/6] regulator: mcp16502: remove void documentation of struct mcp16502

[Claudiu Beznea]

struct mcp16502 has no members called rdev or rmap. Remove the
documentation.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/mcp16502.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/regulator/mcp16502.c b/drivers/regulator/mcp16502.c
index f81afeeddb19..74ad92dc664a 100644
--- a/drivers/regulator/mcp16502.c
+++ b/drivers/regulator/mcp16502.c
@@ -135,8 +135,6 @@ enum {
 
 /*
  * struct mcp16502 - PMIC representation
- * @rdev: the regulators belonging to this chip
- * @rmap: regmap to be used for I2C communication
  * @lpm: LPM GPIO descriptor
  */
 struct mcp16502 {
-- 
2.7.4

Re: [PATCH v3 2/6] regulator: core: do not continue if selector match

[Mark Brown]

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

On Fri, Nov 13, 2020 at 05:21:06PM +0200, Claudiu Beznea wrote:
> Do not continue if selector has already been located.

This doesn't apply against current code, please check and resend.

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

Re: [PATCH v3 0/6] regulator: mcp16502: add support for ramp delay

[Mark Brown]

On Fri, 13 Nov 2020 17:21:04 +0200, Claudiu Beznea wrote:
> This series adds support for ramp delay on mcp16502. It also adds
> some cleanup on mcp16502.
> 
> Apart from that patches 1/6 fixes the selector validation in case
> the regulator::desc::linear_min_sel is not zero.
> 
> Thank you,
> Claudiu Beznea
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/6] regulator: core: validate selector against linear_min_sel
      commit: bdcd1177578cd5556f7494da86d5038db8203a16
[2/6] regulator: core: do not continue if selector match
      (no commit info)
[3/6] regulator: mcp16502: add linear_min_sel
      commit: 478f8089161e9a8f487ef3f560e59d1423b81c05
[4/6] regulator: mcp16502: adapt for get/set on other registers
      commit: 3e5532a011b09861abc2da3aa518b9aafc250570
[5/6] regulator: mcp16502: add support for ramp delay
      commit: 322eb8666d2f50556e89d73b54cf2dad8703c4e0
[6/6] regulator: mcp16502: remove void documentation of struct mcp16502
      commit: 842f44806efaddfae5ecff8f143c2607a4fa65d7

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Jon Hunter]

On 13/11/2020 15:21, Claudiu Beznea wrote:
> There are regulators who's min selector is not zero. Selectors loops
> (looping b/w zero and regulator::desc::n_voltages) might throw errors
> because invalid selectors are used (lower than
> regulator::desc::linear_min_sel). For this situations validate selectors
> against regulator::desc::linear_min_sel.


After this commit was merged, I noticed a regression in the DFLL (CPU
clock source) on Tegra124. The DFLL driver
(drivers/clk/tegra/clk-dfll.c) calls regulator_list_voltage() in a loop
to determine the selector for a given voltage (see function
find_vdd_map_entry_exact()).

Currently, the DFLL driver queries the number of voltages provided by
the regulator by calling regulator_count_voltages() and then starting
from 0, iterates through the number of voltages to find the selector
value for the voltage it is looking for by calling
regulator_list_voltage(). It assumes that any negative value returned by
calling regulator_list_voltage() is an error and this will cause the
loop up to terminate.

In this case the regulator in question is the as3722 and the
linear_min_sel for this regulator is 1 and so when the DFLL driver calls
regulator_list_voltage() with a selector value of 0 it now returns a
negative error code, as expected by this change, and this terminates the
loop up in the DFLL driver. So I can clearly see why this is happening
and I could fix up the DFLL driver to avoid this.

Before doing so, I wanted to ask if that is the correct fix here,
because it seems a bit odd that regulator_count_voltages() returns N
voltages, but if the min selector value is greater than 0, then actually
there are less than N. However, changing the number of voltages
supported by the regulator to be N - linear_min_sel does not make sense
either because then we need to know the linear_min_sel in order to
determine the first valid voltage.

In case of the as3722, the value 0 means that the regulator is powered
down. So it is a valid setting and equates to 0 volts at the output AFAICT.

Please let me know your thoughts are the correct way to fix this up.

Thanks
Jon

-- 
nvpublic

Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Claudiu.Beznea]

Hi Jon,

On 24.11.2020 11:36, Jon Hunter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 13/11/2020 15:21, Claudiu Beznea wrote:
>> There are regulators who's min selector is not zero. Selectors loops
>> (looping b/w zero and regulator::desc::n_voltages) might throw errors
>> because invalid selectors are used (lower than
>> regulator::desc::linear_min_sel). For this situations validate selectors
>> against regulator::desc::linear_min_sel.
> 
> 
> After this commit was merged, I noticed a regression in the DFLL (CPU
> clock source) on Tegra124. The DFLL driver
> (drivers/clk/tegra/clk-dfll.c) calls regulator_list_voltage() in a loop
> to determine the selector for a given voltage (see function
> find_vdd_map_entry_exact()).
> 
> Currently, the DFLL driver queries the number of voltages provided by
> the regulator by calling regulator_count_voltages() and then starting
> from 0, iterates through the number of voltages to find the selector
> value for the voltage it is looking for by calling
> regulator_list_voltage(). It assumes that any negative value returned by
> calling regulator_list_voltage() is an error and this will cause the
> loop up to terminate.
> 
> In this case the regulator in question is the as3722 and the
> linear_min_sel for this regulator is 1 and so when the DFLL driver calls
> regulator_list_voltage() with a selector value of 0 it now returns a
> negative error code, as expected by this change, and this terminates the
> loop up in the DFLL driver. So I can clearly see why this is happening
> and I could fix up the DFLL driver to avoid this.
> 
> Before doing so, I wanted to ask if that is the correct fix here,
> because it seems a bit odd that regulator_count_voltages() returns N
> voltages, but if the min selector value is greater than 0, then actually
> there are less than N. However, changing the number of voltages
> supported by the regulator to be N - linear_min_sel does not make sense
> either because then we need to know the linear_min_sel in order to
> determine the first valid voltage.
> 
> In case of the as3722, the value 0 means that the regulator is powered
> down. So it is a valid setting and equates to 0 volts at the output AFAICT.
> 
> Please let me know your thoughts are the correct way to fix this up.

I would say that a solution would be to have a new helper to retrieve the
linear_min_sel (e.g. regulator_min_sel()) and use this for all the
consumers of regulator_list_voltage() and the other APIs that patch
"regulator: core: validate selector against linear_min_sel" has changed
(regulator_list_voltage_table(), regulator_set_voltage_time()). With this
change the loop in find_vdd_map_entry_exact() should be b/w
regulator_min_sel() and regulator_count_voltages().

Maybe Mark has a better solution for this.

Thank you,
Claudiu

> 
> Thanks
> Jon
> 
> --
> nvpublic
> 

Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Jon Hunter]

On 24/11/2020 11:14, Claudiu.Beznea@microchip.com wrote:

...

> I would say that a solution would be to have a new helper to retrieve the
> linear_min_sel (e.g. regulator_min_sel()) and use this for all the
> consumers of regulator_list_voltage() and the other APIs that patch
> "regulator: core: validate selector against linear_min_sel" has changed
> (regulator_list_voltage_table(), regulator_set_voltage_time()). With this
> change the loop in find_vdd_map_entry_exact() should be b/w
> regulator_min_sel() and regulator_count_voltages().
> 
> Maybe Mark has a better solution for this.


By the way, I don't think that Tegra is alone here. I see some other
drivers doing some similar things [0][1][2] and so I am wondering if
this is going to be a problem for a few drivers.

Jon

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/core/regulator.c#n61
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/s3c2416-cpufreq.c#n263
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-regulator.c#n29

-- 
nvpublic

Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Mark Brown]

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

On Tue, Nov 24, 2020 at 11:14:54AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 24.11.2020 11:36, Jon Hunter wrote:

> > Before doing so, I wanted to ask if that is the correct fix here,
> > because it seems a bit odd that regulator_count_voltages() returns N
> > voltages, but if the min selector value is greater than 0, then actually
> > there are less than N. However, changing the number of voltages
> > supported by the regulator to be N - linear_min_sel does not make sense
> > either because then we need to know the linear_min_sel in order to
> > determine the first valid voltage.

> I would say that a solution would be to have a new helper to retrieve the
> linear_min_sel (e.g. regulator_min_sel()) and use this for all the
> consumers of regulator_list_voltage() and the other APIs that patch
> "regulator: core: validate selector against linear_min_sel" has changed
> (regulator_list_voltage_table(), regulator_set_voltage_time()). With this
> change the loop in find_vdd_map_entry_exact() should be b/w
> regulator_min_sel() and regulator_count_voltages().

We need an incremental fix to return 0 rather than an error for things
below the minimum selector, it's not invalid for there to be holes in
the range of selectors and this is just an example of that.  Consumers
need to be able to cope with skipping over values that can't be mapped.

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

Re: [PATCH v3 1/6] regulator: core: validate selector against linear_min_sel

[Claudiu.Beznea]

On 24.11.2020 15:41, Jon Hunter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 24/11/2020 11:14, Claudiu.Beznea@microchip.com wrote:
> 
> ...
> 
>> I would say that a solution would be to have a new helper to retrieve the
>> linear_min_sel (e.g. regulator_min_sel()) and use this for all the
>> consumers of regulator_list_voltage() and the other APIs that patch
>> "regulator: core: validate selector against linear_min_sel" has changed
>> (regulator_list_voltage_table(), regulator_set_voltage_time()). With this
>> change the loop in find_vdd_map_entry_exact() should be b/w
>> regulator_min_sel() and regulator_count_voltages().
>>
>> Maybe Mark has a better solution for this.
> 
> 
> By the way, I don't think that Tegra is alone here. I see some other
> drivers doing some similar things [0][1][2] and so I am wondering if
> this is going to be a problem for a few drivers.
> 

As far as I can tell most of the regulator_list_voltage() consumers are
checking the return value against a [min_uV, max_uV] range or if the return
value is a negative error code or zero. The consumers are looking a
selector that respect the above rule for the entire [0,
regulator_count_voltages()] range (I have to double check for the rest of
functions modified by my patch). In case of clk-dfll.c the
find_vdd_map_entry_exact() returns if it finds the 1st invalid selector:

	n_voltages = regulator_count_voltages(td->vdd_reg);
	for (i = 0; i < n_voltages; i++) {
		reg_uV = regulator_list_voltage(td->vdd_reg, i);
		if (reg_uV < 0)
			break;

		reg_volt_id = reg_uV / td->soc->alignment.step_uv;

		if (align_step == reg_volt_id)
			return i;
	}

Maybe it would be better if the loop continues in case reg_uV is negative
or zero? (the zero case is good for this function as it will make the
(align_step == reg_volt_id) to be false). But as Mark said in the previous
email, there could be regulators with gaps in between min_sel and n_voltages.

With the previous code it seems it worked because the
regulator_list_voltage() calls the as3722's list_voltage which is
regulator_list_voltage_linear() which checks the selector against
min_selector and returns zero in case the selector is lower than the
min_selector. Please correct me if I'm wrong.

Anyway, I will prepare a fix for my previous patch to return zero in case
the regulator_count_voltages() receives an invalid selector. That should
also fix the case with this driver.

Thank you,
Claudiu

> Jon
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/core/regulator.c#n61
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/s3c2416-cpufreq.c#n263
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-regulator.c#n29
> 
> --
> nvpublic
> 

[PATCH] regulator: core: return zero for selectors lower than linear_min_sel

[Claudiu Beznea]

Selectors lower than linear_min_sel should not be considered invalid.
Thus return zero in case _regulator_list_voltage(),
regulator_list_hardware_vsel() or regulator_list_voltage_table()
receives such selectors as argument.

Fixes: bdcd1177578c ("regulator: core: validate selector against linear_min_sel")
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/core.c    | 10 ++++++----
 drivers/regulator/helpers.c |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 216b0cbce199..ca03d8e70bd1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2956,9 +2956,10 @@ static int _regulator_list_voltage(struct regulator_dev *rdev,
 		return rdev->desc->fixed_uV;
 
 	if (ops->list_voltage) {
-		if (selector >= rdev->desc->n_voltages ||
-		    selector < rdev->desc->linear_min_sel)
+		if (selector >= rdev->desc->n_voltages)
 			return -EINVAL;
+		if (selector < rdev->desc->linear_min_sel)
+			return 0;
 		if (lock)
 			regulator_lock(rdev);
 		ret = ops->list_voltage(rdev, selector);
@@ -3108,9 +3109,10 @@ int regulator_list_hardware_vsel(struct regulator *regulator,
 	struct regulator_dev *rdev = regulator->rdev;
 	const struct regulator_ops *ops = rdev->desc->ops;
 
-	if (selector >= rdev->desc->n_voltages ||
-	    selector < rdev->desc->linear_min_sel)
+	if (selector >= rdev->desc->n_voltages)
 		return -EINVAL;
+	if (selector < rdev->desc->linear_min_sel)
+		return 0;
 	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
 		return -EOPNOTSUPP;
 
diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index 974f1a63993d..f42b394a0c46 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -647,9 +647,10 @@ int regulator_list_voltage_table(struct regulator_dev *rdev,
 		return -EINVAL;
 	}
 
-	if (selector >= rdev->desc->n_voltages ||
-	    selector < rdev->desc->linear_min_sel)
+	if (selector >= rdev->desc->n_voltages)
 		return -EINVAL;
+	if (selector < rdev->desc->linear_min_sel)
+		return 0;
 
 	return rdev->desc->volt_table[selector];
 }
-- 
2.7.4

Re: [PATCH] regulator: core: return zero for selectors lower than linear_min_sel

[Mark Brown]

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

On Wed, Nov 25, 2020 at 01:34:03PM +0200, Claudiu Beznea wrote:
> Selectors lower than linear_min_sel should not be considered invalid.
> Thus return zero in case _regulator_list_voltage(),
> regulator_list_hardware_vsel() or regulator_list_voltage_table()
> receives such selectors as argument.

Please don't send new patches in reply to old threads, it buries things,
makes trying to figure out what current versions are harder and breaks
tooling.

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

Re: [PATCH v3 0/6] regulator: mcp16502: add support for ramp delay

[Mark Brown]

On Fri, 13 Nov 2020 17:21:04 +0200, Claudiu Beznea wrote:
> This series adds support for ramp delay on mcp16502. It also adds
> some cleanup on mcp16502.
> 
> Apart from that patches 1/6 fixes the selector validation in case
> the regulator::desc::linear_min_sel is not zero.
> 
> Thank you,
> Claudiu Beznea
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/6] regulator: core: validate selector against linear_min_sel
      (no commit info)
[2/6] regulator: core: do not continue if selector match
      (no commit info)
[3/6] regulator: mcp16502: add linear_min_sel
      (no commit info)
[4/6] regulator: mcp16502: adapt for get/set on other registers
      (no commit info)
[5/6] regulator: mcp16502: add support for ramp delay
      (no commit info)
[6/6] regulator: mcp16502: remove void documentation of struct mcp16502
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark