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

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

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

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),
};

and then maybe just:

#define RZ_MTU3_NUM_PWM_CHANNELS ARRAY_SIZE(rz_mtu3_pwm_io_map)

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 unsigned int rz_mtu3_hwpwm_ch(u32 hwpwm)
> {
> 	return rz_mtu3_pwm_io_map[hwpwm] >> 1;
> }
> 
> static bool rz_mtu3_hwpwm_is_primary(u32 hwpwm)
> {
> 	return !(rz_mtu3_pwm_io_map[hwpwm] & 1);
> }
> 
> static struct rz_mtu3_pwm_channel *
> rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> {
> 	unsigned int ch = rz_mtu3_hwpwm_ch(hwpwm);
> 
> 	return &rz_mtu3_pwm->channel_data[ch];
> }
> 
> This gets rid of the loop inside rz_mtu3_get_channel() quite nicely.
> 
> priv->map->base_pwm_number == hwpwm checks will in turn be reduced to
> rz_mtu3_hwpwm_is_primary(hwpwm).
> 
> If you decide that we should keep the sibling check inside
> rz_mtu3_pwm_config() as-is then we can do the following, without having
> to encode the number of channels for each HW channel explicitly.
> 
> Please note that hwpwm + 1 is fine in this case as the last hwpwm of
> rz_mtu3_pwm_io_map is never primary.

This needs a comment plus (if possible) an assert.

> static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
> {
> 	if (!rz_mtu3_hwpwm_is_primary(hwpwm))
> 		return hwpwm - 1;
> 
> 	if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
> 		return -EINVAL;
> 
> 	return hwpwm + 1;
> }
> 
> Although, I would much rather have the following logic rather than the
> sibling check, which matches one of the alternatives you proposed earlier.
> 
> Hopefully, the comment explains the situation well.

I got it, thanks.

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

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. The alternative would be to check the hardware for
being on or not. But there are many users of this member, that I also
won't yell at you for also using it.

Best regards
Uwe

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