Re: [PATCH 2/5] iio: accel: mma9551/mma9553: Simplify pm logic

[Alexandru Ardelean <aardelean@deviqon.com>]

On Sun, 23 May 2021 at 19:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> I can't see why we shouldn't sleep in the system resume path to allow
> the device firmware to fully wakeup.  Having done that, the runtime
> system functions are identical (down to an error print) so use
> pm_runtime_force_suspend() and pm_runtime_force_resume() to reduce
> repitition.
>
> General preference in IIO is now to mark these functions __maybe_unused
> instead of using ifdefs as it is easy to get them wrong.
> Here they appear correct, but provide a less than desirable example
> to copy into other drivers.
>

Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/accel/mma9551.c | 37 ++++--------------------------------
>  drivers/iio/accel/mma9553.c | 38 ++++---------------------------------
>  2 files changed, 8 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index 2b74f67536a3..1b4a8b27f14a 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -510,8 +510,7 @@ static int mma9551_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
> -static int mma9551_runtime_suspend(struct device *dev)
> +static __maybe_unused int mma9551_runtime_suspend(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9551_data *data = iio_priv(indio_dev);
> @@ -522,13 +521,13 @@ static int mma9551_runtime_suspend(struct device *dev)
>         mutex_unlock(&data->mutex);
>         if (ret < 0) {
>                 dev_err(&data->client->dev, "powering off device failed\n");
> -               return -EAGAIN;
> +               return ret;
>         }
>
>         return 0;
>  }
>
> -static int mma9551_runtime_resume(struct device *dev)
> +static __maybe_unused int mma9551_runtime_resume(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9551_data *data = iio_priv(indio_dev);
> @@ -542,38 +541,10 @@ static int mma9551_runtime_resume(struct device *dev)
>
>         return 0;
>  }
> -#endif
> -
> -#ifdef CONFIG_PM_SLEEP
> -static int mma9551_suspend(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9551_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -
> -static int mma9551_resume(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9551_data *data = iio_priv(indio_dev);
> -       int ret;
>
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, true);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -#endif
>
>  static const struct dev_pm_ops mma9551_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(mma9551_suspend, mma9551_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>         SET_RUNTIME_PM_OPS(mma9551_runtime_suspend,
>                            mma9551_runtime_resume, NULL)
>  };
> diff --git a/drivers/iio/accel/mma9553.c b/drivers/iio/accel/mma9553.c
> index 32c9a79ebfec..dc2a3316c1a3 100644
> --- a/drivers/iio/accel/mma9553.c
> +++ b/drivers/iio/accel/mma9553.c
> @@ -1149,8 +1149,7 @@ static int mma9553_remove(struct i2c_client *client)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
> -static int mma9553_runtime_suspend(struct device *dev)
> +static __maybe_unused int mma9553_runtime_suspend(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9553_data *data = iio_priv(indio_dev);
> @@ -1161,13 +1160,13 @@ static int mma9553_runtime_suspend(struct device *dev)
>         mutex_unlock(&data->mutex);
>         if (ret < 0) {
>                 dev_err(&data->client->dev, "powering off device failed\n");
> -               return -EAGAIN;
> +               return ret;
>         }
>
>         return 0;
>  }
>
> -static int mma9553_runtime_resume(struct device *dev)
> +static __maybe_unused int mma9553_runtime_resume(struct device *dev)
>  {
>         struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>         struct mma9553_data *data = iio_priv(indio_dev);
> @@ -1181,38 +1180,9 @@ static int mma9553_runtime_resume(struct device *dev)
>
>         return 0;
>  }
> -#endif
> -
> -#ifdef CONFIG_PM_SLEEP
> -static int mma9553_suspend(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9553_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, false);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -
> -static int mma9553_resume(struct device *dev)
> -{
> -       struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -       struct mma9553_data *data = iio_priv(indio_dev);
> -       int ret;
> -
> -       mutex_lock(&data->mutex);
> -       ret = mma9551_set_device_state(data->client, true);
> -       mutex_unlock(&data->mutex);
> -
> -       return ret;
> -}
> -#endif
>
>  static const struct dev_pm_ops mma9553_pm_ops = {
> -       SET_SYSTEM_SLEEP_PM_OPS(mma9553_suspend, mma9553_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>         SET_RUNTIME_PM_OPS(mma9553_runtime_suspend,
>                            mma9553_runtime_resume, NULL)
>  };
> --
> 2.31.1
>