Re: [PATCH v2 02/11] pwm/tracing: Also record trace events for failed API calls

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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