Re: [PATCH v2] i2c: add tracepoints for I2C slave events

[Wolfram Sang <wsa@kernel.org>]

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

Hi,

On Tue, Mar 08, 2022 at 08:33:33AM -0800, Jae Hyun Yoo wrote:
> I2C slave events tracepoints can be enabled by:
> 
> 	echo 1 > /sys/kernel/tracing/events/i2c_slave/enable
> 
> and logs in /sys/kernel/tracing/trace will look like:
> 
> 	... i2c_slave: i2c-0 a=010 WR_REQ []
> 	... i2c_slave: i2c-0 a=010 WR_RCV [02]
> 	... i2c_slave: i2c-0 a=010 WR_RCV [0c]
> 	... i2c_slave: i2c-0 a=010   STOP []
> 	... i2c_slave: i2c-0 a=010 RD_REQ [04]
> 	... i2c_slave: i2c-0 a=010 RD_PRO [b4]
> 	... i2c_slave: i2c-0 a=010   STOP []
> 
> formatted as:
> 
> 	i2c-<adapter-nr>
> 	a=<addr>
> 	<event>
> 	[<data>]
> 
> trace printings can be selected by adding a filter like:
> 
> 	echo adapter_nr==1 >/sys/kernel/tracing/events/i2c_slave/filter
> 
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>

Steven, are you happy with the tracepoint parts of this patch?


> +	if (trace_i2c_slave_enabled() && !ret)
> +		trace_i2c_slave(client, event, val);

Why '!ret'? I think we should always print 'ret' in the trace as well.
Backends are allowed to send errnos on WRITE_RECEIVED to NACK the
reception of a byte. This is useful information, too, or?

Rest looks good to me.

Thanks,

   Wolfram


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