[PATCH v3 0/8] pwm: New abstraction and userspace API

[PATCH v3 0/8] pwm: New abstraction and userspace API

[Uwe Kleine-König]

Hello,

here comes v3 of the series to add support for duty offset in PWM
waveforms. For a single PWM output there is no gain, but if you have a
chip with two (or more) outputs and both operate with the same period,
you can generate an output like:


               ______         ______         ______         ______
   PWM #0  ___/      \_______/      \_______/      \_______/      \_______
                 __             __             __             __    
   PWM #1  _____/  \___________/  \___________/  \___________/  \_________
              ^              ^              ^              ^

Changes since v2, which is available from
https://lore.kernel.org/linux-pwm/cover.1721040875.git.u.kleine-koenig@baylibre.com
include:

 - Degrade a dev_alert() to dev_warn() on feedback by David Lechner

 - Improvement of various comments (partly in reply to David Lechner)

 - Add _ns suffixes for members of pwm_waveform, thanks David for that suggestion.

 - Rebased to v6.11-rc1 + pwm/for-next.

Because of these changes I didn't add Trevor's Reviewed-by tag for patch
#3.

I kept the BUG_ONs. There are a few check_patch warnings about it, but I still
prefer these given that it is safe they don't trigger without further (bogus)
code changes and when they trigger crashing early is better than stack
corruption. Also checkpatch tells
        WARNING: Comparisons should place the constant on the right side of the test
        #158: FILE: drivers/pwm/core.c:262:
        +       BUG_ON(WFHWSIZE < ops->sizeof_wfhw);

But as the BUG_ON is about WFHWSIZE being wrong, this order is OK.

There are a few more checkpatch warnings about line lengths. Breaking
the criticised lines further hurts readability IMHO, so I kept them. It
gets a bit better once stm32_pwm_mul_u64_u64_div_u64_roundup() is
implemented (without the stm32_pwm prefix) alongside
mul_u64_u64_div_u64() in lib/math/div64.c, but I don't want to wait for
that. I will address that once Nicolas's patch improving precision of
mul_u64_u64_div_u64() landed. (Hmm, it's not in next any more since
next-20240724, before it was 3cc8bf1a81efde105d8e57398cf8554b57768777 +
dbbe95af0fad2a9d22a4b910cfc4b87949d61a3c).

Best regards
Uwe

Uwe Kleine-König (8):
  pwm: Simplify pwm_capture()
  pwm: Add more locking
  pwm: New abstraction for PWM waveforms
  pwm: Provide new consumer API functions for waveforms
  pwm: Add support for pwmchip devices for faster and easier userspace
    access
  pwm: Add tracing for waveform callbacks
  pwm: axi-pwmgen: Implementation of the waveform callbacks
  pwm: stm32: Implementation of the waveform callbacks

 drivers/pwm/core.c           | 809 +++++++++++++++++++++++++++++++++--
 drivers/pwm/pwm-axi-pwmgen.c | 154 +++++--
 drivers/pwm/pwm-stm32.c      | 607 ++++++++++++++++----------
 include/linux/pwm.h          |  58 ++-
 include/trace/events/pwm.h   | 134 +++++-
 include/uapi/linux/pwm.h     |  25 ++
 6 files changed, 1479 insertions(+), 308 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

base-commit: b9b6bd3dcceed371829a022caeb6b51cb9f67be9
-- 
2.43.0

[PATCH v3 6/8] pwm: Add tracing for waveform callbacks

[Uwe Kleine-König]

This adds trace events for the recently introduced waveform callbacks.
With the introduction of some helper macros consistency among the
different events is ensured.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c         |  24 +++++--
 include/trace/events/pwm.h | 134 ++++++++++++++++++++++++++++++++++---
 2 files changed, 146 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 888cd4f51c6e..bad0c8e65f56 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -166,30 +166,46 @@ static int __pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *p
 				     const struct pwm_waveform *wf, void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+	ret = ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+	trace_pwm_round_waveform_tohw(pwm, wf, wfhw, ret);
+
+	return ret;
 }
 
 static int __pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
 				       const void *wfhw, struct pwm_waveform *wf)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+	ret = ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+	trace_pwm_round_waveform_fromhw(pwm, wfhw, wf, ret);
+
+	return ret;
 }
 
 static int __pwm_read_waveform(struct pwm_chip *chip, struct pwm_device *pwm, void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->read_waveform(chip, pwm, wfhw);
+	ret = ops->read_waveform(chip, pwm, wfhw);
+	trace_pwm_read_waveform(pwm, wfhw, ret);
+
+	return ret;
 }
 
 static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, const void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->write_waveform(chip, pwm, wfhw);
+	ret = ops->write_waveform(chip, pwm, wfhw);
+	trace_pwm_write_waveform(pwm, wfhw, ret);
+
+	return ret;
 }
 
 #define WFHWSIZE 20
diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
index 8022701c446d..8ba898fd335c 100644
--- a/include/trace/events/pwm.h
+++ b/include/trace/events/pwm.h
@@ -8,15 +8,135 @@
 #include <linux/pwm.h>
 #include <linux/tracepoint.h>
 
+#define TP_PROTO_pwm(args...)							\
+	TP_PROTO(struct pwm_device *pwm, args)
+
+#define TP_ARGS_pwm(args...)							\
+	TP_ARGS(pwm, args)
+
+#define TP_STRUCT__entry_pwm(args...)						\
+	TP_STRUCT__entry(							\
+		__field(unsigned int, chipid)					\
+		__field(unsigned int, hwpwm)					\
+		args)
+
+#define TP_fast_assign_pwm(args...)						\
+	TP_fast_assign(								\
+		__entry->chipid = pwm->chip->id;				\
+		__entry->hwpwm = pwm->hwpwm;					\
+		args)
+
+#define TP_printk_pwm(fmt, args...)						\
+	TP_printk("pwmchip%u.%u: " fmt, __entry->chipid, __entry->hwpwm, args)
+
+#define __field_pwmwf(wf)							\
+	__field(u64, wf ## _period_length_ns)					\
+	__field(u64, wf ## _duty_length_ns)					\
+	__field(u64, wf ## _duty_offset_ns)					\
+
+#define fast_assign_pwmwf(wf)							\
+	__entry->wf ## _period_length_ns = wf->period_length_ns;		\
+	__entry->wf ## _duty_length_ns = wf->duty_length_ns;			\
+	__entry->wf ## _duty_offset_ns = wf->duty_offset_ns
+
+#define printk_pwmwf_format(wf)							\
+	"%lld/%lld [+%lld]"
+
+#define printk_pwmwf_formatargs(wf)						\
+	__entry->wf ## _duty_length_ns, __entry->wf ## _period_length_ns, __entry->wf ## _duty_offset_ns
+
+TRACE_EVENT(pwm_round_waveform_tohw,
+
+	TP_PROTO_pwm(const struct pwm_waveform *wf, void *wfhw, int err),
+
+	TP_ARGS_pwm(wf, wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field_pwmwf(wf)
+		__field(void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		fast_assign_pwmwf(wf);
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm(printk_pwmwf_format(wf) " > %p err=%d",
+		printk_pwmwf_formatargs(wf), __entry->wfhw, __entry->err)
+);
+
+TRACE_EVENT(pwm_round_waveform_fromhw,
+
+	TP_PROTO_pwm(const void *wfhw, struct pwm_waveform *wf, int err),
+
+	TP_ARGS_pwm(wfhw, wf, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(const void *, wfhw)
+		__field_pwmwf(wf)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		fast_assign_pwmwf(wf);
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p > " printk_pwmwf_format(wf) " err=%d",
+		__entry->wfhw, printk_pwmwf_formatargs(wf), __entry->err)
+);
+
+TRACE_EVENT(pwm_read_waveform,
+
+	TP_PROTO_pwm(void *wfhw, int err),
+
+	TP_ARGS_pwm(wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p err=%d",
+		__entry->wfhw, __entry->err)
+);
+
+TRACE_EVENT(pwm_write_waveform,
+
+	TP_PROTO_pwm(const void *wfhw, int err),
+
+	TP_ARGS_pwm(wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(const void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p err=%d",
+		__entry->wfhw, __entry->err)
+);
+
+
 DECLARE_EVENT_CLASS(pwm,
 
 	TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err),
 
 	TP_ARGS(pwm, state, err),
 
-	TP_STRUCT__entry(
-		__field(unsigned int, chipid)
-		__field(unsigned int, hwpwm)
+	TP_STRUCT__entry_pwm(
 		__field(u64, period)
 		__field(u64, duty_cycle)
 		__field(enum pwm_polarity, polarity)
@@ -24,9 +144,7 @@ DECLARE_EVENT_CLASS(pwm,
 		__field(int, err)
 	),
 
-	TP_fast_assign(
-		__entry->chipid = pwm->chip->id;
-		__entry->hwpwm = pwm->hwpwm;
+	TP_fast_assign_pwm(
 		__entry->period = state->period;
 		__entry->duty_cycle = state->duty_cycle;
 		__entry->polarity = state->polarity;
@@ -34,8 +152,8 @@ DECLARE_EVENT_CLASS(pwm,
 		__entry->err = err;
 	),
 
-	TP_printk("pwmchip%u.%u: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d",
-		  __entry->chipid, __entry->hwpwm, __entry->period, __entry->duty_cycle,
+	TP_printk_pwm("period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d",
+		  __entry->period, __entry->duty_cycle,
 		  __entry->polarity, __entry->enabled, __entry->err)
 
 );
-- 
2.43.0

Re: [PATCH v3 6/8] pwm: Add tracing for waveform callbacks

[Steven Rostedt]

On Mon, 29 Jul 2024 16:34:22 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
> index 8022701c446d..8ba898fd335c 100644
> --- a/include/trace/events/pwm.h
> +++ b/include/trace/events/pwm.h
> @@ -8,15 +8,135 @@
>  #include <linux/pwm.h>
>  #include <linux/tracepoint.h>
>  
> +#define TP_PROTO_pwm(args...)							\
> +	TP_PROTO(struct pwm_device *pwm, args)
> +
> +#define TP_ARGS_pwm(args...)							\
> +	TP_ARGS(pwm, args)
> +
> +#define TP_STRUCT__entry_pwm(args...)						\
> +	TP_STRUCT__entry(							\
> +		__field(unsigned int, chipid)					\
> +		__field(unsigned int, hwpwm)					\
> +		args)
> +
> +#define TP_fast_assign_pwm(args...)						\
> +	TP_fast_assign(								\
> +		__entry->chipid = pwm->chip->id;				\
> +		__entry->hwpwm = pwm->hwpwm;					\
> +		args)
> +
> +#define TP_printk_pwm(fmt, args...)						\
> +	TP_printk("pwmchip%u.%u: " fmt, __entry->chipid, __entry->hwpwm, args)
> +
> +#define __field_pwmwf(wf)							\
> +	__field(u64, wf ## _period_length_ns)					\
> +	__field(u64, wf ## _duty_length_ns)					\
> +	__field(u64, wf ## _duty_offset_ns)					\
> +
> +#define fast_assign_pwmwf(wf)							\
> +	__entry->wf ## _period_length_ns = wf->period_length_ns;		\
> +	__entry->wf ## _duty_length_ns = wf->duty_length_ns;			\
> +	__entry->wf ## _duty_offset_ns = wf->duty_offset_ns
> +
> +#define printk_pwmwf_format(wf)							\
> +	"%lld/%lld [+%lld]"
> +
> +#define printk_pwmwf_formatargs(wf)						\
> +	__entry->wf ## _duty_length_ns, __entry->wf ## _period_length_ns, __entry->wf ## _duty_offset_ns
> +

The above is interesting. But if it works for you, then you do you ;-)

-- Steve


> +TRACE_EVENT(pwm_round_waveform_tohw,
> +
> +	TP_PROTO_pwm(const struct pwm_waveform *wf, void *wfhw, int err),
> +
> +	TP_ARGS_pwm(wf, wfhw, err),
> +
> +	TP_STRUCT__entry_pwm(
> +		__field_pwmwf(wf)
> +		__field(void *, wfhw)
> +		__field(int, err)
> +	),
> +
> +	TP_fast_assign_pwm(
> +		fast_assign_pwmwf(wf);
> +		__entry->wfhw = wfhw;
> +		__entry->err = err;
> +	),
> +
> +	TP_printk_pwm(printk_pwmwf_format(wf) " > %p err=%d",
> +		printk_pwmwf_formatargs(wf), __entry->wfhw, __entry->err)
> +);
> +

Re: [PATCH v3 6/8] pwm: Add tracing for waveform callbacks

[Uwe Kleine-König]

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

On Tue, Jul 30, 2024 at 10:12:46AM -0400, Steven Rostedt wrote:
> On Mon, 29 Jul 2024 16:34:22 +0200
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
> > index 8022701c446d..8ba898fd335c 100644
> > --- a/include/trace/events/pwm.h
> > +++ b/include/trace/events/pwm.h
> > @@ -8,15 +8,135 @@
> >  #include <linux/pwm.h>
> >  #include <linux/tracepoint.h>
> >  
> > +#define TP_PROTO_pwm(args...)							\
> > +	TP_PROTO(struct pwm_device *pwm, args)
> > +
> > +#define TP_ARGS_pwm(args...)							\
> > +	TP_ARGS(pwm, args)
> > +
> > +#define TP_STRUCT__entry_pwm(args...)						\
> > +	TP_STRUCT__entry(							\
> > +		__field(unsigned int, chipid)					\
> > +		__field(unsigned int, hwpwm)					\
> > +		args)
> > +
> > +#define TP_fast_assign_pwm(args...)						\
> > +	TP_fast_assign(								\
> > +		__entry->chipid = pwm->chip->id;				\
> > +		__entry->hwpwm = pwm->hwpwm;					\
> > +		args)
> > +
> > +#define TP_printk_pwm(fmt, args...)						\
> > +	TP_printk("pwmchip%u.%u: " fmt, __entry->chipid, __entry->hwpwm, args)
> > +
> > +#define __field_pwmwf(wf)							\
> > +	__field(u64, wf ## _period_length_ns)					\
> > +	__field(u64, wf ## _duty_length_ns)					\
> > +	__field(u64, wf ## _duty_offset_ns)					\
> > +
> > +#define fast_assign_pwmwf(wf)							\
> > +	__entry->wf ## _period_length_ns = wf->period_length_ns;		\
> > +	__entry->wf ## _duty_length_ns = wf->duty_length_ns;			\
> > +	__entry->wf ## _duty_offset_ns = wf->duty_offset_ns
> > +
> > +#define printk_pwmwf_format(wf)							\
> > +	"%lld/%lld [+%lld]"
> > +
> > +#define printk_pwmwf_formatargs(wf)						\
> > +	__entry->wf ## _duty_length_ns, __entry->wf ## _period_length_ns, __entry->wf ## _duty_offset_ns
> > +
> 
> The above is interesting. But if it works for you, then you do you ;-)

Thanks for the feedback. (I just pointed you to this thread in reply to
your other mail. Didn't notice you already replied here. Consider this
done.)

Best regards
Uwe

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

Re: [PATCH v3 0/8] pwm: New abstraction and userspace API

[Uwe Kleine-König]

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

On Mon, Jul 29, 2024 at 04:34:16PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> here comes v3 of the series to add support for duty offset in PWM
> waveforms. For a single PWM output there is no gain, but if you have a
> chip with two (or more) outputs and both operate with the same period,
> you can generate an output like:
> 
> 
>                ______         ______         ______         ______
>    PWM #0  ___/      \_______/      \_______/      \_______/      \_______
>                  __             __             __             __    
>    PWM #1  _____/  \___________/  \___________/  \___________/  \_________
>               ^              ^              ^              ^
> 
> Changes since v2, which is available from
> https://lore.kernel.org/linux-pwm/cover.1721040875.git.u.kleine-koenig@baylibre.com
> include:
> 
>  - Degrade a dev_alert() to dev_warn() on feedback by David Lechner
> 
>  - Improvement of various comments (partly in reply to David Lechner)
> 
>  - Add _ns suffixes for members of pwm_waveform, thanks David for that suggestion.
> 
>  - Rebased to v6.11-rc1 + pwm/for-next.
> 
> Because of these changes I didn't add Trevor's Reviewed-by tag for patch
> #3.
> 
> I kept the BUG_ONs. There are a few check_patch warnings about it, but I still
> prefer these given that it is safe they don't trigger without further (bogus)
> code changes and when they trigger crashing early is better than stack
> corruption. Also checkpatch tells
>         WARNING: Comparisons should place the constant on the right side of the test
>         #158: FILE: drivers/pwm/core.c:262:
>         +       BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> 
> But as the BUG_ON is about WFHWSIZE being wrong, this order is OK.
> 
> There are a few more checkpatch warnings about line lengths. Breaking
> the criticised lines further hurts readability IMHO, so I kept them. It
> gets a bit better once stm32_pwm_mul_u64_u64_div_u64_roundup() is
> implemented (without the stm32_pwm prefix) alongside
> mul_u64_u64_div_u64() in lib/math/div64.c, but I don't want to wait for
> that. I will address that once Nicolas's patch improving precision of
> mul_u64_u64_div_u64() landed. (Hmm, it's not in next any more since
> next-20240724, before it was 3cc8bf1a81efde105d8e57398cf8554b57768777 +
> dbbe95af0fad2a9d22a4b910cfc4b87949d61a3c).
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (8):
>   pwm: Simplify pwm_capture()
>   pwm: Add more locking
>   pwm: New abstraction for PWM waveforms
>   pwm: Provide new consumer API functions for waveforms
>   pwm: Add support for pwmchip devices for faster and easier userspace
>     access
>   pwm: Add tracing for waveform callbacks
>   pwm: axi-pwmgen: Implementation of the waveform callbacks
>   pwm: stm32: Implementation of the waveform callbacks

I applied patch #1 which is a harmless cleanup for now. I will wait a
bit for the rest of the series, as during August I won't be able to
react to fall-outs reliably and quickly. I plan to apply this series
with PWM_IOCTL_GET_NUM_PWMS dropped directly after the next merge window
for cooking in next as long as possible.

Best regards
Uwe

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