| Message ID | 20260104194224.41245-1-marek.vasut+renesas@mailbox.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] regulator: rpi-panel-v2: Convert to new PWM waveform ops | expand |
On Sun, Jan 04, 2026 at 08:41:43PM +0100, 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-pwm@vger.kernel.org > Cc: linux-renesas-soc@vger.kernel.org > --- > V2: - Safeguard against wf->duty_length_ns > wf->period_length_ns I would claim that this is a bug in the core if a driver sees such a wf variable. > V3: - Use PWM_BL_MASK as the maximum period length > --- > Note this now generates warnings: > pwm pwmchip5: Wrong rounding: requested 162/255 [+0], result 19000/31000 [+0] So the driver is wrong, see below. > --- > drivers/regulator/rpi-panel-v2-regulator.c | 53 +++++++++++++++++----- > 1 file changed, 42 insertions(+), 11 deletions(-) > > diff --git a/drivers/regulator/rpi-panel-v2-regulator.c b/drivers/regulator/rpi-panel-v2-regulator.c > index 30b78aa75ee38..e5e12ff649804 100644 > --- a/drivers/regulator/rpi-panel-v2-regulator.c > +++ b/drivers/regulator/rpi-panel-v2-regulator.c > @@ -35,24 +35,55 @@ 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; > + > + if (wf->duty_length_ns > wf->period_length_ns) > + *wfhw = PWM_BL_MASK; > + else > + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); This is wrong. There was already a discussion about this in reply to v2. I'll discard this patch from my queue and continue the v2 thread. Best regards Uwe
On 1/15/26 11:12 AM, Uwe Kleine-König wrote: > On Sun, Jan 04, 2026 at 08:41:43PM +0100, 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-pwm@vger.kernel.org >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> V2: - Safeguard against wf->duty_length_ns > wf->period_length_ns > > I would claim that this is a bug in the core if a driver sees such a wf > variable. > >> V3: - Use PWM_BL_MASK as the maximum period length >> --- >> Note this now generates warnings: >> pwm pwmchip5: Wrong rounding: requested 162/255 [+0], result 19000/31000 [+0] > > So the driver is wrong, see below. > >> --- >> drivers/regulator/rpi-panel-v2-regulator.c | 53 +++++++++++++++++----- >> 1 file changed, 42 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/regulator/rpi-panel-v2-regulator.c b/drivers/regulator/rpi-panel-v2-regulator.c >> index 30b78aa75ee38..e5e12ff649804 100644 >> --- a/drivers/regulator/rpi-panel-v2-regulator.c >> +++ b/drivers/regulator/rpi-panel-v2-regulator.c >> @@ -35,24 +35,55 @@ 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; >> + >> + if (wf->duty_length_ns > wf->period_length_ns) >> + *wfhw = PWM_BL_MASK; >> + else >> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); > > This is wrong. There was already a discussion about this in reply to v2. > I'll discard this patch from my queue and continue the v2 thread. Instead of resuscitating the old thread, could you please tell me how to make the conversion, so it won't break with existing bindings and the result would work as well as the current code ?
Hello Marek, On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: > On 1/15/26 11:12 AM, Uwe Kleine-König wrote: > > On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: > > > - struct regmap *regmap = pwmchip_get_drvdata(chip); > > > - unsigned int duty; > > > + u8 *wfhw = _wfhw; > > > + > > > + if (wf->duty_length_ns > wf->period_length_ns) > > > + *wfhw = PWM_BL_MASK; > > > + else > > > + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); > > > > This is wrong. There was already a discussion about this in reply to v2. > > I'll discard this patch from my queue and continue the v2 thread. > > Instead of resuscitating the old thread, could you please tell me how to > make the conversion, so it won't break with existing bindings and the result > would work as well as the current code ? the only way you can do this correctly is to measure or research the actual period length of the device. As this seems hard, the function I suggested in v2 works for me, too. The specified binding is unaffected by that. The only dts I found using this device (arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-rpi-display-2.dtsi) has backlight { compatible = "pwm-backlight"; pwms = <&mcu 0 255 0>; }; mcu: gpio@45 { compatible = "raspberrypi,touchscreen-panel-regulator-v2"; ... }; . Given that the dt specifies something made up and the driver was written in a way that is wrong but does the right thing in combination with the made up .dts, you cannot fix the driver to be a correct PWM driver without having to adapt the invented values in the .dts, too. (An option would be to adapt the pwm-backlight driver to ignore the provided period, but I think that isn't sensible and badly affects many other machines that have a working PWM driver. Or assume the PWM's period is 255 ns which is probably wrong, but so is 100 ms (the latter probably to a lesser extend).) Maybe the lesson to take away here is: if a driver implements a PWM, Cc: the linux-pwm list and the pwm maintainer on the submission. And let me point out that even get-maintainers.pl knows about that: $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl ... "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) ... linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) ... Best regards Uwe
On 1/15/26 5:37 PM, Uwe Kleine-König wrote: Hello Uwe, > On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: >> On 1/15/26 11:12 AM, Uwe Kleine-König wrote: >>> On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: >>>> - struct regmap *regmap = pwmchip_get_drvdata(chip); >>>> - unsigned int duty; >>>> + u8 *wfhw = _wfhw; >>>> + >>>> + if (wf->duty_length_ns > wf->period_length_ns) >>>> + *wfhw = PWM_BL_MASK; >>>> + else >>>> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); >>> >>> This is wrong. There was already a discussion about this in reply to v2. >>> I'll discard this patch from my queue and continue the v2 thread. >> >> Instead of resuscitating the old thread, could you please tell me how to >> make the conversion, so it won't break with existing bindings and the result >> would work as well as the current code ? > > the only way you can do this correctly is to measure or research the > actual period length of the device. As this seems hard, the function I > suggested in v2 works for me, too. Sadly, that does not work on the board I use , which is the one below. I was also hoping that Dave might have some input on the PWM frequency of this display. > The specified binding is unaffected by that. The only dts I found using > this device > (arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-rpi-display-2.dtsi) > has > > backlight { > compatible = "pwm-backlight"; > pwms = <&mcu 0 255 0>; > }; > > mcu: gpio@45 { > compatible = "raspberrypi,touchscreen-panel-regulator-v2"; > ... > }; > > . Given that the dt specifies something made up and the driver was > written in a way that is wrong but does the right thing in combination > with the made up .dts, you cannot fix the driver to be a correct PWM > driver without having to adapt the invented values in the .dts, too. I think this is what this V3 does. Would that be an option here ? > (An option would be to adapt the pwm-backlight driver to ignore the > provided period, but I think that isn't sensible and badly affects many > other machines that have a working PWM driver. Or assume the PWM's > period is 255 ns which is probably wrong, but so is 100 ms (the latter > probably to a lesser extend).) > > Maybe the lesson to take away here is: if a driver implements a PWM, Cc: > the linux-pwm list and the pwm maintainer on the submission. And let me > point out that even get-maintainers.pl knows about that: > > $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl > ... > "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > ... > linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > ... I do use get_maintainer to CC people when sending patches, hmmmm.
Hello Marek, On Tue, Jan 20, 2026 at 02:50:03PM +0100, Marek Vasut wrote: > On 1/15/26 5:37 PM, Uwe Kleine-König wrote: > > . Given that the dt specifies something made up and the driver was > > written in a way that is wrong but does the right thing in combination > > with the made up .dts, you cannot fix the driver to be a correct PWM > > driver without having to adapt the invented values in the .dts, too. > > I think this is what this V3 does. Would that be an option here ? No, the driver doesn't become a correct one with your patch. Another option I see is to make the driver directly a backlight driver, instead of a (broken) pwm driver + pwm-backlight. > > Maybe the lesson to take away here is: if a driver implements a PWM, Cc: > > the linux-pwm list and the pwm maintainer on the submission. And let me > > point out that even get-maintainers.pl knows about that: > > > > $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl > > ... > > "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > ... > > linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > ... > > I do use get_maintainer to CC people when sending patches, hmmmm. What I looked at before writing this was: $ git log linus/master -- drivers/regulator/rpi-panel-v2-regulator.c | grep Link: Link: https://patch.msgid.link/20250616154018.430004-1-marek.vasut+renesas@mailbox.org Link: https://patch.msgid.link/20250609223012.87764-1-marek.vasut+renesas@mailbox.org Link: https://patch.msgid.link/20250609000748.1665219-2-marek.vasut+renesas@mailbox.org and neither I nor linux-pwm was on Cc: for these. Maybe you used get_maintainer.pl from before 8f960106c150a9733f5d7408b975c0a687617961? :-D Best regards Uwe
Hi Marek On Tue, 20 Jan 2026 at 13:50, Marek Vasut <marek.vasut@mailbox.org> wrote: > > On 1/15/26 5:37 PM, Uwe Kleine-König wrote: > > Hello Uwe, > > > On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: > >> On 1/15/26 11:12 AM, Uwe Kleine-König wrote: > >>> On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: > >>>> - struct regmap *regmap = pwmchip_get_drvdata(chip); > >>>> - unsigned int duty; > >>>> + u8 *wfhw = _wfhw; > >>>> + > >>>> + if (wf->duty_length_ns > wf->period_length_ns) > >>>> + *wfhw = PWM_BL_MASK; > >>>> + else > >>>> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); > >>> > >>> This is wrong. There was already a discussion about this in reply to v2. > >>> I'll discard this patch from my queue and continue the v2 thread. > >> > >> Instead of resuscitating the old thread, could you please tell me how to > >> make the conversion, so it won't break with existing bindings and the result > >> would work as well as the current code ? > > > > the only way you can do this correctly is to measure or research the > > actual period length of the device. As this seems hard, the function I > > suggested in v2 works for me, too. > > Sadly, that does not work on the board I use , which is the one below. > > I was also hoping that Dave might have some input on the PWM frequency > of this display. Sorry, I don't have that information, which is part of the reason why I originally wrote the driver as a backlight driver rather than PWM. Dave > > The specified binding is unaffected by that. The only dts I found using > > this device > > (arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-rpi-display-2.dtsi) > > has > > > > backlight { > > compatible = "pwm-backlight"; > > pwms = <&mcu 0 255 0>; > > }; > > > > mcu: gpio@45 { > > compatible = "raspberrypi,touchscreen-panel-regulator-v2"; > > ... > > }; > > > > . Given that the dt specifies something made up and the driver was > > written in a way that is wrong but does the right thing in combination > > with the made up .dts, you cannot fix the driver to be a correct PWM > > driver without having to adapt the invented values in the .dts, too. > > I think this is what this V3 does. Would that be an option here ? > > > (An option would be to adapt the pwm-backlight driver to ignore the > > provided period, but I think that isn't sensible and badly affects many > > other machines that have a working PWM driver. Or assume the PWM's > > period is 255 ns which is probably wrong, but so is 100 ms (the latter > > probably to a lesser extend).) > > > > Maybe the lesson to take away here is: if a driver implements a PWM, Cc: > > the linux-pwm list and the pwm maintainer on the submission. And let me > > point out that even get-maintainers.pl knows about that: > > > > $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl > > ... > > "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > ... > > linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > ... > I do use get_maintainer to CC people when sending patches, hmmmm.
On 1/20/26 3:41 PM, Dave Stevenson wrote: Hello Dave, >>> On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: >>>> On 1/15/26 11:12 AM, Uwe Kleine-König wrote: >>>>> On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: >>>>>> - struct regmap *regmap = pwmchip_get_drvdata(chip); >>>>>> - unsigned int duty; >>>>>> + u8 *wfhw = _wfhw; >>>>>> + >>>>>> + if (wf->duty_length_ns > wf->period_length_ns) >>>>>> + *wfhw = PWM_BL_MASK; >>>>>> + else >>>>>> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); >>>>> >>>>> This is wrong. There was already a discussion about this in reply to v2. >>>>> I'll discard this patch from my queue and continue the v2 thread. >>>> >>>> Instead of resuscitating the old thread, could you please tell me how to >>>> make the conversion, so it won't break with existing bindings and the result >>>> would work as well as the current code ? >>> >>> the only way you can do this correctly is to measure or research the >>> actual period length of the device. As this seems hard, the function I >>> suggested in v2 works for me, too. >> >> Sadly, that does not work on the board I use , which is the one below. >> >> I was also hoping that Dave might have some input on the PWM frequency >> of this display. > > Sorry, I don't have that information, which is part of the reason why > I originally wrote the driver as a backlight driver rather than PWM. Is there someone you can ask by any chance? Is this hardware made by waveshare ? Maybe they would know ?
On Tue, 20 Jan 2026 at 14:55, Marek Vasut <marek.vasut@mailbox.org> wrote: > > On 1/20/26 3:41 PM, Dave Stevenson wrote: > > Hello Dave, > > >>> On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: > >>>> On 1/15/26 11:12 AM, Uwe Kleine-König wrote: > >>>>> On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: > >>>>>> - struct regmap *regmap = pwmchip_get_drvdata(chip); > >>>>>> - unsigned int duty; > >>>>>> + u8 *wfhw = _wfhw; > >>>>>> + > >>>>>> + if (wf->duty_length_ns > wf->period_length_ns) > >>>>>> + *wfhw = PWM_BL_MASK; > >>>>>> + else > >>>>>> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); > >>>>> > >>>>> This is wrong. There was already a discussion about this in reply to v2. > >>>>> I'll discard this patch from my queue and continue the v2 thread. > >>>> > >>>> Instead of resuscitating the old thread, could you please tell me how to > >>>> make the conversion, so it won't break with existing bindings and the result > >>>> would work as well as the current code ? > >>> > >>> the only way you can do this correctly is to measure or research the > >>> actual period length of the device. As this seems hard, the function I > >>> suggested in v2 works for me, too. > >> > >> Sadly, that does not work on the board I use , which is the one below. > >> > >> I was also hoping that Dave might have some input on the PWM frequency > >> of this display. > > > > Sorry, I don't have that information, which is part of the reason why > > I originally wrote the driver as a backlight driver rather than PWM. > Is there someone you can ask by any chance? Is this hardware made by > waveshare ? Maybe they would know ? It's made by a 3rd party we work with. It's not Waveshare. I can ask the question, although I don't really see the gain in having to insert pwm-backlight into the chain when you don't have full control over the PWM device. Dave
On 1/20/26 4:23 PM, Dave Stevenson wrote: Hello Dave, >>>>> On Thu, Jan 15, 2026 at 02:14:15PM +0100, Marek Vasut wrote: >>>>>> On 1/15/26 11:12 AM, Uwe Kleine-König wrote: >>>>>>> On Sun, Jan 04, 2026 at 08:41:43PM +0100, Marek Vasut wrote: >>>>>>>> - struct regmap *regmap = pwmchip_get_drvdata(chip); >>>>>>>> - unsigned int duty; >>>>>>>> + u8 *wfhw = _wfhw; >>>>>>>> + >>>>>>>> + if (wf->duty_length_ns > wf->period_length_ns) >>>>>>>> + *wfhw = PWM_BL_MASK; >>>>>>>> + else >>>>>>>> + *wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, PWM_BL_MASK, wf->period_length_ns); >>>>>>> >>>>>>> This is wrong. There was already a discussion about this in reply to v2. >>>>>>> I'll discard this patch from my queue and continue the v2 thread. >>>>>> >>>>>> Instead of resuscitating the old thread, could you please tell me how to >>>>>> make the conversion, so it won't break with existing bindings and the result >>>>>> would work as well as the current code ? >>>>> >>>>> the only way you can do this correctly is to measure or research the >>>>> actual period length of the device. As this seems hard, the function I >>>>> suggested in v2 works for me, too. >>>> >>>> Sadly, that does not work on the board I use , which is the one below. >>>> >>>> I was also hoping that Dave might have some input on the PWM frequency >>>> of this display. >>> >>> Sorry, I don't have that information, which is part of the reason why >>> I originally wrote the driver as a backlight driver rather than PWM. >> Is there someone you can ask by any chance? Is this hardware made by >> waveshare ? Maybe they would know ? > > It's made by a 3rd party we work with. It's not Waveshare. > I can ask the question, That would be nice if you could do that. > although I don't really see the gain in having > to insert pwm-backlight into the chain when you don't have full > control over the PWM device. Because we could reuse the pwm-backlight code, that's the only reason.
On 1/20/26 3:19 PM, Uwe Kleine-König wrote: Hello Uwe, > On Tue, Jan 20, 2026 at 02:50:03PM +0100, Marek Vasut wrote: >> On 1/15/26 5:37 PM, Uwe Kleine-König wrote: >>> . Given that the dt specifies something made up and the driver was >>> written in a way that is wrong but does the right thing in combination >>> with the made up .dts, you cannot fix the driver to be a correct PWM >>> driver without having to adapt the invented values in the .dts, too. >> >> I think this is what this V3 does. Would that be an option here ? > > No, the driver doesn't become a correct one with your patch. Hmmmm. Then I really do not know what to do or how to move this forward, because if I go with the V2 approach, I wouldn't be able to use this on the board I use. Can we update the timing information in DT ? I think we can not. > Another option I see is to make the driver directly a backlight driver, > instead of a (broken) pwm driver + pwm-backlight. That's what I mentioned in reply to V2, but then we lose the benefits of reusing pwm-backlight code . >>> Maybe the lesson to take away here is: if a driver implements a PWM, Cc: >>> the linux-pwm list and the pwm maintainer on the submission. And let me >>> point out that even get-maintainers.pl knows about that: >>> >>> $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl >>> ... >>> "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) >>> ... >>> linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) >>> ... >> >> I do use get_maintainer to CC people when sending patches, hmmmm. > > What I looked at before writing this was: > > $ git log linus/master -- drivers/regulator/rpi-panel-v2-regulator.c | grep Link: > Link: https://patch.msgid.link/20250616154018.430004-1-marek.vasut+renesas@mailbox.org > Link: https://patch.msgid.link/20250609223012.87764-1-marek.vasut+renesas@mailbox.org > Link: https://patch.msgid.link/20250609000748.1665219-2-marek.vasut+renesas@mailbox.org > > and neither I nor linux-pwm was on Cc: for these. Maybe you used > get_maintainer.pl from before 8f960106c150a9733f5d7408b975c0a687617961? > :-D No, I use the one in tree only, which has to be the one from linux-next around the time it was posted.
Hello Marek, On Tue, Jan 20, 2026 at 11:25:59PM +0100, Marek Vasut wrote: > On 1/20/26 3:19 PM, Uwe Kleine-König wrote: > > On Tue, Jan 20, 2026 at 02:50:03PM +0100, Marek Vasut wrote: > > > On 1/15/26 5:37 PM, Uwe Kleine-König wrote: > > > > . Given that the dt specifies something made up and the driver was > > > > written in a way that is wrong but does the right thing in combination > > > > with the made up .dts, you cannot fix the driver to be a correct PWM > > > > driver without having to adapt the invented values in the .dts, too. > > > > > > I think this is what this V3 does. Would that be an option here ? > > > > No, the driver doesn't become a correct one with your patch. > > Hmmmm. Then I really do not know what to do or how to move this forward, > because if I go with the V2 approach, I wouldn't be able to use this on the > board I use. > > Can we update the timing information in DT ? I think we can not. Why not? > > Another option I see is to make the driver directly a backlight driver, > > instead of a (broken) pwm driver + pwm-backlight. > > That's what I mentioned in reply to V2, but then we lose the benefits of > reusing pwm-backlight code . I think that benefit isn't that big. And if this driver was relevant for me, I'd go that route. > > > > Maybe the lesson to take away here is: if a driver implements a PWM, Cc: > > > > the linux-pwm list and the pwm maintainer on the submission. And let me > > > > point out that even get-maintainers.pl knows about that: > > > > > > > > $ git format-patch -1 --stdout d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4 | scripts/get_maintainer.pl > > > > ... > > > > "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > > > ... > > > > linux-pwm@vger.kernel.org (open list:PWM SUBSYSTEM:Keyword:pwm_(config|apply_might_sleep|apply_atomic|ops)) > > > > ... > > > > > > I do use get_maintainer to CC people when sending patches, hmmmm. > > > > What I looked at before writing this was: > > > > $ git log linus/master -- drivers/regulator/rpi-panel-v2-regulator.c | grep Link: > > Link: https://patch.msgid.link/20250616154018.430004-1-marek.vasut+renesas@mailbox.org > > Link: https://patch.msgid.link/20250609223012.87764-1-marek.vasut+renesas@mailbox.org > > Link: https://patch.msgid.link/20250609000748.1665219-2-marek.vasut+renesas@mailbox.org > > > > and neither I nor linux-pwm was on Cc: for these. Maybe you used > > get_maintainer.pl from before 8f960106c150a9733f5d7408b975c0a687617961? > > :-D > > No, I use the one in tree only, which has to be the one from linux-next > around the time it was posted. Then I guess there is a bug somewhere in your workflow. The commit creating the driver (d49305862fdc4d9ff1b1093b4ed7d8e0cb9971b4) has an author date of "Mon Jun 9 02:06:42 2025 +0200". So it was probably prepared on top of next-20250606. Running the above command in that tree also emits my email address and linux-pwm. ¯\_(ツ)_/¯ Best regards Uwe
diff --git a/drivers/regulator/rpi-panel-v2-regulator.c b/drivers/regulator/rpi-panel-v2-regulator.c index 30b78aa75ee38..e5e12ff649804 100644 --- a/drivers/regulator/rpi-panel-v2-regulator.c +++ b/drivers/regulator/rpi-panel-v2-regulator.c @@ -35,24 +35,55 @@ 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; + + if (wf->duty_length_ns > wf->period_length_ns) + *wfhw = PWM_BL_MASK; + else + *wfhw = mul_u64_u64_div_u64(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 = PWM_BL_MASK * 1000; /* 31 us ~= 30 kHz */ + wf->duty_length_ns = *wfhw * 1000; /* 0..31us */ + 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, }; /*
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-pwm@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org --- V2: - Safeguard against wf->duty_length_ns > wf->period_length_ns V3: - Use PWM_BL_MASK as the maximum period length --- Note this now generates warnings: pwm pwmchip5: Wrong rounding: requested 162/255 [+0], result 19000/31000 [+0] --- drivers/regulator/rpi-panel-v2-regulator.c | 53 +++++++++++++++++----- 1 file changed, 42 insertions(+), 11 deletions(-)