* [PATCH] iio: cros_ec_sensors: add cros_ec_activity driver
@ 2025-05-20 4:01 Gwendal Grignou
2025-05-21 7:16 ` Tzung-Bi Shih
0 siblings, 1 reply; 2+ messages in thread
From: Gwendal Grignou @ 2025-05-20 4:01 UTC (permalink / raw)
To: jic23; +Cc: tzungbi, chrome-platform, linux-iio, Gwendal Grignou
ChromeOS EC can report activity information derived from the
accelerometer:
- reports on-body/off-body as a proximity event.
- reports significant motion as an activity event.
This new sensor is a virtual sensor, included only when the EC firmware
is compiled with the appropriate module.
Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
drivers/iio/common/cros_ec_sensors/Kconfig | 10 +
drivers/iio/common/cros_ec_sensors/Makefile | 1 +
.../common/cros_ec_sensors/cros_ec_activity.c | 330 ++++++++++++++++++
.../cros_ec_sensors/cros_ec_sensors_core.c | 10 +
.../linux/iio/common/cros_ec_sensors_core.h | 1 +
.../linux/platform_data/cros_ec_commands.h | 36 +-
6 files changed, 382 insertions(+), 6 deletions(-)
create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
index fefad9572790..7ef925850a62 100644
--- a/drivers/iio/common/cros_ec_sensors/Kconfig
+++ b/drivers/iio/common/cros_ec_sensors/Kconfig
@@ -30,3 +30,13 @@ config IIO_CROS_EC_SENSORS_LID_ANGLE
convertible devices.
This module is loaded when the EC can calculate the angle between the base
and the lid.
+
+config IIO_CROS_EC_ACTIVITY
+ tristate "ChromeOS EC Activity Sensors"
+ depends on IIO_CROS_EC_SENSORS_CORE
+ help
+ Module to handle activity events detections presented by the ChromeOS
+ EC Sensor hub.
+ Activities can be a proximity detector (on body/off body detection)
+ or an activity to trigger an event on significant motion.
+ Creates an IIO device to manage all activities.
diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
index e0a33ab66d21..a7849c52f3c8 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_ACTIVITY) += cros_ec_activity.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_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
new file mode 100644
index 000000000000..01e4323abd36
--- /dev/null
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+/*
+ * cros_ec_sensors_activity - Driver for activities/gesture recognition.
+ *
+ * Copyright 2025 Google, Inc
+ *
+ * This driver uses the cros-ec interface to communicate with the Chrome OS
+ * EC about activity data. Accelerometer access is presented through iio sysfs.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/iio/common/cros_ec_sensors_core.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define DRV_NAME "cros-ec-activity"
+
+/* state data for ec_sensors iio driver. */
+struct cros_ec_sensors_state {
+ /* Shared by all sensors */
+ struct cros_ec_sensors_core_state core;
+
+ struct iio_chan_spec *channels;
+
+ int body_detection_channel_index;
+ int sig_motion_channel_index;
+};
+
+static const struct iio_event_spec cros_ec_activity_single_shot[] = {
+ {
+ .type = IIO_EV_TYPE_CHANGE,
+ /* significant motion trigger when we get out of still. */
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+static const struct iio_event_spec cros_ec_body_detect_events[] = {
+ {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+static int ec_sensors_read(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&st->core.cmd_lock);
+ if (chan->type == IIO_PROXIMITY &&
+ mask == IIO_CHAN_INFO_RAW) {
+ st->core.param.cmd = MOTIONSENSE_CMD_GET_ACTIVITY;
+ st->core.param.get_activity.activity =
+ MOTIONSENSE_ACTIVITY_BODY_DETECTION;
+ if (cros_ec_motion_send_host_cmd(&st->core, 0) !=
+ EC_RES_SUCCESS) {
+ ret = -EIO;
+ } else {
+ *val = st->core.resp->get_activity.state;
+ ret = IIO_VAL_INT;
+ }
+ } else {
+ ret = -EINVAL;
+ }
+ mutex_unlock(&st->core.cmd_lock);
+ return ret;
+}
+
+static int cros_ec_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ mutex_lock(&st->core.cmd_lock);
+ st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
+ ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+ if (ret)
+ goto done;
+ switch (chan->type) {
+ case IIO_PROXIMITY:
+ ret = !!(st->core.resp->list_activities.enabled &
+ (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
+ break;
+ case IIO_ACTIVITY:
+ if (chan->channel2 == IIO_MOD_STILL) {
+ ret = !!(st->core.resp->list_activities.enabled &
+ (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
+ } else {
+ dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
+ chan->channel2);
+ ret = -EINVAL;
+ }
+ break;
+ default:
+ dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
+ chan->type);
+ ret = -EINVAL;
+ }
+done:
+ mutex_unlock(&st->core.cmd_lock);
+ return ret;
+}
+
+static int cros_ec_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state)
+{
+ struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (chan->type != IIO_ACTIVITY && chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ mutex_lock(&st->core.cmd_lock);
+ st->core.param.cmd = MOTIONSENSE_CMD_SET_ACTIVITY;
+ switch (chan->type) {
+ case IIO_PROXIMITY:
+ st->core.param.set_activity.activity =
+ MOTIONSENSE_ACTIVITY_BODY_DETECTION;
+ break;
+ case IIO_ACTIVITY:
+ if (chan->channel2 == IIO_MOD_STILL) {
+ st->core.param.set_activity.activity =
+ MOTIONSENSE_ACTIVITY_SIG_MOTION;
+ } else {
+ dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
+ chan->channel2);
+ }
+ break;
+ default:
+ dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
+ chan->type);
+ }
+ st->core.param.set_activity.enable = state;
+
+ ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+
+ mutex_unlock(&st->core.cmd_lock);
+ return ret;
+}
+
+static int cros_ec_activity_push_data(struct iio_dev *indio_dev,
+ s16 *data,
+ s64 timestamp)
+{
+ struct ec_response_activity_data *activity_data =
+ (struct ec_response_activity_data *)data;
+ enum motionsensor_activity activity = activity_data->activity;
+ u8 state = activity_data->state;
+ const struct cros_ec_sensors_state *st = iio_priv(indio_dev);
+ const struct iio_chan_spec *chan;
+ const struct iio_event_spec *event;
+ enum iio_event_direction dir;
+ int index;
+ u64 ev;
+
+ switch (activity) {
+ case MOTIONSENSE_ACTIVITY_BODY_DETECTION:
+ index = st->body_detection_channel_index;
+ dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+ break;
+ case MOTIONSENSE_ACTIVITY_SIG_MOTION:
+ index = st->sig_motion_channel_index;
+ dir = IIO_EV_DIR_FALLING;
+ break;
+ default:
+ dev_warn(&indio_dev->dev, "Unknown activity: %d\n", activity);
+ return 0;
+ }
+ chan = &st->channels[index];
+ event = &chan->event_spec[0];
+
+ ev = IIO_UNMOD_EVENT_CODE(chan->type, index, event->type, dir);
+ iio_push_event(indio_dev, ev, timestamp);
+ return 0;
+}
+
+static irqreturn_t cros_ec_activity_capture(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+
+ dev_warn(&indio_dev->dev, "%s: Not Expected\n", __func__);
+ return IRQ_NONE;
+}
+
+static const struct iio_info ec_sensors_info = {
+ .read_raw = &ec_sensors_read,
+ .read_event_config = cros_ec_read_event_config,
+ .write_event_config = cros_ec_write_event_config,
+};
+
+static int cros_ec_sensors_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ec_device *ec_device = dev_get_drvdata(dev->parent);
+ struct iio_dev *indio_dev;
+ struct cros_ec_sensors_state *st;
+ struct iio_chan_spec *channel;
+ unsigned long activities;
+ int i, index, ret, nb_activities;
+
+ if (!ec_device) {
+ dev_warn(&pdev->dev, "No CROS EC device found.\n");
+ return -EINVAL;
+ }
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
+ cros_ec_activity_capture);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &ec_sensors_info;
+ st = iio_priv(indio_dev);
+ st->core.type = st->core.resp->info.type;
+
+ /*
+ * List all available activities
+ */
+ st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
+ ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+ if (ret)
+ return ret;
+ activities = st->core.resp->list_activities.enabled |
+ st->core.resp->list_activities.disabled;
+ nb_activities = hweight_long(activities) + 1;
+
+ if (!activities)
+ return -ENODEV;
+
+ /* Allocate a channel per activity and one for timestamp */
+ st->channels = devm_kcalloc(&pdev->dev, nb_activities,
+ sizeof(*st->channels), GFP_KERNEL);
+ if (!st->channels)
+ return -ENOMEM;
+
+ channel = &st->channels[0];
+ index = 0;
+ for_each_set_bit(i, &activities, BITS_PER_LONG) {
+ channel->scan_index = index;
+
+ /* List all available activities */
+ if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
+ channel->type = IIO_PROXIMITY;
+ channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ channel->modified = 0;
+ channel->event_spec = cros_ec_body_detect_events;
+ channel->num_event_specs =
+ ARRAY_SIZE(cros_ec_body_detect_events);
+ st->body_detection_channel_index = index;
+ } else {
+ channel->type = IIO_ACTIVITY;
+ channel->modified = 1;
+ channel->event_spec = cros_ec_activity_single_shot;
+ channel->num_event_specs =
+ ARRAY_SIZE(cros_ec_activity_single_shot);
+ if (i == MOTIONSENSE_ACTIVITY_SIG_MOTION) {
+ channel->channel2 = IIO_MOD_STILL;
+ st->sig_motion_channel_index = index;
+ } else {
+ dev_warn(&pdev->dev,
+ "Unknown activity: %d\n", i);
+ continue;
+ }
+ }
+ channel->ext_info = cros_ec_sensors_limited_info;
+ channel++;
+ index++;
+ }
+
+ /* Timestamp */
+ channel->scan_index = index;
+ channel->type = IIO_TIMESTAMP;
+ channel->channel = -1;
+ channel->scan_type.sign = 's';
+ channel->scan_type.realbits = 64;
+ channel->scan_type.storagebits = 64;
+
+ indio_dev->channels = st->channels;
+ indio_dev->num_channels = index + 1;
+
+ st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
+
+ return cros_ec_sensors_core_register(dev, indio_dev,
+ cros_ec_activity_push_data);
+}
+
+static void cros_ec_sensors_remove(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+ iio_device_unregister(indio_dev);
+}
+
+static struct platform_driver cros_ec_sensors_platform_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ },
+ .probe = cros_ec_sensors_probe,
+ .remove = cros_ec_sensors_remove,
+};
+module_platform_driver(cros_ec_sensors_platform_driver);
+
+MODULE_DESCRIPTION("ChromeOS EC activity sensors driver");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_LICENSE("GPL v2");
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 64bada1e8678..1bd07ca3abfe 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
@@ -489,6 +489,16 @@ const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[] = {
};
EXPORT_SYMBOL_GPL(cros_ec_sensors_ext_info);
+const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[] = {
+ {
+ .name = "id",
+ .shared = IIO_SHARED_BY_ALL,
+ .read = cros_ec_sensors_id
+ },
+ { },
+};
+EXPORT_SYMBOL_GPL(cros_ec_sensors_limited_info);
+
/**
* cros_ec_sensors_idx_to_reg - convert index into offset in shared memory
* @st: pointer to state information for device
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index e72167b96d27..bb966abcde53 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -126,5 +126,6 @@ extern const struct dev_pm_ops cros_ec_sensors_pm_ops;
/* List of extended channel specification for all sensors. */
extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[];
+extern const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[];
#endif /* __CROS_EC_SENSORS_CORE_H */
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecf290a0c98f..0f6d53e7cb97 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -2388,6 +2388,12 @@ enum motionsense_command {
*/
MOTIONSENSE_CMD_SENSOR_SCALE = 18,
+ /*
+ * Activity management
+ * Retrieve current status of given activity.
+ */
+ MOTIONSENSE_CMD_GET_ACTIVITY = 20,
+
/* Number of motionsense sub-commands. */
MOTIONSENSE_NUM_CMDS
};
@@ -2447,6 +2453,11 @@ enum motionsensor_orientation {
MOTIONSENSE_ORIENTATION_UNKNOWN = 4,
};
+struct ec_response_activity_data {
+ uint8_t activity; /* motionsensor_activity */
+ uint8_t state;
+} __ec_todo_packed;
+
struct ec_response_motion_sensor_data {
/* Flags for each sensor. */
uint8_t flags;
@@ -2454,15 +2465,14 @@ struct ec_response_motion_sensor_data {
uint8_t sensor_num;
/* Each sensor is up to 3-axis. */
union {
- int16_t data[3];
+ int16_t data[3];
struct __ec_todo_packed {
- uint16_t reserved;
- uint32_t timestamp;
+ uint16_t reserved;
+ uint32_t timestamp;
};
struct __ec_todo_unpacked {
- uint8_t activity; /* motionsensor_activity */
- uint8_t state;
- int16_t add_info[2];
+ struct ec_response_activity_data activity_data;
+ int16_t add_info[2];
};
};
} __ec_todo_packed;
@@ -2494,6 +2504,7 @@ enum motionsensor_activity {
MOTIONSENSE_ACTIVITY_SIG_MOTION = 1,
MOTIONSENSE_ACTIVITY_DOUBLE_TAP = 2,
MOTIONSENSE_ACTIVITY_ORIENTATION = 3,
+ MOTIONSENSE_ACTIVITY_BODY_DETECTION = 4,
};
struct ec_motion_sense_activity {
@@ -2671,6 +2682,7 @@ struct ec_params_motion_sense {
uint32_t max_data_vector;
} fifo_read;
+ /* Used for MOTIONSENSE_CMD_SET_ACTIVITY */
struct ec_motion_sense_activity set_activity;
/* Used for MOTIONSENSE_CMD_LID_ANGLE */
@@ -2716,6 +2728,14 @@ struct ec_params_motion_sense {
*/
int16_t hys_degree;
} tablet_mode_threshold;
+
+ /*
+ * Used for MOTIONSENSE_CMD_GET_ACTIVITY.
+ */
+ struct __ec_todo_unpacked {
+ uint8_t sensor_num;
+ uint8_t activity; /* enum motionsensor_activity */
+ } get_activity;
};
} __ec_todo_packed;
@@ -2833,6 +2853,10 @@ struct ec_response_motion_sense {
uint16_t hys_degree;
} tablet_mode_threshold;
+ /* USED for MOTIONSENSE_CMD_GET_ACTIVITY. */
+ struct __ec_todo_unpacked {
+ uint8_t state;
+ } get_activity;
};
} __ec_todo_packed;
--
2.49.0.1112.g889b7c5bd8-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] iio: cros_ec_sensors: add cros_ec_activity driver
2025-05-20 4:01 [PATCH] iio: cros_ec_sensors: add cros_ec_activity driver Gwendal Grignou
@ 2025-05-21 7:16 ` Tzung-Bi Shih
0 siblings, 0 replies; 2+ messages in thread
From: Tzung-Bi Shih @ 2025-05-21 7:16 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: jic23, chrome-platform, linux-iio, Gwendal Grignou
On Mon, May 19, 2025 at 09:01:00PM -0700, Gwendal Grignou wrote:
> ChromeOS EC can report activity information derived from the
> accelerometer:
> - reports on-body/off-body as a proximity event.
> - reports significant motion as an activity event.
s/^r/R/.
> diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
> [...]
> +config IIO_CROS_EC_ACTIVITY
> + tristate "ChromeOS EC Activity Sensors"
> + depends on IIO_CROS_EC_SENSORS_CORE
> + help
> + Module to handle activity events detections presented by the ChromeOS
^^^^^^^^^^^^^^^^^^^^^^^^^^
I'm confused and wondering if there is a typo. Which one:
- activity event detections
- activity events
- activity detections
> + EC Sensor hub.
s/S/s/?
> + Activities can be a proximity detector (on body/off body detection)
> + or an activity to trigger an event on significant motion.
s/an activity to trigger //?
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c b/drivers/iio/common/cros_ec_sensors/cros_ec_activity.c
> [...]
> +// SPDX-License-Identifier: GPL-2.0
> +//
Drop the empty comment line.
> +/*
> + * cros_ec_sensors_activity - Driver for activities/gesture recognition.
> + *
> + * Copyright 2025 Google, Inc
> + *
> + * This driver uses the cros-ec interface to communicate with the Chrome OS
s/Chrome OS/ChromeOS/.
> +#include <linux/delay.h>
Drop it. The driver doesn't use anything from the header.
> +#include <linux/device.h>
> +#include <linux/iio/common/cros_ec_sensors_core.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
I guess the list may be copied from somewhere drivers. Could you review
again to make sure the driver needs to include them?
> +#include <linux/slab.h>
Drop it. The driver doesn't use anything from the header.
> +static const struct iio_event_spec cros_ec_activity_single_shot[] = {
> + {
> + .type = IIO_EV_TYPE_CHANGE,
> + /* significant motion trigger when we get out of still. */
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + },
Remove the extra space before "}".
> +static const struct iio_event_spec cros_ec_body_detect_events[] = {
> + {
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + },
Remove the extra space before "}".
> +static int ec_sensors_read(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cros_ec_sensors_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->core.cmd_lock);
> + if (chan->type == IIO_PROXIMITY &&
> + mask == IIO_CHAN_INFO_RAW) {
> + st->core.param.cmd = MOTIONSENSE_CMD_GET_ACTIVITY;
> + st->core.param.get_activity.activity =
> + MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> + if (cros_ec_motion_send_host_cmd(&st->core, 0) !=
> + EC_RES_SUCCESS) {
cros_ec_motion_send_host_cmd() returns 0 or negative errno like most other
kernel APIs. Even though EC_RES_SUCCESS is also 0, however, they aren't
the same domain space (see also cros_ec_map_error()). Don't use it.
if (cros_ec_motion_send_host_cmd(&st->core, 0))
> +static int cros_ec_read_event_config(struct iio_dev *indio_dev,
> [...]
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + ret = !!(st->core.resp->list_activities.enabled &
> + (1 << MOTIONSENSE_ACTIVITY_BODY_DETECTION));
> + break;
> + case IIO_ACTIVITY:
> + if (chan->channel2 == IIO_MOD_STILL) {
> + ret = !!(st->core.resp->list_activities.enabled &
> + (1 << MOTIONSENSE_ACTIVITY_SIG_MOTION));
> + } else {
> + dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> + chan->channel2);
> + ret = -EINVAL;
> + }
> + break;
> + default:
> + dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> + chan->type);
> + ret = -EINVAL;
Curious about: wouldn't compiler or some checkers emit warnings about it
lacks a `break`?
> +static int cros_ec_write_event_config(struct iio_dev *indio_dev,
> [...]
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + st->core.param.set_activity.activity =
> + MOTIONSENSE_ACTIVITY_BODY_DETECTION;
> + break;
> + case IIO_ACTIVITY:
> + if (chan->channel2 == IIO_MOD_STILL) {
> + st->core.param.set_activity.activity =
> + MOTIONSENSE_ACTIVITY_SIG_MOTION;
> + } else {
> + dev_warn(&indio_dev->dev, "Unknown activity: %d\n",
> + chan->channel2);
> + }
> + break;
> + default:
> + dev_warn(&indio_dev->dev, "Unknown channel type: %d\n",
> + chan->type);
Same here.
> +static int cros_ec_sensors_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_device *ec_device = dev_get_drvdata(dev->parent);
> + struct iio_dev *indio_dev;
> + struct cros_ec_sensors_state *st;
> + struct iio_chan_spec *channel;
> + unsigned long activities;
> + int i, index, ret, nb_activities;
> +
> + if (!ec_device) {
> + dev_warn(&pdev->dev, "No CROS EC device found.\n");
The function already has a local variable `dev` for `&pdev->dev`. Use it.
> + return -EINVAL;
> + }
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
`dev`.
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> + cros_ec_activity_capture);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &ec_sensors_info;
> + st = iio_priv(indio_dev);
> + st->core.type = st->core.resp->info.type;
> +
> + /*
> + * List all available activities
> + */
> + st->core.param.cmd = MOTIONSENSE_CMD_LIST_ACTIVITIES;
> + ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> + if (ret)
> + return ret;
> + activities = st->core.resp->list_activities.enabled |
> + st->core.resp->list_activities.disabled;
> + nb_activities = hweight_long(activities) + 1;
Moving the line just right before the following devm_kcalloc() for
`st->channels` makes more sense.
> +
> + if (!activities)
> + return -ENODEV;
> +
> + /* Allocate a channel per activity and one for timestamp */
> + st->channels = devm_kcalloc(&pdev->dev, nb_activities,
> + sizeof(*st->channels), GFP_KERNEL);
> + if (!st->channels)
> + return -ENOMEM;
> +
> + channel = &st->channels[0];
> + index = 0;
> + for_each_set_bit(i, &activities, BITS_PER_LONG) {
> + channel->scan_index = index;
It seems the loop body doesn't use the `channel->scan_index`. Move the
assignment to the end of loop body when it's sure to occupy the index?
channel->scan_index = index++;
> +
> + /* List all available activities */
Drop the comment as it doesn't reflect what the following code does?
> + if (i == MOTIONSENSE_ACTIVITY_BODY_DETECTION) {
> + channel->type = IIO_PROXIMITY;
> + channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + channel->modified = 0;
> + channel->event_spec = cros_ec_body_detect_events;
> + channel->num_event_specs =
> + ARRAY_SIZE(cros_ec_body_detect_events);
> + st->body_detection_channel_index = index;
> + } else {
> + channel->type = IIO_ACTIVITY;
> + channel->modified = 1;
> + channel->event_spec = cros_ec_activity_single_shot;
> + channel->num_event_specs =
> + ARRAY_SIZE(cros_ec_activity_single_shot);
> + if (i == MOTIONSENSE_ACTIVITY_SIG_MOTION) {
> + channel->channel2 = IIO_MOD_STILL;
> + st->sig_motion_channel_index = index;
> + } else {
> + dev_warn(&pdev->dev,
`dev`.
> + "Unknown activity: %d\n", i);
> + continue;
> + }
> + }
> + channel->ext_info = cros_ec_sensors_limited_info;
> + channel++;
> + index++;
> + }
> +
> + /* Timestamp */
> + channel->scan_index = index;
> + channel->type = IIO_TIMESTAMP;
> + channel->channel = -1;
> + channel->scan_type.sign = 's';
> + channel->scan_type.realbits = 64;
> + channel->scan_type.storagebits = 64;
> +
> + indio_dev->channels = st->channels;
> + indio_dev->num_channels = index + 1;
> +
> + st->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
The statement here isn't intuitive. Can it move somewhere above when setting
the `st->core`? E.g. right after the assignment for `st->core.type`.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-05-21 7:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-20 4:01 [PATCH] iio: cros_ec_sensors: add cros_ec_activity driver Gwendal Grignou
2025-05-21 7:16 ` Tzung-Bi Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox