Re: [PATCH 2/3] pwm: rp1: Add RP1 PWM controller driver

[Uwe Kleine-König]

Hello Andrea,

On Fri, Apr 10, 2026 at 12:27:35PM +0200, Andrea della Porta wrote:
> On 08:27 Fri 10 Apr     , Uwe Kleine-König wrote:
> > 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.
> 
> Sorry I should've been more clear on this. The pinmux/conf is not changed
> at all by this mask, only the PWM output mode is. The controller can output
> several type of waveforms and clearing PWM_MODE_MASK is just setting the
> controller to output a 0, which is the reset default i.e. the same value
> as just before exporting the channel.
> I guess this is the expected behaviour in case of a fan, it should stop
> spinning in case you unexport the pwm channel, but I see it could be
> different with displays.
> Honestly I don't have a strong opinion about that, please just let me
> know if I should drop that pwm_free entirely.

Yes, in this case drop the function completely. It's the responsibility
of the consumer to stop the PWM before releasing it.

> > > > > +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.
> 
> I'll come up with something, we can always drop this check if deemed
> too 'noisy'. 

Great, thanks
Uwe

Attachment: signature.asc
Description: PGP signature