On Mon, Apr 13, 2020 at 8:17 AM Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> wrote: > > Hi Gwendal, William, > > Thank you for the patch. Some few comments below. > > On 11/4/20 17:00, William Breathitt Gray wrote: > > On Tue, Apr 07, 2020 at 10:36:55PM -0700, Gwendal Grignou wrote: > >> When the camera vsync pin is connected to the embedded controller (EC) of > >> a chromebook, the EC reports a sensor with a counter that increases > >> at each GPIO rising edge. > >> > >> The sensor is presented using the counter subsystem. > >> In addition, it is also presented via the IIO subsystem with a timestamp, > >> allowing synchronisation with sensors connected to the same EC, for > >> image stabilisation or augmented reality applications. > >> > >> To enable the counter: > >> via counter ABI: > >> echo "rising edge" > counterX/count0/signal_action > >> via iio ABI > >> echo 1 > iio:deviceY/en > >> > >> To disable the counter: > >> via counter ABI: > >> echo "none" > counterX/count0/signal_action > >> via iio ABI > >> echo 0 > iio:deviceY/en > >> > >> To read the current counter value: > >> via counter ABI: > >> cat counterX/count0/count > >> via iio ABI > >> cat iio:deviceY/in_count_raw > >> We can also read the value through the IIO buffer: > >> echo 1 > iio:deviceY/scan_elements/in_timestamp_en > >> echo 1 > iio:deviceY/scan_elements/in_count_en > >> echo 1 > iio:deviceY/buffer/enable > >> > >> and each time to counter increase, the following binary blob > >> will be appended to dev/iio:deviceY: > >> 000f 0000 0000 0000 dc66 816c 0071 0000 > >> \ <-- padding -> <-- timestamp ----> > >> count > >> > >> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > >> --- > >> An initial version of the driver was presented few month ago at > >> https://patchwork.kernel.org/patch/11037009/ > >> > >> drivers/counter/Kconfig | 11 + > >> drivers/counter/Makefile | 1 + > >> drivers/counter/cros_ec_sync.c | 342 ++++++++++++++++++ > >> .../cros_ec_sensors/cros_ec_sensors_core.c | 15 + > >> drivers/platform/chrome/cros_ec_sensorhub.c | 3 + > >> .../linux/iio/common/cros_ec_sensors_core.h | 4 +- > >> 6 files changed, 375 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/counter/cros_ec_sync.c > > > > Hi Gwendal, > > > > I'm getting some conflicts when I try to merge this patch, so it looks > > like we're on different bases. Would you be able to rebase on the > > current testing branch of the IIO tree and resubmit this patch? > > > > I think this patch is based on latest patches from the chrome-platform tree, > those landed last week and are merged in current 5.7-rc1. I applies cleanly on > top of current 5.7-rc1 released yesterday. > > > > Thanks, > > > > William Breathitt Gray > > > >> > >> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > >> index c80fa76bb5311..18fde918ff40b 100644 > >> --- a/drivers/counter/Kconfig > >> +++ b/drivers/counter/Kconfig > >> @@ -29,6 +29,17 @@ config 104_QUAD_8 > >> The base port addresses for the devices may be configured via the base > >> array module parameter. > >> > >> +config CROS_EC_SYNC > >> + tristate "ChromeOS EC Counter driver" > >> + depends on IIO_CROS_EC_SENSORS_CORE > >> + help > >> + Module to handle synchronisation sensor presented by the ChromeOS EC > >> + Sensor hub. > >> + Synchronisation sensor sends event to the host when the camera > >> + take a picture. It allows synchronisation with other MEMS sensor, > >> + like gyroscope for image statbilization or augmented reality > >> + application (AR). > >> + > >> config STM32_TIMER_CNT > >> tristate "STM32 Timer encoder counter driver" > >> depends on MFD_STM32_TIMERS || COMPILE_TEST > >> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile > >> index 55142d1f4c436..98378fca50ad6 100644 > >> --- a/drivers/counter/Makefile > >> +++ b/drivers/counter/Makefile > >> @@ -6,6 +6,7 @@ > >> obj-$(CONFIG_COUNTER) += counter.o > >> > >> obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o > >> +obj-$(CONFIG_CROS_EC_SYNC) += cros_ec_sync.o > >> obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o > >> obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o > >> obj-$(CONFIG_TI_EQEP) += ti-eqep.o > >> diff --git a/drivers/counter/cros_ec_sync.c b/drivers/counter/cros_ec_sync.c > >> new file mode 100644 > >> index 0000000000000..94b3ababab4bd > >> --- /dev/null > >> +++ b/drivers/counter/cros_ec_sync.c > >> @@ -0,0 +1,342 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > > Following other ChromeOS drivers this should be GPL-2.0-only > > >> +/* > >> + * Driver for synchronisation sensor behind CrOS EC. > >> + * > >> + * Copyright 2020 Google, Inc > > Copyright 2020 Google LLC. Fixed in v2. > > >> + * > >> + * This software is licensed under the terms of the GNU General Public > >> + * License version 2, as published by the Free Software Foundation, and > >> + * may be copied, distributed, and modified under those terms. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > > Remove the license boiler plate it is redundand and already specified in the > SPDX license tag. Fixed in v2. > > >> + * This driver uses the cros-ec interface to communicate with the Chrome OS > >> + * EC about counter sensors. Counters are presented through > >> + * iio sysfs. > >> + */ > >> + > >> +#include <linux/counter.h> > >> +#include <linux/device.h> > >> +#include <linux/iio/buffer.h> > >> +#include <linux/iio/common/cros_ec_sensors_core.h> > >> +#include <linux/iio/iio.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> > >> + > >> +/* > >> + * One channel for counter, the other for timestamp. > >> + */ > >> +#define MAX_CHANNELS (1 + 1) > >> + > >> +/** > >> + * struct cros_ec_sync_state - device structure > >> + * > >> + * @core: common structure for all cros_ec sensor. > >> + * Must be at the beggining. > >> + * @channels: presented iio channels(2). > >> + * @counter: counter data structure. > >> + */ > >> +struct cros_ec_sync_state { > >> + struct cros_ec_sensors_core_state core; > >> + struct iio_chan_spec channels[MAX_CHANNELS]; > >> + struct counter_device counter; > >> +}; > >> + > >> +/** > >> + * cros_ec_sync_get_enable() - Check if the counter is enabled. > >> + * > >> + * @st: core cros_ec sensor > >> + * @val: status: 0: disabled, 1 enabled. > >> + * > >> + * Return: 0 on success, -errno on failure. > >> + */ > >> +static int cros_ec_sync_get_enable(struct cros_ec_sensors_core_state *st, > >> + int *val) > >> +{ > >> + int ret; > >> + > >> + mutex_lock(&st->cmd_lock); > >> + st->param.cmd = MOTIONSENSE_CMD_SENSOR_ODR; > >> + st->param.sensor_odr.data = EC_MOTION_SENSE_NO_VALUE; > >> + > >> + ret = cros_ec_motion_send_host_cmd(st, 0); > >> + mutex_unlock(&st->cmd_lock); > >> + if (ret) > >> + return ret; > >> + > >> + *val = !!st->resp->sensor_odr.ret; > >> + return 0; > >> +} > >> + > >> +/** > >> + * cros_ec_sync_set_enable() - Allow the counter to count. > >> + * > >> + * When enable, the counter will increase for each VSYNC rising edge > >> + * and will produce an event in the iio buffer, if enabled. > >> + * > >> + * @st: core cros_ec sensor > >> + * @val: 0: disable, 1 enable. > >> + * > >> + * Return: 0 on success, -errno on failure. > >> + */ > >> +static int cros_ec_sync_set_enable(struct cros_ec_sensors_core_state *st, > >> + int val) > >> +{ > >> + int ret; > >> + > >> + mutex_lock(&st->cmd_lock); > >> + st->param.cmd = MOTIONSENSE_CMD_SENSOR_ODR; > >> + st->param.sensor_odr.data = val; > >> + st->param.sensor_odr.roundup = 1; > >> + > >> + ret = cros_ec_motion_send_host_cmd(st, 0); > >> + mutex_unlock(&st->cmd_lock); > >> + return ret; > >> +} > >> + > >> +static int cros_ec_sync_iio_read(struct iio_dev *indio_dev, > >> + struct iio_chan_spec const *chan, > >> + int *val, int *val2, long mask) > >> +{ > >> + struct cros_ec_sensors_core_state *st = iio_priv(indio_dev); > >> + u16 data; > >> + int ret; > >> + int idx = chan->scan_index; > >> + > >> + switch (mask) { > >> + case IIO_CHAN_INFO_RAW: > >> + mutex_lock(&st->cmd_lock); > > I think you need to review the mutex locking logic. > > >> + ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx, &data); > >> + mutex_unlock(&st->cmd_lock); > >> + if (ret < 0) > >> + break; > >> + ret = IIO_VAL_INT; > >> + *val = data; > >> + break; > >> + case IIO_CHAN_INFO_ENABLE: > >> + ret = cros_ec_sync_get_enable(st, val); > >> + if (ret < 0) > >> + break; > >> + ret = IIO_VAL_INT; > >> + break; > >> + default: > >> + ret = -EINVAL; > >> + break; > >> + } > >> + mutex_unlock(&st->cmd_lock); > > Where did you lock the mutex? Fixed in v2. That line is an error. > > >> + return ret; > >> +} > >> + > >> +static int cros_ec_sync_iio_write(struct iio_dev *indio_dev, > >> + struct iio_chan_spec const *chan, > >> + int val, int val2, long mask) > >> +{ > >> + struct cros_ec_sensors_core_state *st = iio_priv(indio_dev); > >> + > >> + switch (mask) { > >> + case IIO_CHAN_INFO_ENABLE: > >> + if (val < 0 || val > 1) > >> + return -EINVAL; > >> + > >> + return cros_ec_sync_set_enable(st, val); > >> + > >> + default: > >> + return -EINVAL; > >> + } > >> +} > >> + > >> +static const struct iio_info cros_ec_sync_info = { > >> + .read_raw = &cros_ec_sync_iio_read, > >> + .write_raw = &cros_ec_sync_iio_write, > >> +}; > >> + > >> +/* The counter can only increase, so only one function present. */ > >> +static enum counter_count_function cros_ec_sync_functions[] = { > >> + COUNTER_COUNT_FUNCTION_INCREASE, > >> +}; > >> + > >> +/* 2 synapse actions allowed: count for each rising edge, or not. */ > >> +static enum counter_synapse_action cros_ec_sync_synapse_actions[] = { > >> + COUNTER_SYNAPSE_ACTION_NONE, > >> + COUNTER_SYNAPSE_ACTION_RISING_EDGE, > >> +}; > >> + > >> +static int cros_ec_sync_read_count(struct counter_device *counter, > >> + struct counter_count *count, > >> + unsigned long *val) > >> +{ > >> + struct cros_ec_sensors_core_state *st = counter->priv; > >> + u16 raw; > >> + int ret; > >> + > >> + mutex_lock(&st->cmd_lock); > >> + ret = cros_ec_sensors_read_cmd(iio_priv_to_dev(st), 1, &raw); > >> + mutex_unlock(&st->cmd_lock); > >> + if (ret) > >> + return ret; > >> + > >> + *val = raw; > >> + return 0; > >> +} > >> + > >> +static int cros_ec_sync_function_get(struct counter_device *counter, > >> + struct counter_count *count, > >> + size_t *function) > >> +{ > >> + *function = 0; > >> + return 0; > >> +} > >> + > >> +static int cros_ec_sync_action_get(struct counter_device *counter, > >> + struct counter_count *count, > >> + struct counter_synapse *synapse, > >> + size_t *action) > >> +{ > >> + struct cros_ec_sensors_core_state *st = counter->priv; > >> + int ret; > >> + int raw; > >> + > >> + ret = cros_ec_sync_get_enable(st, &raw); > >> + if (ret) > >> + return ret; > >> + > >> + *action = !!raw; > >> + return 0; > >> +} > >> + > >> +static int cros_ec_sync_action_set(struct counter_device *counter, > >> + struct counter_count *count, > >> + struct counter_synapse *synapse, > >> + size_t action) > >> +{ > >> + struct cros_ec_sensors_core_state *st = counter->priv; > >> + > >> + return cros_ec_sync_set_enable(st, action); > >> +} > >> + > >> +static const struct counter_ops cros_ec_sync_ops = { > >> + .count_read = cros_ec_sync_read_count, > >> + .function_get = cros_ec_sync_function_get, > >> + .action_get = cros_ec_sync_action_get, > >> + .action_set = cros_ec_sync_action_set, > >> +}; > >> + > >> +static struct counter_signal cros_ec_sync_signals[] = { > >> + { > >> + .id = 0, > >> + .name = "vsync" > >> + } > >> +}; > >> + > >> +static struct counter_synapse cros_ec_sync_synapses[] = { > >> + { > >> + .actions_list = cros_ec_sync_synapse_actions, > >> + .num_actions = ARRAY_SIZE(cros_ec_sync_synapse_actions), > >> + .signal = cros_ec_sync_signals > >> + } > >> +}; > >> + > >> +static struct counter_count cros_ec_sync_counts[] = { > >> + { > >> + .id = 0, > >> + .name = "vsync", > >> + .functions_list = cros_ec_sync_functions, > >> + .num_functions = ARRAY_SIZE(cros_ec_sync_functions), > >> + .synapses = cros_ec_sync_synapses, > >> + .num_synapses = ARRAY_SIZE(cros_ec_sync_synapses), > >> + } > >> +}; > >> + > >> +static int cros_ec_sync_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct iio_dev *indio_dev; > >> + struct cros_ec_sync_state *state; > >> + struct iio_chan_spec *channel; > >> + int ret; > >> + > >> + indio_dev = devm_iio_device_alloc(dev, sizeof(*state)); > >> + if (!indio_dev) > >> + return -ENOMEM; > >> + > >> + ret = cros_ec_sensors_core_init(pdev, indio_dev, true, > >> + cros_ec_sensors_capture, > >> + cros_ec_sensors_push_data); > >> + if (ret) > >> + return ret; > >> + > >> + indio_dev->info = &cros_ec_sync_info; > >> + state = iio_priv(indio_dev); > >> + > >> + /* Initialize IIO device */ > >> + channel = state->channels; > >> + > >> + /* Counter channel */ > >> + channel->type = IIO_COUNT; > >> + channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW); > >> + channel->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_ENABLE); > >> + channel->scan_type.realbits = CROS_EC_SENSOR_BITS; > >> + channel->scan_type.storagebits = CROS_EC_SENSOR_BITS; > >> + channel->scan_type.shift = 0; > >> + channel->scan_index = 0; > >> + channel->ext_info = cros_ec_sensors_limited_info; > >> + channel->scan_type.sign = 'u'; > >> + > >> + /* Timestamp channel */ > >> + channel++; > >> + channel->type = IIO_TIMESTAMP; > >> + channel->channel = -1; > >> + channel->scan_index = 1; > >> + channel->scan_type.sign = 's'; > >> + channel->scan_type.realbits = 64; > >> + channel->scan_type.storagebits = 64; > >> + > >> + indio_dev->channels = state->channels; > >> + indio_dev->num_channels = MAX_CHANNELS; > >> + > >> + state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd; > >> + > >> + /* Initialize Counter device */ > >> + state->counter.name = dev_name(&pdev->dev); > >> + state->counter.parent = &pdev->dev; > >> + state->counter.ops = &cros_ec_sync_ops; > >> + state->counter.counts = cros_ec_sync_counts; > >> + state->counter.num_counts = ARRAY_SIZE(cros_ec_sync_counts); > >> + state->counter.signals = cros_ec_sync_signals; > >> + state->counter.num_signals = ARRAY_SIZE(cros_ec_sync_signals); > >> + state->counter.priv = state; > >> + > >> + ret = devm_iio_device_register(dev, indio_dev); > >> + if (ret) > >> + return ret; > >> + > >> + return devm_counter_register(dev, &state->counter); > >> +} > >> + > >> +static const struct platform_device_id cros_ec_sync_ids[] = { > >> + { > >> + .name = "cros-ec-sync", > >> + }, > >> + { /* sentinel */ } > >> +}; > >> +MODULE_DEVICE_TABLE(platform, cros_ec_sync_ids); > >> + > >> +static struct platform_driver cros_ec_sync_platform_driver = { > >> + .driver = { > >> + .name = "cros-ec-sync", > >> + }, > >> + .probe = cros_ec_sync_probe, > >> + .id_table = cros_ec_sync_ids, > >> +}; > >> +module_platform_driver(cros_ec_sync_platform_driver); > >> + > >> +MODULE_AUTHOR("Gwendal Grignou <gwendal@xxxxxxxxxxxx>"); > >> +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor driver"); > >> +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 c831915ca7e56..3a15094616710 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 > >> @@ -31,6 +31,7 @@ > >> static char *cros_ec_loc[] = { > >> [MOTIONSENSE_LOC_BASE] = "base", > >> [MOTIONSENSE_LOC_LID] = "lid", > >> + [MOTIONSENSE_LOC_CAMERA] = "camera", > >> [MOTIONSENSE_LOC_MAX] = "unknown", > >> }; > >> > >> @@ -467,6 +468,20 @@ 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 > >> + }, > >> + { > >> + .name = "location", > >> + .shared = IIO_SHARED_BY_ALL, > >> + .read = cros_ec_sensors_loc > >> + }, > >> + { }, > >> +}; > >> +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/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c > >> index b7f2c00db5e1e..e4ae0868d1e06 100644 > >> --- a/drivers/platform/chrome/cros_ec_sensorhub.c > >> +++ b/drivers/platform/chrome/cros_ec_sensorhub.c > >> @@ -106,6 +106,9 @@ static int cros_ec_sensorhub_register(struct device *dev, > >> case MOTIONSENSE_TYPE_ACTIVITY: > >> name = "cros-ec-activity"; > >> break; > >> + case MOTIONSENSE_TYPE_SYNC: > >> + name = "cros-ec-sync"; > >> + break; > >> default: > >> dev_warn(dev, "unknown type %d\n", > >> sensorhub->resp->info.type); > >> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h > >> index 7bc961defa87e..e416b28cf24c7 100644 > >> --- a/include/linux/iio/common/cros_ec_sensors_core.h > >> +++ b/include/linux/iio/common/cros_ec_sensors_core.h > >> @@ -114,7 +114,9 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st, > >> struct iio_chan_spec const *chan, > >> int val, int val2, long mask); > >> > >> -/* List of extended channel specification for all sensors */ > >> +/* List of extended channel specification for all sensors. */ > >> +extern const struct iio_chan_spec_ext_info cros_ec_sensors_limited_info[]; > >> +/* Add calibration to set above. */ > > This change is unrelated to this patch. It is needed because the sync sensor does not need calibration. It is used when setting the ext_info of the sync channel. > > >> extern const struct iio_chan_spec_ext_info cros_ec_sensors_ext_info[]; > >> extern const struct attribute *cros_ec_sensor_fifo_attributes[]; > >> > >> -- > >> 2.26.0.110.g2183baf09c-goog > >> > > Thanks, > Enric
