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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]