Some PMICs treat the vsel_reg same as apply-bit. Eg, when voltage range
is changed, the new voltage setting is not taking effect until the vsel
register is written.

Force the vsel value to be written to hardware even if the old selector
was same as the new (but range changed) by using regmap_write_bits() and
not the regmap_update_bits().

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
I have a feeling this same thing should be done with the apply-bit
writes. I just wonder why no-one has reported problems with it. Maybe
all the hardware with the apply bit has 'self clearing' apply register
marked as volatile? Anyways, I'll leave it out of this patch because I
have no hardware to test the apply-bit stuff, and as I haven't seen any
problem reports about it.
---
 drivers/regulator/helpers.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index d49268336553..28c2f40b19cc 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -211,8 +211,14 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
 		if (ret)
 			return ret;
 
-		ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
-				  rdev->desc->vsel_mask, sel);
+		/*
+		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
+		 * to be written even if the old selector were same as the new
+		 * (but range changed) by using regmap_write_bits() and not the
+		 * regmap_update_bits().
+		 */
+		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
+					rdev->desc->vsel_mask, sel);
 	}
 
 	if (ret)
-- 
2.44.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

On Thu, May 16, 2024 at 11:53:46AM +0300, Matti Vaittinen wrote:

> +		/*
> +		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
> +		 * to be written even if the old selector were same as the new
> +		 * (but range changed) by using regmap_write_bits() and not the
> +		 * regmap_update_bits().
> +		 */
> +		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
> +					rdev->desc->vsel_mask, sel);

This feels like a special case for those devices, for devices that don't
have this behaviour it'd be a regression in that it'd increase I/O
traffic.  At least this should be specialised to only devices that have
ranges I think?

Hi Mark,

Added some driver folks to CC. It'd be great to lean how other devices 
avoid having intermediate voltages if range and vsel write is not atomic.

On 5/16/24 14:33, Mark Brown wrote:
> On Thu, May 16, 2024 at 11:53:46AM +0300, Matti Vaittinen wrote:
> 
>> +		/*
>> +		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
>> +		 * to be written even if the old selector were same as the new
>> +		 * (but range changed) by using regmap_write_bits() and not the
>> +		 * regmap_update_bits().
>> +		 */
>> +		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
>> +					rdev->desc->vsel_mask, sel);
> 
> This feels like a special case for those devices, for devices that don't
> have this behaviour it'd be a regression in that it'd increase I/O
> traffic.

I thought most of devices which have pickable voltage ranges and a 
separate register for range and voltage selector would need some 
mechanism to prevent 'intermediate voltages' as writing the vsel and 
range won't be atomic.

Also, the "pickable ranges" is not that widely used, and many of the 
users have the range and vsel selectors in same register (and if they 
don't, they're likely to see unwanted side effects due to non atomic write).

I did actually go through the drivers. These seem to be clear:

atc260x-regulator.c: vsel and range in same register
max77650-regulator.c: vsel and range in same register
tps6287x-regulator.c: According to a data-sheet, this requires a vsel 
write for range change to take effect (just like the device I am working 
with), so the current implementation may be suffering from the bug I am 
trying to solve.

Anyways, following might be unnecessarily impacted:

max77541-regulator.c - but, I briefly looked at the data-sheet, and I am 
not sure the driver operates according to the spec. The spec I found 
stated the range bits must not be changed when the output is enabled.

mt6358-regulator.c: I didn't find proper spec details.

>  At least this should be specialised to only devices that have
> ranges I think?

This operation is the regulator_set_voltage_sel_pickable_regmap() - 
which should be only used for devices which use pickable ranges. 
Furthermore, it's inside the else branch for if (rdev->desc->vsel_reg == 
rdev->desc->vsel_range_reg) meaning the range selection bit is in 
separate register from the voltage selection.

Anyways, thanks for the heads up. I really didn't know we had other 
users which would be affected. I'll consider adding a flag to the 
regulator desc for deciding if the vsel should always be written when 
range changes.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~