| Message ID | 20190912070614.1144169-4-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable motion events for st_lsm6dsx | expand |
On 12/09/2019 09.06, Sean Nyekjaer wrote: > This add ways for the SoC to wake from accelerometer wake events. > > In the suspend function we skip disabling the sensor if wakeup-source > and events are activated. > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > Changes since v4: > * More devices supports wakeup > > Changes since v5: > * None > > Changes since v6: > * None > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index 4198ba263d03..f79978a2870f 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -1858,6 +1858,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, > return err; > } > > + if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source")) > + device_init_wakeup(dev, true); > + > return 0; > } > EXPORT_SYMBOL(st_lsm6dsx_probe); > @@ -1876,6 +1879,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev) > if (!(hw->enable_mask & BIT(sensor->id))) > continue; > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > + /* Enable wake from IRQ */ > + enable_irq_wake(hw->irq); > + continue; > + } > + > if (sensor->id == ST_LSM6DSX_ID_EXT0 || > sensor->id == ST_LSM6DSX_ID_EXT1 || > sensor->id == ST_LSM6DSX_ID_EXT2) > @@ -1908,6 +1917,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev) > if (!(hw->suspend_mask & BIT(sensor->id))) > continue; > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > + disable_irq_wake(hw->irq); > + continue; > + } > + This section needs to move above: > if (!(hw->enable_mask & BIT(sensor->id))) In the current configuration it will never be reached in event only mode With the bug mentioned above fixed, have tested suspend/wake with and without FIFO enabled.
> > > On 12/09/2019 09.06, Sean Nyekjaer wrote: > > This add ways for the SoC to wake from accelerometer wake events. > > > > In the suspend function we skip disabling the sensor if wakeup-source > > and events are activated. > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > --- > > Changes since v4: > > * More devices supports wakeup > > > > Changes since v5: > > * None > > > > Changes since v6: > > * None > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > index 4198ba263d03..f79978a2870f 100644 > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > @@ -1858,6 +1858,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, > > return err; > > } > > + if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source")) > > + device_init_wakeup(dev, true); > > + > > return 0; > > } > > EXPORT_SYMBOL(st_lsm6dsx_probe); > > @@ -1876,6 +1879,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev) > > if (!(hw->enable_mask & BIT(sensor->id))) > > continue; > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > > + /* Enable wake from IRQ */ > > + enable_irq_wake(hw->irq); > > + continue; I guess here we need even to check hw->event_enable, don't we? > > + } > > + > > if (sensor->id == ST_LSM6DSX_ID_EXT0 || > > sensor->id == ST_LSM6DSX_ID_EXT1 || > > sensor->id == ST_LSM6DSX_ID_EXT2) > > @@ -1908,6 +1917,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev) > > if (!(hw->suspend_mask & BIT(sensor->id))) > > continue; > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > > + disable_irq_wake(hw->irq); > > + continue; > > + } > > + > This section needs to move above: agree, and we probably do not need 'continue'. Moreover I guess we should check hw->event_enable even here, right? > > if (!(hw->enable_mask & BIT(sensor->id))) > In the current configuration it will never be reached in event only mode > > With the bug mentioned above fixed, have tested suspend/wake with and > without FIFO enabled.
On 13/09/2019 10.27, Lorenzo Bianconi wrote: >> >> >> On 12/09/2019 09.06, Sean Nyekjaer wrote: >>> This add ways for the SoC to wake from accelerometer wake events. >>> >>> In the suspend function we skip disabling the sensor if wakeup-source >>> and events are activated. >>> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >>> --- >>> Changes since v4: >>> * More devices supports wakeup >>> >>> Changes since v5: >>> * None >>> >>> Changes since v6: >>> * None >>> >>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >>> index 4198ba263d03..f79978a2870f 100644 >>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >>> @@ -1858,6 +1858,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, >>> return err; >>> } >>> + if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source")) >>> + device_init_wakeup(dev, true); >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL(st_lsm6dsx_probe); >>> @@ -1876,6 +1879,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev) >>> if (!(hw->enable_mask & BIT(sensor->id))) >>> continue; >>> + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { >>> + /* Enable wake from IRQ */ >>> + enable_irq_wake(hw->irq); >>> + continue; > > I guess here we need even to check hw->event_enable, don't we? Good idea :) > >>> + } >>> + >>> if (sensor->id == ST_LSM6DSX_ID_EXT0 || >>> sensor->id == ST_LSM6DSX_ID_EXT1 || >>> sensor->id == ST_LSM6DSX_ID_EXT2) >>> @@ -1908,6 +1917,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev) >>> if (!(hw->suspend_mask & BIT(sensor->id))) >>> continue; >>> + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { >>> + disable_irq_wake(hw->irq); >>> + continue; >>> + } >>> + >> This section needs to move above: > > agree, and we probably do not need 'continue'. Moreover I guess we should check > hw->event_enable even here, right? Correct we don't need the 'continue' here... The 'hw->suspend_mask' is not set as we skip it in the resume part. I'll add the 'hw->enable_event' check here as well. Is there more stuff that blocks this series from your point of view? /Sean
> > > On 13/09/2019 10.27, Lorenzo Bianconi wrote: > > > > > > > > > On 12/09/2019 09.06, Sean Nyekjaer wrote: > > > > This add ways for the SoC to wake from accelerometer wake events. > > > > > > > > In the suspend function we skip disabling the sensor if wakeup-source > > > > and events are activated. > > > > > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > > > --- > > > > Changes since v4: > > > > * More devices supports wakeup > > > > > > > > Changes since v5: > > > > * None > > > > > > > > Changes since v6: > > > > * None > > > > > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > index 4198ba263d03..f79978a2870f 100644 > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > > > > @@ -1858,6 +1858,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, > > > > return err; > > > > } > > > > + if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source")) > > > > + device_init_wakeup(dev, true); > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL(st_lsm6dsx_probe); > > > > @@ -1876,6 +1879,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev) > > > > if (!(hw->enable_mask & BIT(sensor->id))) > > > > continue; > > > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > > > > + /* Enable wake from IRQ */ > > > > + enable_irq_wake(hw->irq); > > > > + continue; > > > > I guess here we need even to check hw->event_enable, don't we? > Good idea :) > > > > > > > + } > > > > + > > > > if (sensor->id == ST_LSM6DSX_ID_EXT0 || > > > > sensor->id == ST_LSM6DSX_ID_EXT1 || > > > > sensor->id == ST_LSM6DSX_ID_EXT2) > > > > @@ -1908,6 +1917,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev) > > > > if (!(hw->suspend_mask & BIT(sensor->id))) > > > > continue; > > > > + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { > > > > + disable_irq_wake(hw->irq); > > > > + continue; > > > > + } > > > > + > > > This section needs to move above: > > > > agree, and we probably do not need 'continue'. Moreover I guess we should check > > hw->event_enable even here, right? > > Correct we don't need the 'continue' here... The 'hw->suspend_mask' is not > set as we skip it in the resume part. > I'll add the 'hw->enable_event' check here as well. > > Is there more stuff that blocks this series from your point of view? Actually I have not tested, just compiled. Looking at the code seems fine. Regards, Lorenzo > > /Sean
On 13/09/2019 10.58, Lorenzo Bianconi wrote: > Actually I have not tested, just compiled. Looking at the code seems fine. > > Regards, > Lorenzo I'll post V8 with updated [PATCH 3/5] for Jonathan :-) /Sean
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index 4198ba263d03..f79978a2870f 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -1858,6 +1858,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, return err; } + if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source")) + device_init_wakeup(dev, true); + return 0; } EXPORT_SYMBOL(st_lsm6dsx_probe); @@ -1876,6 +1879,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev) if (!(hw->enable_mask & BIT(sensor->id))) continue; + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { + /* Enable wake from IRQ */ + enable_irq_wake(hw->irq); + continue; + } + if (sensor->id == ST_LSM6DSX_ID_EXT0 || sensor->id == ST_LSM6DSX_ID_EXT1 || sensor->id == ST_LSM6DSX_ID_EXT2) @@ -1908,6 +1917,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev) if (!(hw->suspend_mask & BIT(sensor->id))) continue; + if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) { + disable_irq_wake(hw->irq); + continue; + } + if (sensor->id == ST_LSM6DSX_ID_EXT0 || sensor->id == ST_LSM6DSX_ID_EXT1 || sensor->id == ST_LSM6DSX_ID_EXT2)
This add ways for the SoC to wake from accelerometer wake events. In the suspend function we skip disabling the sensor if wakeup-source and events are activated. Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- Changes since v4: * More devices supports wakeup Changes since v5: * None Changes since v6: * None drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)