| Message ID | 20221130152148.2769768-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: Allow .get_state to fail | expand |
On Wed, 30 Nov 2022 16:21:39 +0100 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h > index cf243de41cc8..12b35e4ff917 100644 > --- a/include/trace/events/pwm.h > +++ b/include/trace/events/pwm.h > @@ -10,9 +10,9 @@ > > DECLARE_EVENT_CLASS(pwm, > > - TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state), > + TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err), > > - TP_ARGS(pwm, state), > + TP_ARGS(pwm, state, err), > > TP_STRUCT__entry( > __field(struct pwm_device *, pwm) > @@ -20,6 +20,7 @@ DECLARE_EVENT_CLASS(pwm, > __field(u64, duty_cycle) > __field(enum pwm_polarity, polarity) > __field(bool, enabled) > + __field(int, err) > ), If you are changing this, perhaps order it a bit like: TP_STRUCT__entry( __field(u64, period) __field(u64, duty_cycle) __field(struct pwm_device *, pwm) __field(enum pwm_polarity, polarity) __field(bool, enabled) __field(int, err) ), And that way the struct pwm_device pointer will not cause a 4 byte hole on 32bit architectures. > > TP_fast_assign( > @@ -28,28 +29,27 @@ DECLARE_EVENT_CLASS(pwm, > __entry->duty_cycle = state->duty_cycle; > __entry->polarity = state->polarity; > __entry->enabled = state->enabled; > + __entry->err = err; > ), > > - TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d", > + TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d", > __entry->pwm, __entry->period, __entry->duty_cycle, > - __entry->polarity, __entry->enabled) > + __entry->polarity, __entry->enabled, __entry->err) Hmm, and why not show the values here: TRACE_DEFINE_ENUM(PWM_POLARITY_NORMAL); TRACE_DEFINE_ENUM(PWM_POLARITY_INVERSED); TP_printk("%p: period=%llu duty_cycle=%llu polarity=%s enabled=%d err=%d", __entry->pwm, __entry->period, __entry->duty_cycle, __print_symbolic(__entry->polarity, {PWM_POLARITY_NORMAL, "normal"}, {PWM_POLARITY_INVERSED, "inversed"}), __entry->enabled, __entry->err) -- Steve > > );
Hello Steven, On Wed, Nov 30, 2022 at 03:15:11PM -0500, Steven Rostedt wrote: > On Wed, 30 Nov 2022 16:21:39 +0100 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h > > index cf243de41cc8..12b35e4ff917 100644 > > --- a/include/trace/events/pwm.h > > +++ b/include/trace/events/pwm.h > > @@ -10,9 +10,9 @@ > > > > DECLARE_EVENT_CLASS(pwm, > > > > - TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state), > > + TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err), > > > > - TP_ARGS(pwm, state), > > + TP_ARGS(pwm, state, err), > > > > TP_STRUCT__entry( > > __field(struct pwm_device *, pwm) > > @@ -20,6 +20,7 @@ DECLARE_EVENT_CLASS(pwm, > > __field(u64, duty_cycle) > > __field(enum pwm_polarity, polarity) > > __field(bool, enabled) > > + __field(int, err) > > ), > > If you are changing this, perhaps order it a bit like: > > TP_STRUCT__entry( > __field(u64, period) > __field(u64, duty_cycle) > __field(struct pwm_device *, pwm) > __field(enum pwm_polarity, polarity) > __field(bool, enabled) > __field(int, err) > ), > > And that way the struct pwm_device pointer will not cause a 4 byte hole on > 32bit architectures. I'd do that in a separate patch, thanks for the feedback. > > TP_fast_assign( > > @@ -28,28 +29,27 @@ DECLARE_EVENT_CLASS(pwm, > > __entry->duty_cycle = state->duty_cycle; > > __entry->polarity = state->polarity; > > __entry->enabled = state->enabled; > > + __entry->err = err; > > ), > > > > - TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d", > > + TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d", > > __entry->pwm, __entry->period, __entry->duty_cycle, > > - __entry->polarity, __entry->enabled) > > + __entry->polarity, __entry->enabled, __entry->err) > > Hmm, and why not show the values here: > > TRACE_DEFINE_ENUM(PWM_POLARITY_NORMAL); > TRACE_DEFINE_ENUM(PWM_POLARITY_INVERSED); > > TP_printk("%p: period=%llu duty_cycle=%llu polarity=%s enabled=%d err=%d", > __entry->pwm, __entry->period, __entry->duty_cycle, > __print_symbolic(__entry->polarity, > {PWM_POLARITY_NORMAL, "normal"}, > {PWM_POLARITY_INVERSED, "inversed"}), > __entry->enabled, __entry->err) Ditto. @Thierry: While the suggestions by Steven are cleanups that usually come first in a series and only then new stuff is added, I suggest to be not religious about that and implement these in patches based on this series. Independent of that, I'll wait some time to give others a change for feedback. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index d333e7422f4a..21e1553495ee 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -108,8 +108,8 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label) } if (pwm->chip->ops->get_state) { - pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state); - trace_pwm_get(pwm, &pwm->state); + err = pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state); + trace_pwm_get(pwm, &pwm->state, err); if (IS_ENABLED(CONFIG_PWM_DEBUG)) pwm->last = pwm->state; @@ -457,8 +457,8 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, * checks. */ - chip->ops->get_state(chip, pwm, &s1); - trace_pwm_get(pwm, &s1); + err = chip->ops->get_state(chip, pwm, &s1); + trace_pwm_get(pwm, &s1, err); /* * The lowlevel driver either ignored .polarity (which is a bug) or as @@ -514,16 +514,15 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, /* reapply the state that the driver reported being configured. */ err = chip->ops->apply(chip, pwm, &s1); + trace_pwm_apply(pwm, &s1, err); if (err) { *last = s1; dev_err(chip->dev, "failed to reapply current setting\n"); return; } - trace_pwm_apply(pwm, &s1); - - chip->ops->get_state(chip, pwm, last); - trace_pwm_get(pwm, last); + err = chip->ops->get_state(chip, pwm, last); + trace_pwm_get(pwm, last, err); /* reapplication of the current state should give an exact match */ if (s1.enabled != last->enabled || @@ -571,11 +570,10 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) return 0; err = chip->ops->apply(chip, pwm, state); + trace_pwm_apply(pwm, state, err); if (err) return err; - trace_pwm_apply(pwm, state); - pwm->state = *state; /* diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h index cf243de41cc8..12b35e4ff917 100644 --- a/include/trace/events/pwm.h +++ b/include/trace/events/pwm.h @@ -10,9 +10,9 @@ DECLARE_EVENT_CLASS(pwm, - TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state), + TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err), - TP_ARGS(pwm, state), + TP_ARGS(pwm, state, err), TP_STRUCT__entry( __field(struct pwm_device *, pwm) @@ -20,6 +20,7 @@ DECLARE_EVENT_CLASS(pwm, __field(u64, duty_cycle) __field(enum pwm_polarity, polarity) __field(bool, enabled) + __field(int, err) ), TP_fast_assign( @@ -28,28 +29,27 @@ DECLARE_EVENT_CLASS(pwm, __entry->duty_cycle = state->duty_cycle; __entry->polarity = state->polarity; __entry->enabled = state->enabled; + __entry->err = err; ), - TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d", + TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d", __entry->pwm, __entry->period, __entry->duty_cycle, - __entry->polarity, __entry->enabled) + __entry->polarity, __entry->enabled, __entry->err) ); DEFINE_EVENT(pwm, pwm_apply, - TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state), - - TP_ARGS(pwm, state) + TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err), + TP_ARGS(pwm, state, err) ); DEFINE_EVENT(pwm, pwm_get, - TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state), - - TP_ARGS(pwm, state) + TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err), + TP_ARGS(pwm, state, err) ); #endif /* _TRACE_PWM_H */
Record and report an error code for the events. This allows to report about failed calls without ambiguity and so gives a more complete picture. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/core.c | 18 ++++++++---------- include/trace/events/pwm.h | 20 ++++++++++---------- 2 files changed, 18 insertions(+), 20 deletions(-)