From: Doug Anderson <dianders@chromium.org>
To: Gwendal Grignou <gwendal@chromium.org>
Cc: Jonathan Cameron <jic23@kernel.org>,
Stephen Boyd <swboyd@chromium.org>,
linux-iio <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v2] iio: cros: Register FIFO callback after sensor is registered
Date: Mon, 27 Jun 2022 08:41:18 -0700 [thread overview]
Message-ID: <CAD=FV=UvSfo82=w5R7R4zhkuj6Z+xffyLe6HRsLzUommTvyWag@mail.gmail.com> (raw)
In-Reply-To: <20220625222443.2906866-1-gwendal@chromium.org>
Hi,
On Sat, Jun 25, 2022 at 3:24 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Instead of registering callback to process sensor events right at
> initialization time, wait for the sensor to be register in the iio
> subsystem.
>
> Events can come at probe time (in case the kernel rebooted abruptly
> without switching the sensor off for instance), and be sent to IIO core
> before the sensor is fully registered.
>
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
> Changes since v1:
> - renamed from "iio: cros: Add cros_ec_sensors_core_register"
> - Call devm_iio_device_register() inside cros_ec_sensors_core_register.
>
> drivers/iio/accel/cros_ec_accel_legacy.c | 4 +-
> .../cros_ec_sensors/cros_ec_lid_angle.c | 4 +-
> .../common/cros_ec_sensors/cros_ec_sensors.c | 6 +-
> .../cros_ec_sensors/cros_ec_sensors_core.c | 58 ++++++++++++++-----
> drivers/iio/light/cros_ec_light_prox.c | 6 +-
> drivers/iio/pressure/cros_ec_baro.c | 6 +-
> .../linux/iio/common/cros_ec_sensors_core.h | 7 ++-
> 7 files changed, 60 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> index 1c0171f26e99e..0f403342b1fc0 100644
> --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> @@ -215,7 +215,7 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> - cros_ec_sensors_capture, NULL);
> + cros_ec_sensors_capture);
> if (ret)
> return ret;
>
> @@ -235,7 +235,7 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
> state->sign[CROS_EC_SENSOR_Z] = -1;
> }
>
> - return devm_iio_device_register(dev, indio_dev);
> + return cros_ec_sensors_core_register(dev, indio_dev, NULL);
In the case where the last argument is NULL then the new
cros_ec_sensors_core_register() is always equivalent to the old
devm_iio_device_register(), right? ...but I guess it's more idiomatic
to always use the cros_ec version, so I'm OK with this.
> @@ -372,6 +358,46 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
>
> +/**
> + * cros_ec_sensors_core_register() - Register callback to FIFO and IIO when
> + * sensor is ready.
> + * It must be called at the end of the sensor probe routine.
> + * @dev: device created for the sensor
> + * @indio_dev: iio device structure of the device
> + * @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_register(struct device *dev,
> + struct iio_dev *indio_dev,
> + cros_ec_sensorhub_push_data_cb_t push_data)
> +{
> + struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
> + struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
> + struct platform_device *pdev = to_platform_device(dev);
> + struct cros_ec_dev *ec = sensor_hub->ec;
> + int ret = 0;
nit: don't init "ret" to 0 when you simply assign it right below.
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO) &&
> + push_data != NULL) {
I think the check for push_data should be first so it can short
circuit and avoid the call to cros_ec_check_features(), right?
In the past I've been yelled at for using "!= NULL" and told that
thing should simply be "&& push_data". I'll leave it up to you about
whether it's something that should be changed here.
Also: you can reduce indentation of the function and simply if you just do:
if (!push_data || !cros_ec_check_features(...))
return 0;
-Doug
next prev parent reply other threads:[~2022-06-27 15:41 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-21 18:28 [PATCH] iio: cros: Add cros_ec_sensors_core_register Gwendal Grignou
2022-06-23 0:44 ` Stephen Boyd
2022-06-25 22:22 ` Gwendal Grignou
2022-06-25 22:24 ` [PATCH v2] iio: cros: Register FIFO callback after sensor is registered Gwendal Grignou
2022-06-27 15:41 ` Doug Anderson [this message]
2022-07-11 14:47 ` Gwendal Grignou
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='CAD=FV=UvSfo82=w5R7R4zhkuj6Z+xffyLe6HRsLzUommTvyWag@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=gwendal@chromium.org \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=swboyd@chromium.org \
/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).