Hi, On Sat, Jun 25, 2022 at 3:24 PM Gwendal Grignou <gwendal@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > --- > 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