| Message ID | 20180803140029.29810-1-matthieu.castet@parrot.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: Add tracepoint for apply state | expand |
On Fri, Aug 03, 2018 at 04:00:29PM +0200, Matthieu CASTET wrote: > This allows to trace pwm with ftrace > > Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com> > --- > drivers/pwm/core.c | 5 ++++ > include/trace/events/pwm.h | 48 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) > create mode 100644 include/trace/events/pwm.h I like this. A couple of comments below. > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 1581f6ab1b1f..4342974af31a 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -32,6 +32,9 @@ > > #include <dt-bindings/pwm/pwm.h> > > +#define CREATE_TRACE_POINTS > +#include <trace/events/pwm.h> > + > #define MAX_PWMS 1024 > > static DEFINE_MUTEX(pwm_lookup_lock); > @@ -475,6 +478,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > if (!memcmp(state, &pwm->state, sizeof(*state))) > return 0; > > + trace_pwm_apply_state(pwm, state); > + > if (pwm->chip->ops->apply) { > err = pwm->chip->ops->apply(pwm->chip, pwm, state); > if (err) > diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h > new file mode 100644 > index 000000000000..0c28a76b1f60 > --- /dev/null > +++ b/include/trace/events/pwm.h > @@ -0,0 +1,48 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM pwm > + > +#if !defined(_TRACE_PWM_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_PWM_H > + > +#include <linux/pwm.h> > +#include <linux/tracepoint.h> > + > + > +TRACE_EVENT(pwm_apply_state, > + > + TP_PROTO(struct pwm_device *pwm, struct pwm_state *state), > + > + TP_ARGS(pwm, state), > + > + TP_STRUCT__entry( > + __string(name, pwm->label) > + __field(struct pwm_device *, pwm) > + __field(unsigned int, num) If we already carry a pointer to the struct pwm_device, why store separate copies of the name and number? > + __field(unsigned int, period) > + __field(unsigned int, duty_cycle) > + __field(enum pwm_polarity, polarity) > + __field(bool, enabled) > + ), > + > + TP_fast_assign( > + __assign_str(name, pwm->label); > + __entry->pwm = pwm; > + __entry->num = pwm->pwm; > + __entry->period = state->period; > + __entry->duty_cycle = state->duty_cycle; > + __entry->polarity = state->polarity; > + __entry->enabled = state->enabled; > + ), > + > + TP_printk("%s: apply state struct pwm[%p]: pwm%u " > + "enabled=%d polarity=%u period_ns=%u duty_ns=%u", > + __get_str(name), __entry->pwm, __entry->num, > + __entry->enabled, __entry->polarity, > + __entry->period, __entry->duty_cycle) Note that not all PWMs have a label assigned to them. Output will look rather messy if the label is NULL. Also, printing the pointer value seems somewhat unnecessary to me. There's not really anything we can derive from that value. Perhaps a better identifier could be derived from the chip's name and the PWM index concatenated. Something along these lines: TP_printk("applying state to PWM %s.%u: ...", dev_name(__entry->pwm->chip->dev), __entry->pwm->hwpwm, ...); ? Thierry
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1581f6ab1b1f..4342974af31a 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -32,6 +32,9 @@ #include <dt-bindings/pwm/pwm.h> +#define CREATE_TRACE_POINTS +#include <trace/events/pwm.h> + #define MAX_PWMS 1024 static DEFINE_MUTEX(pwm_lookup_lock); @@ -475,6 +478,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) if (!memcmp(state, &pwm->state, sizeof(*state))) return 0; + trace_pwm_apply_state(pwm, state); + if (pwm->chip->ops->apply) { err = pwm->chip->ops->apply(pwm->chip, pwm, state); if (err) diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h new file mode 100644 index 000000000000..0c28a76b1f60 --- /dev/null +++ b/include/trace/events/pwm.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM pwm + +#if !defined(_TRACE_PWM_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_PWM_H + +#include <linux/pwm.h> +#include <linux/tracepoint.h> + + +TRACE_EVENT(pwm_apply_state, + + TP_PROTO(struct pwm_device *pwm, struct pwm_state *state), + + TP_ARGS(pwm, state), + + TP_STRUCT__entry( + __string(name, pwm->label) + __field(struct pwm_device *, pwm) + __field(unsigned int, num) + __field(unsigned int, period) + __field(unsigned int, duty_cycle) + __field(enum pwm_polarity, polarity) + __field(bool, enabled) + ), + + TP_fast_assign( + __assign_str(name, pwm->label); + __entry->pwm = pwm; + __entry->num = pwm->pwm; + __entry->period = state->period; + __entry->duty_cycle = state->duty_cycle; + __entry->polarity = state->polarity; + __entry->enabled = state->enabled; + ), + + TP_printk("%s: apply state struct pwm[%p]: pwm%u " + "enabled=%d polarity=%u period_ns=%u duty_ns=%u", + __get_str(name), __entry->pwm, __entry->num, + __entry->enabled, __entry->polarity, + __entry->period, __entry->duty_cycle) +); + +#endif /* _TRACE_PWM_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h>
This allows to trace pwm with ftrace Signed-off-by: Matthieu Castet <matthieu.castet@parrot.com> --- drivers/pwm/core.c | 5 ++++ include/trace/events/pwm.h | 48 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 include/trace/events/pwm.h