Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Uwe Kleine-König]

Hello Cosmin,

On Tue, Mar 17, 2026 at 11:02:12PM +0000, Cosmin-Gabriel Tanislav wrote:
> > From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> > Sent: Tuesday, March 17, 2026 11:12 AM
> > 
> > On Mon, Mar 16, 2026 at 03:49:35PM +0000, Cosmin-Gabriel Tanislav wrote:
> > > static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > 			      const struct pwm_state *state)
> > > {
> > > 	...
> > >
> > > 	u32 enable_count;
> > >
> > > 	...
> > >
> > > 	/*
> > > 	 * Account for the case where one IO is already enabled and this call
> > > 	 * enables the second one, to prevent the prescale from being changed.
> > > 	 * If this PWM is currently disabled it will be enabled by this call,
> > > 	 * so include it in the enable count. If it is already enabled, it has
> > > 	 * already been accounted for.
> > > 	 */
> > > 	enable_count = rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);
> > >
> > > 	...
> > >
> > > 	if (enable_count > 1) {
> > > 		if (rz_mtu3_pwm->prescale[ch] > prescale)
> > > 			return -EBUSY;
> > >
> > > 		prescale = rz_mtu3_pwm->prescale[ch];
> > > 	}
> > >
> > > Please let me know what you think so we can proceed with the work
> > > internally.
> > 
> > I'd prefer the `rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);`
> > variant. I understand that this is also the variant you prefer, so
> > that's great, but I wouldn't stop you using the sibling option.
> 
> I realized the check could be simplified quite a bit while achieving
> the same outcome.
> 
> 	if (rz_mtu3_pwm->enable_count[ch] > pwm->state.enabled) {
> 		...
> 	}
> 
> 2 > 1 -> true, prescale gets checked when updating one of the IOs if
> both are enabled
> 
> 1 > 0 -> true, prescale gets checked when enabling the second IO
> 
> 1 > 1 -> false, prescale is not checked when updating a single enabled
> IO
> 
> 0 > 0 -> false, prescale is not checked when enabling the first IO
> 
> 2 > 0 and 0 > 1 -> impossible since enable_count is always in sync
> with PWM state

I didn't try to understand that, but on first glance it doesn't look
intuitive, so needs a code comment.

> > You can gain some extra points for not using pwm->state. This is a relic
> > from the legacy pwm abstraction and doesn't make much sense with the
> > waveform callbacks. 
> 
> I can switch from enable_count to an enable_mask in a later commit, and
> that will allow us to both get rid of PWM state access entirely and also
> make the sibling check more obvious, by doing something like:
> 
> 	if (rz_mtu3_pwm->enable_mask[ch] & ~BIT(rz_mtu3_hwpwm_io(pwm->hwpwm))) {
> 		...
> 	}
> 
> Which would read like "is any other IO enabled?". If yes, don't touch
> prescale.
> 
> But for the scope of these fixes we need to keep accessing PWM state as I
> would like them to be backported to stable.

ack, getting rid of pwm->state is a separate patch that should be
addressed only after the fixes under discussion.
 
> enable_mask must remain per-HW channel because it makes the enable /
> disable checks simpler.
> 
> If this sounds good to you, I will proceed with all of these changes.

It does, so go.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature