The subject line could be a little clearer. Perhaps "Wait for sensors to
probe before sending events" or something like that.
Quoting Gwendal Grignou (2022-06-21 11:28:59)
> Instead of registering callback to process sensor events right at
> initialization time, wait for the sensor to be register in the iio
> subsystem.
Please elaborate. A crash is seen if sensor events come in while sensors
are being registered.
>
> Events can come at probe time (in case the kernel rebooted abruptly
> without switching the sensor off for instance).
>
> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
Should add
Reported-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
and also some sort of Fixes tag.
> 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 e5ccedef13a80..5bf5cfb0e746b 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
> @@ -340,17 +337,6 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> if (ret)
> return ret;
>
> - 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);
> - if (ret)
> - return ret;
> -
> /* Timestamp coming from FIFO are in ns since boot. */
> ret = iio_device_set_clock(indio_dev, CLOCK_BOOTTIME);
> if (ret)
> @@ -372,6 +358,39 @@ 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 when sensor is
> + * ready.
Please document 'dev' as well here.
> + * @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;
> +
> + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
Just curious if this condition is ever false? Or if the case when it is
true is when 'push_data' is cros_ec_sensors_push_data()?
> + 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 ret;
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> +
> /**
> * cros_ec_motion_send_host_cmd() - send motion sense host command
> * @state: pointer to state information for device
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 25217279f3507..82f20c738a165 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -139,8 +139,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> - cros_ec_sensors_capture,
> - cros_ec_sensors_push_data);
> + cros_ec_sensors_capture);
> if (ret)
> return ret;
>
> @@ -185,7 +184,13 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
>
> state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>
> - return devm_iio_device_register(dev, indio_dev);
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + /* Ready to receive samples from the EC. */
> + return cros_ec_sensors_core_register(dev, indio_dev,
> + cros_ec_sensors_push_data);
Would it make sense to add the devm_iio_device_register() call into
cros_ec_sensors_core_register()? Then the caller can't mess up the order
and register the push_data callback before the iio device. And is the
callback ever going to be not cros_ec_sensors_push_data() (or NULL in
case of not motion sense)? Seems like that could be hardcoded into the
function as well.
