mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
       [not found] <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.44efb263-5265-4fae-9cff-e8eaefbc0afd@emailsignatures365.codetwo.com>
@ 2026-11-06 12:59 ` Mike Looijmans
       [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.6954786b-65e0-4206-a0c4-60cd251dbec9@emailsignatures365.codetwo.com>
                     ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Mike Looijmans @ 2026-11-06 12:59 UTC (permalink / raw)
  To: linux-kernel; +Cc: Mike Looijmans, Liam Girdwood, Mark Brown

This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
output voltage for disabled PWMs")

A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
this could also mean that its output pin is tristated for example.

If the PWM range is limited to something else than 0..100% this causes the
probe function to fail with messages like:
 dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
 pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
 pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131

The reason is that regulator_register() calls get_voltage(). The PWM is
disabled at this point, and then the validity check triggers.

Anything that calls get_voltage() while the PWM is disabled would get an
-ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

 drivers/regulator/pwm-regulator.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 7434b6b22d325..f50530c77d92f 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -157,13 +157,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
 
 	pwm_get_state(drvdata->pwm, &pstate);
 
-	if (!pstate.enabled) {
-		if (pstate.polarity == PWM_POLARITY_INVERSED)
-			pstate.duty_cycle = pstate.period;
-		else
-			pstate.duty_cycle = 0;
-	}
-
 	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
 	if (voltage < min(max_uV_duty, min_uV_duty) ||
 	    voltage > max(max_uV_duty, min_uV_duty))
---
base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
branch: linux-master-pwm-regulator

-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
       [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.6954786b-65e0-4206-a0c4-60cd251dbec9@emailsignatures365.codetwo.com>
@ 2026-11-06 12:59     ` Mike Looijmans
  2026-11-06 13:37       ` Mark Brown
  2026-11-08 15:13       ` Mark Brown
  0 siblings, 2 replies; 14+ messages in thread
From: Mike Looijmans @ 2026-11-06 12:59 UTC (permalink / raw)
  To: linux-kernel; +Cc: Mike Looijmans, Liam Girdwood, Mark Brown

This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
checks in continuous .get_voltage").

The core, and in particular the machine_constraints_voltage() method,
expects the regulator to report what the current setting is, even if that
is outside the allowed range. The core will then bring the voltage into
that range. It cannot do that if the regulator refuses to return its
setting, and causes probe() to fail for a pwm-regulator when it has
restrictions to the PWM range with messages like:
  dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
  pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
    regulator pwm-regulator
  pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
    with error -131

Besides that, regulator_get_voltage() is not supposed to return any
state information, as is mentioned in the API documentation.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---

 drivers/regulator/pwm-regulator.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index f50530c77d92f..00fcf9a172b47 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -158,9 +158,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
 	pwm_get_state(drvdata->pwm, &pstate);
 
 	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
-	if (voltage < min(max_uV_duty, min_uV_duty) ||
-	    voltage > max(max_uV_duty, min_uV_duty))
-		return -ENOTRECOVERABLE;
 
 	/*
 	 * The dutycycle for min_uV might be greater than the one for max_uV.
---
base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
branch: linux-master-pwm-regulator

-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-06 12:59 ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mike Looijmans
       [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.6954786b-65e0-4206-a0c4-60cd251dbec9@emailsignatures365.codetwo.com>
@ 2026-11-06 13:36   ` Mark Brown
  2026-11-06 13:56     ` Mike Looijmans
  2026-11-06 21:57     ` Martin Blumenstingl
  2026-11-10  9:33   ` Uwe Kleine-König
  2 siblings, 2 replies; 14+ messages in thread
From: Mark Brown @ 2026-11-06 13:36 UTC (permalink / raw)
  To: Mike Looijmans, Martin Blumenstingl; +Cc: linux-kernel, Liam Girdwood

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

On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
> output voltage for disabled PWMs")

Adding Martin who wrote that patch - please CC people if you're directly
updating their patches.

> 
> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
> this could also mean that its output pin is tristated for example.
> 
> If the PWM range is limited to something else than 0..100% this causes the
> probe function to fail with messages like:
>  dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>  pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
>  pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
> 
> The reason is that regulator_register() calls get_voltage(). The PWM is
> disabled at this point, and then the validity check triggers.
> 
> Anything that calls get_voltage() while the PWM is disabled would get an
> -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  drivers/regulator/pwm-regulator.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 7434b6b22d325..f50530c77d92f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -157,13 +157,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
> -	if (!pstate.enabled) {
> -		if (pstate.polarity == PWM_POLARITY_INVERSED)
> -			pstate.duty_cycle = pstate.period;
> -		else
> -			pstate.duty_cycle = 0;
> -	}
> -
>  	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
>  	if (voltage < min(max_uV_duty, min_uV_duty) ||
>  	    voltage > max(max_uV_duty, min_uV_duty))
> ---
> base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
> branch: linux-master-pwm-regulator
> 
> -- 
> 2.43.0
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topic.nl
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-06 12:59     ` [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage" Mike Looijmans
@ 2026-11-06 13:37       ` Mark Brown
  2026-11-06 21:45         ` Martin Blumenstingl
  2026-11-08 15:13       ` Mark Brown
  1 sibling, 1 reply; 14+ messages in thread
From: Mark Brown @ 2026-11-06 13:37 UTC (permalink / raw)
  To: Mike Looijmans, Martin Blumenstingl; +Cc: linux-kernel, Liam Girdwood

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

On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:
> This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
> checks in continuous .get_voltage").

And again...  please don't try to sneak changes past people :/

> The core, and in particular the machine_constraints_voltage() method,
> expects the regulator to report what the current setting is, even if that
> is outside the allowed range. The core will then bring the voltage into
> that range. It cannot do that if the regulator refuses to return its
> setting, and causes probe() to fail for a pwm-regulator when it has
> restrictions to the PWM range with messages like:
>   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
>     regulator pwm-regulator
>   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
>     with error -131
> 
> Besides that, regulator_get_voltage() is not supposed to return any
> state information, as is mentioned in the API documentation.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  drivers/regulator/pwm-regulator.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index f50530c77d92f..00fcf9a172b47 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -158,9 +158,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
>  	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> -	if (voltage < min(max_uV_duty, min_uV_duty) ||
> -	    voltage > max(max_uV_duty, min_uV_duty))
> -		return -ENOTRECOVERABLE;
>  
>  	/*
>  	 * The dutycycle for min_uV might be greater than the one for max_uV.
> ---
> base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
> branch: linux-master-pwm-regulator
> 
> -- 
> 2.43.0
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topic.nl
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-06 13:36   ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mark Brown
@ 2026-11-06 13:56     ` Mike Looijmans
  2026-11-06 21:57     ` Martin Blumenstingl
  1 sibling, 0 replies; 14+ messages in thread
From: Mike Looijmans @ 2026-11-06 13:56 UTC (permalink / raw)
  To: Mark Brown, Martin Blumenstingl; +Cc: linux-kernel, Liam Girdwood

On 06-11-2026 14:36, Mark Brown wrote:
> On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
>> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
>> output voltage for disabled PWMs")
> Adding Martin who wrote that patch - please CC people if you're directly
> updating their patches.

Sorry, that wasn't intentional. I was just used to get_maintainer.pl and 
patman "doing the right thing" that I didn't consider checking that. I 
promise to do better next time.


>
>> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
>> this could also mean that its output pin is tristated for example.
>>
>> If the PWM range is limited to something else than 0..100% this causes the
>> probe function to fail with messages like:
>>   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>>   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
>>   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
>>
>> The reason is that regulator_register() calls get_voltage(). The PWM is
>> disabled at this point, and then the validity check triggers.
>>
>> Anything that calls get_voltage() while the PWM is disabled would get an
>> -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>
>>   drivers/regulator/pwm-regulator.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
>> index 7434b6b22d325..f50530c77d92f 100644
>> --- a/drivers/regulator/pwm-regulator.c
>> +++ b/drivers/regulator/pwm-regulator.c
>> @@ -157,13 +157,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>>   
>>   	pwm_get_state(drvdata->pwm, &pstate);
>>   
>> -	if (!pstate.enabled) {
>> -		if (pstate.polarity == PWM_POLARITY_INVERSED)
>> -			pstate.duty_cycle = pstate.period;
>> -		else
>> -			pstate.duty_cycle = 0;
>> -	}
>> -
>>   	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
>>   	if (voltage < min(max_uV_duty, min_uV_duty) ||
>>   	    voltage > max(max_uV_duty, min_uV_duty))
>> ---
>> base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
>> branch: linux-master-pwm-regulator
>>
>> -- 
>> 2.43.0
>>
>>

-- 
Mike Looijmans




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-06 13:37       ` Mark Brown
@ 2026-11-06 21:45         ` Martin Blumenstingl
  2026-11-07 12:25           ` Mark Brown
  2026-11-09  6:34           ` Mike Looijmans
  0 siblings, 2 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2026-11-06 21:45 UTC (permalink / raw)
  To: Mark Brown; +Cc: Mike Looijmans, linux-kernel, Liam Girdwood

On Fri, Nov 6, 2026 at 2:46 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:
> > This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
> > checks in continuous .get_voltage").
>
> And again...  please don't try to sneak changes past people :/
Thank you for looping me in, Mark!

> > The core, and in particular the machine_constraints_voltage() method,
> > expects the regulator to report what the current setting is, even if that
> > is outside the allowed range. The core will then bring the voltage into
> > that range. It cannot do that if the regulator refuses to return its
> > setting, and causes probe() to fail for a pwm-regulator when it has
> > restrictions to the PWM range with messages like:
> >   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
> >   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
> >     regulator pwm-regulator
> >   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
> >     with error -131
> >
> > Besides that, regulator_get_voltage() is not supposed to return any
> > state information, as is mentioned in the API documentation.
according to include/linux/regulator/driver.h:
 * @get_voltage: Return the currently configured voltage for the regulator;
 *                   return -ENOTRECOVERABLE if regulator can't be read at
 *                   bootup and hasn't been set yet.

Can you please specify which API documentation you are referring to?

I believe back when this change was authored it didn't make a
difference on my board (but made sense based on review feedback).
So I'm not against reverting as long as we have a proper explanation
why the original patch is incorrect.


Best regards,
Martin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-06 13:36   ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mark Brown
  2026-11-06 13:56     ` Mike Looijmans
@ 2026-11-06 21:57     ` Martin Blumenstingl
  2026-11-09  6:09       ` Mike Looijmans
  1 sibling, 1 reply; 14+ messages in thread
From: Martin Blumenstingl @ 2026-11-06 21:57 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-kernel, Liam Girdwood, Mark Brown

On Fri, Nov 6, 2026 at 2:42 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
> > This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
> > output voltage for disabled PWMs")
>
> Adding Martin who wrote that patch - please CC people if you're directly
> updating their patches.
IIRC get_maintainers.pl includes the author of the patch blamed by the
Fixes: tag (if there's one).

> >
> > A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
> > this could also mean that its output pin is tristated for example.
How do we know if it's 0% or tri-stated (and what's the behavior if
it's tri-stated)?

> > If the PWM range is limited to something else than 0..100% this causes the
> > probe function to fail with messages like:
> >  dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
> >  pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
> >  pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
What board are you testing with? I'd like to see the .dts to get a
better understanding of where your board differs from mine.
I think Uwe (PWM maintainer) also previously mentioned that there's no
defined behavior for disabling a PWM and then reading it's state
again. I'll see if I can find his explanation and whether I can come
up with a patch that may help here.

> > The reason is that regulator_register() calls get_voltage(). The PWM is
> > disabled at this point, and then the validity check triggers.
> >
> > Anything that calls get_voltage() while the PWM is disabled would get an
> > -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
Why is the revert from your patch 2/2 not enough (it gets rid of
-ENOTRECOVERABLE)?
The problem with this specific revert (your patch 1/2) is that (from
memory) it will break these boards:
- arch/arm/boot/dts/amlogic/meson8b-ec100.dts
- arch/arm/boot/dts/amlogic/meson8b-mxq.dts
- arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
- arch/arm/boot/dts/amlogic/meson8m2-wetek-core.dts


Best regards,
Martin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-06 21:45         ` Martin Blumenstingl
@ 2026-11-07 12:25           ` Mark Brown
  2026-11-09  6:34           ` Mike Looijmans
  1 sibling, 0 replies; 14+ messages in thread
From: Mark Brown @ 2026-11-07 12:25 UTC (permalink / raw)
  To: Martin Blumenstingl; +Cc: Mike Looijmans, linux-kernel, Liam Girdwood

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

On Fri, Nov 06, 2026 at 10:45:31PM +0100, Martin Blumenstingl wrote:
> On Fri, Nov 6, 2026 at 2:46 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:

> > > The core, and in particular the machine_constraints_voltage() method,
> > > expects the regulator to report what the current setting is, even if that
> > > is outside the allowed range. The core will then bring the voltage into
> > > that range. It cannot do that if the regulator refuses to return its
> > > setting, and causes probe() to fail for a pwm-regulator when it has
> > > restrictions to the PWM range with messages like:

...

> > > Besides that, regulator_get_voltage() is not supposed to return any
> > > state information, as is mentioned in the API documentation.
> according to include/linux/regulator/driver.h:

>  * @get_voltage: Return the currently configured voltage for the regulator;
>  *                   return -ENOTRECOVERABLE if regulator can't be read at
>  *                   bootup and hasn't been set yet.

> Can you please specify which API documentation you are referring to?

> I believe back when this change was authored it didn't make a
> difference on my board (but made sense based on review feedback).
> So I'm not against reverting as long as we have a proper explanation
> why the original patch is incorrect.

The PWM driver is a bit weird here since everything is very software
defined.  If the regulator is disabled and the state that the software
will apply on enable is known then it should return a value, if we have
no idea what the hardware will do on enable then we can't safely say
what the value that will be provided is.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-06 12:59     ` [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage" Mike Looijmans
  2026-11-06 13:37       ` Mark Brown
@ 2026-11-08 15:13       ` Mark Brown
  2026-11-09  6:22         ` Mike Looijmans
  1 sibling, 1 reply; 14+ messages in thread
From: Mark Brown @ 2026-11-08 15:13 UTC (permalink / raw)
  To: Mike Looijmans; +Cc: linux-kernel, Liam Girdwood, Martin Blumenstingl

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

On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:
> This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
> checks in continuous .get_voltage").

> The core, and in particular the machine_constraints_voltage() method,
> expects the regulator to report what the current setting is, even if that
> is outside the allowed range. The core will then bring the voltage into
> that range. It cannot do that if the regulator refuses to return its
> setting, and causes probe() to fail for a pwm-regulator when it has
> restrictions to the PWM range with messages like:
>   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
>     regulator pwm-regulator
>   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
>     with error -131

Are you sure?  We have specific handling for -ENOTRECOVERABLE which just
blindly writes in the range specified in the constraints if the current
voltage is unreadable.  If that mechanism isn't working we should fix it
rather than changing drivers.

> Besides that, regulator_get_voltage() is not supposed to return any
> state information, as is mentioned in the API documentation.

>  	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> -	if (voltage < min(max_uV_duty, min_uV_duty) ||
> -	    voltage > max(max_uV_duty, min_uV_duty))
> -		return -ENOTRECOVERABLE;

The rest of the function relies on this to avoid having to deal with
wrapping when things are out of range.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-06 21:57     ` Martin Blumenstingl
@ 2026-11-09  6:09       ` Mike Looijmans
  2026-11-09 21:30         ` Martin Blumenstingl
  0 siblings, 1 reply; 14+ messages in thread
From: Mike Looijmans @ 2026-11-09  6:09 UTC (permalink / raw)
  To: Martin Blumenstingl; +Cc: linux-kernel, Liam Girdwood, Mark Brown

On 06-11-2026 22:57, Martin Blumenstingl wrote:
> On Fri, Nov 6, 2026 at 2:42 PM Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
>>> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
>>> output voltage for disabled PWMs")
>> Adding Martin who wrote that patch - please CC people if you're directly
>> updating their patches.
> IIRC get_maintainers.pl includes the author of the patch blamed by the
> Fixes: tag (if there's one).
>
>>> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
>>> this could also mean that its output pin is tristated for example.
> How do we know if it's 0% or tri-stated (and what's the behavior if
> it's tri-stated)?

We don't know, that's the point I'm trying to make. If the PWM is 
disabled, likely its clock is also turned off, and the actual state of 
the output is not known. Depending on hardware, it could be high, low, 
tristated, pulled up/down, ...

What may happen now is:

- The PWM is set at 50% and disabled.

- This function returns "0%" as being the current level during probe of 
the supply.

- The core wants it to be at 0%, so it doesn't change the setting and 
enables the PWM.

- The PWM now runs at 50% and the core thinks it's at 0%.


What happens in my case:

- This function returns 0% because the PWM is still disabled

- Core wants it to be 50%, so it calls set_voltage() which sets the PWM 
but doesn't enable it yet

- Core calls this function again and (still) gets 0% as being the setting.

- This goes on until the supply (and hence the PWM) is enabled.


What the callers expect is that this function returns the value that 
would be set if the PWM were enabled.


>>> If the PWM range is limited to something else than 0..100% this causes the
>>> probe function to fail with messages like:
>>>   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>>>   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
>>>   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
> What board are you testing with? I'd like to see the .dts to get a
> better understanding of where your board differs from mine.
> I think Uwe (PWM maintainer) also previously mentioned that there's no
> defined behavior for disabling a PWM and then reading it's state
> again. I'll see if I can find his explanation and whether I can come
> up with a patch that may help here.

Here's the DTS fragment for my power supply. You can trigger the 
behaviour with "pwm-dutycycle-range":

             dsp7_supply_pwm: dsp7-supply-pwm {
                 compatible = "pwm-regulator";
                 pwms = <&pwm 1 50000 0>; /* Period of 50000ns means 
20kHz */
                 regulator-name = "dsp7-pwm";
                 /* VCOM = 3.9V + 0.9V * duty_cycle. */
                 regulator-min-microvolt = <3900000>;
                 regulator-max-microvolt = <4800000>;
                 pwm-dutycycle-unit = <100>;
                 pwm-dutycycle-range = <50 100>; /* Must be >= 4.35V */
                 pwm-supply = <&dsp7_supply>;
             };

>>> The reason is that regulator_register() calls get_voltage(). The PWM is
>>> disabled at this point, and then the validity check triggers.
>>>
>>> Anything that calls get_voltage() while the PWM is disabled would get an
>>> -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
> Why is the revert from your patch 2/2 not enough (it gets rid of
> -ENOTRECOVERABLE)?

This also triggers that. It will get rid of ENOTRECOVERABLE but it still 
may cause the PWM to be set to incorrect values.

> The problem with this specific revert (your patch 1/2) is that (from
> memory) it will break these boards:
> - arch/arm/boot/dts/amlogic/meson8b-ec100.dts
> - arch/arm/boot/dts/amlogic/meson8b-mxq.dts
> - arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> - arch/arm/boot/dts/amlogic/meson8m2-wetek-core.dts

I honestly don't see how...


>
> Best regards,
> Martin


-- 
Mike Looijmans




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-08 15:13       ` Mark Brown
@ 2026-11-09  6:22         ` Mike Looijmans
  0 siblings, 0 replies; 14+ messages in thread
From: Mike Looijmans @ 2026-11-09  6:22 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-kernel, Liam Girdwood, Martin Blumenstingl

On 08-11-2026 16:13, Mark Brown wrote:
> On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:
>> This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
>> checks in continuous .get_voltage").
>> The core, and in particular the machine_constraints_voltage() method,
>> expects the regulator to report what the current setting is, even if that
>> is outside the allowed range. The core will then bring the voltage into
>> that range. It cannot do that if the regulator refuses to return its
>> setting, and causes probe() to fail for a pwm-regulator when it has
>> restrictions to the PWM range with messages like:
>>    dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>>    pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
>>      regulator pwm-regulator
>>    pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
>>      with error -131
> Are you sure?  We have specific handling for -ENOTRECOVERABLE which just
> blindly writes in the range specified in the constraints if the current
> voltage is unreadable.  If that mechanism isn't working we should fix it
> rather than changing drivers.

In core.c:

         if (current_uV == -ENOTRECOVERABLE) {
             /* This regulator can't be read and must be initialized */
             rdev_info(rdev, "Setting %d-%duV\n",
                   rdev->constraints->min_uV,
                   rdev->constraints->max_uV);
             _regulator_do_set_voltage(rdev,
                           rdev->constraints->min_uV,
                           rdev->constraints->max_uV);
             current_uV = regulator_get_voltage_rdev(rdev);
         }

         if (current_uV < 0) {
             if (current_uV != -EPROBE_DEFER)
                 rdev_err(rdev,
                      "failed to get the current voltage: %pe\n",
                      ERR_PTR(current_uV));
             return current_uV;
         }

After _regulator_do_set_voltage() it calls regulator_get_voltage_rdev() 
again, which keeps returning -ENOTRECOVERABLE, and thus the error path 
kicks in.


>
>> Besides that, regulator_get_voltage() is not supposed to return any
>> state information, as is mentioned in the API documentation.
>>   	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
>> -	if (voltage < min(max_uV_duty, min_uV_duty) ||
>> -	    voltage > max(max_uV_duty, min_uV_duty))
>> -		return -ENOTRECOVERABLE;
> The rest of the function relies on this to avoid having to deal with
> wrapping when things are out of range.

It's core.c that will handle that. It expects regulator_get_voltage() to 
return something out of range, and will correct that situation.



-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage"
  2026-11-06 21:45         ` Martin Blumenstingl
  2026-11-07 12:25           ` Mark Brown
@ 2026-11-09  6:34           ` Mike Looijmans
  1 sibling, 0 replies; 14+ messages in thread
From: Mike Looijmans @ 2026-11-09  6:34 UTC (permalink / raw)
  To: Martin Blumenstingl, Mark Brown; +Cc: linux-kernel, Liam Girdwood

On 06-11-2026 22:45, Martin Blumenstingl wrote:
> On Fri, Nov 6, 2026 at 2:46 PM Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Nov 06, 2026 at 01:59:56PM +0100, Mike Looijmans wrote:
>>> This reverts commit c92688cac239 ("regulator: pwm-regulator: Add validity
>>> checks in continuous .get_voltage").
>> And again...  please don't try to sneak changes past people :/
> Thank you for looping me in, Mark!
>
>>> The core, and in particular the machine_constraints_voltage() method,
>>> expects the regulator to report what the current setting is, even if that
>>> is outside the allowed range. The core will then bring the voltage into
>>> that range. It cannot do that if the regulator refuses to return its
>>> setting, and causes probe() to fail for a pwm-regulator when it has
>>> restrictions to the PWM range with messages like:
>>>    dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>>>    pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register
>>>      regulator pwm-regulator
>>>    pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed
>>>      with error -131
>>>
>>> Besides that, regulator_get_voltage() is not supposed to return any
>>> state information, as is mentioned in the API documentation.
> according to include/linux/regulator/driver.h:
>   * @get_voltage: Return the currently configured voltage for the regulator;
>   *                   return -ENOTRECOVERABLE if regulator can't be read at
>   *                   bootup and hasn't been set yet.
>
> Can you please specify which API documentation you are referring to?

https://docs.kernel.org/driver-api/regulator.html#c.regulator_get_voltage

And the text above clearly states "can't be read at bootup and hasn't 
been set yet", which is not the same as "whenever it's set out of range".


> I believe back when this change was authored it didn't make a
> difference on my board (but made sense based on review feedback).
> So I'm not against reverting as long as we have a proper explanation
> why the original patch is incorrect.
>
>
> Best regards,
> Martin


-- 
Mike Looijmans




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-09  6:09       ` Mike Looijmans
@ 2026-11-09 21:30         ` Martin Blumenstingl
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Blumenstingl @ 2026-11-09 21:30 UTC (permalink / raw)
  To: Mike Looijmans
  Cc: linux-kernel, Liam Girdwood, Mark Brown, Uwe Kleine-König

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

On Mon, Nov 9, 2026 at 7:09 AM Mike Looijmans <mike.looijmans@topic.nl> wrote:
>
> On 06-11-2026 22:57, Martin Blumenstingl wrote:
> > On Fri, Nov 6, 2026 at 2:42 PM Mark Brown <broonie@kernel.org> wrote:
> >> On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
> >>> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
> >>> output voltage for disabled PWMs")
> >> Adding Martin who wrote that patch - please CC people if you're directly
> >> updating their patches.
> > IIRC get_maintainers.pl includes the author of the patch blamed by the
> > Fixes: tag (if there's one).
> >
> >>> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
> >>> this could also mean that its output pin is tristated for example.
> > How do we know if it's 0% or tri-stated (and what's the behavior if
> > it's tri-stated)?
>
> We don't know, that's the point I'm trying to make. If the PWM is
> disabled, likely its clock is also turned off, and the actual state of
> the output is not known. Depending on hardware, it could be high, low,
> tristated, pulled up/down, ...
Not knowing the actual status is what Uwe also noted 2 years ago: [0]

> What may happen now is:
>
> - The PWM is set at 50% and disabled.
>
> - This function returns "0%" as being the current level during probe of
> the supply.
>
> - The core wants it to be at 0%, so it doesn't change the setting and
> enables the PWM.
>
> - The PWM now runs at 50% and the core thinks it's at 0%.
>
>
> What happens in my case:
>
> - This function returns 0% because the PWM is still disabled
>
> - Core wants it to be 50%, so it calls set_voltage() which sets the PWM
> but doesn't enable it yet
>
> - Core calls this function again and (still) gets 0% as being the setting.
>
> - This goes on until the supply (and hence the PWM) is enabled.
>
>
> What the callers expect is that this function returns the value that
> would be set if the PWM were enabled.
I think this is a good description and something we can improve upon.
I attached a patch - please try it. If it's not enough: please apply
your patch 2/2 on top.
The goal of that patch is:
- keep duty cycle around once it has been set
- only reset to duty cycle 0 at initialization and only if the
regulator has been described with regulator-boot-on
- (it achieves this by keeping a copy of struct pwm_state around)

I'd like to hear about your testing results and initial code-review feedback.
+Cc Uwe - happy to take your feedback as well.

>
> >>> If the PWM range is limited to something else than 0..100% this causes the
> >>> probe function to fail with messages like:
> >>>   dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
> >>>   pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
> >>>   pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
> > What board are you testing with? I'd like to see the .dts to get a
> > better understanding of where your board differs from mine.
> > I think Uwe (PWM maintainer) also previously mentioned that there's no
> > defined behavior for disabling a PWM and then reading it's state
> > again. I'll see if I can find his explanation and whether I can come
> > up with a patch that may help here.
>
> Here's the DTS fragment for my power supply. You can trigger the
> behaviour with "pwm-dutycycle-range":
>
>              dsp7_supply_pwm: dsp7-supply-pwm {
>                  compatible = "pwm-regulator";
>                  pwms = <&pwm 1 50000 0>; /* Period of 50000ns means
> 20kHz */
>                  regulator-name = "dsp7-pwm";
>                  /* VCOM = 3.9V + 0.9V * duty_cycle. */
>                  regulator-min-microvolt = <3900000>;
>                  regulator-max-microvolt = <4800000>;
>                  pwm-dutycycle-unit = <100>;
>                  pwm-dutycycle-range = <50 100>; /* Must be >= 4.35V */
>                  pwm-supply = <&dsp7_supply>;
>              };
Thank you. Since this doesn't seem to be upstream yet I'm against
reverting my original patch (more below).

[...]
> > The problem with this specific revert (your patch 1/2) is that (from
> > memory) it will break these boards:
> > - arch/arm/boot/dts/amlogic/meson8b-ec100.dts
> > - arch/arm/boot/dts/amlogic/meson8b-mxq.dts
> > - arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> > - arch/arm/boot/dts/amlogic/meson8m2-wetek-core.dts
>
> I honestly don't see how...
Back in the day I tried to describe it in detail: [1]
The short summary is:
- PWM settings match 860mV but PWM output is disabled (on this
SoC/board that means: duty cycle = 0)
- SoC is not stable below 1100mV
- pwm_enable() was used during boot and pwm-regulator did not adjust
the duty cycle and/or period before
-> hard lockup during boot


Best regards,
Martin


[0] https://lore.kernel.org/all/y7svv2jvcof3fnezvjbirkian5b7ajic7ajxgtyiejj3364ie7@uvmqlpjj6tv5/
[1] https://lore.kernel.org/all/20240113224628.377993-4-martin.blumenstingl@googlemail.com/

[-- Attachment #2: pwm-regulator-pwmstate.diff --]
[-- Type: text/x-patch, Size: 7453 bytes --]

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -41,6 +41,8 @@ struct pwm_regulator_data {
 
 	/* Enable GPIO */
 	struct gpio_desc *enb_gpio;
+
+	struct pwm_state pwm_state;
 };
 
 struct pwm_voltages {
@@ -48,18 +50,40 @@ struct pwm_voltages {
 	unsigned int dutycycle;
 };
 
+static int pwm_regulator_update_dutycycle(struct regulator_dev *rdev,
+					  unsigned int dutycyle,
+					  unsigned int dutycycle_unit)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	int ret;
+
+	pwm_set_relative_duty_cycle(&drvdata->pwm_state, dutycyle, dutycycle_unit);
+
+	if (!drvdata->pwm_state.enabled)
+		/*
+		 * Keep the "disabled configuration" (disabled = false,
+		 * period = 0, duty_cycle = 0) on the actual PWM output until
+		 * the regulator is getting enabled.
+		 */
+		return 0;
+
+	ret = pwm_apply_might_sleep(drvdata->pwm, &drvdata->pwm_state);
+	if (ret)
+		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
+
+	return ret;
+}
+
 /*
  * Voltage table call-backs
  */
 static void pwm_regulator_init_state(struct regulator_dev *rdev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
-	struct pwm_state pwm_state;
 	unsigned int dutycycle;
 	int i;
 
-	pwm_get_state(drvdata->pwm, &pwm_state);
-	dutycycle = pwm_get_relative_duty_cycle(&pwm_state, 100);
+	dutycycle = pwm_get_relative_duty_cycle(&drvdata->pwm_state, 100);
 
 	for (i = 0; i < rdev->desc->n_voltages; i++) {
 		if (dutycycle == drvdata->duty_cycle_table[i].dutycycle) {
@@ -83,18 +107,13 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 					 unsigned selector)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
-	struct pwm_state pstate;
 	int ret;
 
-	pwm_init_state(drvdata->pwm, &pstate);
-	pwm_set_relative_duty_cycle(&pstate,
-			drvdata->duty_cycle_table[selector].dutycycle, 100);
-
-	ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
+	ret = pwm_regulator_update_dutycycle(rdev,
+					     drvdata->duty_cycle_table[selector].dutycycle,
+					     100);
+	if (ret)
 		return ret;
-	}
 
 	drvdata->state = selector;
 
@@ -115,17 +134,41 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
 static int pwm_regulator_enable(struct regulator_dev *dev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	int ret;
 
 	gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
 
-	return pwm_enable(drvdata->pwm);
+	drvdata->pwm_state.enabled = true;
+
+	ret = pwm_apply_might_sleep(drvdata->pwm, &drvdata->pwm_state);
+	if (ret)
+		dev_err(&dev->dev, "Failed to enable PWM: %d\n", ret);
+
+	return ret;
 }
 
 static int pwm_regulator_disable(struct regulator_dev *dev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	struct pwm_state new_pstate;
+	int ret;
+
+	drvdata->pwm_state.enabled = false;
+
+	/*
+	 * Keep the period and duty_cycle within the copy in drvdata untouched
+	 * so the regulator can be enabled again without a .set_voltage{,_sel}
+	 * call prior to re-enabling it.
+	 */
+	new_pstate = drvdata->pwm_state;
+	new_pstate.period = 0;
+	new_pstate.duty_cycle = 0;
 
-	pwm_disable(drvdata->pwm);
+	ret = pwm_apply_might_sleep(drvdata->pwm, &new_pstate);
+	if (ret) {
+		dev_err(&dev->dev, "Failed to disable PWM: %d\n", ret);
+		return ret;
+	}
 
 	gpiod_set_value_cansleep(drvdata->enb_gpio, 0);
 
@@ -139,7 +182,7 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev)
 	if (drvdata->enb_gpio && !gpiod_get_value_cansleep(drvdata->enb_gpio))
 		return false;
 
-	return pwm_is_enabled(drvdata->pwm);
+	return drvdata->pwm_state.enabled;
 }
 
 static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
@@ -151,20 +194,10 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
 	int min_uV = rdev->constraints->min_uV;
 	int max_uV = rdev->constraints->max_uV;
 	int diff_uV = max_uV - min_uV;
-	struct pwm_state pstate;
 	unsigned int diff_duty;
 	unsigned int voltage;
 
-	pwm_get_state(drvdata->pwm, &pstate);
-
-	if (!pstate.enabled) {
-		if (pstate.polarity == PWM_POLARITY_INVERSED)
-			pstate.duty_cycle = pstate.period;
-		else
-			pstate.duty_cycle = 0;
-	}
-
-	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
+	voltage = pwm_get_relative_duty_cycle(&drvdata->pwm_state, duty_unit);
 	if (voltage < min(max_uV_duty, min_uV_duty) ||
 	    voltage > max(max_uV_duty, min_uV_duty))
 		return -ENOTRECOVERABLE;
@@ -198,12 +231,8 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	int min_uV = rdev->constraints->min_uV;
 	int max_uV = rdev->constraints->max_uV;
 	int diff_uV = max_uV - min_uV;
-	struct pwm_state pstate;
 	unsigned int diff_duty;
 	unsigned int dutycycle;
-	int ret;
-
-	pwm_init_state(drvdata->pwm, &pstate);
 
 	/*
 	 * The dutycycle for min_uV might be greater than the one for max_uV.
@@ -224,15 +253,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	else
 		dutycycle = min_uV_duty + dutycycle;
 
-	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
-
-	ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
-	if (ret) {
-		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
+	return pwm_regulator_update_dutycycle(rdev, dutycycle, duty_unit);
 }
 
 static const struct regulator_ops pwm_regulator_voltage_table_ops = {
@@ -321,32 +342,6 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev,
 	return 0;
 }
 
-static int pwm_regulator_init_boot_on(struct platform_device *pdev,
-				      struct pwm_regulator_data *drvdata,
-				      const struct regulator_init_data *init_data)
-{
-	struct pwm_state pstate;
-
-	if (!init_data->constraints.boot_on || drvdata->enb_gpio)
-		return 0;
-
-	pwm_get_state(drvdata->pwm, &pstate);
-	if (pstate.enabled)
-		return 0;
-
-	/*
-	 * Update the duty cycle so the output does not change
-	 * when the regulator core enables the regulator (and
-	 * thus the PWM channel).
-	 */
-	if (pstate.polarity == PWM_POLARITY_INVERSED)
-		pstate.duty_cycle = pstate.period;
-	else
-		pstate.duty_cycle = 0;
-
-	return pwm_apply_might_sleep(drvdata->pwm, &pstate);
-}
-
 static int pwm_regulator_probe(struct platform_device *pdev)
 {
 	const struct regulator_init_data *init_data;
@@ -404,10 +399,23 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = pwm_regulator_init_boot_on(pdev, drvdata, init_data);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "Failed to apply boot_on settings\n");
+	pwm_init_state(drvdata->pwm, &drvdata->pwm_state);
+
+	if (init_data->constraints.boot_on && !drvdata->enb_gpio &&
+	    !drvdata->pwm_state.enabled)
+		/*
+		* In general it's unknown what the output of a disabled PWM is.
+		* The only sane assumption here is it emits the inactive level
+		* which corresponds to duty_cycle = 0 (independent of the
+		* polarity).
+		*
+		* Update the duty cycle so the output does not change when the
+		* regulator core enables the regulator (and thus the PWM
+		* channel) and there's no change to the duty cycle because the
+		* voltage that is achieved with a disabled PWM is already the
+		* desired one.
+		*/
+		drvdata->pwm_state.duty_cycle = 0;
 
 	regulator = devm_regulator_register(&pdev->dev,
 					    &drvdata->desc, &config);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
  2026-11-06 12:59 ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mike Looijmans
       [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.6954786b-65e0-4206-a0c4-60cd251dbec9@emailsignatures365.codetwo.com>
  2026-11-06 13:36   ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mark Brown
@ 2026-11-10  9:33   ` Uwe Kleine-König
  2 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2026-11-10  9:33 UTC (permalink / raw)
  To: Mike Looijmans
  Cc: linux-kernel, Liam Girdwood, Mark Brown, Martin Blumenstingl

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

Hello,

thanks to Martin for looping me in.

On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
> output voltage for disabled PWMs")
> 
> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
> this could also mean that its output pin is tristated for example.

or even active.

> If the PWM range is limited to something else than 0..100% this causes the
> probe function to fail with messages like:
>  dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>  pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
>  pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
> 
> The reason is that regulator_register() calls get_voltage(). The PWM is
> disabled at this point, and then the validity check triggers.
> 
> Anything that calls get_voltage() while the PWM is disabled would get an
> -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  drivers/regulator/pwm-regulator.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 7434b6b22d325..f50530c77d92f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -157,13 +157,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
> -	if (!pstate.enabled) {
> -		if (pstate.polarity == PWM_POLARITY_INVERSED)
> -			pstate.duty_cycle = pstate.period;
> -		else
> -			pstate.duty_cycle = 0;
> -	}
> -

The status quo (ie. with the if removed in this patch) relies on
undefined behaviour because (as noted in the commit log) the output of a
disabled PWM is unknown. It might be high, or low, or tristate, or
active, or inactive or it might even still toggling. So without further
information about the involved hardware, you cannot say what the PWM
(and so the regulator) does.

However removing that block makes the driver ignore if the PWM is
enabled or not and diagnoses something invented. With a driver
implementing the waveform API (instead of the legacy .apply() and
.get_state()) you get

pstate = { .period = 0, .duty_cycle = 0, polarity = PWM_POLARITY_NORMAL,
.enabled = false, .usage_power = false }

For a driver with .get_state() you might get

pstate = { .period = 100000, .duty_cycle = 100000, polarity = PWM_POLARITY_NORMAL,
.enabled = false, .usage_power = false }

>  	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);

... which is interpreted as fully off and fully on respectively.

I think assuming a disabled regulator when the PWM is disabled is a sane
choice here, but IMHO definitively needs a comment and explict handling.
Something like:

	if (!pstate.enabled)
		/*
		 * We don't know what the PWM does here, it might just
		 * have stopped toggling at the active state. Let's
		 * assume the regulator is off instead of erroring out.
		 */
		return 0;

Technically more correct would be to say "I don't know", but that
probably involves some hassles.

Also note that pwm_get_state() returns the last requested state and not
the actually emitted waveform. (So if pwm_apply*() was called with
state->duty_cycle = 300 ns and the hardware rounds that down to 0
because the smallest duty_cycle the hardware can do it 400 ns,
pwm_get_state() still gives you .duty_cycle = 300.) Also pwm_get_state()
just reports "disabled" for drivers without a .get_state() callback if
pwm_apply*() wasn't called yet. You might want to switch that to
pwm_get_state_hw().

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2026-11-10  9:33 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.44efb263-5265-4fae-9cff-e8eaefbc0afd@emailsignatures365.codetwo.com>
2026-11-06 12:59 ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mike Looijmans
     [not found]   ` <1b153bce-a66a-45ee-a5c6-963ea6fb1c82.949ef384-8293-46b8-903f-40a477c056ae.6954786b-65e0-4206-a0c4-60cd251dbec9@emailsignatures365.codetwo.com>
2026-11-06 12:59     ` [PATCH 2/2] regulator: pwm-regulator: Revert "Add validity checks in continuous .get_voltage" Mike Looijmans
2026-11-06 13:37       ` Mark Brown
2026-11-06 21:45         ` Martin Blumenstingl
2026-11-07 12:25           ` Mark Brown
2026-11-09  6:34           ` Mike Looijmans
2026-11-08 15:13       ` Mark Brown
2026-11-09  6:22         ` Mike Looijmans
2026-11-06 13:36   ` [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs" Mark Brown
2026-11-06 13:56     ` Mike Looijmans
2026-11-06 21:57     ` Martin Blumenstingl
2026-11-09  6:09       ` Mike Looijmans
2026-11-09 21:30         ` Martin Blumenstingl
2026-11-10  9:33   ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®