[v2,1/1] counter: cros_ec: Add synchronization sensor

Commit Message

Fabien Lahoudere Aug. 23, 2019, 12:41 p.m. UTC

From: Gwendal Grignou <[e-mail]>

EC returns a counter when there is an event on camera vsync.
This patch comes from chromeos kernel 4.4

Signed-off-by: Gwendal Grignou <[e-mail]>
Signed-off-by: Fabien Lahoudere <[e-mail]>

CROS EC sync sensor was originally designed as an IIO device.
Now that the counter subsystem will replace IIO_COUNTER, we
have to implement a new way to get sync count.

Signed-off-by: Fabien Lahoudere <[e-mail]>
---
 Documentation/driver-api/generic-counter.rst  |   3 +
 MAINTAINERS                                   |   7 +
 drivers/counter/Kconfig                       |   9 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/counter.c                     |   2 +
 drivers/counter/cros_ec_sensors_sync.c        | 208 ++++++++++++++++++
 .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
 drivers/mfd/cros_ec_dev.c                     |   3 +
 include/linux/counter.h                       |   1 +
 9 files changed, 235 insertions(+)
 create mode 100644 drivers/counter/cros_ec_sensors_sync.c

Comments

Jonathan Cameron Aug. 26, 2019, 8:56 a.m. UTC | #1

On Fri, 23 Aug 2019 14:41:27 +0200
Fabien Lahoudere <[e-mail]> wrote:

> From: Gwendal Grignou <[e-mail]>
> 
> EC returns a counter when there is an event on camera vsync.
> This patch comes from chromeos kernel 4.4
> 
> Signed-off-by: Gwendal Grignou <[e-mail]>
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> 
> CROS EC sync sensor was originally designed as an IIO device.
> Now that the counter subsystem will replace IIO_COUNTER, we
> have to implement a new way to get sync count.

I'm curious.  What is this counter used for?

This combined counter and iio driver isn't something we would normally
want to support.  What is the reasoning behind doing both interfaces?

> 
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> ---
>  Documentation/driver-api/generic-counter.rst  |   3 +
>  MAINTAINERS                                   |   7 +
>  drivers/counter/Kconfig                       |   9 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/counter.c                     |   2 +
>  drivers/counter/cros_ec_sensors_sync.c        | 208 ++++++++++++++++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
>  drivers/mfd/cros_ec_dev.c                     |   3 +
>  include/linux/counter.h                       |   1 +
>  9 files changed, 235 insertions(+)
>  create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> 
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index 8382f01a53e3..beb80714ac8b 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -44,6 +44,9 @@ Counter interface provides the following available count data types:
>  * COUNT_POSITION:
>    Unsigned integer value representing position.
>  
> +* COUNT_TALLY:
> +  Unsigned integer value representing tally.
> +
>  A Count has a count function mode which represents the update behavior
>  for the count data. The Generic Counter interface provides the following
>  available count function modes:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e60f5c361969..83bd291d103e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3902,6 +3902,13 @@ R:	Guenter Roeck <[e-mail]>
>  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
>  F:	sound/soc/codecs/cros_ec_codec.*
>  
> +CHROMEOS EC COUNTER DRIVER
> +M:	Fabien Lahoudere <[e-mail]>
> +M:	William Breathitt Gray <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/counter/cros_ec_sensors_sync.c
> +
>  CIRRUS LOGIC AUDIO CODEC DRIVERS
>  M:	Brian Austin <[e-mail]>
>  M:	Paul Handrigan <[e-mail]>
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..22287f5715e5 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,13 @@ config FTM_QUADDEC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ftm-quaddec.
>  
> +config IIO_CROS_EC_SENSORS_SYNC
> +	tristate "ChromeOS EC Counter Sensors"
> +	depends on IIO_CROS_EC_SENSORS_CORE && IIO
> +	help
> +	  Module to handle synchronisation sensors presented by the ChromeOS EC
> +	  Sensor hub.
> +	  Synchronisation sensors are counter sensors triggered when events
> +	  occurs from other subsystems.
> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..6fe4c98a446f 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..53525b109094 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct counter_count_read_value *const val,
>  {
>  	switch (type) {
>  	case COUNTER_COUNT_POSITION:
> +	case COUNTER_COUNT_TALLY:
>  		val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
>  		break;
>  	default:
> @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const data,
>  
>  	switch (type) {
>  	case COUNTER_COUNT_POSITION:
> +	case COUNTER_COUNT_TALLY:
>  		err = kstrtoul(val->buf, 0, data);
>  		if (err)
>  			return err;
> diff --git a/drivers/counter/cros_ec_sensors_sync.c b/drivers/counter/cros_ec_sensors_sync.c
> new file mode 100644
> index 000000000000..b6f5e2c6da9f
> --- /dev/null
> +++ b/drivers/counter/cros_ec_sensors_sync.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver of counter incremented after events on interrupt line in EC.
> + *
> + * Copyright 2018 Google, Inc
> + */
> +
> +#include <linux/device.h>
> +#include <linux/counter.h>
> +#include <linux/iio/common/cros_ec_sensors_core.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "cros-ec-sync"
> +
> +/*
> + * One channel for counter, the other for timestamp.
> + */
> +#define MAX_CHANNELS (1)
> +
> +/* State data for ec_sensors iio driver. */
> +struct cros_ec_sensors_sync_state {
> +	/* Shared by all sensors */
> +	struct cros_ec_sensors_core_state core;
> +	struct counter_device counter;
> +	struct iio_chan_spec channels[MAX_CHANNELS];
> +};
> +
> +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     int *val, int *val2, long mask)
> +{
> +	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> +	u16 data;
> +	int ret;
> +
> +	mutex_lock(&st->core.cmd_lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &data);
> +		if (ret < 0)
> +			break;
> +		ret = IIO_VAL_INT;
> +		*val = data;
> +		break;
> +	default:
> +		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> +						mask);
> +		break;
> +	}
> +	mutex_unlock(&st->core.cmd_lock);
> +	return ret;
> +}
> +
> +static struct iio_info cros_ec_sensors_sync_info = {
> +	.read_raw = &cros_ec_sensors_sync_read,
> +	.read_avail = &cros_ec_sensors_core_read_avail,
> +};
> +
> +static struct counter_count cros_ec_sync_counts = {
> +	.id = 0,
> +	.name = "Cros EC sync counter",
> +};
> +
> +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				struct counter_count_read_value *val)
> +{
> +	s16 cnt;
> +	int ret;
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +	unsigned long data;
> +
> +	mutex_lock(&st->core.cmd_lock);
> +	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> +	mutex_unlock(&st->core.cmd_lock);
> +	if (ret != 0) {
> +		dev_warn(&indio_dev->dev, "Unable to read sensor data\n");
> +		return ret;
> +	}
> +
> +	data = (unsigned long) cnt;
> +	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops cros_ec_sync_cnt_ops = {
> +	.count_read = cros_ec_sync_cnt_read,
> +};
> +
> +static char *cros_ec_loc[] = {
> +	[MOTIONSENSE_LOC_BASE] = "base",
> +	[MOTIONSENSE_LOC_LID] = "lid",
> +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> +	[MOTIONSENSE_LOC_MAX] = "unknown",
> +};
> +
> +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> +				  void *private, char *buf)
> +{
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", st->core.param.info.sensor_num);
> +}
> +
> +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> +				   void *private, char *buf)
> +{
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->core.loc]);
> +}
> +
> +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> +	{
> +		.name = "id",
> +		.read = cros_ec_sync_id
> +	},
> +	{
> +		.name = "location",
> +		.read = cros_ec_sync_loc
> +	},
> +};
> +
> +static int cros_ec_sensors_sync_probe(struct platform_device *pdev)
> +{
> +	struct cros_ec_sensors_sync_state *state;
> +	struct device *dev = &pdev->dev;
> +	struct iio_chan_spec *channel;
> +	struct iio_dev *indio_dev;
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &cros_ec_sensors_sync_info;
> +	state = iio_priv(indio_dev);
> +
> +	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> +		return -EINVAL;
> +
> +	/* Initialize IIO device */
> +	channel = state->channels;
> +	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;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      cros_ec_sensors_capture, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);

Hmm. Wasn't expecting to see that here if it's a counter device.


> +	if (ret)
> +		return ret;
> +
> +	/* Initialize counter device */
> +	state->counter.name = dev_name(&pdev->dev);
> +	state->counter.parent = &pdev->dev;
> +	state->counter.counts = &cros_ec_sync_counts;
> +	state->counter.num_counts = 1;
> +	state->counter.priv = indio_dev;
> +	state->counter.ops = &cros_ec_sync_cnt_ops;
> +	state->counter.ext = cros_ec_sync_cnt_ext;
> +	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> +
> +	return devm_counter_register(&pdev->dev, &state->counter);
> +}
> +
> +static const struct platform_device_id cros_ec_sensors_sync_ids[] = {
> +	{ .name = DRV_NAME, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> +
> +static struct platform_driver cros_ec_sensors_sync_platform_driver = {
> +	.driver = {
> +		.name	= DRV_NAME,
> +		.pm	= &cros_ec_sensors_pm_ops,
> +	},
> +	.probe		= cros_ec_sensors_sync_probe,
> +	.id_table	= cros_ec_sensors_sync_ids,
> +};
> +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> +
> +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor 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 805652250960..2bf183425eaf 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
> @@ -22,6 +22,7 @@
>  static char *cros_ec_loc[] = {
>  	[MOTIONSENSE_LOC_BASE] = "base",
>  	[MOTIONSENSE_LOC_LID] = "lid",
> +	[MOTIONSENSE_LOC_CAMERA] = "camera",
>  	[MOTIONSENSE_LOC_MAX] = "unknown",
>  };
>  
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 41dccced5026..1c5c2c38af88 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  		case MOTIONSENSE_TYPE_ACTIVITY:
>  			sensor_cells[id].name = "cros-ec-activity";
>  			break;
> +		case MOTIONSENSE_TYPE_SYNC:
> +			sensor_cells[id].name = "cros-ec-sync";
> +			break;
>  		default:
>  			dev_warn(ec->dev, "unknown type %d\n", resp->info.type);
>  			continue;
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..1198e675306f 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -488,6 +488,7 @@ enum counter_signal_value_type {
>  
>  enum counter_count_value_type {
>  	COUNTER_COUNT_POSITION = 0,
> +	COUNTER_COUNT_TALLY
>  };
>  
>  void counter_signal_read_value_set(struct counter_signal_read_value *const val,

Fabien Lahoudere Aug. 29, 2019, 11:10 a.m. UTC | #2

Le lundi 26 août 2019 à 09:56 +0100, Jonathan Cameron a écrit :
> On Fri, 23 Aug 2019 14:41:27 +0200
> Fabien Lahoudere <[e-mail]> wrote:
> 
> > From: Gwendal Grignou <[e-mail]>
> > 
> > EC returns a counter when there is an event on camera vsync.
> > This patch comes from chromeos kernel 4.4
> > 
> > Signed-off-by: Gwendal Grignou <[e-mail]>
> > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > 
> > CROS EC sync sensor was originally designed as an IIO device.
> > Now that the counter subsystem will replace IIO_COUNTER, we
> > have to implement a new way to get sync count.
> 
> I'm curious.  What is this counter used for?

It is a counter of a pin connected to a camera vsync.
It is used to manage correct orientation of the camera when the
gyroscope detect a change.

> 
> This combined counter and iio driver isn't something we would
> normally
> want to support.  What is the reasoning behind doing both interfaces?

That patch was originally designed using only IIO with IIO_count and
IIO_TIMESTAMP channel. As IIO_COUNT is deprecated, we need to use the
new counter subsystem. So I just split the driver in two part counter
and iio timestamp.

However after your email I did some investigation and did not find any
use of the timestamp for that sensor. Timestamp is just a common
channel implemented on all CROS EC sensors.

So I think I will drop it now and if a timestamp is needed, I will
probably restart the discussion about timestamp in the counter
subsystemif that make sense to have one.

Thanks

Fabien

> 
> > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > ---
> >  Documentation/driver-api/generic-counter.rst  |   3 +
> >  MAINTAINERS                                   |   7 +
> >  drivers/counter/Kconfig                       |   9 +
> >  drivers/counter/Makefile                      |   1 +
> >  drivers/counter/counter.c                     |   2 +
> >  drivers/counter/cros_ec_sensors_sync.c        | 208
> > ++++++++++++++++++
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
> >  drivers/mfd/cros_ec_dev.c                     |   3 +
> >  include/linux/counter.h                       |   1 +
> >  9 files changed, 235 insertions(+)
> >  create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> > 
> > diff --git a/Documentation/driver-api/generic-counter.rst
> > b/Documentation/driver-api/generic-counter.rst
> > index 8382f01a53e3..beb80714ac8b 100644
> > --- a/Documentation/driver-api/generic-counter.rst
> > +++ b/Documentation/driver-api/generic-counter.rst
> > @@ -44,6 +44,9 @@ Counter interface provides the following
> > available count data types:
> >  * COUNT_POSITION:
> >    Unsigned integer value representing position.
> >  
> > +* COUNT_TALLY:
> > +  Unsigned integer value representing tally.
> > +
> >  A Count has a count function mode which represents the update
> > behavior
> >  for the count data. The Generic Counter interface provides the
> > following
> >  available count function modes:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e60f5c361969..83bd291d103e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3902,6 +3902,13 @@ R:	Guenter Roeck <[e-mail]>
> >  F:	Documentation/devicetree/bindings/sound/google,cros-ec-
> > codec.txt
> >  F:	sound/soc/codecs/cros_ec_codec.*
> >  
> > +CHROMEOS EC COUNTER DRIVER
> > +M:	Fabien Lahoudere <[e-mail]>
> > +M:	William Breathitt Gray <[e-mail]>
> > +L:	[e-mail]
> > +S:	Maintained
> > +F:	drivers/counter/cros_ec_sensors_sync.c
> > +
> >  CIRRUS LOGIC AUDIO CODEC DRIVERS
> >  M:	Brian Austin <[e-mail]>
> >  M:	Paul Handrigan <[e-mail]>
> > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> > index 2967d0a9ff91..22287f5715e5 100644
> > --- a/drivers/counter/Kconfig
> > +++ b/drivers/counter/Kconfig
> > @@ -59,4 +59,13 @@ config FTM_QUADDEC
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ftm-quaddec.
> >  
> > +config IIO_CROS_EC_SENSORS_SYNC
> > +	tristate "ChromeOS EC Counter Sensors"
> > +	depends on IIO_CROS_EC_SENSORS_CORE && IIO
> > +	help
> > +	  Module to handle synchronisation sensors presented by the
> > ChromeOS EC
> > +	  Sensor hub.
> > +	  Synchronisation sensors are counter sensors triggered when
> > events
> > +	  occurs from other subsystems.
> > +
> >  endif # COUNTER
> > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> > index 40d35522937d..6fe4c98a446f 100644
> > --- a/drivers/counter/Makefile
> > +++ b/drivers/counter/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
> >  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> >  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> >  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> > +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> > diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> > index 106bc7180cd8..53525b109094 100644
> > --- a/drivers/counter/counter.c
> > +++ b/drivers/counter/counter.c
> > @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct
> > counter_count_read_value *const val,
> >  {
> >  	switch (type) {
> >  	case COUNTER_COUNT_POSITION:
> > +	case COUNTER_COUNT_TALLY:
> >  		val->len = sprintf(val->buf, "%lu\n", *(unsigned long
> > *)data);
> >  		break;
> >  	default:
> > @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const
> > data,
> >  
> >  	switch (type) {
> >  	case COUNTER_COUNT_POSITION:
> > +	case COUNTER_COUNT_TALLY:
> >  		err = kstrtoul(val->buf, 0, data);
> >  		if (err)
> >  			return err;
> > diff --git a/drivers/counter/cros_ec_sensors_sync.c
> > b/drivers/counter/cros_ec_sensors_sync.c
> > new file mode 100644
> > index 000000000000..b6f5e2c6da9f
> > --- /dev/null
> > +++ b/drivers/counter/cros_ec_sensors_sync.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver of counter incremented after events on interrupt line in
> > EC.
> > + *
> > + * Copyright 2018 Google, Inc
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/counter.h>
> > +#include <linux/iio/common/cros_ec_sensors_core.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/cros_ec.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRV_NAME "cros-ec-sync"
> > +
> > +/*
> > + * One channel for counter, the other for timestamp.
> > + */
> > +#define MAX_CHANNELS (1)
> > +
> > +/* State data for ec_sensors iio driver. */
> > +struct cros_ec_sensors_sync_state {
> > +	/* Shared by all sensors */
> > +	struct cros_ec_sensors_core_state core;
> > +	struct counter_device counter;
> > +	struct iio_chan_spec channels[MAX_CHANNELS];
> > +};
> > +
> > +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> > +				     struct iio_chan_spec const *chan,
> > +				     int *val, int *val2, long mask)
> > +{
> > +	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> > +	u16 data;
> > +	int ret;
> > +
> > +	mutex_lock(&st->core.cmd_lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0),
> > &data);
> > +		if (ret < 0)
> > +			break;
> > +		ret = IIO_VAL_INT;
> > +		*val = data;
> > +		break;
> > +	default:
> > +		ret = cros_ec_sensors_core_read(&st->core, chan, val,
> > val2,
> > +						mask);
> > +		break;
> > +	}
> > +	mutex_unlock(&st->core.cmd_lock);
> > +	return ret;
> > +}
> > +
> > +static struct iio_info cros_ec_sensors_sync_info = {
> > +	.read_raw = &cros_ec_sensors_sync_read,
> > +	.read_avail = &cros_ec_sensors_core_read_avail,
> > +};
> > +
> > +static struct counter_count cros_ec_sync_counts = {
> > +	.id = 0,
> > +	.name = "Cros EC sync counter",
> > +};
> > +
> > +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> > +				struct counter_count *count,
> > +				struct counter_count_read_value *val)
> > +{
> > +	s16 cnt;
> > +	int ret;
> > +	struct iio_dev *indio_dev = counter->priv;
> > +	struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > +	unsigned long data;
> > +
> > +	mutex_lock(&st->core.cmd_lock);
> > +	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> > +	mutex_unlock(&st->core.cmd_lock);
> > +	if (ret != 0) {
> > +		dev_warn(&indio_dev->dev, "Unable to read sensor
> > data\n");
> > +		return ret;
> > +	}
> > +
> > +	data = (unsigned long) cnt;
> > +	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct counter_ops cros_ec_sync_cnt_ops = {
> > +	.count_read = cros_ec_sync_cnt_read,
> > +};
> > +
> > +static char *cros_ec_loc[] = {
> > +	[MOTIONSENSE_LOC_BASE] = "base",
> > +	[MOTIONSENSE_LOC_LID] = "lid",
> > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> > +	[MOTIONSENSE_LOC_MAX] = "unknown",
> > +};
> > +
> > +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> > +				  void *private, char *buf)
> > +{
> > +	struct iio_dev *indio_dev = counter->priv;
> > +	struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > +
> > +	return snprintf(buf, PAGE_SIZE, "%d\n", st-
> > >core.param.info.sensor_num);
> > +}
> > +
> > +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> > +				   void *private, char *buf)
> > +{
> > +	struct iio_dev *indio_dev = counter->priv;
> > +	struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > +
> > +	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st-
> > >core.loc]);
> > +}
> > +
> > +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> > +	{
> > +		.name = "id",
> > +		.read = cros_ec_sync_id
> > +	},
> > +	{
> > +		.name = "location",
> > +		.read = cros_ec_sync_loc
> > +	},
> > +};
> > +
> > +static int cros_ec_sensors_sync_probe(struct platform_device
> > *pdev)
> > +{
> > +	struct cros_ec_sensors_sync_state *state;
> > +	struct device *dev = &pdev->dev;
> > +	struct iio_chan_spec *channel;
> > +	struct iio_dev *indio_dev;
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->info = &cros_ec_sensors_sync_info;
> > +	state = iio_priv(indio_dev);
> > +
> > +	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> > +		return -EINVAL;
> > +
> > +	/* Initialize IIO device */
> > +	channel = state->channels;
> > +	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;
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      cros_ec_sensors_capture,
> > NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> 
> Hmm. Wasn't expecting to see that here if it's a counter device.
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Initialize counter device */
> > +	state->counter.name = dev_name(&pdev->dev);
> > +	state->counter.parent = &pdev->dev;
> > +	state->counter.counts = &cros_ec_sync_counts;
> > +	state->counter.num_counts = 1;
> > +	state->counter.priv = indio_dev;
> > +	state->counter.ops = &cros_ec_sync_cnt_ops;
> > +	state->counter.ext = cros_ec_sync_cnt_ext;
> > +	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> > +
> > +	return devm_counter_register(&pdev->dev, &state->counter);
> > +}
> > +
> > +static const struct platform_device_id cros_ec_sensors_sync_ids[]
> > = {
> > +	{ .name = DRV_NAME, },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> > +
> > +static struct platform_driver cros_ec_sensors_sync_platform_driver
> > = {
> > +	.driver = {
> > +		.name	= DRV_NAME,
> > +		.pm	= &cros_ec_sensors_pm_ops,
> > +	},
> > +	.probe		= cros_ec_sensors_sync_probe,
> > +	.id_table	= cros_ec_sensors_sync_ids,
> > +};
> > +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> > +
> > +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor 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 805652250960..2bf183425eaf 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
> > @@ -22,6 +22,7 @@
> >  static char *cros_ec_loc[] = {
> >  	[MOTIONSENSE_LOC_BASE] = "base",
> >  	[MOTIONSENSE_LOC_LID] = "lid",
> > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> >  	[MOTIONSENSE_LOC_MAX] = "unknown",
> >  };
> >  
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index 41dccced5026..1c5c2c38af88 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct
> > cros_ec_dev *ec)
> >  		case MOTIONSENSE_TYPE_ACTIVITY:
> >  			sensor_cells[id].name = "cros-ec-activity";
> >  			break;
> > +		case MOTIONSENSE_TYPE_SYNC:
> > +			sensor_cells[id].name = "cros-ec-sync";
> > +			break;
> >  		default:
> >  			dev_warn(ec->dev, "unknown type %d\n", resp-
> > >info.type);
> >  			continue;
> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index a061cdcdef7c..1198e675306f 100644
> > --- a/include/linux/counter.h
> > +++ b/include/linux/counter.h
> > @@ -488,6 +488,7 @@ enum counter_signal_value_type {
> >  
> >  enum counter_count_value_type {
> >  	COUNTER_COUNT_POSITION = 0,
> > +	COUNTER_COUNT_TALLY
> >  };
> >  
> >  void counter_signal_read_value_set(struct
> > counter_signal_read_value *const val,

William Breathitt Gray Aug. 29, 2019, 11:34 a.m. UTC | #3

On Fri, Aug 23, 2019 at 02:41:27PM +0200, Fabien Lahoudere wrote:
> From: Gwendal Grignou <[e-mail]>
> 
> EC returns a counter when there is an event on camera vsync.
> This patch comes from chromeos kernel 4.4
> 
> Signed-off-by: Gwendal Grignou <[e-mail]>
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> 
> CROS EC sync sensor was originally designed as an IIO device.
> Now that the counter subsystem will replace IIO_COUNTER, we
> have to implement a new way to get sync count.
> 
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> ---
>  Documentation/driver-api/generic-counter.rst  |   3 +
>  MAINTAINERS                                   |   7 +
>  drivers/counter/Kconfig                       |   9 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/counter.c                     |   2 +
>  drivers/counter/cros_ec_sensors_sync.c        | 208 ++++++++++++++++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
>  drivers/mfd/cros_ec_dev.c                     |   3 +
>  include/linux/counter.h                       |   1 +
>  9 files changed, 235 insertions(+)
>  create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> 
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index 8382f01a53e3..beb80714ac8b 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -44,6 +44,9 @@ Counter interface provides the following available count data types:
>  * COUNT_POSITION:
>    Unsigned integer value representing position.
>  
> +* COUNT_TALLY:
> +  Unsigned integer value representing tally.
> +
>  A Count has a count function mode which represents the update behavior
>  for the count data. The Generic Counter interface provides the following
>  available count function modes:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e60f5c361969..83bd291d103e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3902,6 +3902,13 @@ R:	Guenter Roeck <[e-mail]>
>  F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
>  F:	sound/soc/codecs/cros_ec_codec.*
>  
> +CHROMEOS EC COUNTER DRIVER
> +M:	Fabien Lahoudere <[e-mail]>
> +M:	William Breathitt Gray <[e-mail]>

No need to include me here since I'm already listed as the maintainer of
the Counter subsystem in its respective entry.

> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/counter/cros_ec_sensors_sync.c
> +
>  CIRRUS LOGIC AUDIO CODEC DRIVERS
>  M:	Brian Austin <[e-mail]>
>  M:	Paul Handrigan <[e-mail]>
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..22287f5715e5 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,13 @@ config FTM_QUADDEC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ftm-quaddec.
>  
> +config IIO_CROS_EC_SENSORS_SYNC
> +	tristate "ChromeOS EC Counter Sensors"
> +	depends on IIO_CROS_EC_SENSORS_CORE && IIO
> +	help
> +	  Module to handle synchronisation sensors presented by the ChromeOS EC
> +	  Sensor hub.
> +	  Synchronisation sensors are counter sensors triggered when events
> +	  occurs from other subsystems.
> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..6fe4c98a446f 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..53525b109094 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct counter_count_read_value *const val,
>  {
>  	switch (type) {
>  	case COUNTER_COUNT_POSITION:
> +	case COUNTER_COUNT_TALLY:
>  		val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
>  		break;
>  	default:
> @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const data,
>  
>  	switch (type) {
>  	case COUNTER_COUNT_POSITION:
> +	case COUNTER_COUNT_TALLY:
>  		err = kstrtoul(val->buf, 0, data);
>  		if (err)
>  			return err;

Dedicate the core counter subsystem related changes to their own patch
so that we can commit them separately. That'll make it easier both to
evaluate the changes and debug later by keeping the history clear.

> diff --git a/drivers/counter/cros_ec_sensors_sync.c b/drivers/counter/cros_ec_sensors_sync.c
> new file mode 100644
> index 000000000000..b6f5e2c6da9f
> --- /dev/null
> +++ b/drivers/counter/cros_ec_sensors_sync.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver of counter incremented after events on interrupt line in EC.
> + *
> + * Copyright 2018 Google, Inc
> + */
> +
> +#include <linux/device.h>
> +#include <linux/counter.h>
> +#include <linux/iio/common/cros_ec_sensors_core.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "cros-ec-sync"
> +
> +/*
> + * One channel for counter, the other for timestamp.
> + */
> +#define MAX_CHANNELS (1)
> +
> +/* State data for ec_sensors iio driver. */
> +struct cros_ec_sensors_sync_state {
> +	/* Shared by all sensors */
> +	struct cros_ec_sensors_core_state core;
> +	struct counter_device counter;
> +	struct iio_chan_spec channels[MAX_CHANNELS];
> +};
> +
> +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     int *val, int *val2, long mask)
> +{
> +	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> +	u16 data;
> +	int ret;
> +
> +	mutex_lock(&st->core.cmd_lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &data);
> +		if (ret < 0)
> +			break;
> +		ret = IIO_VAL_INT;
> +		*val = data;
> +		break;
> +	default:
> +		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> +						mask);
> +		break;
> +	}
> +	mutex_unlock(&st->core.cmd_lock);
> +	return ret;
> +}
> +
> +static struct iio_info cros_ec_sensors_sync_info = {
> +	.read_raw = &cros_ec_sensors_sync_read,
> +	.read_avail = &cros_ec_sensors_core_read_avail,
> +};
> +
> +static struct counter_count cros_ec_sync_counts = {
> +	.id = 0,
> +	.name = "Cros EC sync counter",
> +};
> +
> +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				struct counter_count_read_value *val)
> +{
> +	s16 cnt;
> +	int ret;
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +	unsigned long data;
> +
> +	mutex_lock(&st->core.cmd_lock);
> +	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> +	mutex_unlock(&st->core.cmd_lock);
> +	if (ret != 0) {
> +		dev_warn(&indio_dev->dev, "Unable to read sensor data\n");
> +		return ret;
> +	}
> +
> +	data = (unsigned long) cnt;

This cast is not necessary since the data variable is already unsigned
long; conversion can occur implicitly with a simple set operation.

> +	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops cros_ec_sync_cnt_ops = {
> +	.count_read = cros_ec_sync_cnt_read,
> +};

Like in my previous review: does the hardware allow the counter to be
reset back to zero, or is it purely read-only? It's okay if you want to
focus on just reading for this patch, but it'll be good to know the
capabilities of this hardware if we want support for it in the future.

> +
> +static char *cros_ec_loc[] = {
> +	[MOTIONSENSE_LOC_BASE] = "base",
> +	[MOTIONSENSE_LOC_LID] = "lid",
> +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> +	[MOTIONSENSE_LOC_MAX] = "unknown",
> +};
> +
> +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> +				  void *private, char *buf)
> +{
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", st->core.param.info.sensor_num);
> +}
> +
> +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> +				   void *private, char *buf)
> +{
> +	struct iio_dev *indio_dev = counter->priv;
> +	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->core.loc]);
> +}
> +
> +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> +	{
> +		.name = "id",
> +		.read = cros_ec_sync_id
> +	},
> +	{
> +		.name = "location",
> +		.read = cros_ec_sync_loc
> +	},
> +};

Create a corresponding documentation file to describe these attributes:
Documentation/ABI/testing/sysfs-bus-counter-cros_ec_sensors_sync

> +
> +static int cros_ec_sensors_sync_probe(struct platform_device *pdev)
> +{
> +	struct cros_ec_sensors_sync_state *state;
> +	struct device *dev = &pdev->dev;
> +	struct iio_chan_spec *channel;
> +	struct iio_dev *indio_dev;
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &cros_ec_sensors_sync_info;
> +	state = iio_priv(indio_dev);
> +
> +	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> +		return -EINVAL;
> +
> +	/* Initialize IIO device */
> +	channel = state->channels;
> +	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;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      cros_ec_sensors_capture, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Initialize counter device */
> +	state->counter.name = dev_name(&pdev->dev);
> +	state->counter.parent = &pdev->dev;
> +	state->counter.counts = &cros_ec_sync_counts;
> +	state->counter.num_counts = 1;
> +	state->counter.priv = indio_dev;
> +	state->counter.ops = &cros_ec_sync_cnt_ops;
> +	state->counter.ext = cros_ec_sync_cnt_ext;
> +	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> +
> +	return devm_counter_register(&pdev->dev, &state->counter);
> +}
> +
> +static const struct platform_device_id cros_ec_sensors_sync_ids[] = {
> +	{ .name = DRV_NAME, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> +
> +static struct platform_driver cros_ec_sensors_sync_platform_driver = {
> +	.driver = {
> +		.name	= DRV_NAME,
> +		.pm	= &cros_ec_sensors_pm_ops,
> +	},
> +	.probe		= cros_ec_sensors_sync_probe,
> +	.id_table	= cros_ec_sensors_sync_ids,
> +};
> +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> +
> +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor driver");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_LICENSE("GPL v2");

From the email you sent in response to Jonathan Cameron's comments, the
IIO code in this driver will be removed so that should take care of the
error message reported by the kbuild test robot. With only the Counter
subsystem to worry about, this driver should end up a lot simpler in v3.
I'm looking forward to it. :-)

William Breathitt Gray

> 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 805652250960..2bf183425eaf 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
> @@ -22,6 +22,7 @@
>  static char *cros_ec_loc[] = {
>  	[MOTIONSENSE_LOC_BASE] = "base",
>  	[MOTIONSENSE_LOC_LID] = "lid",
> +	[MOTIONSENSE_LOC_CAMERA] = "camera",
>  	[MOTIONSENSE_LOC_MAX] = "unknown",
>  };
>  
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 41dccced5026..1c5c2c38af88 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  		case MOTIONSENSE_TYPE_ACTIVITY:
>  			sensor_cells[id].name = "cros-ec-activity";
>  			break;
> +		case MOTIONSENSE_TYPE_SYNC:
> +			sensor_cells[id].name = "cros-ec-sync";
> +			break;
>  		default:
>  			dev_warn(ec->dev, "unknown type %d\n", resp->info.type);
>  			continue;
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..1198e675306f 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -488,6 +488,7 @@ enum counter_signal_value_type {
>  
>  enum counter_count_value_type {
>  	COUNTER_COUNT_POSITION = 0,
> +	COUNTER_COUNT_TALLY
>  };
>  
>  void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> -- 
> 2.20.1
>

Lee Jones Sept. 2, 2019, 9:40 a.m. UTC | #4

On Fri, 23 Aug 2019, Fabien Lahoudere wrote:

> From: Gwendal Grignou <[e-mail]>
> 
> EC returns a counter when there is an event on camera vsync.
> This patch comes from chromeos kernel 4.4
> 
> Signed-off-by: Gwendal Grignou <[e-mail]>
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> 
> CROS EC sync sensor was originally designed as an IIO device.
> Now that the counter subsystem will replace IIO_COUNTER, we
> have to implement a new way to get sync count.
> 
> Signed-off-by: Fabien Lahoudere <[e-mail]>
> ---
>  Documentation/driver-api/generic-counter.rst  |   3 +
>  MAINTAINERS                                   |   7 +
>  drivers/counter/Kconfig                       |   9 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/counter.c                     |   2 +
>  drivers/counter/cros_ec_sensors_sync.c        | 208 ++++++++++++++++++
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +

>  drivers/mfd/cros_ec_dev.c                     |   3 +

I can't see any reason for this change to be squashed into this patch.

Please separate it out.

>  include/linux/counter.h                       |   1 +
>  9 files changed, 235 insertions(+)
>  create mode 100644 drivers/counter/cros_ec_sensors_sync.c

Fabien Lahoudere Sept. 24, 2019, 2:20 p.m. UTC | #5

Hi all,

After some discussions and investigation, the timestamp is very
important for that sync driver.
Google team uses that timestamp to compare with gyroscope timestamp.

So the important data is timestamp and counter value is useless.
Just the event of counter increment is important to get a timestamp.

In that case, my idea was to just use an IIO driver with a single
channel with IIO_TIMESTAMP. We discuss this here and it seems
controversial.

So my question to Jonathan is if we have a timestamp coming from the EC
itself, can we consider this timestamp as a good IIO driver?

Any other idea is welcome, however Google team would like to manage
only IIO drivers if possible.

Thanks


Le jeudi 29 août 2019 à 13:10 +0200, Fabien Lahoudere a écrit :
> Le lundi 26 août 2019 à 09:56 +0100, Jonathan Cameron a écrit :
> > On Fri, 23 Aug 2019 14:41:27 +0200
> > Fabien Lahoudere <[e-mail]> wrote:
> > 
> > > From: Gwendal Grignou <[e-mail]>
> > > 
> > > EC returns a counter when there is an event on camera vsync.
> > > This patch comes from chromeos kernel 4.4
> > > 
> > > Signed-off-by: Gwendal Grignou <[e-mail]>
> > > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > > 
> > > CROS EC sync sensor was originally designed as an IIO device.
> > > Now that the counter subsystem will replace IIO_COUNTER, we
> > > have to implement a new way to get sync count.
> > 
> > I'm curious.  What is this counter used for?
> 
> It is a counter of a pin connected to a camera vsync.
> It is used to manage correct orientation of the camera when the
> gyroscope detect a change.
> 
> > This combined counter and iio driver isn't something we would
> > normally
> > want to support.  What is the reasoning behind doing both
> > interfaces?
> 
> That patch was originally designed using only IIO with IIO_count and
> IIO_TIMESTAMP channel. As IIO_COUNT is deprecated, we need to use the
> new counter subsystem. So I just split the driver in two part counter
> and iio timestamp.
> 
> However after your email I did some investigation and did not find
> any
> use of the timestamp for that sensor. Timestamp is just a common
> channel implemented on all CROS EC sensors.
> 
> So I think I will drop it now and if a timestamp is needed, I will
> probably restart the discussion about timestamp in the counter
> subsystemif that make sense to have one.
> 
> Thanks
> 
> Fabien
> 
> > > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > > ---
> > >  Documentation/driver-api/generic-counter.rst  |   3 +
> > >  MAINTAINERS                                   |   7 +
> > >  drivers/counter/Kconfig                       |   9 +
> > >  drivers/counter/Makefile                      |   1 +
> > >  drivers/counter/counter.c                     |   2 +
> > >  drivers/counter/cros_ec_sensors_sync.c        | 208
> > > ++++++++++++++++++
> > >  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
> > >  drivers/mfd/cros_ec_dev.c                     |   3 +
> > >  include/linux/counter.h                       |   1 +
> > >  9 files changed, 235 insertions(+)
> > >  create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> > > 
> > > diff --git a/Documentation/driver-api/generic-counter.rst
> > > b/Documentation/driver-api/generic-counter.rst
> > > index 8382f01a53e3..beb80714ac8b 100644
> > > --- a/Documentation/driver-api/generic-counter.rst
> > > +++ b/Documentation/driver-api/generic-counter.rst
> > > @@ -44,6 +44,9 @@ Counter interface provides the following
> > > available count data types:
> > >  * COUNT_POSITION:
> > >    Unsigned integer value representing position.
> > >  
> > > +* COUNT_TALLY:
> > > +  Unsigned integer value representing tally.
> > > +
> > >  A Count has a count function mode which represents the update
> > > behavior
> > >  for the count data. The Generic Counter interface provides the
> > > following
> > >  available count function modes:
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e60f5c361969..83bd291d103e 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -3902,6 +3902,13 @@ R:	Guenter Roeck <[e-mail]>
> > >  F:	Documentation/devicetree/bindings/sound/google,cros-ec-
> > > codec.txt
> > >  F:	sound/soc/codecs/cros_ec_codec.*
> > >  
> > > +CHROMEOS EC COUNTER DRIVER
> > > +M:	Fabien Lahoudere <[e-mail]>
> > > +M:	William Breathitt Gray <[e-mail]>
> > > +L:	[e-mail]
> > > +S:	Maintained
> > > +F:	drivers/counter/cros_ec_sensors_sync.c
> > > +
> > >  CIRRUS LOGIC AUDIO CODEC DRIVERS
> > >  M:	Brian Austin <[e-mail]>
> > >  M:	Paul Handrigan <[e-mail]>
> > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> > > index 2967d0a9ff91..22287f5715e5 100644
> > > --- a/drivers/counter/Kconfig
> > > +++ b/drivers/counter/Kconfig
> > > @@ -59,4 +59,13 @@ config FTM_QUADDEC
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called ftm-quaddec.
> > >  
> > > +config IIO_CROS_EC_SENSORS_SYNC
> > > +	tristate "ChromeOS EC Counter Sensors"
> > > +	depends on IIO_CROS_EC_SENSORS_CORE && IIO
> > > +	help
> > > +	  Module to handle synchronisation sensors presented by the
> > > ChromeOS EC
> > > +	  Sensor hub.
> > > +	  Synchronisation sensors are counter sensors triggered when
> > > events
> > > +	  occurs from other subsystems.
> > > +
> > >  endif # COUNTER
> > > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> > > index 40d35522937d..6fe4c98a446f 100644
> > > --- a/drivers/counter/Makefile
> > > +++ b/drivers/counter/Makefile
> > > @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
> > >  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> > >  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> > >  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> > > +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> > > diff --git a/drivers/counter/counter.c
> > > b/drivers/counter/counter.c
> > > index 106bc7180cd8..53525b109094 100644
> > > --- a/drivers/counter/counter.c
> > > +++ b/drivers/counter/counter.c
> > > @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct
> > > counter_count_read_value *const val,
> > >  {
> > >  	switch (type) {
> > >  	case COUNTER_COUNT_POSITION:
> > > +	case COUNTER_COUNT_TALLY:
> > >  		val->len = sprintf(val->buf, "%lu\n", *(unsigned long
> > > *)data);
> > >  		break;
> > >  	default:
> > > @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const
> > > data,
> > >  
> > >  	switch (type) {
> > >  	case COUNTER_COUNT_POSITION:
> > > +	case COUNTER_COUNT_TALLY:
> > >  		err = kstrtoul(val->buf, 0, data);
> > >  		if (err)
> > >  			return err;
> > > diff --git a/drivers/counter/cros_ec_sensors_sync.c
> > > b/drivers/counter/cros_ec_sensors_sync.c
> > > new file mode 100644
> > > index 000000000000..b6f5e2c6da9f
> > > --- /dev/null
> > > +++ b/drivers/counter/cros_ec_sensors_sync.c
> > > @@ -0,0 +1,208 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver of counter incremented after events on interrupt line
> > > in
> > > EC.
> > > + *
> > > + * Copyright 2018 Google, Inc
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/counter.h>
> > > +#include <linux/iio/common/cros_ec_sensors_core.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mfd/cros_ec.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define DRV_NAME "cros-ec-sync"
> > > +
> > > +/*
> > > + * One channel for counter, the other for timestamp.
> > > + */
> > > +#define MAX_CHANNELS (1)
> > > +
> > > +/* State data for ec_sensors iio driver. */
> > > +struct cros_ec_sensors_sync_state {
> > > +	/* Shared by all sensors */
> > > +	struct cros_ec_sensors_core_state core;
> > > +	struct counter_device counter;
> > > +	struct iio_chan_spec channels[MAX_CHANNELS];
> > > +};
> > > +
> > > +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> > > +				     struct iio_chan_spec const *chan,
> > > +				     int *val, int *val2, long mask)
> > > +{
> > > +	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> > > +	u16 data;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&st->core.cmd_lock);
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0),
> > > &data);
> > > +		if (ret < 0)
> > > +			break;
> > > +		ret = IIO_VAL_INT;
> > > +		*val = data;
> > > +		break;
> > > +	default:
> > > +		ret = cros_ec_sensors_core_read(&st->core, chan, val,
> > > val2,
> > > +						mask);
> > > +		break;
> > > +	}
> > > +	mutex_unlock(&st->core.cmd_lock);
> > > +	return ret;
> > > +}
> > > +
> > > +static struct iio_info cros_ec_sensors_sync_info = {
> > > +	.read_raw = &cros_ec_sensors_sync_read,
> > > +	.read_avail = &cros_ec_sensors_core_read_avail,
> > > +};
> > > +
> > > +static struct counter_count cros_ec_sync_counts = {
> > > +	.id = 0,
> > > +	.name = "Cros EC sync counter",
> > > +};
> > > +
> > > +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> > > +				struct counter_count *count,
> > > +				struct counter_count_read_value *val)
> > > +{
> > > +	s16 cnt;
> > > +	int ret;
> > > +	struct iio_dev *indio_dev = counter->priv;
> > > +	struct cros_ec_sensors_sync_state *const st =
> > > iio_priv(indio_dev);
> > > +	unsigned long data;
> > > +
> > > +	mutex_lock(&st->core.cmd_lock);
> > > +	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> > > +	mutex_unlock(&st->core.cmd_lock);
> > > +	if (ret != 0) {
> > > +		dev_warn(&indio_dev->dev, "Unable to read sensor
> > > data\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	data = (unsigned long) cnt;
> > > +	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct counter_ops cros_ec_sync_cnt_ops = {
> > > +	.count_read = cros_ec_sync_cnt_read,
> > > +};
> > > +
> > > +static char *cros_ec_loc[] = {
> > > +	[MOTIONSENSE_LOC_BASE] = "base",
> > > +	[MOTIONSENSE_LOC_LID] = "lid",
> > > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> > > +	[MOTIONSENSE_LOC_MAX] = "unknown",
> > > +};
> > > +
> > > +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> > > +				  void *private, char *buf)
> > > +{
> > > +	struct iio_dev *indio_dev = counter->priv;
> > > +	struct cros_ec_sensors_sync_state *const st =
> > > iio_priv(indio_dev);
> > > +
> > > +	return snprintf(buf, PAGE_SIZE, "%d\n", st-
> > > > core.param.info.sensor_num);
> > > +}
> > > +
> > > +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> > > +				   void *private, char *buf)
> > > +{
> > > +	struct iio_dev *indio_dev = counter->priv;
> > > +	struct cros_ec_sensors_sync_state *const st =
> > > iio_priv(indio_dev);
> > > +
> > > +	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st-
> > > > core.loc]);
> > > +}
> > > +
> > > +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> > > +	{
> > > +		.name = "id",
> > > +		.read = cros_ec_sync_id
> > > +	},
> > > +	{
> > > +		.name = "location",
> > > +		.read = cros_ec_sync_loc
> > > +	},
> > > +};
> > > +
> > > +static int cros_ec_sensors_sync_probe(struct platform_device
> > > *pdev)
> > > +{
> > > +	struct cros_ec_sensors_sync_state *state;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct iio_chan_spec *channel;
> > > +	struct iio_dev *indio_dev;
> > > +	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);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	indio_dev->info = &cros_ec_sensors_sync_info;
> > > +	state = iio_priv(indio_dev);
> > > +
> > > +	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> > > +		return -EINVAL;
> > > +
> > > +	/* Initialize IIO device */
> > > +	channel = state->channels;
> > > +	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;
> > > +
> > > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > +					      cros_ec_sensors_capture,
> > > NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = devm_iio_device_register(dev, indio_dev);
> > 
> > Hmm. Wasn't expecting to see that here if it's a counter device.
> > 
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Initialize counter device */
> > > +	state->counter.name = dev_name(&pdev->dev);
> > > +	state->counter.parent = &pdev->dev;
> > > +	state->counter.counts = &cros_ec_sync_counts;
> > > +	state->counter.num_counts = 1;
> > > +	state->counter.priv = indio_dev;
> > > +	state->counter.ops = &cros_ec_sync_cnt_ops;
> > > +	state->counter.ext = cros_ec_sync_cnt_ext;
> > > +	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> > > +
> > > +	return devm_counter_register(&pdev->dev, &state->counter);
> > > +}
> > > +
> > > +static const struct platform_device_id
> > > cros_ec_sensors_sync_ids[]
> > > = {
> > > +	{ .name = DRV_NAME, },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> > > +
> > > +static struct platform_driver
> > > cros_ec_sensors_sync_platform_driver
> > > = {
> > > +	.driver = {
> > > +		.name	= DRV_NAME,
> > > +		.pm	= &cros_ec_sensors_pm_ops,
> > > +	},
> > > +	.probe		= cros_ec_sensors_sync_probe,
> > > +	.id_table	= cros_ec_sensors_sync_ids,
> > > +};
> > > +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> > > +
> > > +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor 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 805652250960..2bf183425eaf 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
> > > @@ -22,6 +22,7 @@
> > >  static char *cros_ec_loc[] = {
> > >  	[MOTIONSENSE_LOC_BASE] = "base",
> > >  	[MOTIONSENSE_LOC_LID] = "lid",
> > > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> > >  	[MOTIONSENSE_LOC_MAX] = "unknown",
> > >  };
> > >  
> > > diff --git a/drivers/mfd/cros_ec_dev.c
> > > b/drivers/mfd/cros_ec_dev.c
> > > index 41dccced5026..1c5c2c38af88 100644
> > > --- a/drivers/mfd/cros_ec_dev.c
> > > +++ b/drivers/mfd/cros_ec_dev.c
> > > @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct
> > > cros_ec_dev *ec)
> > >  		case MOTIONSENSE_TYPE_ACTIVITY:
> > >  			sensor_cells[id].name = "cros-ec-activity";
> > >  			break;
> > > +		case MOTIONSENSE_TYPE_SYNC:
> > > +			sensor_cells[id].name = "cros-ec-sync";
> > > +			break;
> > >  		default:
> > >  			dev_warn(ec->dev, "unknown type %d\n", resp-
> > > > info.type);
> > >  			continue;
> > > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > > index a061cdcdef7c..1198e675306f 100644
> > > --- a/include/linux/counter.h
> > > +++ b/include/linux/counter.h
> > > @@ -488,6 +488,7 @@ enum counter_signal_value_type {
> > >  
> > >  enum counter_count_value_type {
> > >  	COUNTER_COUNT_POSITION = 0,
> > > +	COUNTER_COUNT_TALLY
> > >  };
> > >  
> > >  void counter_signal_read_value_set(struct
> > > counter_signal_read_value *const val,
> 
>

William Breathitt Gray Sept. 24, 2019, 2:52 p.m. UTC | #6

On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> Hi all,
> 
> After some discussions and investigation, the timestamp is very
> important for that sync driver.
> Google team uses that timestamp to compare with gyroscope timestamp.
> 
> So the important data is timestamp and counter value is useless.
> Just the event of counter increment is important to get a timestamp.
> 
> In that case, my idea was to just use an IIO driver with a single
> channel with IIO_TIMESTAMP. We discuss this here and it seems
> controversial.
> 
> So my question to Jonathan is if we have a timestamp coming from the EC
> itself, can we consider this timestamp as a good IIO driver?
> 
> Any other idea is welcome, however Google team would like to manage
> only IIO drivers if possible.
> 
> Thanks

Keep in mind that while you will not need the counter value for your
purposes, someone else might want it for some reason or other. Since it
is a counter, somewhere down the line this device will end up with a
driver in the Counter subsystem; and because timestamp support is
already being discussed for other counter devices (cf. Intel QEP driver
https://www.spinics.net/lists/linux-iio/msg46680.html), a conflict will
arise if this device has an IIO driver holding the memory space for
timestamps while a Counter driver tries to hold the same resources.

It's better in this case to access timestamps through the Counter
subsystem, rather than only partially support this device through IIO
just for timestamps.

William Breathitt Gray

> Le jeudi 29 août 2019 à 13:10 +0200, Fabien Lahoudere a écrit :
> > Le lundi 26 août 2019 à 09:56 +0100, Jonathan Cameron a écrit :
> > > On Fri, 23 Aug 2019 14:41:27 +0200
> > > Fabien Lahoudere <[e-mail]> wrote:
> > > 
> > > > From: Gwendal Grignou <[e-mail]>
> > > > 
> > > > EC returns a counter when there is an event on camera vsync.
> > > > This patch comes from chromeos kernel 4.4
> > > > 
> > > > Signed-off-by: Gwendal Grignou <[e-mail]>
> > > > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > > > 
> > > > CROS EC sync sensor was originally designed as an IIO device.
> > > > Now that the counter subsystem will replace IIO_COUNTER, we
> > > > have to implement a new way to get sync count.
> > > 
> > > I'm curious.  What is this counter used for?
> > 
> > It is a counter of a pin connected to a camera vsync.
> > It is used to manage correct orientation of the camera when the
> > gyroscope detect a change.
> > 
> > > This combined counter and iio driver isn't something we would
> > > normally
> > > want to support.  What is the reasoning behind doing both
> > > interfaces?
> > 
> > That patch was originally designed using only IIO with IIO_count and
> > IIO_TIMESTAMP channel. As IIO_COUNT is deprecated, we need to use the
> > new counter subsystem. So I just split the driver in two part counter
> > and iio timestamp.
> > 
> > However after your email I did some investigation and did not find
> > any
> > use of the timestamp for that sensor. Timestamp is just a common
> > channel implemented on all CROS EC sensors.
> > 
> > So I think I will drop it now and if a timestamp is needed, I will
> > probably restart the discussion about timestamp in the counter
> > subsystemif that make sense to have one.
> > 
> > Thanks
> > 
> > Fabien
> > 
> > > > Signed-off-by: Fabien Lahoudere <[e-mail]>
> > > > ---
> > > >  Documentation/driver-api/generic-counter.rst  |   3 +
> > > >  MAINTAINERS                                   |   7 +
> > > >  drivers/counter/Kconfig                       |   9 +
> > > >  drivers/counter/Makefile                      |   1 +
> > > >  drivers/counter/counter.c                     |   2 +
> > > >  drivers/counter/cros_ec_sensors_sync.c        | 208
> > > > ++++++++++++++++++
> > > >  .../cros_ec_sensors/cros_ec_sensors_core.c    |   1 +
> > > >  drivers/mfd/cros_ec_dev.c                     |   3 +
> > > >  include/linux/counter.h                       |   1 +
> > > >  9 files changed, 235 insertions(+)
> > > >  create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> > > > 
> > > > diff --git a/Documentation/driver-api/generic-counter.rst
> > > > b/Documentation/driver-api/generic-counter.rst
> > > > index 8382f01a53e3..beb80714ac8b 100644
> > > > --- a/Documentation/driver-api/generic-counter.rst
> > > > +++ b/Documentation/driver-api/generic-counter.rst
> > > > @@ -44,6 +44,9 @@ Counter interface provides the following
> > > > available count data types:
> > > >  * COUNT_POSITION:
> > > >    Unsigned integer value representing position.
> > > >  
> > > > +* COUNT_TALLY:
> > > > +  Unsigned integer value representing tally.
> > > > +
> > > >  A Count has a count function mode which represents the update
> > > > behavior
> > > >  for the count data. The Generic Counter interface provides the
> > > > following
> > > >  available count function modes:
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index e60f5c361969..83bd291d103e 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -3902,6 +3902,13 @@ R:	Guenter Roeck <[e-mail]>
> > > >  F:	Documentation/devicetree/bindings/sound/google,cros-ec-
> > > > codec.txt
> > > >  F:	sound/soc/codecs/cros_ec_codec.*
> > > >  
> > > > +CHROMEOS EC COUNTER DRIVER
> > > > +M:	Fabien Lahoudere <[e-mail]>
> > > > +M:	William Breathitt Gray <[e-mail]>
> > > > +L:	[e-mail]
> > > > +S:	Maintained
> > > > +F:	drivers/counter/cros_ec_sensors_sync.c
> > > > +
> > > >  CIRRUS LOGIC AUDIO CODEC DRIVERS
> > > >  M:	Brian Austin <[e-mail]>
> > > >  M:	Paul Handrigan <[e-mail]>
> > > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> > > > index 2967d0a9ff91..22287f5715e5 100644
> > > > --- a/drivers/counter/Kconfig
> > > > +++ b/drivers/counter/Kconfig
> > > > @@ -59,4 +59,13 @@ config FTM_QUADDEC
> > > >  	  To compile this driver as a module, choose M here: the
> > > >  	  module will be called ftm-quaddec.
> > > >  
> > > > +config IIO_CROS_EC_SENSORS_SYNC
> > > > +	tristate "ChromeOS EC Counter Sensors"
> > > > +	depends on IIO_CROS_EC_SENSORS_CORE && IIO
> > > > +	help
> > > > +	  Module to handle synchronisation sensors presented by the
> > > > ChromeOS EC
> > > > +	  Sensor hub.
> > > > +	  Synchronisation sensors are counter sensors triggered when
> > > > events
> > > > +	  occurs from other subsystems.
> > > > +
> > > >  endif # COUNTER
> > > > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> > > > index 40d35522937d..6fe4c98a446f 100644
> > > > --- a/drivers/counter/Makefile
> > > > +++ b/drivers/counter/Makefile
> > > > @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
> > > >  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> > > >  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> > > >  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> > > > +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> > > > diff --git a/drivers/counter/counter.c
> > > > b/drivers/counter/counter.c
> > > > index 106bc7180cd8..53525b109094 100644
> > > > --- a/drivers/counter/counter.c
> > > > +++ b/drivers/counter/counter.c
> > > > @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct
> > > > counter_count_read_value *const val,
> > > >  {
> > > >  	switch (type) {
> > > >  	case COUNTER_COUNT_POSITION:
> > > > +	case COUNTER_COUNT_TALLY:
> > > >  		val->len = sprintf(val->buf, "%lu\n", *(unsigned long
> > > > *)data);
> > > >  		break;
> > > >  	default:
> > > > @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const
> > > > data,
> > > >  
> > > >  	switch (type) {
> > > >  	case COUNTER_COUNT_POSITION:
> > > > +	case COUNTER_COUNT_TALLY:
> > > >  		err = kstrtoul(val->buf, 0, data);
> > > >  		if (err)
> > > >  			return err;
> > > > diff --git a/drivers/counter/cros_ec_sensors_sync.c
> > > > b/drivers/counter/cros_ec_sensors_sync.c
> > > > new file mode 100644
> > > > index 000000000000..b6f5e2c6da9f
> > > > --- /dev/null
> > > > +++ b/drivers/counter/cros_ec_sensors_sync.c
> > > > @@ -0,0 +1,208 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Driver of counter incremented after events on interrupt line
> > > > in
> > > > EC.
> > > > + *
> > > > + * Copyright 2018 Google, Inc
> > > > + */
> > > > +
> > > > +#include <linux/device.h>
> > > > +#include <linux/counter.h>
> > > > +#include <linux/iio/common/cros_ec_sensors_core.h>
> > > > +#include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mfd/cros_ec.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define DRV_NAME "cros-ec-sync"
> > > > +
> > > > +/*
> > > > + * One channel for counter, the other for timestamp.
> > > > + */
> > > > +#define MAX_CHANNELS (1)
> > > > +
> > > > +/* State data for ec_sensors iio driver. */
> > > > +struct cros_ec_sensors_sync_state {
> > > > +	/* Shared by all sensors */
> > > > +	struct cros_ec_sensors_core_state core;
> > > > +	struct counter_device counter;
> > > > +	struct iio_chan_spec channels[MAX_CHANNELS];
> > > > +};
> > > > +
> > > > +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> > > > +				     struct iio_chan_spec const *chan,
> > > > +				     int *val, int *val2, long mask)
> > > > +{
> > > > +	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> > > > +	u16 data;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&st->core.cmd_lock);
> > > > +	switch (mask) {
> > > > +	case IIO_CHAN_INFO_RAW:
> > > > +		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0),
> > > > &data);
> > > > +		if (ret < 0)
> > > > +			break;
> > > > +		ret = IIO_VAL_INT;
> > > > +		*val = data;
> > > > +		break;
> > > > +	default:
> > > > +		ret = cros_ec_sensors_core_read(&st->core, chan, val,
> > > > val2,
> > > > +						mask);
> > > > +		break;
> > > > +	}
> > > > +	mutex_unlock(&st->core.cmd_lock);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static struct iio_info cros_ec_sensors_sync_info = {
> > > > +	.read_raw = &cros_ec_sensors_sync_read,
> > > > +	.read_avail = &cros_ec_sensors_core_read_avail,
> > > > +};
> > > > +
> > > > +static struct counter_count cros_ec_sync_counts = {
> > > > +	.id = 0,
> > > > +	.name = "Cros EC sync counter",
> > > > +};
> > > > +
> > > > +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> > > > +				struct counter_count *count,
> > > > +				struct counter_count_read_value *val)
> > > > +{
> > > > +	s16 cnt;
> > > > +	int ret;
> > > > +	struct iio_dev *indio_dev = counter->priv;
> > > > +	struct cros_ec_sensors_sync_state *const st =
> > > > iio_priv(indio_dev);
> > > > +	unsigned long data;
> > > > +
> > > > +	mutex_lock(&st->core.cmd_lock);
> > > > +	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> > > > +	mutex_unlock(&st->core.cmd_lock);
> > > > +	if (ret != 0) {
> > > > +		dev_warn(&indio_dev->dev, "Unable to read sensor
> > > > data\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	data = (unsigned long) cnt;
> > > > +	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct counter_ops cros_ec_sync_cnt_ops = {
> > > > +	.count_read = cros_ec_sync_cnt_read,
> > > > +};
> > > > +
> > > > +static char *cros_ec_loc[] = {
> > > > +	[MOTIONSENSE_LOC_BASE] = "base",
> > > > +	[MOTIONSENSE_LOC_LID] = "lid",
> > > > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> > > > +	[MOTIONSENSE_LOC_MAX] = "unknown",
> > > > +};
> > > > +
> > > > +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> > > > +				  void *private, char *buf)
> > > > +{
> > > > +	struct iio_dev *indio_dev = counter->priv;
> > > > +	struct cros_ec_sensors_sync_state *const st =
> > > > iio_priv(indio_dev);
> > > > +
> > > > +	return snprintf(buf, PAGE_SIZE, "%d\n", st-
> > > > > core.param.info.sensor_num);
> > > > +}
> > > > +
> > > > +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> > > > +				   void *private, char *buf)
> > > > +{
> > > > +	struct iio_dev *indio_dev = counter->priv;
> > > > +	struct cros_ec_sensors_sync_state *const st =
> > > > iio_priv(indio_dev);
> > > > +
> > > > +	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st-
> > > > > core.loc]);
> > > > +}
> > > > +
> > > > +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> > > > +	{
> > > > +		.name = "id",
> > > > +		.read = cros_ec_sync_id
> > > > +	},
> > > > +	{
> > > > +		.name = "location",
> > > > +		.read = cros_ec_sync_loc
> > > > +	},
> > > > +};
> > > > +
> > > > +static int cros_ec_sensors_sync_probe(struct platform_device
> > > > *pdev)
> > > > +{
> > > > +	struct cros_ec_sensors_sync_state *state;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct iio_chan_spec *channel;
> > > > +	struct iio_dev *indio_dev;
> > > > +	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);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	indio_dev->info = &cros_ec_sensors_sync_info;
> > > > +	state = iio_priv(indio_dev);
> > > > +
> > > > +	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* Initialize IIO device */
> > > > +	channel = state->channels;
> > > > +	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;
> > > > +
> > > > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > > +					      cros_ec_sensors_capture,
> > > > NULL);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = devm_iio_device_register(dev, indio_dev);
> > > 
> > > Hmm. Wasn't expecting to see that here if it's a counter device.
> > > 
> > > 
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Initialize counter device */
> > > > +	state->counter.name = dev_name(&pdev->dev);
> > > > +	state->counter.parent = &pdev->dev;
> > > > +	state->counter.counts = &cros_ec_sync_counts;
> > > > +	state->counter.num_counts = 1;
> > > > +	state->counter.priv = indio_dev;
> > > > +	state->counter.ops = &cros_ec_sync_cnt_ops;
> > > > +	state->counter.ext = cros_ec_sync_cnt_ext;
> > > > +	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> > > > +
> > > > +	return devm_counter_register(&pdev->dev, &state->counter);
> > > > +}
> > > > +
> > > > +static const struct platform_device_id
> > > > cros_ec_sensors_sync_ids[]
> > > > = {
> > > > +	{ .name = DRV_NAME, },
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> > > > +
> > > > +static struct platform_driver
> > > > cros_ec_sensors_sync_platform_driver
> > > > = {
> > > > +	.driver = {
> > > > +		.name	= DRV_NAME,
> > > > +		.pm	= &cros_ec_sensors_pm_ops,
> > > > +	},
> > > > +	.probe		= cros_ec_sensors_sync_probe,
> > > > +	.id_table	= cros_ec_sensors_sync_ids,
> > > > +};
> > > > +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor 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 805652250960..2bf183425eaf 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
> > > > @@ -22,6 +22,7 @@
> > > >  static char *cros_ec_loc[] = {
> > > >  	[MOTIONSENSE_LOC_BASE] = "base",
> > > >  	[MOTIONSENSE_LOC_LID] = "lid",
> > > > +	[MOTIONSENSE_LOC_CAMERA] = "camera",
> > > >  	[MOTIONSENSE_LOC_MAX] = "unknown",
> > > >  };
> > > >  
> > > > diff --git a/drivers/mfd/cros_ec_dev.c
> > > > b/drivers/mfd/cros_ec_dev.c
> > > > index 41dccced5026..1c5c2c38af88 100644
> > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct
> > > > cros_ec_dev *ec)
> > > >  		case MOTIONSENSE_TYPE_ACTIVITY:
> > > >  			sensor_cells[id].name = "cros-ec-activity";
> > > >  			break;
> > > > +		case MOTIONSENSE_TYPE_SYNC:
> > > > +			sensor_cells[id].name = "cros-ec-sync";
> > > > +			break;
> > > >  		default:
> > > >  			dev_warn(ec->dev, "unknown type %d\n", resp-
> > > > > info.type);
> > > >  			continue;
> > > > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > > > index a061cdcdef7c..1198e675306f 100644
> > > > --- a/include/linux/counter.h
> > > > +++ b/include/linux/counter.h
> > > > @@ -488,6 +488,7 @@ enum counter_signal_value_type {
> > > >  
> > > >  enum counter_count_value_type {
> > > >  	COUNTER_COUNT_POSITION = 0,
> > > > +	COUNTER_COUNT_TALLY
> > > >  };
> > > >  
> > > >  void counter_signal_read_value_set(struct
> > > > counter_signal_read_value *const val,
> > 
> > 
>

William Breathitt Gray Nov. 10, 2019, 3:14 p.m. UTC | #7

On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> Hi all,
> 
> After some discussions and investigation, the timestamp is very
> important for that sync driver.
> Google team uses that timestamp to compare with gyroscope timestamp.
> 
> So the important data is timestamp and counter value is useless.
> Just the event of counter increment is important to get a timestamp.
> 
> In that case, my idea was to just use an IIO driver with a single
> channel with IIO_TIMESTAMP. We discuss this here and it seems
> controversial.
> 
> So my question to Jonathan is if we have a timestamp coming from the EC
> itself, can we consider this timestamp as a good IIO driver?
> 
> Any other idea is welcome, however Google team would like to manage
> only IIO drivers if possible.
> 
> Thanks

Jonathan,

Should the the timestamp from the EC be introduced as an IIO driver
using IIO_TIMESTAMP?

Since there is no corresponding EC Counter driver in the baseline right
now we don't have a conflict yet. If the EC timestamp is introduced as
an IIO driver then we should make any future EC Counter driver mutually
exclusive with the IIO driver in order to prevent any memory space
conflict. At that point we may deprecate the IIO driver and move the
timestamp functionality to the corresponding Counter driver.

That's assuming someone is interested in the Count component enough to
implement an EC Counter driver; otherwise, the IIO driver will serve
just fine if timestamp is the only data desired from this device.

William Breathitt Gray

Jonathan Cameron Nov. 11, 2019, 11:49 a.m. UTC | #8

On Sun, 10 Nov 2019 10:14:08 -0500
William Breathitt Gray <[e-mail]> wrote:

> On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> > Hi all,
> > 
> > After some discussions and investigation, the timestamp is very
> > important for that sync driver.
> > Google team uses that timestamp to compare with gyroscope timestamp.
> > 
> > So the important data is timestamp and counter value is useless.
> > Just the event of counter increment is important to get a timestamp.
> > 
> > In that case, my idea was to just use an IIO driver with a single
> > channel with IIO_TIMESTAMP. We discuss this here and it seems
> > controversial.
> > 
> > So my question to Jonathan is if we have a timestamp coming from the EC
> > itself, can we consider this timestamp as a good IIO driver?
> > 
> > Any other idea is welcome, however Google team would like to manage
> > only IIO drivers if possible.
> > 
> > Thanks  
> 
> Jonathan,
> 
> Should the the timestamp from the EC be introduced as an IIO driver
> using IIO_TIMESTAMP?

It is is a rather odd driver but I suppose it would be fine with lots
of clear docs on why it is how it is...

> 
> Since there is no corresponding EC Counter driver in the baseline right
> now we don't have a conflict yet. If the EC timestamp is introduced as
> an IIO driver then we should make any future EC Counter driver mutually
> exclusive with the IIO driver in order to prevent any memory space
> conflict. At that point we may deprecate the IIO driver and move the
> timestamp functionality to the corresponding Counter driver.

That route does become somewhat of a mess so I suspect we'd have to have
a single driver supporting both userspace interfaces.  If you are happy
that we'd be adding a bit of legacy to support for ever then we can go
that way.

> 
> That's assuming someone is interested in the Count component enough to
> implement an EC Counter driver; otherwise, the IIO driver will serve
> just fine if timestamp is the only data desired from this device.
> 
> William Breathitt Gray

William Breathitt Gray Nov. 12, 2019, 1:16 a.m. UTC | #9

On Mon, Nov 11, 2019 at 11:49:55AM +0000, Jonathan Cameron wrote:
> On Sun, 10 Nov 2019 10:14:08 -0500
> William Breathitt Gray <[e-mail]> wrote:
> 
> > On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> > > Hi all,
> > > 
> > > After some discussions and investigation, the timestamp is very
> > > important for that sync driver.
> > > Google team uses that timestamp to compare with gyroscope timestamp.
> > > 
> > > So the important data is timestamp and counter value is useless.
> > > Just the event of counter increment is important to get a timestamp.
> > > 
> > > In that case, my idea was to just use an IIO driver with a single
> > > channel with IIO_TIMESTAMP. We discuss this here and it seems
> > > controversial.
> > > 
> > > So my question to Jonathan is if we have a timestamp coming from the EC
> > > itself, can we consider this timestamp as a good IIO driver?
> > > 
> > > Any other idea is welcome, however Google team would like to manage
> > > only IIO drivers if possible.
> > > 
> > > Thanks  
> > 
> > Jonathan,
> > 
> > Should the the timestamp from the EC be introduced as an IIO driver
> > using IIO_TIMESTAMP?
> 
> It is is a rather odd driver but I suppose it would be fine with lots
> of clear docs on why it is how it is...
> 
> > 
> > Since there is no corresponding EC Counter driver in the baseline right
> > now we don't have a conflict yet. If the EC timestamp is introduced as
> > an IIO driver then we should make any future EC Counter driver mutually
> > exclusive with the IIO driver in order to prevent any memory space
> > conflict. At that point we may deprecate the IIO driver and move the
> > timestamp functionality to the corresponding Counter driver.
> 
> That route does become somewhat of a mess so I suspect we'd have to have
> a single driver supporting both userspace interfaces.  If you are happy
> that we'd be adding a bit of legacy to support for ever then we can go
> that way.

Generally I'd prefer all components of a device to be supported, but
if this is as Fabien suggests that due to the nature of this particular
device the counter value is of no interest, then a Counter driver is of
little practical use here. In this particular case, it seems better to
restrict the driver support to just the timestamp functionality that
will be used, rather than introduce extra code to expose values that
will likely be ignored and risk adding code to the kernel that becomes
unmaintained due to lack of exposure or interest.

William Breathitt Gray

> 
> > 
> > That's assuming someone is interested in the Count component enough to
> > implement an EC Counter driver; otherwise, the IIO driver will serve
> > just fine if timestamp is the only data desired from this device.
> > 
> > William Breathitt Gray
> 
>

Gwendal Grignou April 8, 2020, 5:10 p.m. UTC | #10

I resend a counter driver for the EC at
https://patchwork.kernel.org/patch/11479437/

I tried a timestamp only IIO sensor, but this is not allowed, as the
timestamp channel is very specific: no extended parameters can be
added.
I did not add a COUNTER_COUNT_TALLY type, as a newer function
COUNTER_COUNT_FUNCTION_INCREASE, fits the counter better.
I am still using IIO_COUNT (inspired by the st counter driver) to
gather the event in real time on the iio side.

Gwendal.


On Mon, Nov 11, 2019 at 5:16 PM William Breathitt Gray
<[e-mail]> wrote:
>
> On Mon, Nov 11, 2019 at 11:49:55AM +0000, Jonathan Cameron wrote:
> > On Sun, 10 Nov 2019 10:14:08 -0500
> > William Breathitt Gray <[e-mail]> wrote:
> >
> > > On Tue, Sep 24, 2019 at 04:20:51PM +0200, Fabien Lahoudere wrote:
> > > > Hi all,
> > > >
> > > > After some discussions and investigation, the timestamp is very
> > > > important for that sync driver.
> > > > Google team uses that timestamp to compare with gyroscope timestamp.
> > > >
> > > > So the important data is timestamp and counter value is useless.
> > > > Just the event of counter increment is important to get a timestamp.
> > > >
> > > > In that case, my idea was to just use an IIO driver with a single
> > > > channel with IIO_TIMESTAMP. We discuss this here and it seems
> > > > controversial.
> > > >
> > > > So my question to Jonathan is if we have a timestamp coming from the EC
> > > > itself, can we consider this timestamp as a good IIO driver?
> > > >
> > > > Any other idea is welcome, however Google team would like to manage
> > > > only IIO drivers if possible.
> > > >
> > > > Thanks
> > >
> > > Jonathan,
> > >
> > > Should the the timestamp from the EC be introduced as an IIO driver
> > > using IIO_TIMESTAMP?
> >
> > It is is a rather odd driver but I suppose it would be fine with lots
> > of clear docs on why it is how it is...
> >
> > >
> > > Since there is no corresponding EC Counter driver in the baseline right
> > > now we don't have a conflict yet. If the EC timestamp is introduced as
> > > an IIO driver then we should make any future EC Counter driver mutually
> > > exclusive with the IIO driver in order to prevent any memory space
> > > conflict. At that point we may deprecate the IIO driver and move the
> > > timestamp functionality to the corresponding Counter driver.
> >
> > That route does become somewhat of a mess so I suspect we'd have to have
> > a single driver supporting both userspace interfaces.  If you are happy
> > that we'd be adding a bit of legacy to support for ever then we can go
> > that way.
>
> Generally I'd prefer all components of a device to be supported, but
> if this is as Fabien suggests that due to the nature of this particular
> device the counter value is of no interest, then a Counter driver is of
> little practical use here. In this particular case, it seems better to
> restrict the driver support to just the timestamp functionality that
> will be used, rather than introduce extra code to expose values that
> will likely be ignored and risk adding code to the kernel that becomes
> unmaintained due to lack of exposure or interest.
>
> William Breathitt Gray
>
> >
> > >
> > > That's assuming someone is interested in the Count component enough to
> > > implement an EC Counter driver; otherwise, the IIO driver will serve
> > > just fine if timestamp is the only data desired from this device.
> > >
> > > William Breathitt Gray
> >
> >

William Breathitt Gray April 8, 2020, 5:24 p.m. UTC | #11

On Wed, Apr 08, 2020 at 10:10:28AM -0700, Gwendal Grignou wrote:
> I resend a counter driver for the EC at
> https://patchwork.kernel.org/patch/11479437/
> 
> I tried a timestamp only IIO sensor, but this is not allowed, as the
> timestamp channel is very specific: no extended parameters can be
> added.
> I did not add a COUNTER_COUNT_TALLY type, as a newer function
> COUNTER_COUNT_FUNCTION_INCREASE, fits the counter better.
> I am still using IIO_COUNT (inspired by the st counter driver) to
> gather the event in real time on the iio side.
> 
> Gwendal.

Hi Gwendal,

Despite the silence I haven't yet forgotten about this. I'm working on
an update to the Counter subsystem code that should help develop a
real time timestamp functionality for counter drivers. It's not yet
complete right now, but I'm hoping to release it later this month. I'll
CC you when the time comes to keep you updated and make sure it fits for
use cases. :-)

I'll review your current EC patch for a counter driver in the coming
days -- I have some IRL work I'm taking care of this week but I should
be able to take a look this weekend or soon after.

Thanks,

William Breathitt Gray

Patch

diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index 8382f01a53e3..beb80714ac8b 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -44,6 +44,9 @@  Counter interface provides the following available count data types:
 * COUNT_POSITION:
   Unsigned integer value representing position.
 
+* COUNT_TALLY:
+  Unsigned integer value representing tally.
+
 A Count has a count function mode which represents the update behavior
 for the count data. The Generic Counter interface provides the following
 available count function modes:
diff --git a/MAINTAINERS b/MAINTAINERS
index e60f5c361969..83bd291d103e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3902,6 +3902,13 @@  R:	Guenter Roeck <[e-mail]>
 F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
 F:	sound/soc/codecs/cros_ec_codec.*
 
+CHROMEOS EC COUNTER DRIVER
+M:	Fabien Lahoudere <[e-mail]>
+M:	William Breathitt Gray <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/counter/cros_ec_sensors_sync.c
+
 CIRRUS LOGIC AUDIO CODEC DRIVERS
 M:	Brian Austin <[e-mail]>
 M:	Paul Handrigan <[e-mail]>
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..22287f5715e5 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -59,4 +59,13 @@  config FTM_QUADDEC
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftm-quaddec.
 
+config IIO_CROS_EC_SENSORS_SYNC
+	tristate "ChromeOS EC Counter Sensors"
+	depends on IIO_CROS_EC_SENSORS_CORE && IIO
+	help
+	  Module to handle synchronisation sensors presented by the ChromeOS EC
+	  Sensor hub.
+	  Synchronisation sensors are counter sensors triggered when events
+	  occurs from other subsystems.
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..6fe4c98a446f 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
+obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 106bc7180cd8..53525b109094 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -261,6 +261,7 @@  void counter_count_read_value_set(struct counter_count_read_value *const val,
 {
 	switch (type) {
 	case COUNTER_COUNT_POSITION:
+	case COUNTER_COUNT_TALLY:
 		val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
 		break;
 	default:
@@ -290,6 +291,7 @@  int counter_count_write_value_get(void *const data,
 
 	switch (type) {
 	case COUNTER_COUNT_POSITION:
+	case COUNTER_COUNT_TALLY:
 		err = kstrtoul(val->buf, 0, data);
 		if (err)
 			return err;
diff --git a/drivers/counter/cros_ec_sensors_sync.c b/drivers/counter/cros_ec_sensors_sync.c
new file mode 100644
index 000000000000..b6f5e2c6da9f
--- /dev/null
+++ b/drivers/counter/cros_ec_sensors_sync.c
@@ -0,0 +1,208 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver of counter incremented after events on interrupt line in EC.
+ *
+ * Copyright 2018 Google, Inc
+ */
+
+#include <linux/device.h>
+#include <linux/counter.h>
+#include <linux/iio/common/cros_ec_sensors_core.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "cros-ec-sync"
+
+/*
+ * One channel for counter, the other for timestamp.
+ */
+#define MAX_CHANNELS (1)
+
+/* State data for ec_sensors iio driver. */
+struct cros_ec_sensors_sync_state {
+	/* Shared by all sensors */
+	struct cros_ec_sensors_core_state core;
+	struct counter_device counter;
+	struct iio_chan_spec channels[MAX_CHANNELS];
+};
+
+static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     int *val, int *val2, long mask)
+{
+	struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
+	u16 data;
+	int ret;
+
+	mutex_lock(&st->core.cmd_lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &data);
+		if (ret < 0)
+			break;
+		ret = IIO_VAL_INT;
+		*val = data;
+		break;
+	default:
+		ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
+						mask);
+		break;
+	}
+	mutex_unlock(&st->core.cmd_lock);
+	return ret;
+}
+
+static struct iio_info cros_ec_sensors_sync_info = {
+	.read_raw = &cros_ec_sensors_sync_read,
+	.read_avail = &cros_ec_sensors_core_read_avail,
+};
+
+static struct counter_count cros_ec_sync_counts = {
+	.id = 0,
+	.name = "Cros EC sync counter",
+};
+
+static int cros_ec_sync_cnt_read(struct counter_device *counter,
+				struct counter_count *count,
+				struct counter_count_read_value *val)
+{
+	s16 cnt;
+	int ret;
+	struct iio_dev *indio_dev = counter->priv;
+	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
+	unsigned long data;
+
+	mutex_lock(&st->core.cmd_lock);
+	ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
+	mutex_unlock(&st->core.cmd_lock);
+	if (ret != 0) {
+		dev_warn(&indio_dev->dev, "Unable to read sensor data\n");
+		return ret;
+	}
+
+	data = (unsigned long) cnt;
+	counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
+
+	return 0;
+}
+
+static const struct counter_ops cros_ec_sync_cnt_ops = {
+	.count_read = cros_ec_sync_cnt_read,
+};
+
+static char *cros_ec_loc[] = {
+	[MOTIONSENSE_LOC_BASE] = "base",
+	[MOTIONSENSE_LOC_LID] = "lid",
+	[MOTIONSENSE_LOC_CAMERA] = "camera",
+	[MOTIONSENSE_LOC_MAX] = "unknown",
+};
+
+static ssize_t cros_ec_sync_id(struct counter_device *counter,
+				  void *private, char *buf)
+{
+	struct iio_dev *indio_dev = counter->priv;
+	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", st->core.param.info.sensor_num);
+}
+
+static ssize_t cros_ec_sync_loc(struct counter_device *counter,
+				   void *private, char *buf)
+{
+	struct iio_dev *indio_dev = counter->priv;
+	struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->core.loc]);
+}
+
+static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
+	{
+		.name = "id",
+		.read = cros_ec_sync_id
+	},
+	{
+		.name = "location",
+		.read = cros_ec_sync_loc
+	},
+};
+
+static int cros_ec_sensors_sync_probe(struct platform_device *pdev)
+{
+	struct cros_ec_sensors_sync_state *state;
+	struct device *dev = &pdev->dev;
+	struct iio_chan_spec *channel;
+	struct iio_dev *indio_dev;
+	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);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &cros_ec_sensors_sync_info;
+	state = iio_priv(indio_dev);
+
+	if (state->core.type != MOTIONSENSE_TYPE_SYNC)
+		return -EINVAL;
+
+	/* Initialize IIO device */
+	channel = state->channels;
+	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;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      cros_ec_sensors_capture, NULL);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	/* Initialize counter device */
+	state->counter.name = dev_name(&pdev->dev);
+	state->counter.parent = &pdev->dev;
+	state->counter.counts = &cros_ec_sync_counts;
+	state->counter.num_counts = 1;
+	state->counter.priv = indio_dev;
+	state->counter.ops = &cros_ec_sync_cnt_ops;
+	state->counter.ext = cros_ec_sync_cnt_ext;
+	state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
+
+	return devm_counter_register(&pdev->dev, &state->counter);
+}
+
+static const struct platform_device_id cros_ec_sensors_sync_ids[] = {
+	{ .name = DRV_NAME, },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
+
+static struct platform_driver cros_ec_sensors_sync_platform_driver = {
+	.driver = {
+		.name	= DRV_NAME,
+		.pm	= &cros_ec_sensors_pm_ops,
+	},
+	.probe		= cros_ec_sensors_sync_probe,
+	.id_table	= cros_ec_sensors_sync_ids,
+};
+module_platform_driver(cros_ec_sensors_sync_platform_driver);
+
+MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor 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 805652250960..2bf183425eaf 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
@@ -22,6 +22,7 @@ 
 static char *cros_ec_loc[] = {
 	[MOTIONSENSE_LOC_BASE] = "base",
 	[MOTIONSENSE_LOC_LID] = "lid",
+	[MOTIONSENSE_LOC_CAMERA] = "camera",
 	[MOTIONSENSE_LOC_MAX] = "unknown",
 };
 
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 41dccced5026..1c5c2c38af88 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -332,6 +332,9 @@  static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 		case MOTIONSENSE_TYPE_ACTIVITY:
 			sensor_cells[id].name = "cros-ec-activity";
 			break;
+		case MOTIONSENSE_TYPE_SYNC:
+			sensor_cells[id].name = "cros-ec-sync";
+			break;
 		default:
 			dev_warn(ec->dev, "unknown type %d\n", resp->info.type);
 			continue;
diff --git a/include/linux/counter.h b/include/linux/counter.h
index a061cdcdef7c..1198e675306f 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -488,6 +488,7 @@  enum counter_signal_value_type {
 
 enum counter_count_value_type {
 	COUNTER_COUNT_POSITION = 0,
+	COUNTER_COUNT_TALLY
 };
 
 void counter_signal_read_value_set(struct counter_signal_read_value *const val,