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

[Cosmin-Gabriel Tanislav]

> From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> Sent: Tuesday, March 17, 2026 11:12 AM
> 
> Hello,
> 
> On Mon, Mar 16, 2026 at 03:49:35PM +0000, Cosmin-Gabriel Tanislav wrote:
> > What do you think about this setup for mapping from PWM to HW?
> >
> > #define RZ_MTU3_PWM_IO(ch, secondary) \
> > 	(((ch) << 1) | (secondary))
> >
> > #define RZ_MTU3_PWM_1_IO(ch) \
> > 	RZ_MTU3_PWM_IO(ch, 0)
> >
> > #define RZ_MTU3_PWM_2_IO(ch) \
> > 	RZ_MTU3_PWM_IO(ch, 0), \
> > 	RZ_MTU3_PWM_IO(ch, 1)
> >
> > static const u8 rz_mtu3_pwm_io_map[] = {
> > 	RZ_MTU3_PWM_2_IO(0), /* MTU0 */
> > 	RZ_MTU3_PWM_1_IO(1), /* MTU1 */
> > 	RZ_MTU3_PWM_1_IO(2), /* MTU2 */
> > 	RZ_MTU3_PWM_2_IO(3), /* MTU3 */
> > 	RZ_MTU3_PWM_2_IO(4), /* MTU4 */
> > 	RZ_MTU3_PWM_2_IO(5), /* MTU6 */
> > 	RZ_MTU3_PWM_2_IO(6), /* MTU7 */
> > };
> > static_assert(ARRAY_SIZE(rz_mtu3_pwm_io_map) == RZ_MTU3_MAX_PWM_CHANNELS);
> 
> I think the RZ_MTU3_PWM_1_IO and RZ_MTU3_PWM_2_IO macros are a bit too
> magic and would use
> 
> static const u8 rz_mtu3_pwm_io_map[] = {
> 	RZ_MTU3_PWM_IO(0, 0),
> 	RZ_MTU3_PWM_IO(0, 1),
> 	RZ_MTU3_PWM_IO(1, 0),
> 	RZ_MTU3_PWM_IO(2, 0),
> 	RZ_MTU3_PWM_IO(3, 0),
> 	RZ_MTU3_PWM_IO(3, 1),
> 	RZ_MTU3_PWM_IO(4, 0),
> 	RZ_MTU3_PWM_IO(4, 1),
> 	RZ_MTU3_PWM_IO(5, 0),
> 	RZ_MTU3_PWM_IO(5, 1),
> 	RZ_MTU3_PWM_IO(6, 0),
> 	RZ_MTU3_PWM_IO(6, 1),
> };
> 

Sure, I'll remove the extra macros.

> and then maybe just:
> 
> #define RZ_MTU3_NUM_PWM_CHANNELS ARRAY_SIZE(rz_mtu3_pwm_io_map)
> 

Good idea, I'll change to this.

> instead of the static_assert. But I guess this is mostly subjective and
> I won't try to convince you of my approach if you prefer your
> suggestion.
> 

...

> 
> > 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

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

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.

Thank you.