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
Date: Mon, 21 Oct 2019 17:38:35 +0100 [thread overview]
Message-ID: <20191021173835.16a7830c@archlinux> (raw)
In-Reply-To: <20191021055403.67849-15-gwendal@chromium.org>
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);
next prev parent reply other threads:[~2019-10-21 16:38 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-21 5:53 [PATCH v2 00/18] cros_ec: Add sensorhub driver and FIFO processing*** SUBJECT HERE Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 01/18] platform: chrome: Put docs with the code Gwendal Grignou
2019-10-21 15:31 ` Jonathan Cameron
2019-10-24 17:03 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 02/18] mfd: cros_ec: Add sensor_count and make check_features public Gwendal Grignou
2019-10-21 15:12 ` Enric Balletbo i Serra
2019-10-21 15:49 ` Jonathan Cameron
2019-11-01 9:00 ` Lee Jones
2019-10-21 5:53 ` [PATCH v2 03/18] platform: cros_ec: Add cros_ec_sensor_hub driver Gwendal Grignou
2019-10-21 15:59 ` Jonathan Cameron
2019-10-22 8:32 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 04/18] platform/mfd:iio: cros_ec: Register sensor through sensorhub Gwendal Grignou
2019-10-21 16:00 ` Jonathan Cameron
2019-10-22 9:39 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 05/18] platform: chrome: cros-ec: record event timestamp in the hard irq Gwendal Grignou
2019-10-22 10:03 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 06/18] platform: chrome: cros_ec: Do not attempt to register a non-positive IRQ number Gwendal Grignou
2019-10-22 10:05 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 07/18] platform: chrome: cros_ec: handle MKBP more events flag Gwendal Grignou
2019-10-21 16:07 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 08/18] Revert "Input: cros_ec_keyb - add back missing mask for event_type" Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 09/18] Revert "Input: cros_ec_keyb: mask out extra flags in event_type" Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 10/18] platform: chrome: sensorhub: Add FIFO support Gwendal Grignou
2019-10-21 16:27 ` Jonathan Cameron
2019-10-21 22:09 ` Gwendal Grignou
2019-10-22 10:42 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 11/18] platform: chrome: sensorhub: Add code to spread timestmap Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 12/18] platform: chrome: sensorhub: Add median filter Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 13/18] iio: cros_ec: Move function description to .c file Gwendal Grignou
2019-10-21 16:33 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 14/18] iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO Gwendal Grignou
2019-10-21 16:38 ` Jonathan Cameron [this message]
2019-10-21 5:54 ` [PATCH v2 15/18] iio: cros_ec: Remove pm function Gwendal Grignou
2019-10-21 16:39 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 16/18] iio: cros_ec: Expose hwfifo_timeout Gwendal Grignou
2019-10-21 16:42 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 17/18] iio: cros_ec: Report hwfifo_watermark_max Gwendal Grignou
2019-10-21 16:42 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 18/18] iio: cros_ec: Use Hertz as unit for sampling frequency Gwendal Grignou
2019-10-21 16:45 ` Jonathan Cameron
2019-10-22 19:11 ` Gwendal Grignou
2019-10-21 15:29 ` [PATCH v2 00/18] cros_ec: Add sensorhub driver and FIFO processing*** SUBJECT HERE Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191021173835.16a7830c@archlinux \
--to=jic23@kernel.org \
--cc=bleung@chromium.org \
--cc=briannorris@chromium.org \
--cc=dianders@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=fabien.lahoudere@collabora.com \
--cc=groeck@chromium.org \
--cc=gwendal@chromium.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=lee.jones@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).