[PATCH 8/9] pinctrl: samsung: Add runtime PM support

[Tomasz Figa]

Hi,

2016-12-23 21:24 GMT+09:00 Marek Szyprowski <m.szyprowski at samsung.com>:
> This patch adds runtime power management support to Samsung pin controller
> driver. It uses recently introduced property to mark some pin states as
> suitable for power off. When all pins for given controller are set to this
> special state, the controller is able to enter runtime suspend state. This
> in turn might allow to turn respective power domain off to reduce power
> consumption.
>
> This patch moves saving driver state to runtime pm callbacks and implements
> system sleep suspend/resume callbacks with pm_runtime_force_suspend/resume
> helpers to keep the runtime pm state consistent with the hardware both
> during runtime and system sleep transitions.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++++++++++++++++++--
>  drivers/pinctrl/samsung/pinctrl-samsung.h |  1 +
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 301169d2b6e1..c741e93d65b8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -28,6 +28,7 @@
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>
>  #include "../core.h"
> @@ -378,6 +379,17 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
>                 shift -= 32;
>                 reg += 4;
>         }

nit: IMHO a blank line here would look better.

> +       if (func->rpm_active) {
> +               if (!(bank->rpm_map & (1 << pin_offset))) {

I think rpm_map could benefit from generic bitmap helpers.

> +                       bank->rpm_map |= 1 << pin_offset;
> +                       pm_runtime_get_sync(drvdata->dev);
> +               }
> +       } else {
> +               if ((bank->rpm_map & (1 << pin_offset))) {
> +                       bank->rpm_map &= ~(1 << pin_offset);
> +                       pm_runtime_put(drvdata->dev);
> +               }
> +       }

Then, the above could be simplified into:

        if (func->rpm_active && !test_bit(pin_offset, &bank->rpm_map)) {
                __set_bit(pin_offset, &bank->rpm_map);
                pm_runtime_get_sync(drvdata->dev);
        } else if (!func->rpm_active && test_bit(pin_offset, &bank->rpm_map) {
                __clear_bit(pin_offset, &bank->rpm_map);
                pm_runtime_put(drvdata->dev);
        }

>
>         spin_lock_irqsave(&bank->slock, flags);
>
> @@ -427,6 +439,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>         if (cfg_type >= PINCFG_TYPE_NUM || !type->fld_width[cfg_type])
>                 return -EINVAL;
>
> +       pm_runtime_get_sync(drvdata->dev);
> +
>         width = type->fld_width[cfg_type];
>         cfg_reg = type->reg_offset[cfg_type];
>
> @@ -449,6 +463,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>
>         spin_unlock_irqrestore(&bank->slock, flags);
>
> +       pm_runtime_put(drvdata->dev);
> +
>         return 0;
>  }
>
> @@ -1096,6 +1112,8 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>                 ctrl->retention_init(drvdata);
>
>         platform_set_drvdata(pdev, drvdata);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);

Can we really enable this unconditionally on all platforms?

>
>         return 0;
>  }
> @@ -1242,8 +1260,10 @@ static int samsung_pinctrl_resume(struct device *dev)
>  MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
>
>  static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
> -                                    samsung_pinctrl_resume)
> +       SET_RUNTIME_PM_OPS(samsung_pinctrl_suspend,
> +                          samsung_pinctrl_resume, NULL)
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                    pm_runtime_force_resume)
>  };
>
>  static struct platform_driver samsung_pinctrl_driver = {
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index edeafa00abd3..ccb24ec46796 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -172,6 +172,7 @@ struct samsung_pin_bank {
>         const char      *name;
>
>         u32             pin_base;
> +       u32             rpm_map;

This could be an unsigned long instead and then could benefit from the
bitmap helpers.

Best regards,
Tomasz