[PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Marek Vasut]

Convert the driver from legacy PWM apply ops to modern waveform ops.
There is no functional change.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/regulator/rpi-panel-v2-regulator.c | 50 +++++++++++++++++-----
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/rpi-panel-v2-regulator.c b/drivers/regulator/rpi-panel-v2-regulator.c
index 30b78aa75ee3..be42afc81d72 100644
--- a/drivers/regulator/rpi-panel-v2-regulator.c
+++ b/drivers/regulator/rpi-panel-v2-regulator.c
@@ -35,24 +35,52 @@ static const struct regmap_config rpi_panel_regmap_config = {
 	.can_sleep = true,
 };
 
-static int rpi_panel_v2_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-				  const struct pwm_state *state)
+static int rpi_panel_v2_pwm_round_waveform_tohw(struct pwm_chip *chip,
+						struct pwm_device *pwm,
+						const struct pwm_waveform *wf,
+						void *_wfhw)
 {
-	struct regmap *regmap = pwmchip_get_drvdata(chip);
-	unsigned int duty;
+	u8 *wfhw = _wfhw;
+
+	*wfhw = DIV_ROUND_CLOSEST_ULL(wf->duty_length_ns * PWM_BL_MASK, wf->period_length_ns);
+
+	return 0;
+}
 
-	if (state->polarity != PWM_POLARITY_NORMAL)
-		return -EINVAL;
+static int rpi_panel_v2_pwm_round_waveform_fromhw(struct pwm_chip *chip,
+						  struct pwm_device *pwm,
+						  const void *_wfhw,
+						  struct pwm_waveform *wf)
+{
+	const u8 *wfhw = _wfhw;
+
+	/*
+	 * These numbers here are utter fabrications, the device is sealed
+	 * in metal casing and difficult to take apart and measure, so we
+	 * pick some arbitrary values here, values which fit nicely.
+	 */
+	wf->period_length_ns = 100 * 1000;	/* 100 us ~= 10 kHz */
+	wf->duty_length_ns = *wfhw * 1000;	/* 0..100us */
+	wf->duty_offset_ns = 0;
+
+	return 0;
+}
 
-	if (!state->enabled)
-		return regmap_write(regmap, REG_PWM, 0);
+static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const void *_wfhw)
+{
+	struct regmap *regmap = pwmchip_get_drvdata(chip);
+	const u8 *wfhw = _wfhw;
 
-	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
-	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
+	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));
 }
 
 static const struct pwm_ops rpi_panel_v2_pwm_ops = {
-	.apply = rpi_panel_v2_pwm_apply,
+	.sizeof_wfhw		= sizeof(u8),
+	.round_waveform_fromhw	= rpi_panel_v2_pwm_round_waveform_fromhw,
+	.round_waveform_tohw	= rpi_panel_v2_pwm_round_waveform_tohw,
+	.write_waveform		= rpi_panel_v2_pwm_write_waveform,
 };
 
 /*
-- 
2.47.2

Re: [PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Uwe Kleine-König]

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

Hello Marek,

[Cc: += linux-pwm@vger.kernel.org]

On Tue, Jun 17, 2025 at 02:50:25AM +0200, Marek Vasut wrote:
> Convert the driver from legacy PWM apply ops to modern waveform ops.
> There is no functional change.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/regulator/rpi-panel-v2-regulator.c | 50 +++++++++++++++++-----
>  1 file changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/rpi-panel-v2-regulator.c b/drivers/regulator/rpi-panel-v2-regulator.c
> index 30b78aa75ee3..be42afc81d72 100644
> --- a/drivers/regulator/rpi-panel-v2-regulator.c
> +++ b/drivers/regulator/rpi-panel-v2-regulator.c
> @@ -35,24 +35,52 @@ static const struct regmap_config rpi_panel_regmap_config = {
>  	.can_sleep = true,
>  };
>  
> -static int rpi_panel_v2_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> -				  const struct pwm_state *state)
> +static int rpi_panel_v2_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +						struct pwm_device *pwm,
> +						const struct pwm_waveform *wf,
> +						void *_wfhw)
>  {
> -	struct regmap *regmap = pwmchip_get_drvdata(chip);
> -	unsigned int duty;
> +	u8 *wfhw = _wfhw;
> +
> +	*wfhw = DIV_ROUND_CLOSEST_ULL(wf->duty_length_ns * PWM_BL_MASK, wf->period_length_ns);

To have this consistent with rpi_panel_v2_pwm_round_waveform_fromhw()
and not suffer from potential overflow this should be:

/* The actual value isn't known, so this is made up. */
#define RPI_PANEL_V2_FIXED_PERIOD_NS 100000


static int rpi_panel_v2_pwm_round_waveform_tohw(...)
{
	...

	if (wf->duty_length_ns > RPI_PANEL_V2_FIXED_PERIOD_NS)
		*wfhw = 100;
	else
		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns * 100, RPI_PANEL_V2_FIXED_PERIOD_NS);

	return 0;
}

> +
> +	return 0;
> +}
>  
> -	if (state->polarity != PWM_POLARITY_NORMAL)
> -		return -EINVAL;
> +static int rpi_panel_v2_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +						  struct pwm_device *pwm,
> +						  const void *_wfhw,
> +						  struct pwm_waveform *wf)
> +{
> +	const u8 *wfhw = _wfhw;
> +
> +	/*
> +	 * These numbers here are utter fabrications, the device is sealed
> +	 * in metal casing and difficult to take apart and measure, so we
> +	 * pick some arbitrary values here, values which fit nicely.
> +	 */
> +	wf->period_length_ns = 100 * 1000;	/* 100 us ~= 10 kHz */
> +	wf->duty_length_ns = *wfhw * 1000;	/* 0..100us */
> +	wf->duty_offset_ns = 0;
> +
> +	return 0;
> +}
>  
> -	if (!state->enabled)
> -		return regmap_write(regmap, REG_PWM, 0);
> +static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const void *_wfhw)
> +{
> +	struct regmap *regmap = pwmchip_get_drvdata(chip);
> +	const u8 *wfhw = _wfhw;
>  
> -	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
> -	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
> +	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));

How does the PWM behave without PWM_BL_ENABLE set?

>  }
>  
>  static const struct pwm_ops rpi_panel_v2_pwm_ops = {
> -	.apply = rpi_panel_v2_pwm_apply,
> +	.sizeof_wfhw		= sizeof(u8),
> +	.round_waveform_fromhw	= rpi_panel_v2_pwm_round_waveform_fromhw,
> +	.round_waveform_tohw	= rpi_panel_v2_pwm_round_waveform_tohw,
> +	.write_waveform		= rpi_panel_v2_pwm_write_waveform,

Personally I don't like aligning the =. If this was a driver in
drivers/pwm I'd ask you to use a single space before the =.

Best regards
Uwe

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

Re: [PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Marek Vasut]

On 6/17/25 10:14 AM, Uwe Kleine-König wrote:

Hi,

[...]

> /* The actual value isn't known, so this is made up. */
> #define RPI_PANEL_V2_FIXED_PERIOD_NS 100000
> 
> 
> static int rpi_panel_v2_pwm_round_waveform_tohw(...)
> {
> 	...
> 
> 	if (wf->duty_length_ns > RPI_PANEL_V2_FIXED_PERIOD_NS)
> 		*wfhw = 100;
> 	else
> 		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns * 100, RPI_PANEL_V2_FIXED_PERIOD_NS);
> 
> 	return 0;
> }

This fixed period specified in driver has one problem -- what if the 
period is also specified in DT by the consumer, e.g. pwm-backlight pwms 
property, and it does not match RPI_PANEL_V2_FIXED_PERIOD_NS ?

This is easy to solve for this tohw function, but what about the fromhw 
which assigns period_ns ?

>> +static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
>> +					   struct pwm_device *pwm,
>> +					   const void *_wfhw)
>> +{
>> +	struct regmap *regmap = pwmchip_get_drvdata(chip);
>> +	const u8 *wfhw = _wfhw;
>>   
>> -	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
>> -	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
>> +	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));
> 
> How does the PWM behave without PWM_BL_ENABLE set?
The display stays dark.

Re: [PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Uwe Kleine-König]

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

Hello Marek,

On Tue, Jun 17, 2025 at 11:38:50AM +0200, Marek Vasut wrote:
> On 6/17/25 10:14 AM, Uwe Kleine-König wrote:
> 
> Hi,
> 
> [...]
> 
> > /* The actual value isn't known, so this is made up. */
> > #define RPI_PANEL_V2_FIXED_PERIOD_NS 100000
> > 
> > 
> > static int rpi_panel_v2_pwm_round_waveform_tohw(...)
> > {
> > 	...
> > 
> > 	if (wf->duty_length_ns > RPI_PANEL_V2_FIXED_PERIOD_NS)
> > 		*wfhw = 100;
> > 	else
> > 		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns * 100, RPI_PANEL_V2_FIXED_PERIOD_NS);
> > 
> > 	return 0;
> > }
> 
> This fixed period specified in driver has one problem -- what if the period
> is also specified in DT by the consumer, e.g. pwm-backlight pwms property,
> and it does not match RPI_PANEL_V2_FIXED_PERIOD_NS ?

If it's off by only a little amount you will hardly notice. If it's
further off that's less optimal. I don't think this is a problem. If the
device tree is wrong, the machine doesn't work optimally. That's quite
expected.

> This is easy to solve for this tohw function, but what about the fromhw
> which assigns period_ns ?

If you only write waveforms to the hardware (probably using
pwm_apply_might_sleep()), the fromhw callback isn't involved (apart from
debugging). And all fromhw callbacks are supposed to assign
period_length_ns. I suspect we don't have the same understanding of the
PWM abstraction.

> > > +static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
> > > +					   struct pwm_device *pwm,
> > > +					   const void *_wfhw)
> > > +{
> > > +	struct regmap *regmap = pwmchip_get_drvdata(chip);
> > > +	const u8 *wfhw = _wfhw;
> > > -	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
> > > -	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
> > > +	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));
> > 
> > How does the PWM behave without PWM_BL_ENABLE set?
> The display stays dark.

So you cannot distinguish writing `0` (or any other value) and
`PWM_BL_ENABLE | 0` I guess?

I ask because for some hardware's it makes a difference, the output
might e.g go High-Z on disable.

Best regards
Uwe

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

Re: [PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Marek Vasut]

On 6/17/25 4:13 PM, Uwe Kleine-König wrote:
> Hello Marek,

Hi,

>>> /* The actual value isn't known, so this is made up. */
>>> #define RPI_PANEL_V2_FIXED_PERIOD_NS 100000
>>>
>>>
>>> static int rpi_panel_v2_pwm_round_waveform_tohw(...)
>>> {
>>> 	...
>>>
>>> 	if (wf->duty_length_ns > RPI_PANEL_V2_FIXED_PERIOD_NS)
>>> 		*wfhw = 100;
>>> 	else
>>> 		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns * 100, RPI_PANEL_V2_FIXED_PERIOD_NS);
>>>
>>> 	return 0;
>>> }
>>
>> This fixed period specified in driver has one problem -- what if the period
>> is also specified in DT by the consumer, e.g. pwm-backlight pwms property,
>> and it does not match RPI_PANEL_V2_FIXED_PERIOD_NS ?
> 
> If it's off by only a little amount you will hardly notice. If it's
> further off that's less optimal. I don't think this is a problem. If the
> device tree is wrong, the machine doesn't work optimally. That's quite
> expected.

Before this rework, the DT period and duty cycle were scaled to match, 
so the period mismatch problem never occurred no matter what period was 
set in DT. I suspect with this rework, such a behavior is no longer 
possible, or is it ?

>> This is easy to solve for this tohw function, but what about the fromhw
>> which assigns period_ns ?
> 
> If you only write waveforms to the hardware (probably using
> pwm_apply_might_sleep()), the fromhw callback isn't involved (apart from
> debugging). And all fromhw callbacks are supposed to assign
> period_length_ns. I suspect we don't have the same understanding of the
> PWM abstraction.

The core does check whether the fromhw is implemented alongside 
.write_waveform , so how should the fromhw behave in such a case ?

See this drivers/pwm/core.c :

1633 static bool pwm_ops_check(const struct pwm_chip *chip)
1634 {
1635         const struct pwm_ops *ops = chip->ops;
1636
1637         if (ops->write_waveform) {
1638                 if (!ops->round_waveform_tohw ||
1639                     !ops->round_waveform_fromhw || // <------ HERE
1640                     !ops->write_waveform)
1641                         return false;

Maybe this should be patched out of the core.c and fromhw removed from 
this driver ?

>>>> +static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
>>>> +					   struct pwm_device *pwm,
>>>> +					   const void *_wfhw)
>>>> +{
>>>> +	struct regmap *regmap = pwmchip_get_drvdata(chip);
>>>> +	const u8 *wfhw = _wfhw;
>>>> -	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
>>>> -	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
>>>> +	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));
>>>
>>> How does the PWM behave without PWM_BL_ENABLE set?
>> The display stays dark.
> 
> So you cannot distinguish writing `0` (or any other value) and
> `PWM_BL_ENABLE | 0` I guess?

I think no.

> I ask because for some hardware's it makes a difference, the output
> might e.g go High-Z on disable.
Maybe Dave can clarify this, although I suspect this is also some third 
party hardware with no documentation.

Re: [PATCH] regulator: rpi-panel-v2: Convert to new PWM waveform ops

[Uwe Kleine-König]

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

Hello Marek,

On Sat, Jun 21, 2025 at 06:18:01PM +0200, Marek Vasut wrote:
> On 6/17/25 4:13 PM, Uwe Kleine-König wrote:
> > > > /* The actual value isn't known, so this is made up. */
> > > > #define RPI_PANEL_V2_FIXED_PERIOD_NS 100000
> > > > 
> > > > 
> > > > static int rpi_panel_v2_pwm_round_waveform_tohw(...)
> > > > {
> > > > 	...
> > > > 
> > > > 	if (wf->duty_length_ns > RPI_PANEL_V2_FIXED_PERIOD_NS)
> > > > 		*wfhw = 100;
> > > > 	else
> > > > 		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns * 100, RPI_PANEL_V2_FIXED_PERIOD_NS);
> > > > 
> > > > 	return 0;
> > > > }
> > > 
> > > This fixed period specified in driver has one problem -- what if the period
> > > is also specified in DT by the consumer, e.g. pwm-backlight pwms property,
> > > and it does not match RPI_PANEL_V2_FIXED_PERIOD_NS ?
> > 
> > If it's off by only a little amount you will hardly notice. If it's
> > further off that's less optimal. I don't think this is a problem. If the
> > device tree is wrong, the machine doesn't work optimally. That's quite
> > expected.
> 
> Before this rework, the DT period and duty cycle were scaled to match, so
> the period mismatch problem never occurred no matter what period was set in
> DT. I suspect with this rework, such a behavior is no longer possible, or is
> it ?

I see your point. But looking at it from the other side (which is the
official one) the duty_cycle was set very inconsistently before. I'm
aware that whatever policy is the one that you should base the hardware
settings on has strange corner cases. The one that was chosen is one
that typically is easy to implement. And with the waveform API the
consumers that would benefit from a "implement a setting that minimizes
the difference in the relative duty_cycles" policy can cope now. But
that only works if the drivers implement the promise this API gives.

> > > This is easy to solve for this tohw function, but what about the fromhw
> > > which assigns period_ns ?
> > 
> > If you only write waveforms to the hardware (probably using
> > pwm_apply_might_sleep()), the fromhw callback isn't involved (apart from
> > debugging). And all fromhw callbacks are supposed to assign
> > period_length_ns. I suspect we don't have the same understanding of the
> > PWM abstraction.
> 
> The core does check whether the fromhw is implemented alongside
> .write_waveform , so how should the fromhw behave in such a case ?
> 
> See this drivers/pwm/core.c :
> 
> 1633 static bool pwm_ops_check(const struct pwm_chip *chip)
> 1634 {
> 1635         const struct pwm_ops *ops = chip->ops;
> 1636
> 1637         if (ops->write_waveform) {
> 1638                 if (!ops->round_waveform_tohw ||
> 1639                     !ops->round_waveform_fromhw || // <------ HERE
> 1640                     !ops->write_waveform)
> 1641                         return false;
> 
> Maybe this should be patched out of the core.c and fromhw removed from this
> driver ?

Most drivers can read out the current setting and those benefit from a
fromhw callback, as it's needed to interpret the read settings. Every
driver can implement fromhw. Additionally I find it usually easier to
understand the fromhw callback than the tohw one. And it's needed for
the PWM_DEBUG checks. So no, I don't want to make it optional.

> > > > > +static int rpi_panel_v2_pwm_write_waveform(struct pwm_chip *chip,
> > > > > +					   struct pwm_device *pwm,
> > > > > +					   const void *_wfhw)
> > > > > +{
> > > > > +	struct regmap *regmap = pwmchip_get_drvdata(chip);
> > > > > +	const u8 *wfhw = _wfhw;
> > > > > -	duty = pwm_get_relative_duty_cycle(state, PWM_BL_MASK);
> > > > > -	return regmap_write(regmap, REG_PWM, duty | PWM_BL_ENABLE);
> > > > > +	return regmap_write(regmap, REG_PWM, *wfhw | (*wfhw ? PWM_BL_ENABLE : 0));
> > > > 
> > > > How does the PWM behave without PWM_BL_ENABLE set?
> > > The display stays dark.
> > 
> > So you cannot distinguish writing `0` (or any other value) and
> > `PWM_BL_ENABLE | 0` I guess?
> 
> I think no.
> 
> > I ask because for some hardware's it makes a difference, the output
> > might e.g go High-Z on disable.
> 
> Maybe Dave can clarify this, although I suspect this is also some third
> party hardware with no documentation.

Maybe this question is better directed to the vendor of the device?

Best regards
Uwe

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