[v7,3/5] iio: imu: st_lsm6dsx: add wakeup-source option
diff mbox series

Message ID 20190912070614.1144169-4-sean@geanix.com
State New
Headers show
Series
  • enable motion events for st_lsm6dsx
Related show

Commit Message

Sean Nyekjaer Sept. 12, 2019, 7:06 a.m. UTC
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(+)

Comments

Sean Nyekjaer Sept. 13, 2019, 6:08 a.m. UTC | #1
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.
Lorenzo Bianconi Sept. 13, 2019, 8:27 a.m. UTC | #2
> 
> 
> 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.
Sean Nyekjaer Sept. 13, 2019, 8:43 a.m. UTC | #3
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
Lorenzo Bianconi Sept. 13, 2019, 8:58 a.m. UTC | #4
> 
> 
> 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
Sean Nyekjaer Sept. 13, 2019, 9 a.m. UTC | #5
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

Patch
diff mbox series

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)