* [PATCH] iio: cros: Add cros_ec_sensors_core_register @ 2022-06-21 18:28 Gwendal Grignou 2022-06-23 0:44 ` Stephen Boyd 0 siblings, 1 reply; 6+ messages in thread From: Gwendal Grignou @ 2022-06-21 18:28 UTC (permalink / raw) To: jic23, dianders, swboyd; +Cc: linux-iio, Gwendal Grignou 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). Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- .../cros_ec_sensors/cros_ec_lid_angle.c | 2 +- .../common/cros_ec_sensors/cros_ec_sensors.c | 11 +++-- .../cros_ec_sensors/cros_ec_sensors_core.c | 49 +++++++++++++------ drivers/iio/light/cros_ec_light_prox.c | 10 ++-- drivers/iio/pressure/cros_ec_baro.c | 11 +++-- .../linux/iio/common/cros_ec_sensors_core.h | 7 ++- 6 files changed, 63 insertions(+), 27 deletions(-) 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 9f780fafaed9f..4e569f76bb8cd 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 @@ -98,7 +98,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, NULL, NULL); + ret = cros_ec_sensors_core_init(pdev, indio_dev, false, 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 61e07a7bb1995..d3c456e8c313d 100644 --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c @@ -236,8 +236,7 @@ static int cros_ec_sensors_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; @@ -298,7 +297,13 @@ static int cros_ec_sensors_probe(struct platform_device *pdev) else 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); } static const struct platform_device_id cros_ec_sensors_ids[] = { 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 @@ -233,16 +233,13 @@ static void cros_ec_sensors_core_clean(void *arg) * @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, - cros_ec_sensors_capture_t trigger_capture, - cros_ec_sensorhub_push_data_cb_t push_data) + cros_ec_sensors_capture_t trigger_capture) { struct device *dev = &pdev->dev; struct cros_ec_sensors_core_state *state = iio_priv(indio_dev); @@ -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. + * @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)) { + 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/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c index e345e0f71b740..ec176b1ac84db 100644 --- a/drivers/iio/light/cros_ec_light_prox.c +++ b/drivers/iio/light/cros_ec_light_prox.c @@ -182,8 +182,7 @@ static int cros_ec_light_prox_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; @@ -239,7 +238,12 @@ static int cros_ec_light_prox_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; + + return cros_ec_sensors_core_register(dev, indio_dev, + cros_ec_sensors_push_data); } static const struct platform_device_id cros_ec_light_prox_ids[] = { 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); } static const struct platform_device_id cros_ec_baro_ids[] = { diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h index a8259c8822f56..e72167b96d27e 100644 --- a/include/linux/iio/common/cros_ec_sensors_core.h +++ b/include/linux/iio/common/cros_ec_sensors_core.h @@ -93,8 +93,11 @@ 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, - cros_ec_sensors_capture_t trigger_capture, - cros_ec_sensorhub_push_data_cb_t push_data); + cros_ec_sensors_capture_t trigger_capture); + +int cros_ec_sensors_core_register(struct device *dev, + struct iio_dev *indio_dev, + 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, -- 2.37.0.rc0.104.g0611611a94-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: cros: Add cros_ec_sensors_core_register 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 0 siblings, 1 reply; 6+ messages in thread From: Stephen Boyd @ 2022-06-23 0:44 UTC (permalink / raw) To: Gwendal Grignou, dianders, jic23; +Cc: linux-iio 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@chromium.org> Should add Reported-by: Douglas Anderson <dianders@chromium.org> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: cros: Add cros_ec_sensors_core_register 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 0 siblings, 1 reply; 6+ messages in thread From: Gwendal Grignou @ 2022-06-25 22:22 UTC (permalink / raw) To: Stephen Boyd; +Cc: dianders, jic23, linux-iio On Wed, Jun 22, 2022 at 5:44 PM Stephen Boyd <swboyd@chromium.org> wrote: > > 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. The crash only happens to a sensor that is not upstream yet. (the channels are allocated at probe time and we get a NULL dereference). For existing sensors, the channels are allocated at compilation time, so the kernel does not crash. We do send data to IIO core while it is still initializing the sensor. > > > > > 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@chromium.org> > > Should add > > Reported-by: Douglas Anderson <dianders@chromium.org> Done in v2. > > and also some sort of Fixes tag. Done in v2. > > > 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. Done in v2. > > > + * @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()? It is false on older chromebooks, where sensor data needs to be polled by the host. > > > + 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. Good idea, done in v2. > 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. The callback can be different from the default cros_ec_sensors_push_data(). ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] iio: cros: Register FIFO callback after sensor is registered 2022-06-25 22:22 ` Gwendal Grignou @ 2022-06-25 22:24 ` Gwendal Grignou 2022-06-27 15:41 ` Doug Anderson 0 siblings, 1 reply; 6+ messages in thread From: Gwendal Grignou @ 2022-06-25 22:24 UTC (permalink / raw) To: jic23, dianders, swboyd; +Cc: linux-iio, Gwendal Grignou 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); } static struct platform_driver cros_ec_accel_platform_driver = { 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 9f780fafaed9f..119acb078af3b 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 @@ -98,7 +98,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, NULL, NULL); + ret = cros_ec_sensors_core_init(pdev, indio_dev, false, NULL); if (ret) return ret; @@ -114,7 +114,7 @@ static int cros_ec_lid_angle_probe(struct platform_device *pdev) if (ret) return ret; - return devm_iio_device_register(dev, indio_dev); + return cros_ec_sensors_core_register(dev, indio_dev, NULL); } static const struct platform_device_id cros_ec_lid_angle_ids[] = { 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 61e07a7bb1995..66153b1850f10 100644 --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c @@ -236,8 +236,7 @@ static int cros_ec_sensors_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; @@ -298,7 +297,8 @@ static int cros_ec_sensors_probe(struct platform_device *pdev) else state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd; - return devm_iio_device_register(dev, indio_dev); + return cros_ec_sensors_core_register(dev, indio_dev, + cros_ec_sensors_push_data); } static const struct platform_device_id cros_ec_sensors_ids[] = { 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..b5317e6339598 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 @@ -228,21 +228,18 @@ static void cros_ec_sensors_core_clean(void *arg) /** * cros_ec_sensors_core_init() - basic initialization of the core structure - * @pdev: platform device created for the sensors + * @pdev: platform device created for the sensor * @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, - cros_ec_sensors_capture_t trigger_capture, - cros_ec_sensorhub_push_data_cb_t push_data) + cros_ec_sensors_capture_t trigger_capture) { struct device *dev = &pdev->dev; struct cros_ec_sensors_core_state *state = iio_priv(indio_dev); @@ -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,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; + + 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) { + 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/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c index e345e0f71b740..19e529c84e957 100644 --- a/drivers/iio/light/cros_ec_light_prox.c +++ b/drivers/iio/light/cros_ec_light_prox.c @@ -182,8 +182,7 @@ static int cros_ec_light_prox_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; @@ -239,7 +238,8 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev) state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd; - return devm_iio_device_register(dev, indio_dev); + return cros_ec_sensors_core_register(dev, indio_dev, + cros_ec_sensors_push_data); } static const struct platform_device_id cros_ec_light_prox_ids[] = { diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c index 25217279f3507..2649c2f89e898 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,8 @@ 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); + return cros_ec_sensors_core_register(dev, indio_dev, + cros_ec_sensors_push_data); } static const struct platform_device_id cros_ec_baro_ids[] = { diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h index a8259c8822f56..e72167b96d27e 100644 --- a/include/linux/iio/common/cros_ec_sensors_core.h +++ b/include/linux/iio/common/cros_ec_sensors_core.h @@ -93,8 +93,11 @@ 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, - cros_ec_sensors_capture_t trigger_capture, - cros_ec_sensorhub_push_data_cb_t push_data); + cros_ec_sensors_capture_t trigger_capture); + +int cros_ec_sensors_core_register(struct device *dev, + struct iio_dev *indio_dev, + 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, -- 2.37.0.rc0.161.g10f37bed90-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: cros: Register FIFO callback after sensor is registered 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 2022-07-11 14:47 ` Gwendal Grignou 0 siblings, 1 reply; 6+ messages in thread From: Doug Anderson @ 2022-06-27 15:41 UTC (permalink / raw) To: Gwendal Grignou; +Cc: Jonathan Cameron, Stephen Boyd, linux-iio 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: cros: Register FIFO callback after sensor is registered 2022-06-27 15:41 ` Doug Anderson @ 2022-07-11 14:47 ` Gwendal Grignou 0 siblings, 0 replies; 6+ messages in thread From: Gwendal Grignou @ 2022-07-11 14:47 UTC (permalink / raw) To: Doug Anderson; +Cc: Jonathan Cameron, Stephen Boyd, linux-iio On Mon, Jun 27, 2022 at 5:41 PM Doug Anderson <dianders@chromium.org> wrote: > > 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. Yes, it is equivalent. > > > > @@ -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. Done > > > > + 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; Done. > > -Doug ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-11 14:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2022-07-11 14:47 ` Gwendal Grignou
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.