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

[Andrea della Porta]

Hi Uwe,

On 08:27 Fri 10 Apr     , Uwe Kleine-König wrote:
> 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.

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.

> 
> > > > +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'. 

Many thanks,
Andrea

> 
> Best regards
> Uwe