lkml.org 
[lkml]   [2026]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/3] pwm: rp1: Add RP1 PWM controller driver
Hello Andrea,

On Thu, Apr 09, 2026 at 06:16:41PM +0200, Andrea della Porta wrote:
> On 23:45 Sun 05 Apr , Uwe Kleine-König wrote:
> > On Fri, Apr 03, 2026 at 04:31:55PM +0200, Andrea della Porta wrote:
> > > +static void rp1_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > > +{
> > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > > + u32 value;
> > > +
> > > + value = readl(rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> > > + value &= ~PWM_MODE_MASK;
> > > + writel(value, rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> > > +
> > > + rp1_pwm_apply_config(chip, pwm);
> >
> > What is the purpose of this call?
>
> To update the configuration on the next PWM strobe in order to avoid
> glitches. I'll add a short comment in the code.

.pwm_free() should not touch the hardware configuration. Changing the
pinmuxing (which I guess is the purpose of clearing PWM_MODE_MASK) is
somewhat a grey area. If that saves energy, that's okish. Otherwise
not interfering with the operation of the PWM (e.g. to keep a display on
during kexec or so) is preferred.

> > > +static int rp1_pwm_resume(struct device *dev)
> > > +{
> > > + struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> > > +
> > > + return clk_prepare_enable(rp1->clk);
> >
> > Hmm, if this fails and then the driver is unbound, the clk operations
> > are not balanced.
>
> I'll add some flags to check if the clock is really enabled or not.

To be honest, I guess that is a problem of several drivers, not only in
drivers/pwm. If this complicates the driver, I guess addressing this
isn't very critical.

Best regards
Uwe
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2026-04-10 08:30    [W:0.138 / U:6.062 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog