Re: [PATCH v2 2/2] iio: accel: fxls8962af: add wake on event

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Aug 24, 2021 at 2:39 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> This adds 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.
>
> If buffered reads are enabled they will be deactivated before suspend.
> As the onboard buffer is only holding up to 32 12-bit X/Y/Z data
> triplets.

Assuming it works, the code looks good to me
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

One niy-pick below, though.

> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v1:
>  - Fixed comments fron Andy (Thanks)
>
>  drivers/iio/accel/fxls8962af-core.c | 47 +++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index fd35f8685e62..ecd138f595d4 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -160,6 +160,7 @@ struct fxls8962af_data {
>         } scan;
>         int64_t timestamp, old_timestamp;       /* Only used in hw fifo mode. */
>         struct iio_mount_matrix orientation;
> +       int irq;
>         u8 watermark;
>         u8 enable_event;
>         u16 lower_thres;
> @@ -1121,6 +1122,7 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>         data = iio_priv(indio_dev);
>         dev_set_drvdata(dev, indio_dev);
>         data->regmap = regmap;
> +       data->irq = irq;
>
>         ret = iio_read_mount_matrix(dev, &data->orientation);
>         if (ret)
> @@ -1190,6 +1192,9 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>         if (ret)
>                 return ret;
>
> +       if (device_property_read_bool(dev, "wakeup-source"))
> +               device_init_wakeup(dev, true);
> +
>         return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(fxls8962af_core_probe);
> @@ -1215,9 +1220,47 @@ static int __maybe_unused fxls8962af_runtime_resume(struct device *dev)
>         return fxls8962af_active(data);
>  }
>
> +static int __maybe_unused fxls8962af_suspend(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct fxls8962af_data *data = iio_priv(indio_dev);
> +
> +       if (device_may_wakeup(dev) && data->enable_event) {
> +               enable_irq_wake(data->irq);
> +
> +               /*
> +                * Disable buffer, as the buffer is so small the device will wake
> +                * almost immediately.
> +                */
> +               if (iio_buffer_enabled(indio_dev))
> +                       fxls8962af_buffer_predisable(indio_dev);
> +       } else {
> +               fxls8962af_runtime_suspend(dev);
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused fxls8962af_resume(struct device *dev)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +       struct fxls8962af_data *data = iio_priv(indio_dev);
> +
> +       if (device_may_wakeup(dev) && data->enable_event) {
> +               disable_irq_wake(data->irq);
> +
> +               if (iio_buffer_enabled(indio_dev))
> +                       fxls8962af_buffer_postenable(indio_dev);
> +       } else {
> +               fxls8962af_runtime_resume(dev);
> +       }
> +
> +       return 0;
> +}
> +
>  const struct dev_pm_ops fxls8962af_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -                               pm_runtime_force_resume)

> +       SET_SYSTEM_SLEEP_PM_OPS(fxls8962af_suspend,
> +                               fxls8962af_resume)

Now one line?

>         SET_RUNTIME_PM_OPS(fxls8962af_runtime_suspend,
>                            fxls8962af_runtime_resume, NULL)
>  };

-- 
With Best Regards,
Andy Shevchenko