pwm: stm32: Mark capture support as broken

As described in the added comment, calling capture on one channel stops
all PWM output on the other channels. While it probably isn't hard to
fix, I cannot test it. Also this bug supports my guess that capture
isn't used in practise. So instead of an untested fix, mark capture
support as broken. If indeed there are users this will hopefully make
them speak up and provide a tested fix.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stm32.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)


base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86

On 11/15/23 22:18, Uwe Kleine-König wrote:
> As described in the added comment, calling capture on one channel stops
> all PWM output on the other channels. While it probably isn't hard to
> fix, I cannot test it. Also this bug supports my guess that capture

Hi Uwe,

No, there's a check in stm32_pwm_capture() not to break a running PWM
channel:

	if (active_channels(priv)) {
		ret = -EBUSY;
		goto unlock;
	}

> isn't used in practise. So instead of an untested fix, mark capture
> support as broken. If indeed there are users this will hopefully make
> them speak up and provide a tested fix.

That looks like a revival of earlier discussion[1] to deprecate the
capture interface[1].

[1]
https://lore.kernel.org/linux-pwm/Yz%2F4V0gH%2FvrWSS8U@orome/T/#mc5fb080c2576e3c2558a646dc5db40f6ff00c9b0

Please notice there's a on-going work to move on the counter interface
to support capture[1]. There's still some pending work & review at my
end to address.
So I don't agree to mark this as broken. For now, there may be users
without alternative.

Best Regards,
Fabrice

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stm32.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 3303a754ea02..20705bc814f1 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -488,7 +488,13 @@ static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  static const struct pwm_ops stm32pwm_ops = {
>  	.apply = stm32_pwm_apply_locked,
> -	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
> +	/*
> +	 * stm32_pwm_raw_capture() (which is called by stm32_pwm_capture())
> +	 * unconditionally clears TIM_CR1_CEN. So capturing on one channel
> +	 * results in stopping all PWM outputs on the other channels of the same
> +	 * chip.
> +	 */
> +	.capture = IS_ENABLED(CONFIG_BROKEN) && IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
>  };
>  
>  static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
> 
> base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86

Hello Fabrice,

On Thu, Nov 16, 2023 at 03:59:48PM +0100, Fabrice Gasnier wrote:
> On 11/15/23 22:18, Uwe Kleine-König wrote:
> > As described in the added comment, calling capture on one channel stops
> > all PWM output on the other channels. While it probably isn't hard to
> > fix, I cannot test it. Also this bug supports my guess that capture
> 
> No, there's a check in stm32_pwm_capture() not to break a running PWM
> channel:
> 
> 	if (active_channels(priv)) {
> 		ret = -EBUSY;
> 		goto unlock;
> 	}

Ahh, indeed. Is this a hardware limitation, or could capturing also work
without toggling TIM_CR1_CEN?

> > isn't used in practise. So instead of an untested fix, mark capture
> > support as broken. If indeed there are users this will hopefully make
> > them speak up and provide a tested fix.
> 
> That looks like a revival of earlier discussion[1] to deprecate the
> capture interface[1].
> 
> [1]
> https://lore.kernel.org/linux-pwm/Yz%2F4V0gH%2FvrWSS8U@orome/T/#mc5fb080c2576e3c2558a646dc5db40f6ff00c9b0

Yes, I still think capturing is an alien part of the PWM framework.

> Please notice there's a on-going work to move on the counter interface
> to support capture[1]. There's still some pending work & review at my
> end to address.
> So I don't agree to mark this as broken. For now, there may be users
> without alternative.

OK, I'll discard my patch from my mailbox and patchwork.

Thanks for the feedback,
Uwe