* [PATCH v2] iio: cros_ec: Trace EC sensors command
@ 2025-01-17 4:20 Gwendal Grignou
2025-01-17 15:47 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Gwendal Grignou @ 2025-01-17 4:20 UTC (permalink / raw)
To: jic23; +Cc: tzungbi, linux-iio, Gwendal Grignou
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>
---
Changes from v1 (https://patchwork.kernel.org/patch/13942368/):
- fix merging issue with iio.git/togreg.
drivers/iio/common/cros_ec_sensors/Makefile | 3 +-
.../cros_ec_sensors/cros_ec_sensors_core.c | 3 +
.../cros_ec_sensors/cros_ec_sensors_trace.c | 32 +++++++++++
.../cros_ec_sensors/cros_ec_sensors_trace.h | 56 +++++++++++++++++++
4 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.c
create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.h
diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
index e0a33ab66d21a..c358fa0328abd 100644
--- a/drivers/iio/common/cros_ec_sensors/Makefile
+++ b/drivers/iio/common/cros_ec_sensors/Makefile
@@ -3,6 +3,7 @@
# Makefile for sensors seen through the ChromeOS EC sensor hub.
#
-obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros_ec_sensors_core.o
+cros-ec-sensors-core-objs += cros_ec_sensors_core.o cros_ec_sensors_trace.o
+obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros-ec-sensors-core.o
obj-$(CONFIG_IIO_CROS_EC_SENSORS) += cros_ec_sensors.o
obj-$(CONFIG_IIO_CROS_EC_SENSORS_LID_ANGLE) += cros_ec_lid_angle.o
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);
if (ret < 0)
return ret;
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.c
new file mode 100644
index 0000000000000..c4db949fa7750
--- /dev/null
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+// Trace events for the ChromeOS Embedded Controller
+//
+// Copyright 2025 Google LLC.
+
+#define TRACE_SYMBOL(a) {a, #a}
+
+// Generate the list using the following script:
+// sed -n 's/^.*\(MOTIONSENSE_CMD.*\) = .*,$/\tTRACE_SYMBOL(\1), \\/p' include/linux/platform_data/cros_ec_commands.h
+#define MOTIONSENSE_CMDS \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_DUMP), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_INFO), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_EC_RATE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SENSOR_ODR), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SENSOR_RANGE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_KB_WAKE_ANGLE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_DATA), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_FIFO_INFO), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_FIFO_FLUSH), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_FIFO_READ), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_PERFORM_CALIB), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SENSOR_OFFSET), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_LIST_ACTIVITIES), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SET_ACTIVITY), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_LID_ANGLE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_FIFO_INT_ENABLE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SPOOF), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_TABLET_MODE_LID_ANGLE), \
+ TRACE_SYMBOL(MOTIONSENSE_CMD_SENSOR_SCALE)
+
+#define CREATE_TRACE_POINTS
+#include "cros_ec_sensors_trace.h"
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.h b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.h
new file mode 100644
index 0000000000000..61853e410e96c
--- /dev/null
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_trace.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Trace events for the ChromeOS Embedded Controller
+ *
+ * Copyright 2025 Google LLC.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM cros_ec
+
+#if !defined(_CROS_EC_SENSORS_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
+#define _CROS_EC_SENSORS_TRACE_H_
+
+#include <linux/bits.h>
+#include <linux/types.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(cros_ec_motion_host_cmd,
+ TP_PROTO(struct ec_params_motion_sense *param,
+ struct ec_response_motion_sense *resp,
+ int retval),
+ TP_ARGS(param, resp, retval),
+ TP_STRUCT__entry(__field(uint8_t, cmd)
+ __field(uint8_t, sensor_id)
+ __field(uint32_t, data)
+ __field(int, retval)
+ __field(int32_t, ret)
+ ),
+ TP_fast_assign(__entry->cmd = param->cmd;
+ __entry->sensor_id = param->sensor_odr.sensor_num;
+ __entry->data = param->sensor_odr.data;
+ __entry->retval = retval;
+ __entry->ret = retval > 0 ? resp->sensor_odr.ret : -1;
+ ),
+ TP_printk("%s, id: %d, data: %u, result: %u, return: %d",
+ __print_symbolic(__entry->cmd, MOTIONSENSE_CMDS),
+ __entry->sensor_id,
+ __entry->data,
+ __entry->retval,
+ __entry->ret)
+);
+
+#endif /* _CROS_EC_SENSORS_TRACE_H_ */
+
+/* this part must be outside header guard */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE cros_ec_sensors_trace
+
+#include <trace/define_trace.h>
--
2.48.0.rc2.279.g1de40edade-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] iio: cros_ec: Trace EC sensors command
2025-01-17 4:20 [PATCH v2] iio: cros_ec: Trace EC sensors command Gwendal Grignou
@ 2025-01-17 15:47 ` Jonathan Cameron
2025-01-17 23:02 ` Gwendal Grignou
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2025-01-17 15:47 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: jic23, tzungbi, linux-iio
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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] iio: cros_ec: Trace EC sensors command
2025-01-17 15:47 ` Jonathan Cameron
@ 2025-01-17 23:02 ` Gwendal Grignou
0 siblings, 0 replies; 3+ messages in thread
From: Gwendal Grignou @ 2025-01-17 23:02 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: jic23, tzungbi, linux-iio
On Fri, Jan 17, 2025 at 7:47 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> 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.
It is for debug only, will update the commit message.
>
> 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 *?
Fixed in v3: `data` is a char array, but in the context of
cros_ec_motion_send_host_cmd(), it is a
>
> > if (ret < 0)
> > return ret;
> >
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-01-17 23:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-17 4:20 [PATCH v2] iio: cros_ec: Trace EC sensors command Gwendal Grignou
2025-01-17 15:47 ` Jonathan Cameron
2025-01-17 23:02 ` Gwendal Grignou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox