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

[Uwe Kleine-König]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20250622/fb6e78ed/attachment.sig>