Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 21 Oct 2019 17:38:35 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Gwendal Grignou <gwendal@chromium.org>
Cc:     briannorris@chromium.org, knaack.h@gmx.de, lars@metafoo.de,
        pmeerw@pmeerw.net, lee.jones@linaro.org, bleung@chromium.org,
        enric.balletbo@collabora.com, dianders@chromium.org,
        groeck@chromium.org, fabien.lahoudere@collabora.com,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 14/18] iio: cros_ec: Register to cros_ec_sensorhub
 when EC supports FIFO
Message-ID: <20191021173835.16a7830c@archlinux>
In-Reply-To: <20191021055403.67849-15-gwendal@chromium.org>
References: <20191021055403.67849-1-gwendal@chromium.org>
        <20191021055403.67849-15-gwendal@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 20 Oct 2019 22:53:59 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> When EC supports FIFO, each IIO device registers a callback, to put
> samples in the buffer when they arrives from the FIFO.
> We can still use a trigger to collect samples, but there may be some
> duplications in the buffer: EC has a single FIFO, so once one sensor is
> using it, all sensors event will be in the FIFO.
> To be sure events generated by cros_ec_sensorhub or the trigger uses the
> same time domain, current_timestamp_clock must be set to "boottime".
> 
> When no FIFO, the user space app needs to call trigger_new, or better
> register a high precision timer.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
A couple of nitpicks inline that would be nice to tidy up.

> ---
> Change in v2 from "Use triggered buffer only when EC does not support
> FIFO":
> - Keep trigger all the time.
> - Add  devm_add_action to cleanup callback registration.
> - EC that "reports" legacy sensors do not have FIFO.
> - Use iiio_is_buffer_enabled instead of checking the scan_mask
>   before sending samples to buffer.

Hmm. I'm a bit dubious about this one as kind of indicates something
is wrong with when the setup is done.  We should be ready to 
receive data before any turns up.  Perhaps things are more complex
here and we can't ensure that for some reason.

> - Add empty lines for visibility.
> 
>  drivers/iio/accel/cros_ec_accel_legacy.c      |  8 +-
>  .../cros_ec_sensors/cros_ec_lid_angle.c       |  2 +-
>  .../common/cros_ec_sensors/cros_ec_sensors.c  |  8 +-
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 80 ++++++++++++++++++-
>  drivers/iio/light/cros_ec_light_prox.c        |  8 +-
>  drivers/iio/pressure/cros_ec_baro.c           |  8 +-
>  .../linux/iio/common/cros_ec_sensors_core.h   | 12 ++-
>  7 files changed, 96 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> index 65f85faf6f31d..66683df9fc433 100644
> --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> @@ -171,7 +171,8 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> +			cros_ec_sensors_capture, NULL);
>  	if (ret)
>  		return ret;
>  
> @@ -191,11 +192,6 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
>  		state->sign[CROS_EC_SENSOR_Z] = -1;
>  	}
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> -			cros_ec_sensors_capture, NULL);
> -	if (ret)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c b/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> index 1dcc2a16ab2dd..e30a59fcf0f95 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_lid_angle.c
> @@ -97,7 +97,7 @@ static int cros_ec_lid_angle_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, false);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, false, NULL, NULL);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 7dce044734678..9e7903ff99f80 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -231,7 +231,8 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> +			cros_ec_sensors_capture, cros_ec_sensors_push_data);
>  	if (ret)
>  		return ret;
>  
> @@ -293,11 +294,6 @@ static int cros_ec_sensors_probe(struct platform_device *pdev)
>  	else
>  		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;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> 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 4acb8b7310d43..3d2e17093c75a 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
> @@ -12,6 +12,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/module.h>
> @@ -83,17 +84,72 @@ static void get_default_min_max_freq(enum motionsensor_type type,
>  	}
>  }
>  
> +int cros_ec_sensors_push_data(
> +		struct iio_dev *indio_dev,
> +		s16 *data,
> +		s64 timestamp)
Whilst you have some cases in here that can't, this can be nicely aligned.
> +int cros_ec_sensors_push_data(struct iio_dev *indio_dev, s16 *data,
				 s64 timestamp)

> +{
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	s16 *out;
> +	unsigned int i;
> +
> +	/*
> +	 * It can happen if we get a samples before the iio device is fully
> +	 * registered.
> +	 */
> +	if (!st)
> +		return 0;
> +
> +	/*
> +	 * Ignore samples if the buffer is not set: it is needed if the ODR is
> +	 * set but the buffer is not enabled yet.
> +	 */
> +	if (!iio_buffer_enabled(indio_dev))
> +		return 0;
> +
> +	out = (s16 *)st->samples;
> +	for_each_set_bit(i,
> +			 indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		*out = data[i];
> +		out++;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->samples, timestamp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_push_data);
> +
> +static void cros_ec_sensors_core_clean(void *arg)
> +{
> +	struct platform_device *pdev = (struct platform_device *)arg;
> +	struct cros_ec_sensorhub *sensor_hub =
> +		dev_get_drvdata(pdev->dev.parent);
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	u8 sensor_num = st->param.info.sensor_num;
> +
> +	cros_ec_sensorhub_unregister_push_data(sensor_hub, sensor_num);
> +}
> +
>  /**
>   * cros_ec_sensors_core_init() - basic initialization of the core structure
>   * @pdev:		platform device created for the sensors
>   * @indio_dev:		iio device structure of the device
>   * @physical_device:	true if the device refers to a physical device
> + * @trigger_capture:    function pointer to call buffer is triggered,
> + *    for backward compatibility.
> + * @push_data:          function to call when cros_ec_sensorhub receives
> + *    a sample for that sensor.
>   *
>   * Return: 0 on success, -errno on failure.
>   */
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			      struct iio_dev *indio_dev,
> -			      bool physical_device)
> +			      bool physical_device,
> +			      cros_ec_sensors_capture_t trigger_capture,
> +			      cros_ec_sensorhub_push_data_cb_t push_data)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> @@ -132,8 +188,6 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  	indio_dev->name = pdev->name;
>  
>  	if (physical_device) {
> -		indio_dev->modes = INDIO_DIRECT_MODE;
> -
>  		state->param.cmd = MOTIONSENSE_CMD_INFO;
>  		state->param.info.sensor_num = sensor_platform->sensor_num;
>  		ret = cros_ec_motion_send_host_cmd(state, 0);
> @@ -162,9 +216,27 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			state->frequencies[2] =
>  			    state->resp->info_3.max_frequency;
>  		}
> +
> +		ret = devm_iio_triggered_buffer_setup(
> +				dev, indio_dev, NULL,
> +				trigger_capture, NULL);
> +		if (ret)
> +			return ret;
> +
> +		if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
> +			ret = cros_ec_sensorhub_register_push_data(
> +					sensor_hub,
> +					sensor_platform->sensor_num,
> +					indio_dev, push_data);
> +			if (ret)
> +				return ret;
> +
> +			ret = devm_add_action_or_reset(
> +					dev, cros_ec_sensors_core_clean, pdev);
> +		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
>  
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index d85a391e50c59..da40c38370965 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -178,7 +178,8 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> +			cros_ec_sensors_capture, cros_ec_sensors_push_data);
>  	if (ret)
>  		return ret;
>  
> @@ -237,11 +238,6 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  
>  	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;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 2354302375dee..fb7daeb4b29f9 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -134,7 +134,8 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> -	ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> +	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> +			cros_ec_sensors_capture, cros_ec_sensors_push_data);
>  	if (ret)
>  		return ret;
>  
> @@ -180,11 +181,6 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>  
>  	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;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 0af918978f975..b4eb3790cde11 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -12,6 +12,7 @@
>  #include <linux/irqreturn.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_data/cros_ec_sensorhub.h>
>  
>  enum {
>  	CROS_EC_SENSOR_X,
> @@ -32,6 +33,9 @@ enum {
>  /* Minimum sampling period to use when device is suspending */
>  #define CROS_EC_MIN_SUSPEND_SAMPLING_FREQUENCY 1000  /* 1 second */
>  
> +typedef irqreturn_t (*cros_ec_sensors_capture_t)(int irq, void *p);
> +
Nitpick if you are rolling again.  One blank line is enough ;)
> +
>  /**
>   * struct cros_ec_sensors_core_state - state data for EC sensors IIO driver
>   * @ec:				cros EC device structure
> @@ -87,9 +91,15 @@ int cros_ec_sensors_read_cmd(struct iio_dev *indio_dev, unsigned long scan_mask,
>  
>  struct platform_device;
>  int cros_ec_sensors_core_init(struct platform_device *pdev,
> -			      struct iio_dev *indio_dev, bool physical_device);
> +			      struct iio_dev *indio_dev, bool physical_device,
> +			      cros_ec_sensors_capture_t trigger_capture,
> +			      cros_ec_sensorhub_push_data_cb_t push_data);
>  
>  irqreturn_t cros_ec_sensors_capture(int irq, void *p);
> +int cros_ec_sensors_push_data(
> +		struct iio_dev *indio_dev,
> +		s16 *data,
> +		s64 timestamp);
>  
>  int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *st,
>  				 u16 opt_length);