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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Fri, Dec 23, 2016 at 01:24:48PM +0100, Marek Szyprowski wrote:
> 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@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;
>  	}
> +	if (func->rpm_active) {
> +		if (!(bank->rpm_map & (1 << pin_offset))) {
> +			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);
> +		}
> +	}
>  
>  	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);
>  
>  	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 {

A nit:
Missing kernel doc update.

It took me a while to understand the logic behind the rpm_active (the
code is simple but logic was not that easy to see) but it looks good and
sensible. I didn't find any issues with that approach except the usage
of DT configuration as a "notification" mechanism.

For this itself:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


>  	const char	*name;
>  
>  	u32		pin_base;
> +	u32		rpm_map;
>  	void		*soc_priv;
>  	struct device_node *of_node;
>  	struct samsung_pinctrl_drv_data *drvdata;
> -- 
> 1.9.1
>