Re: [PATCH v2] iio: cros_ec: Trace EC sensors command

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 16 Jan 2025 20:20:59 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:

> Add tracing for EC_CMD_MOTION_SENSE_CMD command:
> - decode the name of the subcommand
> - provide internal information for the most common sub-commands:
>   setting range, frequency, EC probing frequency, ...
> - display return status.
> 
> When enabled, the tracing output is similar to:
> /sys/kernel/debug/tracing # echo 1 > events/cros_ec/enable ; echo 1 > tracing_on ; cat trace_pipe | grep MOTIONSENSE_CMD_SENSOR_ODR
>  SensorDeviceImp-814     [003] .....   686.176782: cros_ec_motion_host_cmd: MOTIONSENSE_CMD_SENSOR_ODR, id: 1, data: 200000, result: 4, return: 12500
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
What this is missing is why?

I'm not against adding tracepoints in drivers, but some statement of
the usecase etc would be helpful. If it's debug only that makes
for different requirements than some tool is going to connect to
this stream.

Jonathan


> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 9fc71a73caa12..075196ca804a1 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -23,6 +23,8 @@
>  #include <linux/platform_data/cros_ec_sensorhub.h>
>  #include <linux/platform_device.h>
>  
> +#include "cros_ec_sensors_trace.h"
> +
>  /*
>   * Hard coded to the first device to support sensor fifo.  The EC has a 2048
>   * byte fifo and will trigger an interrupt when fifo is 2/3 full.
> @@ -423,6 +425,7 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
>  	memcpy(state->msg->data, &state->param, sizeof(state->param));
>  
>  	ret = cros_ec_cmd_xfer_status(state->ec, state->msg);
> +	trace_cros_ec_motion_host_cmd(&state->param, (void *)state->msg->data, ret);

Can we cast to the right type rather than a void *?

>  	if (ret < 0)
>  		return ret;
>