[PATCH 00/12] Move pad retention control to Exynos pin controller driver

[PATCH 00/12] Move pad retention control to Exynos pin controller driver

[Marek Szyprowski]

Hello,

This patchset is a follow-up of my work on adding runtime PM support
to Exynos pin controller driver:
http://www.spinics.net/lists/arm-kernel/msg550161.html

Runtime PM support itself needs a bit more discussion, so lets first focus on
the prerequisites.

In case of Exynos pin controller driver it is a pad retention control. In
current code it was handled by machine and PMU code and had no relation to
what pin controller driver does. This patch series moves pad retention
control to pin controller driver. While implmenting it, I also did a little
cleanup of both Exynos PMU and pin controller drivers.

Patches are based on linux-next from 2017.01.16 with Exynos4415 support
removal patch applied: https://lkml.org/lkml/2017/1/14/137


Changelog:

v1:
- removed the need to add PMU phandles to all pin controller nodes, so old DTBs
  are properly supported. This has been achieved by getting PMU regmap from
  the "exynos-pmu" device of fixed name.
- more cleanup in Exynos pin controller driver: added missing entries in DT
  documentation, removed "memory allocation failed" messages and added
  initconst annotations.
- added support for s5pv210.
- reworked retention control code to be simpler and ready for adding Exynos5433
  support.

v0: http://www.spinics.net/lists/arm-kernel/msg550161.html
- initial version
- part of "Runtime PM for Exynos pin controller driver" patchset

Patch summary:

Marek Szyprowski (12):
  soc: samsung: pmu: Use common device name to let others to find it
    easily
  soc: samsung: pmu: Use of_device_get_match_data helper
  soc: samsung: pmu: Remove messages for failed memory allocation
  pinctrl: samsung: Document Exynos3250 SoC support
  pinctrl: samsung: Remove messages for failed memory allocation
  pinctrl: samsung: Add missing initconst annotation
  pinctrl: samsung: Remove dead code
  pinctrl: samsung: Use generic of_device_get_match_data helper
  pinctrl: samsung: Add infrastructure for pin-bank retention control
  pinctrl: samsung: Move retention control from mach-exynos to the
    pinctrl driver
  pinctrl: samsung: Move retention control from mach-s5pv210 to the
    pinctrl driver
  pinctrl: samsung: Replace syscore ops with standard platform device
    pm_ops

 .../bindings/pinctrl/samsung-pinctrl.txt           |   1 +
 arch/arm/mach-exynos/suspend.c                     |  64 ------
 arch/arm/mach-s5pv210/pm.c                         |   7 -
 arch/arm/mach-s5pv210/regs-clock.h                 |   4 -
 drivers/pinctrl/samsung/pinctrl-exynos.c           | 235 +++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-s3c64xx.c          |  12 +-
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 122 +++--------
 drivers/pinctrl/samsung/pinctrl-samsung.h          |  42 ++++
 drivers/soc/samsung/exynos-pmu.c                   |  12 +-
 include/linux/soc/samsung/exynos-pmu.h             |  19 ++
 10 files changed, 321 insertions(+), 197 deletions(-)

-- 
1.9.1

[PATCH 01/12] soc: samsung: pmu: Use common device name to let others to find it easily

[Marek Szyprowski]

This patch always set device name to "exynos-pmu" to let other drivers to
find PMU device easily. This is done mainly to get regmap to access PMU
registers from other drivers. This way it can be avoided to add phandle to
the PMU node to almost all drivers in the SoC just to get a regmap access
in the drivers. PMU is something like a SoC wide service, so there is no
point modeling it as hardware dependency for all devices in device tree.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/soc/samsung/exynos-pmu.c       |  1 +
 include/linux/soc/samsung/exynos-pmu.h | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 0acdfd82e751..63bb471845cb 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -120,6 +120,7 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 		pmu_context->pmu_data->pmu_init();
 
 	platform_set_drvdata(pdev, pmu_context);
+	dev_set_name(dev, EXYNOS_PMU_DEV_NAME);
 
 	dev_dbg(dev, "Exynos PMU Driver probe done\n");
 	return 0;
diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
index e2e9de1acc5b..90d9205805ea 100644
--- a/include/linux/soc/samsung/exynos-pmu.h
+++ b/include/linux/soc/samsung/exynos-pmu.h
@@ -12,6 +12,10 @@
 #ifndef __LINUX_SOC_EXYNOS_PMU_H
 #define __LINUX_SOC_EXYNOS_PMU_H
 
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
 enum sys_powerdown {
 	SYS_AFTR,
 	SYS_LPA,
@@ -21,4 +25,19 @@ enum sys_powerdown {
 
 extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
 
+#define EXYNOS_PMU_DEV_NAME "exynos-pmu"
+
+static inline struct regmap *exynos_get_pmu_regs(void)
+{
+	struct device *dev = bus_find_device_by_name(&platform_bus_type, NULL,
+						     EXYNOS_PMU_DEV_NAME);
+	if (dev) {
+		struct regmap *regs = syscon_node_to_regmap(dev->of_node);
+		put_device(dev);
+		if (!IS_ERR(regs))
+			return regs;
+	}
+	return NULL;
+}
+
 #endif /* __LINUX_SOC_EXYNOS_PMU_H */
-- 
1.9.1

Re: [PATCH 01/12] soc: samsung: pmu: Use common device name to let others to find it easily

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:44:56AM +0100, Marek Szyprowski wrote:
> This patch always set device name to "exynos-pmu" to let other drivers to
> find PMU device easily. This is done mainly to get regmap to access PMU
> registers from other drivers. This way it can be avoided to add phandle to
> the PMU node to almost all drivers in the SoC just to get a regmap access
> in the drivers. PMU is something like a SoC wide service, so there is no
> point modeling it as hardware dependency for all devices in device tree.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/soc/samsung/exynos-pmu.c       |  1 +
>  include/linux/soc/samsung/exynos-pmu.h | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 0acdfd82e751..63bb471845cb 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -120,6 +120,7 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>  		pmu_context->pmu_data->pmu_init();
>  
>  	platform_set_drvdata(pdev, pmu_context);
> +	dev_set_name(dev, EXYNOS_PMU_DEV_NAME);
>  
>  	dev_dbg(dev, "Exynos PMU Driver probe done\n");
>  	return 0;
> diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> index e2e9de1acc5b..90d9205805ea 100644
> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -12,6 +12,10 @@
>  #ifndef __LINUX_SOC_EXYNOS_PMU_H
>  #define __LINUX_SOC_EXYNOS_PMU_H
>  
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
>  enum sys_powerdown {
>  	SYS_AFTR,
>  	SYS_LPA,
> @@ -21,4 +25,19 @@ enum sys_powerdown {
>  
>  extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
>  
> +#define EXYNOS_PMU_DEV_NAME "exynos-pmu"

The define is not used outside so it should not be defined in header.
You need it only for the code below (see next comment).

> +
> +static inline struct regmap *exynos_get_pmu_regs(void)
> +{
> +	struct device *dev = bus_find_device_by_name(&platform_bus_type, NULL,
> +						     EXYNOS_PMU_DEV_NAME);
> +	if (dev) {
> +		struct regmap *regs = syscon_node_to_regmap(dev->of_node);
> +		put_device(dev);
> +		if (!IS_ERR(regs))
> +			return regs;
> +	}
> +	return NULL;
> +}

Any particular reason why definion of this is in header? This rather
looks like candidate for EXPORTED_SYMBOL defined in .c (plus Kconfig
dependency).

Best regards,
Krzysztof

> +
>  #endif /* __LINUX_SOC_EXYNOS_PMU_H */
> -- 
> 1.9.1
> 

Re: [PATCH 01/12] soc: samsung: pmu: Use common device name to let others to find it easily

[Tomasz Figa]

Hi Marek,

2017-01-16 15:44 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> This patch always set device name to "exynos-pmu" to let other drivers to
> find PMU device easily. This is done mainly to get regmap to access PMU
> registers from other drivers. This way it can be avoided to add phandle to
> the PMU node to almost all drivers in the SoC just to get a regmap access
> in the drivers. PMU is something like a SoC wide service, so there is no
> point modeling it as hardware dependency for all devices in device tree.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/soc/samsung/exynos-pmu.c       |  1 +
>  include/linux/soc/samsung/exynos-pmu.h | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 0acdfd82e751..63bb471845cb 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -120,6 +120,7 @@ static int exynos_pmu_probe(struct platform_device *pdev)
>                 pmu_context->pmu_data->pmu_init();
>
>         platform_set_drvdata(pdev, pmu_context);
> +       dev_set_name(dev, EXYNOS_PMU_DEV_NAME);
>
>         dev_dbg(dev, "Exynos PMU Driver probe done\n");
>         return 0;
> diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
> index e2e9de1acc5b..90d9205805ea 100644
> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -12,6 +12,10 @@
>  #ifndef __LINUX_SOC_EXYNOS_PMU_H
>  #define __LINUX_SOC_EXYNOS_PMU_H
>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
>  enum sys_powerdown {
>         SYS_AFTR,
>         SYS_LPA,
> @@ -21,4 +25,19 @@ enum sys_powerdown {
>
>  extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
>
> +#define EXYNOS_PMU_DEV_NAME "exynos-pmu"
> +
> +static inline struct regmap *exynos_get_pmu_regs(void)
> +{
> +       struct device *dev = bus_find_device_by_name(&platform_bus_type, NULL,
> +                                                    EXYNOS_PMU_DEV_NAME);
> +       if (dev) {
> +               struct regmap *regs = syscon_node_to_regmap(dev->of_node);
> +               put_device(dev);
> +               if (!IS_ERR(regs))
> +                       return regs;
> +       }
> +       return NULL;

As discussed offline, since this function relies on the PMU driver
probing, we need to defer here if someone calls this function before
that. Since PMU is something that should be assumed to be available in
any Exynos system (it controls to many critical aspects of the SoC), I
think we can simply return ERR_PTR(-EDEFER_PROBE) here instead of
NULL. Maybe also a pr_debug() before deferring could be useful to
debug PMU probe failures.

Best regards,
Tomasz

[PATCH 02/12] soc: samsung: pmu: Use of_device_get_match_data helper

[Marek Szyprowski]

Replace custom code with generic helper to retrieve driver data.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/soc/samsung/exynos-pmu.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 63bb471845cb..0f4b4fce9d90 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -11,6 +11,7 @@
 
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
 
@@ -94,7 +95,6 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
 
 static int exynos_pmu_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 
@@ -111,10 +111,7 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 	pmu_context->dev = dev;
-
-	match = of_match_node(exynos_pmu_of_device_ids, dev->of_node);
-
-	pmu_context->pmu_data = match->data;
+	pmu_context->pmu_data = of_device_get_match_data(dev);
 
 	if (pmu_context->pmu_data->pmu_init)
 		pmu_context->pmu_data->pmu_init();
-- 
1.9.1

[PATCH 03/12] soc: samsung: pmu: Remove messages for failed memory allocation

[Marek Szyprowski]

Memory subsystem already prints message about failed memory
allocation, there is no need to do it in the driver.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/soc/samsung/exynos-pmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 0f4b4fce9d90..43937a3aea28 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -106,10 +106,8 @@ static int exynos_pmu_probe(struct platform_device *pdev)
 	pmu_context = devm_kzalloc(&pdev->dev,
 			sizeof(struct exynos_pmu_context),
 			GFP_KERNEL);
-	if (!pmu_context) {
-		dev_err(dev, "Cannot allocate memory.\n");
+	if (!pmu_context)
 		return -ENOMEM;
-	}
 	pmu_context->dev = dev;
 	pmu_context->pmu_data = of_device_get_match_data(dev);
 
-- 
1.9.1

[PATCH 04/12] pinctrl: samsung: Document Exynos3250 SoC support

[Marek Szyprowski]

Add missing compatible id for Exynos3250 SoC to device tree docs.
Exynos pin control driver supports it since commit d97f5b9804bfcdc1
("pinctrl: exynos: Add driver data for Exynos3250").

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index 1baf19eecabf..5e00a21de2bf 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -13,6 +13,7 @@ Required Properties:
   - "samsung,s3c2450-pinctrl": for S3C2450-compatible pin-controller,
   - "samsung,s3c64xx-pinctrl": for S3C64xx-compatible pin-controller,
   - "samsung,s5pv210-pinctrl": for S5PV210-compatible pin-controller,
+  - "samsung,exynos3250-pinctrl": for Exynos3250 compatible pin-controller.
   - "samsung,exynos4210-pinctrl": for Exynos4210 compatible pin-controller.
   - "samsung,exynos4x12-pinctrl": for Exynos4x12 compatible pin-controller.
   - "samsung,exynos5250-pinctrl": for Exynos5250 compatible pin-controller.
-- 
1.9.1

Re: [PATCH 04/12] pinctrl: samsung: Document Exynos3250 SoC support

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:44:59AM +0100, Marek Szyprowski wrote:
> Add missing compatible id for Exynos3250 SoC to device tree docs.
> Exynos pin control driver supports it since commit d97f5b9804bfcdc1
> ("pinctrl: exynos: Add driver data for Exynos3250").
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Best regards,
Krzysztof

[PATCH 05/12] pinctrl: samsung: Remove messages for failed memory allocation

[Marek Szyprowski]

Memory subsystem already prints message about failed memory
allocation, there is no need to do it in the drivers.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-exynos.c  |  8 ++------
 drivers/pinctrl/samsung/pinctrl-s3c64xx.c | 12 +++---------
 drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++------------------
 3 files changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 24814db251a7..bf753a596209 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -528,10 +528,8 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 
 		weint_data = devm_kzalloc(dev, bank->nr_pins
 					* sizeof(*weint_data), GFP_KERNEL);
-		if (!weint_data) {
-			dev_err(dev, "could not allocate memory for weint_data\n");
+		if (!weint_data)
 			return -ENOMEM;
-		}
 
 		for (idx = 0; idx < bank->nr_pins; ++idx) {
 			irq = irq_of_parse_and_map(bank->of_node, idx);
@@ -559,10 +557,8 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 
 	muxed_data = devm_kzalloc(dev, sizeof(*muxed_data)
 		+ muxed_banks*sizeof(struct samsung_pin_bank *), GFP_KERNEL);
-	if (!muxed_data) {
-		dev_err(dev, "could not allocate memory for muxed_data\n");
+	if (!muxed_data)
 		return -ENOMEM;
-	}
 
 	irq_set_chained_handler_and_data(irq, exynos_irq_demux_eint16_31,
 					 muxed_data);
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
index 4c632812ccff..f17890aa6e25 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
@@ -489,10 +489,8 @@ static int s3c64xx_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 
 	data = devm_kzalloc(dev, sizeof(*data)
 			+ nr_domains * sizeof(*data->domains), GFP_KERNEL);
-	if (!data) {
-		dev_err(dev, "failed to allocate handler data\n");
+	if (!data)
 		return -ENOMEM;
-	}
 	data->drvdata = d;
 
 	bank = d->pin_banks;
@@ -715,10 +713,8 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 		return -ENODEV;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		dev_err(dev, "could not allocate memory for wkup eint data\n");
+	if (!data)
 		return -ENOMEM;
-	}
 	data->drvdata = d;
 
 	for (i = 0; i < NUM_EINT0_IRQ; ++i) {
@@ -751,10 +747,8 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 
 		ddata = devm_kzalloc(dev,
 				sizeof(*ddata) + nr_eints, GFP_KERNEL);
-		if (!ddata) {
-			dev_err(dev, "failed to allocate domain data\n");
+		if (!ddata)
 			return -ENOMEM;
-		}
 		ddata->bank = bank;
 
 		bank->irq_domain = irq_domain_add_linear(bank->of_node,
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 3bc925f61b71..b11e67e85460 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -93,10 +93,8 @@ static int reserve_map(struct device *dev, struct pinctrl_map **map,
 		return 0;
 
 	new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
-	if (!new_map) {
-		dev_err(dev, "krealloc(map) failed\n");
+	if (!new_map)
 		return -ENOMEM;
-	}
 
 	memset(new_map + old_num, 0, (new_num - old_num) * sizeof(*new_map));
 
@@ -133,10 +131,8 @@ static int add_map_configs(struct device *dev, struct pinctrl_map **map,
 
 	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
 			      GFP_KERNEL);
-	if (!dup_configs) {
-		dev_err(dev, "kmemdup(configs) failed\n");
+	if (!dup_configs)
 		return -ENOMEM;
-	}
 
 	(*map)[*num_maps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
 	(*map)[*num_maps].data.configs.group_or_pin = group;
@@ -156,10 +152,8 @@ static int add_config(struct device *dev, unsigned long **configs,
 
 	new_configs = krealloc(*configs, sizeof(*new_configs) * new_num,
 			       GFP_KERNEL);
-	if (!new_configs) {
-		dev_err(dev, "krealloc(configs) failed\n");
+	if (!new_configs)
 		return -ENOMEM;
-	}
 
 	new_configs[old_num] = config;
 
@@ -756,10 +750,8 @@ static struct samsung_pmx_func *samsung_pinctrl_create_functions(
 
 	functions = devm_kzalloc(dev, func_cnt * sizeof(*functions),
 					GFP_KERNEL);
-	if (!functions) {
-		dev_err(dev, "failed to allocate memory for function list\n");
+	if (!functions)
 		return ERR_PTR(-EINVAL);
-	}
 	func = functions;
 
 	/*
@@ -850,10 +842,8 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 
 	pindesc = devm_kzalloc(&pdev->dev, sizeof(*pindesc) *
 			drvdata->nr_pins, GFP_KERNEL);
-	if (!pindesc) {
-		dev_err(&pdev->dev, "mem alloc for pin descriptors failed\n");
+	if (!pindesc)
 		return -ENOMEM;
-	}
 	ctrldesc->pins = pindesc;
 	ctrldesc->npins = drvdata->nr_pins;
 
@@ -867,10 +857,8 @@ static int samsung_pinctrl_register(struct platform_device *pdev,
 	 */
 	pin_names = devm_kzalloc(&pdev->dev, sizeof(char) * PIN_NAME_LENGTH *
 					drvdata->nr_pins, GFP_KERNEL);
-	if (!pin_names) {
-		dev_err(&pdev->dev, "mem alloc for pin names failed\n");
+	if (!pin_names)
 		return -ENOMEM;
-	}
 
 	/* for each pin, the name of the pin is pin-bank name + pin number */
 	for (bank = 0; bank < drvdata->nr_banks; bank++) {
-- 
1.9.1

Re: [PATCH 05/12] pinctrl: samsung: Remove messages for failed memory allocation

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:45:00AM +0100, Marek Szyprowski wrote:
> Memory subsystem already prints message about failed memory
> allocation, there is no need to do it in the drivers.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c  |  8 ++------
>  drivers/pinctrl/samsung/pinctrl-s3c64xx.c | 12 +++---------
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 24 ++++++------------------
>  3 files changed, 11 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index 24814db251a7..bf753a596209 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -528,10 +528,8 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
>  
>  		weint_data = devm_kzalloc(dev, bank->nr_pins
>  					* sizeof(*weint_data), GFP_KERNEL);
> -		if (!weint_data) {
> -			dev_err(dev, "could not allocate memory for weint_data\n");
> +		if (!weint_data)
>  			return -ENOMEM;
> -		}
>  
>  		for (idx = 0; idx < bank->nr_pins; ++idx) {
>  			irq = irq_of_parse_and_map(bank->of_node, idx);
> @@ -559,10 +557,8 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
>  
>  	muxed_data = devm_kzalloc(dev, sizeof(*muxed_data)
>  		+ muxed_banks*sizeof(struct samsung_pin_bank *), GFP_KERNEL);
> -	if (!muxed_data) {
> -		dev_err(dev, "could not allocate memory for muxed_data\n");
> +	if (!muxed_data)
>  		return -ENOMEM;
> -	}
>  
>  	irq_set_chained_handler_and_data(irq, exynos_irq_demux_eint16_31,
>  					 muxed_data);
> diff --git a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
> index 4c632812ccff..f17890aa6e25 100644
> --- a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
> +++ b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
> @@ -489,10 +489,8 @@ static int s3c64xx_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
>  
>  	data = devm_kzalloc(dev, sizeof(*data)
>  			+ nr_domains * sizeof(*data->domains), GFP_KERNEL);
> -	if (!data) {
> -		dev_err(dev, "failed to allocate handler data\n");
> +	if (!data)
>  		return -ENOMEM;
> -	}
>  	data->drvdata = d;
>  
>  	bank = d->pin_banks;
> @@ -715,10 +713,8 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
>  		return -ENODEV;
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> -	if (!data) {
> -		dev_err(dev, "could not allocate memory for wkup eint data\n");
> +	if (!data)
>  		return -ENOMEM;
> -	}
>  	data->drvdata = d;
>  
>  	for (i = 0; i < NUM_EINT0_IRQ; ++i) {
> @@ -751,10 +747,8 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
>  
>  		ddata = devm_kzalloc(dev,
>  				sizeof(*ddata) + nr_eints, GFP_KERNEL);
> -		if (!ddata) {
> -			dev_err(dev, "failed to allocate domain data\n");
> +		if (!ddata)
>  			return -ENOMEM;
> -		}
>  		ddata->bank = bank;
>  
>  		bank->irq_domain = irq_domain_add_linear(bank->of_node,
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 3bc925f61b71..b11e67e85460 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -93,10 +93,8 @@ static int reserve_map(struct device *dev, struct pinctrl_map **map,
>  		return 0;
>  
>  	new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
> -	if (!new_map) {
> -		dev_err(dev, "krealloc(map) failed\n");
> +	if (!new_map)
>  		return -ENOMEM;
> -	}
>  
>  	memset(new_map + old_num, 0, (new_num - old_num) * sizeof(*new_map));
>  
> @@ -133,10 +131,8 @@ static int add_map_configs(struct device *dev, struct pinctrl_map **map,
>  
>  	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
>  			      GFP_KERNEL);
> -	if (!dup_configs) {
> -		dev_err(dev, "kmemdup(configs) failed\n");
> +	if (!dup_configs)
>  		return -ENOMEM;
> -	}
>  
>  	(*map)[*num_maps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
>  	(*map)[*num_maps].data.configs.group_or_pin = group;
> @@ -156,10 +152,8 @@ static int add_config(struct device *dev, unsigned long **configs,
>  
>  	new_configs = krealloc(*configs, sizeof(*new_configs) * new_num,
>  			       GFP_KERNEL);
> -	if (!new_configs) {
> -		dev_err(dev, "krealloc(configs) failed\n");
> +	if (!new_configs)
>  		return -ENOMEM;
> -	}
>  
>  	new_configs[old_num] = config;
>  
> @@ -756,10 +750,8 @@ static struct samsung_pmx_func *samsung_pinctrl_create_functions(
>  
>  	functions = devm_kzalloc(dev, func_cnt * sizeof(*functions),
>  					GFP_KERNEL);
> -	if (!functions) {
> -		dev_err(dev, "failed to allocate memory for function list\n");
> +	if (!functions)
>  		return ERR_PTR(-EINVAL);

Out of scope of this patch - why are we returning EINVAL instead of
ENOMEM?

Regardless:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[PATCH 06/12] pinctrl: samsung: Add missing initconst annotation

[Marek Szyprowski]

Exynos5433 support has been added in parallel to adding initconst
annotation to most of the init data structures, so add those
annotations also to Exynos5433 structures.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index bf753a596209..70b94ad10cc1 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 };
 
 /* pin banks of exynos5433 pin-controller - ALIVE */
-static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
 	EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04),
 	EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08),
@@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 };
 
 /* pin banks of exynos5433 pin-controller - AUD */
-static const struct samsung_pin_bank_data exynos5433_pin_banks1[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks1[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00),
 	EXYNOS5433_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04),
 };
 
 /* pin banks of exynos5433 pin-controller - CPIF */
-static const struct samsung_pin_bank_data exynos5433_pin_banks2[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks2[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(2, 0x000, "gpv6", 0x00),
 };
 
 /* pin banks of exynos5433 pin-controller - eSE */
-static const struct samsung_pin_bank_data exynos5433_pin_banks3[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks3[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj2", 0x00),
 };
 
 /* pin banks of exynos5433 pin-controller - FINGER */
-static const struct samsung_pin_bank_data exynos5433_pin_banks4[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks4[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(4, 0x000, "gpd5", 0x00),
 };
 
 /* pin banks of exynos5433 pin-controller - FSYS */
-static const struct samsung_pin_bank_data exynos5433_pin_banks5[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks5[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gph1", 0x00),
 	EXYNOS5433_PIN_BANK_EINTG(7, 0x020, "gpr4", 0x04),
 	EXYNOS5433_PIN_BANK_EINTG(5, 0x040, "gpr0", 0x08),
@@ -1310,17 +1310,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 };
 
 /* pin banks of exynos5433 pin-controller - IMEM */
-static const struct samsung_pin_bank_data exynos5433_pin_banks6[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks6[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(8, 0x000, "gpf0", 0x00),
 };
 
 /* pin banks of exynos5433 pin-controller - NFC */
-static const struct samsung_pin_bank_data exynos5433_pin_banks7[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks7[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj0", 0x00),
 };
 
 /* pin banks of exynos5433 pin-controller - PERIC */
-static const struct samsung_pin_bank_data exynos5433_pin_banks8[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks8[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gpv7", 0x00),
 	EXYNOS5433_PIN_BANK_EINTG(5, 0x020, "gpb0", 0x04),
 	EXYNOS5433_PIN_BANK_EINTG(8, 0x040, "gpc0", 0x08),
@@ -1341,7 +1341,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 };
 
 /* pin banks of exynos5433 pin-controller - TOUCH */
-static const struct samsung_pin_bank_data exynos5433_pin_banks9[] = {
+static const struct samsung_pin_bank_data exynos5433_pin_banks9[] __initconst = {
 	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj1", 0x00),
 };
 
@@ -1349,7 +1349,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
  * Samsung pinctrl driver data for Exynos5433 SoC. Exynos5433 SoC includes
  * ten gpio/pin-mux/pinconfig controllers.
  */
-const struct samsung_pin_ctrl exynos5433_pin_ctrl[] = {
+const struct samsung_pin_ctrl exynos5433_pin_ctrl[] __initconst = {
 	{
 		/* pin-controller instance 0 data */
 		.pin_banks	= exynos5433_pin_banks0,
-- 
1.9.1

Re: [PATCH 06/12] pinctrl: samsung: Add missing initconst annotation

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote:
> Exynos5433 support has been added in parallel to adding initconst
> annotation to most of the init data structures, so add those
> annotations also to Exynos5433 structures.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index bf753a596209..70b94ad10cc1 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  };
>  
>  /* pin banks of exynos5433 pin-controller - ALIVE */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
>  	EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04),
>  	EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08),
> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  };

Your change is aligned with existing code... but after I started to look
into it I think this should not be initconst.

The pin_banks (initconst) are referenced in pin_ctrl (initconst) which
is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match
then is used in samsung_pinctrl_driver (for obvious reasons not
initconst).

We suppress the bind so this looks safe - this data should not be ever used
after init - but it is not correct strictly speaking.

Let's imagine some weird future platform which will use DT overlays with
pinctrl. I think the overlays could affect the tree after the init
stage.

The other question is why the DEBUG_SECTION_MISMATCH is not complaining
about this...

Best regards,
Krzysztof

>  
>  /* pin banks of exynos5433 pin-controller - AUD */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks1[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks1[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00),
>  	EXYNOS5433_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - CPIF */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks2[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks2[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(2, 0x000, "gpv6", 0x00),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - eSE */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks3[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks3[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj2", 0x00),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - FINGER */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks4[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks4[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(4, 0x000, "gpd5", 0x00),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - FSYS */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks5[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks5[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gph1", 0x00),
>  	EXYNOS5433_PIN_BANK_EINTG(7, 0x020, "gpr4", 0x04),
>  	EXYNOS5433_PIN_BANK_EINTG(5, 0x040, "gpr0", 0x08),
> @@ -1310,17 +1310,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  };
>  
>  /* pin banks of exynos5433 pin-controller - IMEM */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks6[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks6[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(8, 0x000, "gpf0", 0x00),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - NFC */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks7[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks7[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj0", 0x00),
>  };
>  
>  /* pin banks of exynos5433 pin-controller - PERIC */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks8[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks8[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gpv7", 0x00),
>  	EXYNOS5433_PIN_BANK_EINTG(5, 0x020, "gpb0", 0x04),
>  	EXYNOS5433_PIN_BANK_EINTG(8, 0x040, "gpc0", 0x08),
> @@ -1341,7 +1341,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  };
>  
>  /* pin banks of exynos5433 pin-controller - TOUCH */
> -static const struct samsung_pin_bank_data exynos5433_pin_banks9[] = {
> +static const struct samsung_pin_bank_data exynos5433_pin_banks9[] __initconst = {
>  	EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj1", 0x00),
>  };
>  
> @@ -1349,7 +1349,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>   * Samsung pinctrl driver data for Exynos5433 SoC. Exynos5433 SoC includes
>   * ten gpio/pin-mux/pinconfig controllers.
>   */
> -const struct samsung_pin_ctrl exynos5433_pin_ctrl[] = {
> +const struct samsung_pin_ctrl exynos5433_pin_ctrl[] __initconst = {
>  	{
>  		/* pin-controller instance 0 data */
>  		.pin_banks	= exynos5433_pin_banks0,
> -- 
> 1.9.1
> 

Re: [PATCH 06/12] pinctrl: samsung: Add missing initconst annotation

[Tomasz Figa]

2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote:
>> Exynos5433 support has been added in parallel to adding initconst
>> annotation to most of the init data structures, so add those
>> annotations also to Exynos5433 structures.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
>> index bf753a596209..70b94ad10cc1 100644
>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
>> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>  };
>>
>>  /* pin banks of exynos5433 pin-controller - ALIVE */
>> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = {
>> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = {
>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04),
>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08),
>> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>  };
>
> Your change is aligned with existing code... but after I started to look
> into it I think this should not be initconst.
>
> The pin_banks (initconst) are referenced in pin_ctrl (initconst) which
> is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match
> then is used in samsung_pinctrl_driver (for obvious reasons not
> initconst).
>
> We suppress the bind so this looks safe - this data should not be ever used
> after init - but it is not correct strictly speaking.
>
> Let's imagine some weird future platform which will use DT overlays with
> pinctrl. I think the overlays could affect the tree after the init
> stage.

I think it's not very realistic to have a Samsung on-SoC pin
controller in an overlay. AFAIR we already assume in several places
that this driver fully initializes in init stage, so we can save some
memory by discarding this data

Still, I guess we could add some measure to make sure nothing attempts
to probe this driver after the data is discarded.

Best regards,
Tomasz

Re: [PATCH 06/12] pinctrl: samsung: Add missing initconst annotation

[Krzysztof Kozlowski]

On Tue, Jan 17, 2017 at 6:44 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
>> On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote:
>>> Exynos5433 support has been added in parallel to adding initconst
>>> annotation to most of the init data structures, so add those
>>> annotations also to Exynos5433 structures.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>  drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
>>> index bf753a596209..70b94ad10cc1 100644
>>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
>>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
>>> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>>  };
>>>
>>>  /* pin banks of exynos5433 pin-controller - ALIVE */
>>> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = {
>>> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = {
>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04),
>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08),
>>> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>>  };
>>
>> Your change is aligned with existing code... but after I started to look
>> into it I think this should not be initconst.
>>
>> The pin_banks (initconst) are referenced in pin_ctrl (initconst) which
>> is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match
>> then is used in samsung_pinctrl_driver (for obvious reasons not
>> initconst).
>>
>> We suppress the bind so this looks safe - this data should not be ever used
>> after init - but it is not correct strictly speaking.
>>
>> Let's imagine some weird future platform which will use DT overlays with
>> pinctrl. I think the overlays could affect the tree after the init
>> stage.
>
> I think it's not very realistic to have a Samsung on-SoC pin
> controller in an overlay. AFAIR we already assume in several places
> that this driver fully initializes in init stage, so we can save some
> memory by discarding this data

Two things here. Either we write proper code or we write code for
"believe it will work". There is a problem with second approach -
after some time, the driver will be developed further it your
assumptions might change... then stuff might get broken because no one
expected that driver data is discarded. Code working only with some
assumptions is a more difficult to maintain than code which is
correct.

In the same time if you believe your code is "correct" then just mark
samsung_pinctrl_dt_match initconst... Marking only few parts in the
chain is broken.

Second thing. How much memory you are saving? 5 kB in total? 10 kB? Is
it worth the risk?

> Still, I guess we could add some measure to make sure nothing attempts
> to probe this driver after the data is discarded.

That could work... but last time I asked for checking the return value
of match_data then you (I think) and Bartlomiej responded "no sense".
So please be consistent. Anyway it is not possible to NULL-ify the
match_data after first probe because samsung_pinctrl_dt_match is
const. This means you would have to add some crazy logic to check for
late (from overlays) or second probe and discard it... just because we
wanted to save some memory and marked initconst something referenced
from non-initconst section.

Best regards,
Krzysztof

Re: [PATCH 06/12] pinctrl: samsung: Add missing initconst annotation

[Tomasz Figa]

2017-01-17 15:34 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Tue, Jan 17, 2017 at 6:44 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> 2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
>>> On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote:
>>>> Exynos5433 support has been added in parallel to adding initconst
>>>> annotation to most of the init data structures, so add those
>>>> annotations also to Exynos5433 structures.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>  drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++-----------
>>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
>>>> index bf753a596209..70b94ad10cc1 100644
>>>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
>>>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
>>>> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>>>  };
>>>>
>>>>  /* pin banks of exynos5433 pin-controller - ALIVE */
>>>> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = {
>>>> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = {
>>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
>>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04),
>>>>       EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08),
>>>> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>>>>  };
>>>
>>> Your change is aligned with existing code... but after I started to look
>>> into it I think this should not be initconst.
>>>
>>> The pin_banks (initconst) are referenced in pin_ctrl (initconst) which
>>> is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match
>>> then is used in samsung_pinctrl_driver (for obvious reasons not
>>> initconst).
>>>
>>> We suppress the bind so this looks safe - this data should not be ever used
>>> after init - but it is not correct strictly speaking.
>>>
>>> Let's imagine some weird future platform which will use DT overlays with
>>> pinctrl. I think the overlays could affect the tree after the init
>>> stage.
>>
>> I think it's not very realistic to have a Samsung on-SoC pin
>> controller in an overlay. AFAIR we already assume in several places
>> that this driver fully initializes in init stage, so we can save some
>> memory by discarding this data
>
> Two things here. Either we write proper code or we write code for
> "believe it will work". There is a problem with second approach -
> after some time, the driver will be developed further it your
> assumptions might change... then stuff might get broken because no one
> expected that driver data is discarded. Code working only with some
> assumptions is a more difficult to maintain than code which is
> correct.

We write code that is proper for the assumptions we made during the
design, i.e. design decisions. Otherwise you would end up with the
driver bloated everywhere with handing things that don't make any
sense, which would be definitely _not_ easier to maintain. Of course
we can have some trivial code to enforce the assumptions, as I
mentioned before.

This is a driver for an on-chip low level device that is very, very
unlikely to be hotpluggable and almost any other driver for remaining
on-chip devices depend on it. It doesn't mean that we can't change the
design decision later, if in future it becomes feasible to load
low-level SoC drivers from modules then we can make the driver support
so.

>
> In the same time if you believe your code is "correct" then just mark
> samsung_pinctrl_dt_match initconst... Marking only few parts in the
> chain is broken.

I haven't said that samsung_pinctrl_dt_match should be kept
non-initconst. It's indeed a good idea to make it initconst as well.

>
> Second thing. How much memory you are saving? 5 kB in total? 10 kB? Is
> it worth the risk?

10 kB in one driver. Multiply by the number of similar low level
drivers for all SoCs supported by ARM v7 multiplatform build and you
can get quite a significant number of memory saved...

>
>> Still, I guess we could add some measure to make sure nothing attempts
>> to probe this driver after the data is discarded.
>
> That could work... but last time I asked for checking the return value
> of match_data then you (I think) and Bartlomiej responded "no sense".
> So please be consistent.

That's because this is already enforced by the design of the driver model...

> Anyway it is not possible to NULL-ify the
> match_data after first probe because samsung_pinctrl_dt_match is
> const. This means you would have to add some crazy logic to check for
> late (from overlays) or second probe and discard it... just because we
> wanted to save some memory and marked initconst something referenced
> from non-initconst section.

...and similarly the assumptions we are talking about in case of this
problem could be enforced the same way. There is already
platform_driver_probe() that is supposed to be used for such on-SoC
non-hotpluggable devices and which triggers a one-time probe.
Unfortunately it is a bit broken currently, because it can't handle
deferred probes.

Anyway, I'm not even insisting that we should keep this initconst
annotation. Just wanted to point out that there are valid reasons for
it to be useful and correct (in terms of the design decisions taken
when writing the driver).

Best regards,
Tomasz

[PATCH 07/12] pinctrl: samsung: Remove dead code

[Marek Szyprowski]

'enable' parameter has been removed a while ago, so all code for handling
it can be simply removed.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index b11e67e85460..7648a280c0f4 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -350,7 +350,7 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,
 
 /* enable or disable a pinmux function */
 static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
-					unsigned group, bool enable)
+					unsigned group)
 {
 	struct samsung_pinctrl_drv_data *drvdata;
 	const struct samsung_pin_bank_type *type;
@@ -380,8 +380,7 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 
 	data = readl(reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 	data &= ~(mask << shift);
-	if (enable)
-		data |= func->val << shift;
+	data |= func->val << shift;
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
@@ -392,7 +391,7 @@ static int samsung_pinmux_set_mux(struct pinctrl_dev *pctldev,
 				  unsigned selector,
 				  unsigned group)
 {
-	samsung_pinmux_setup(pctldev, selector, group, true);
+	samsung_pinmux_setup(pctldev, selector, group);
 	return 0;
 }
 
-- 
1.9.1

[PATCH 08/12] pinctrl: samsung: Use generic of_device_get_match_data helper

[Marek Szyprowski]

Replace custom code with generic helper.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 7648a280c0f4..86f23842f681 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -27,6 +27,7 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
+#include <linux/of_device.h>
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 
@@ -955,15 +956,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
 	return 0;
 }
 
-static const struct of_device_id samsung_pinctrl_dt_match[];
-
 /* retrieve the soc specific data */
 static const struct samsung_pin_ctrl *
 samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 			     struct platform_device *pdev)
 {
 	int id;
-	const struct of_device_id *match;
+	const struct samsung_pin_ctrl *match_data;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *np;
 	const struct samsung_pin_bank_data *bdata;
@@ -978,8 +977,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id\n");
 		return ERR_PTR(-ENOENT);
 	}
-	match = of_match_node(samsung_pinctrl_dt_match, node);
-	ctrl = (struct samsung_pin_ctrl *)match->data + id;
+	match_data = of_device_get_match_data(&pdev->dev);
+	ctrl = match_data + id;
 
 	d->suspend = ctrl->suspend;
 	d->resume = ctrl->resume;
-- 
1.9.1

Re: [PATCH 08/12] pinctrl: samsung: Use generic of_device_get_match_data helper

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:45:03AM +0100, Marek Szyprowski wrote:
> Replace custom code with generic helper.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 7648a280c0f4..86f23842f681 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -27,6 +27,7 @@
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/syscore_ops.h>
>  
> @@ -955,15 +956,13 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static const struct of_device_id samsung_pinctrl_dt_match[];
> -
>  /* retrieve the soc specific data */
>  static const struct samsung_pin_ctrl *
>  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
>  			     struct platform_device *pdev)
>  {
>  	int id;
> -	const struct of_device_id *match;
> +	const struct samsung_pin_ctrl *match_data;
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *np;
>  	const struct samsung_pin_bank_data *bdata;
> @@ -978,8 +977,8 @@ static int samsung_gpiolib_unregister(struct platform_device *pdev,
>  		dev_err(&pdev->dev, "failed to get alias id\n");
>  		return ERR_PTR(-ENOENT);
>  	}
> -	match = of_match_node(samsung_pinctrl_dt_match, node);
> -	ctrl = (struct samsung_pin_ctrl *)match->data + id;
> +	match_data = of_device_get_match_data(&pdev->dev);
> +	ctrl = match_data + id;

I did not receive answer for my previous question here - why you need
two samsung_pin_ctrl* variables? Why you need additional match_data?

Just make it:
	ctrl = of_device_get_match_data(&pdev->dev);
	ctrl += id;
To me it looks simpler and obvious. Having two variables for the same
brings questions.

If you do not agree, say it. I don't mind. But I would appreciate if
comments were not ignored.

Best regards,
Krzysztof

[PATCH 09/12] pinctrl: samsung: Add infrastructure for pin-bank retention control

[Marek Szyprowski]

Pad retention control after suspend/resume cycle should be done from pin
controller driver instead of PMU (power management unit) driver to avoid
possible ordering and logical dependencies. Till now it worked fine only
because PMU driver registered its sys_ops after pin controller.

This patch adds infrastructure to handle pad retention during pin control
driver resume.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 12 ++++++---
 drivers/pinctrl/samsung/pinctrl-samsung.h | 42 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 86f23842f681..95a84086a2e9 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -1075,6 +1075,9 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->eint_gpio_init(drvdata);
 	if (ctrl->eint_wkup_init)
 		ctrl->eint_wkup_init(drvdata);
+	if (ctrl->retention_data && ctrl->retention_data->init)
+		drvdata->retention_ctrl = ctrl->retention_data->init(drvdata,
+							  ctrl->retention_data);
 
 	platform_set_drvdata(pdev, drvdata);
 
@@ -1127,15 +1130,15 @@ static void samsung_pinctrl_suspend_dev(
 
 	if (drvdata->suspend)
 		drvdata->suspend(drvdata);
+	if (drvdata->retention_ctrl && drvdata->retention_ctrl->on)
+		drvdata->retention_ctrl->on(drvdata);
+
 }
 
 /**
  * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
  *
  * Restore one of the banks that was saved during suspend.
- *
- * We don't bother doing anything complicated to avoid glitching lines since
- * we're called before pad retention is turned off.
  */
 static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 {
@@ -1174,6 +1177,9 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 			if (widths[type])
 				writel(bank->pm_save[type], reg + offs[type]);
 	}
+
+	if (drvdata->retention_ctrl && drvdata->retention_ctrl->off)
+		drvdata->retention_ctrl->off(drvdata);
 }
 
 /**
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 6f7ce7539a00..6079142422f8 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -185,10 +185,48 @@ struct samsung_pin_bank {
 };
 
 /**
+ * struct samsung_retention_data: runtime pin-bank retention control data.
+ * @regs: array of PMU registers to control pad retention.
+ * @nr_regs: number of registers in @regs array.
+ * @value: value to store to registers to turn off retention.
+ * @refcnt: atomic counter if retention control affects more than one bank.
+ * @priv: retention control code private data
+ * @on: platform specific callback to enter retention mode.
+ * @off: platform specific callback to exit retention mode.
+ **/
+struct samsung_retention_ctrl {
+	const u32	*regs;
+	int		nr_regs;
+	u32		value;
+	atomic_t	*refcnt;
+	void		*priv;
+	void		(*on)(struct samsung_pinctrl_drv_data *);
+	void		(*off)(struct samsung_pinctrl_drv_data *);
+};
+
+/**
+ * struct samsung_retention_data: represent a pin-bank retention control data.
+ * @regs: array of PMU registers to control pad retention.
+ * @nr_regs: number of registers in @regs array.
+ * @value: value to store to registers to turn off retention.
+ * @refcnt: atomic counter if retention control affects more than one bank.
+ * @init: platform specific callback to initialize retention control.
+ **/
+struct samsung_retention_data {
+	const u32	*regs;
+	int		nr_regs;
+	u32		value;
+	atomic_t	*refcnt;
+	struct samsung_retention_ctrl *(*init)(struct samsung_pinctrl_drv_data *,
+					const struct samsung_retention_data *);
+};
+
+/**
  * struct samsung_pin_ctrl: represent a pin controller.
  * @pin_banks: list of pin banks included in this controller.
  * @nr_banks: number of pin banks.
  * @nr_ext_resources: number of the extra base address for pin banks.
+ * @retention_data: configuration data for retention control.
  * @eint_gpio_init: platform specific callback to setup the external gpio
  *	interrupts for the controller.
  * @eint_wkup_init: platform specific callback to setup the external wakeup
@@ -198,6 +236,7 @@ struct samsung_pin_ctrl {
 	const struct samsung_pin_bank_data *pin_banks;
 	u32		nr_banks;
 	int		nr_ext_resources;
+	const struct samsung_retention_data *retention_data;
 
 	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
 	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
@@ -219,6 +258,7 @@ struct samsung_pin_ctrl {
  * @nr_function: number of such pin functions.
  * @pin_base: starting system wide pin number.
  * @nr_pins: number of pins supported by the controller.
+ * @retention_ctrl: retention control runtime data.
  */
 struct samsung_pinctrl_drv_data {
 	struct list_head		node;
@@ -238,6 +278,8 @@ struct samsung_pinctrl_drv_data {
 	unsigned int			pin_base;
 	unsigned int			nr_pins;
 
+	const struct samsung_retention_ctrl *retention_ctrl;
+
 	void (*suspend)(struct samsung_pinctrl_drv_data *);
 	void (*resume)(struct samsung_pinctrl_drv_data *);
 };
-- 
1.9.1

Re: [PATCH 09/12] pinctrl: samsung: Add infrastructure for pin-bank retention control

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:45:04AM +0100, Marek Szyprowski wrote:
> Pad retention control after suspend/resume cycle should be done from pin
> controller driver instead of PMU (power management unit) driver to avoid
> possible ordering and logical dependencies. Till now it worked fine only
> because PMU driver registered its sys_ops after pin controller.
> 
> This patch adds infrastructure to handle pad retention during pin control
> driver resume.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 12 ++++++---
>  drivers/pinctrl/samsung/pinctrl-samsung.h | 42 +++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 86f23842f681..95a84086a2e9 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1075,6 +1075,9 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>  		ctrl->eint_gpio_init(drvdata);
>  	if (ctrl->eint_wkup_init)
>  		ctrl->eint_wkup_init(drvdata);
> +	if (ctrl->retention_data && ctrl->retention_data->init)
> +		drvdata->retention_ctrl = ctrl->retention_data->init(drvdata,
> +							  ctrl->retention_data);
>  
>  	platform_set_drvdata(pdev, drvdata);
>  
> @@ -1127,15 +1130,15 @@ static void samsung_pinctrl_suspend_dev(
>  
>  	if (drvdata->suspend)
>  		drvdata->suspend(drvdata);
> +	if (drvdata->retention_ctrl && drvdata->retention_ctrl->on)
> +		drvdata->retention_ctrl->on(drvdata);
> +

This new line is not needed (checkpatch might complain... either in
normal mode or in strict). Beside that, thanks for splitting the
patch for interface from the implementation:

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

Best regards,
Krzysztof

Re: [PATCH 09/12] pinctrl: samsung: Add infrastructure for pin-bank retention control

[Tomasz Figa]

Hi Marek,

2017-01-16 15:45 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Pad retention control after suspend/resume cycle should be done from pin
> controller driver instead of PMU (power management unit) driver to avoid
> possible ordering and logical dependencies. Till now it worked fine only
> because PMU driver registered its sys_ops after pin controller.
>
> This patch adds infrastructure to handle pad retention during pin control
> driver resume.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 12 ++++++---
>  drivers/pinctrl/samsung/pinctrl-samsung.h | 42 +++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 3 deletions(-)

Please see my comments inline.

> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 86f23842f681..95a84086a2e9 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1075,6 +1075,9 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>                 ctrl->eint_gpio_init(drvdata);
>         if (ctrl->eint_wkup_init)
>                 ctrl->eint_wkup_init(drvdata);
> +       if (ctrl->retention_data && ctrl->retention_data->init)
> +               drvdata->retention_ctrl = ctrl->retention_data->init(drvdata,
> +                                                         ctrl->retention_data);

Is there really a use case for init being optional? Also maybe it
could make sense to add a pr_debug() in case retention_data is not
present (maybe even a warning would make sense, since the old code
should be deprecated...).

>
>         platform_set_drvdata(pdev, drvdata);
>
> @@ -1127,15 +1130,15 @@ static void samsung_pinctrl_suspend_dev(
>
>         if (drvdata->suspend)
>                 drvdata->suspend(drvdata);
> +       if (drvdata->retention_ctrl && drvdata->retention_ctrl->on)
> +               drvdata->retention_ctrl->on(drvdata);
> +

nit: Stray blank line.

>  }
>
>  /**
>   * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
>   *
>   * Restore one of the banks that was saved during suspend.
> - *
> - * We don't bother doing anything complicated to avoid glitching lines since
> - * we're called before pad retention is turned off.

I think the comment is still valid. Just could be adjusted to take
into account that now we are in charge of turning off the retention
after restoring the registers.

>   */
>  static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
>  {
> @@ -1174,6 +1177,9 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
>                         if (widths[type])
>                                 writel(bank->pm_save[type], reg + offs[type]);
>         }
> +
> +       if (drvdata->retention_ctrl && drvdata->retention_ctrl->off)
> +               drvdata->retention_ctrl->off(drvdata);
>  }
>
>  /**
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 6f7ce7539a00..6079142422f8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -185,10 +185,48 @@ struct samsung_pin_bank {
>  };
>
>  /**
> + * struct samsung_retention_data: runtime pin-bank retention control data.
> + * @regs: array of PMU registers to control pad retention.
> + * @nr_regs: number of registers in @regs array.
> + * @value: value to store to registers to turn off retention.
> + * @refcnt: atomic counter if retention control affects more than one bank.
> + * @priv: retention control code private data
> + * @on: platform specific callback to enter retention mode.
> + * @off: platform specific callback to exit retention mode.
> + **/
> +struct samsung_retention_ctrl {
> +       const u32       *regs;
> +       int             nr_regs;
> +       u32             value;
> +       atomic_t        *refcnt;
> +       void            *priv;
> +       void            (*on)(struct samsung_pinctrl_drv_data *);
> +       void            (*off)(struct samsung_pinctrl_drv_data *);

bikeshed: I think enable/disable is more commonly used for this kind
of operations.

Best regards,
Tomasz

[PATCH 10/12] pinctrl: samsung: Move retention control from mach-exynos to the pinctrl driver

[Marek Szyprowski]

This patch moves pad retention control from PMU driver to Exynos pin
controller driver. This helps to avoid possible ordering and logical
dependencies between machine, PMU and pin control code. Till now it
worked fine only because sys_ops for PMU and pin controller were called
in registration order.
This is also a preparation for adding new features to Exynos pin
controller driver, like runtime power management and suspending
individual pin controllers, which might be a part of some power domain.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/mach-exynos/suspend.c           |  64 -------------
 drivers/pinctrl/samsung/pinctrl-exynos.c | 149 +++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 64 deletions(-)

diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index 25e7c5326259..312d3a886e92 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -57,7 +57,6 @@ struct exynos_wkup_irq {
 struct exynos_pm_data {
 	const struct exynos_wkup_irq *wkup_irq;
 	unsigned int wake_disable_mask;
-	const unsigned int *release_ret_regs;
 
 	void (*pm_prepare)(void);
 	void (*pm_resume_prepare)(void);
@@ -95,47 +94,6 @@ struct exynos_pm_data {
 	{ /* sentinel */ },
 };
 
-static const unsigned int exynos_release_ret_regs[] = {
-	S5P_PAD_RET_MAUDIO_OPTION,
-	S5P_PAD_RET_GPIO_OPTION,
-	S5P_PAD_RET_UART_OPTION,
-	S5P_PAD_RET_MMCA_OPTION,
-	S5P_PAD_RET_MMCB_OPTION,
-	S5P_PAD_RET_EBIA_OPTION,
-	S5P_PAD_RET_EBIB_OPTION,
-	REG_TABLE_END,
-};
-
-static const unsigned int exynos3250_release_ret_regs[] = {
-	S5P_PAD_RET_MAUDIO_OPTION,
-	S5P_PAD_RET_GPIO_OPTION,
-	S5P_PAD_RET_UART_OPTION,
-	S5P_PAD_RET_MMCA_OPTION,
-	S5P_PAD_RET_MMCB_OPTION,
-	S5P_PAD_RET_EBIA_OPTION,
-	S5P_PAD_RET_EBIB_OPTION,
-	S5P_PAD_RET_MMC2_OPTION,
-	S5P_PAD_RET_SPI_OPTION,
-	REG_TABLE_END,
-};
-
-static const unsigned int exynos5420_release_ret_regs[] = {
-	EXYNOS_PAD_RET_DRAM_OPTION,
-	EXYNOS_PAD_RET_MAUDIO_OPTION,
-	EXYNOS_PAD_RET_JTAG_OPTION,
-	EXYNOS5420_PAD_RET_GPIO_OPTION,
-	EXYNOS5420_PAD_RET_UART_OPTION,
-	EXYNOS5420_PAD_RET_MMCA_OPTION,
-	EXYNOS5420_PAD_RET_MMCB_OPTION,
-	EXYNOS5420_PAD_RET_MMCC_OPTION,
-	EXYNOS5420_PAD_RET_HSI_OPTION,
-	EXYNOS_PAD_RET_EBIA_OPTION,
-	EXYNOS_PAD_RET_EBIB_OPTION,
-	EXYNOS5420_PAD_RET_SPI_OPTION,
-	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
-	REG_TABLE_END,
-};
-
 static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
 {
 	const struct exynos_wkup_irq *wkup_irq;
@@ -441,15 +399,6 @@ static int exynos5420_pm_suspend(void)
 	return 0;
 }
 
-static void exynos_pm_release_retention(void)
-{
-	unsigned int i;
-
-	for (i = 0; (pm_data->release_ret_regs[i] != REG_TABLE_END); i++)
-		pmu_raw_writel(EXYNOS_WAKEUP_FROM_LOWPWR,
-				pm_data->release_ret_regs[i]);
-}
-
 static void exynos_pm_resume(void)
 {
 	u32 cpuid = read_cpuid_part();
@@ -457,9 +406,6 @@ static void exynos_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	if (cpuid == ARM_CPU_PART_CORTEX_A9)
 		scu_enable(S5P_VA_SCU);
 
@@ -481,9 +427,6 @@ static void exynos3250_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	pmu_raw_writel(S5P_USE_STANDBY_WFI_ALL, S5P_CENTRAL_SEQ_OPTION);
 
 	if (call_firmware_op(resume) == -ENOSYS
@@ -521,9 +464,6 @@ static void exynos5420_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	/* For release retention */
-	exynos_pm_release_retention();
-
 	pmu_raw_writel(exynos_pmu_spare3, S5P_PMU_SPARE3);
 
 early_wakeup:
@@ -636,7 +576,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos3250_pm_data = {
 	.wkup_irq	= exynos3250_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos3250_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos3250_pm_resume,
 	.pm_prepare	= exynos3250_pm_prepare,
@@ -646,7 +585,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos4_pm_data = {
 	.wkup_irq	= exynos4_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos_pm_resume,
 	.pm_prepare	= exynos_pm_prepare,
@@ -656,7 +594,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos5250_pm_data = {
 	.wkup_irq	= exynos5250_wkup_irq,
 	.wake_disable_mask = ((0xFF << 8) | (0x1F << 1)),
-	.release_ret_regs = exynos_release_ret_regs,
 	.pm_suspend	= exynos_pm_suspend,
 	.pm_resume	= exynos_pm_resume,
 	.pm_prepare	= exynos_pm_prepare,
@@ -666,7 +603,6 @@ static void exynos_suspend_finish(void)
 static const struct exynos_pm_data exynos5420_pm_data = {
 	.wkup_irq	= exynos5250_wkup_irq,
 	.wake_disable_mask = (0x7F << 7) | (0x1F << 1),
-	.release_ret_regs = exynos5420_release_ret_regs,
 	.pm_resume_prepare = exynos5420_prepare_pm_resume,
 	.pm_resume	= exynos5420_pm_resume,
 	.pm_suspend	= exynos5420_pm_suspend,
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 70b94ad10cc1..652d3eabe1e5 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -28,7 +28,10 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/regmap.h>
 #include <linux/err.h>
+#include <linux/soc/samsung/exynos-pmu.h>
+#include <linux/soc/samsung/exynos-regs-pmu.h>
 
 #include "pinctrl-samsung.h"
 #include "pinctrl-exynos.h"
@@ -690,6 +693,58 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	},
 };
 
+/* Pad retention control code for accessing PMU regmap */
+static atomic_t exynos_shared_retention_refcnt;
+
+static void exynos_retention_on(struct samsung_pinctrl_drv_data *drvdata)
+{
+	if (drvdata->retention_ctrl->refcnt)
+		atomic_inc(drvdata->retention_ctrl->refcnt);
+}
+
+static void exynos_retention_off(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct regmap *pmu_regs = drvdata->retention_ctrl->priv;
+	int i;
+
+	if (drvdata->retention_ctrl->refcnt &&
+	    !atomic_dec_and_test(drvdata->retention_ctrl->refcnt))
+		return;
+
+	for (i = 0; i < drvdata->retention_ctrl->nr_regs; i++)
+		regmap_write(pmu_regs, drvdata->retention_ctrl->regs[i],
+			     drvdata->retention_ctrl->value);
+}
+
+static struct samsung_retention_ctrl *
+exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
+		      const struct samsung_retention_data *data)
+{
+	struct samsung_retention_ctrl *ctrl;
+	struct regmap *pmu_regs;
+
+	ctrl = devm_kzalloc(drvdata->dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	pmu_regs = exynos_get_pmu_regs();
+	if (!pmu_regs) {
+		dev_err(drvdata->dev,
+			"failed to lookup PMU regmap, no support for pad retention\n");
+		return NULL;
+	}
+
+	ctrl->priv = pmu_regs;
+	ctrl->regs = data->regs;
+	ctrl->nr_regs = data->nr_regs;
+	ctrl->value = data->value;
+	ctrl->refcnt = data->refcnt;
+	ctrl->on = exynos_retention_on;
+	ctrl->off = exynos_retention_off;
+
+	return ctrl;
+}
+
 /* pin banks of exynos3250 pin-controller 0 */
 static const struct samsung_pin_bank_data exynos3250_pin_banks0[] __initconst = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -722,6 +777,30 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 };
 
 /*
+ * PMU pad retention groups for Exynos3250 doesn't match pin banks, so handle
+ * them all together
+ */
+static const u32 exynos3250_retention_regs[] = {
+	S5P_PAD_RET_MAUDIO_OPTION,
+	S5P_PAD_RET_GPIO_OPTION,
+	S5P_PAD_RET_UART_OPTION,
+	S5P_PAD_RET_MMCA_OPTION,
+	S5P_PAD_RET_MMCB_OPTION,
+	S5P_PAD_RET_EBIA_OPTION,
+	S5P_PAD_RET_EBIB_OPTION,
+	S5P_PAD_RET_MMC2_OPTION,
+	S5P_PAD_RET_SPI_OPTION,
+};
+
+static const struct samsung_retention_data exynos3250_retention_data __initconst = {
+	.regs	 = exynos3250_retention_regs,
+	.nr_regs = ARRAY_SIZE(exynos3250_retention_regs),
+	.value	 = EXYNOS_WAKEUP_FROM_LOWPWR,
+	.refcnt	 = &exynos_shared_retention_refcnt,
+	.init	 = exynos_retention_init,
+};
+
+/*
  * Samsung pinctrl driver data for Exynos3250 SoC. Exynos3250 SoC includes
  * two gpio/pin-mux/pinconfig controllers.
  */
@@ -733,6 +812,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos3250_retention_data,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos3250_pin_banks1,
@@ -741,6 +821,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos3250_retention_data,
 	},
 };
 
@@ -793,6 +874,36 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	EXYNOS_PIN_BANK_EINTN(7, 0x000, "gpz"),
 };
 
+/* PMU pad retention groups registers for Exynos4 (without audio) */
+static const u32 exynos4_retention_regs[] = {
+	S5P_PAD_RET_GPIO_OPTION,
+	S5P_PAD_RET_UART_OPTION,
+	S5P_PAD_RET_MMCA_OPTION,
+	S5P_PAD_RET_MMCB_OPTION,
+	S5P_PAD_RET_EBIA_OPTION,
+	S5P_PAD_RET_EBIB_OPTION,
+};
+
+static const struct samsung_retention_data exynos4_retention_data __initconst = {
+	.regs	 = exynos4_retention_regs,
+	.nr_regs = ARRAY_SIZE(exynos4_retention_regs),
+	.value	 = EXYNOS_WAKEUP_FROM_LOWPWR,
+	.refcnt	 = &exynos_shared_retention_refcnt,
+	.init	 = exynos_retention_init,
+};
+
+/* PMU retention control for audio pins can be tied to audio pin bank */
+static const u32 exynos4_audio_retention_regs[] = {
+	S5P_PAD_RET_MAUDIO_OPTION,
+};
+
+static const struct samsung_retention_data exynos4_audio_retention_data __initconst = {
+	.regs	 = exynos4_audio_retention_regs,
+	.nr_regs = ARRAY_SIZE(exynos4_audio_retention_regs),
+	.value	 = EXYNOS_WAKEUP_FROM_LOWPWR,
+	.init	 = exynos_retention_init,
+};
+
 /*
  * Samsung pinctrl driver data for Exynos4210 SoC. Exynos4210 SoC includes
  * three gpio/pin-mux/pinconfig controllers.
@@ -805,6 +916,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos4210_pin_banks1,
@@ -813,10 +925,12 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos4210_pin_banks2,
 		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks2),
+		.retention_data	= &exynos4_audio_retention_data,
 	},
 };
 
@@ -890,6 +1004,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos4x12_pin_banks1,
@@ -898,6 +1013,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos4x12_pin_banks2,
@@ -905,6 +1021,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_audio_retention_data,
 	}, {
 		/* pin-controller instance 3 data */
 		.pin_banks	= exynos4x12_pin_banks3,
@@ -984,6 +1101,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos5250_pin_banks1,
@@ -991,6 +1109,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_retention_data,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos5250_pin_banks2,
@@ -1005,6 +1124,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &exynos4_audio_retention_data,
 	},
 };
 
@@ -1231,6 +1351,30 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 	EXYNOS_PIN_BANK_EINTG(7, 0x000, "gpz", 0x00),
 };
 
+/* PMU pad retention groups registers for Exynos5420 (without audio) */
+static const u32 exynos5420_retention_regs[] = {
+	EXYNOS_PAD_RET_DRAM_OPTION,
+	EXYNOS_PAD_RET_JTAG_OPTION,
+	EXYNOS5420_PAD_RET_GPIO_OPTION,
+	EXYNOS5420_PAD_RET_UART_OPTION,
+	EXYNOS5420_PAD_RET_MMCA_OPTION,
+	EXYNOS5420_PAD_RET_MMCB_OPTION,
+	EXYNOS5420_PAD_RET_MMCC_OPTION,
+	EXYNOS5420_PAD_RET_HSI_OPTION,
+	EXYNOS_PAD_RET_EBIA_OPTION,
+	EXYNOS_PAD_RET_EBIB_OPTION,
+	EXYNOS5420_PAD_RET_SPI_OPTION,
+	EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION,
+};
+
+static const struct samsung_retention_data exynos5420_retention_data __initconst = {
+	.regs	 = exynos5420_retention_regs,
+	.nr_regs = ARRAY_SIZE(exynos5420_retention_regs),
+	.value	 = EXYNOS_WAKEUP_FROM_LOWPWR,
+	.refcnt	 = &exynos_shared_retention_refcnt,
+	.init	 = exynos_retention_init,
+};
+
 /*
  * Samsung pinctrl driver data for Exynos5420 SoC. Exynos5420 SoC includes
  * four gpio/pin-mux/pinconfig controllers.
@@ -1242,26 +1386,31 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks0),
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.retention_data	= &exynos5420_retention_data,
 	}, {
 		/* pin-controller instance 1 data */
 		.pin_banks	= exynos5420_pin_banks1,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks1),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_data	= &exynos5420_retention_data,
 	}, {
 		/* pin-controller instance 2 data */
 		.pin_banks	= exynos5420_pin_banks2,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks2),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_data	= &exynos5420_retention_data,
 	}, {
 		/* pin-controller instance 3 data */
 		.pin_banks	= exynos5420_pin_banks3,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks3),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_data	= &exynos5420_retention_data,
 	}, {
 		/* pin-controller instance 4 data */
 		.pin_banks	= exynos5420_pin_banks4,
 		.nr_banks	= ARRAY_SIZE(exynos5420_pin_banks4),
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.retention_data	= &exynos4_audio_retention_data,
 	},
 };
 
-- 
1.9.1

[PATCH 11/12] pinctrl: samsung: Move retention control from mach-s5pv210 to the pinctrl driver

[Marek Szyprowski]

This patch moves pad retention control from S5PV210 machine code to
Exynos pin controller driver. This helps to avoid possible ordering
and logical dependencies between machine and pin control code. Till
now it worked fine only because sys_ops for machine code and pin
controller were called in registration order.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/mach-s5pv210/pm.c               |  7 ----
 arch/arm/mach-s5pv210/regs-clock.h       |  4 ---
 drivers/pinctrl/samsung/pinctrl-exynos.c | 56 ++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-s5pv210/pm.c b/arch/arm/mach-s5pv210/pm.c
index 21b4b13c5ab7..7d69666de5ba 100644
--- a/arch/arm/mach-s5pv210/pm.c
+++ b/arch/arm/mach-s5pv210/pm.c
@@ -155,13 +155,6 @@ static void s5pv210_suspend_finish(void)
  */
 static void s5pv210_pm_resume(void)
 {
-	u32 tmp;
-
-	tmp = __raw_readl(S5P_OTHERS);
-	tmp |= (S5P_OTHERS_RET_IO | S5P_OTHERS_RET_CF |\
-		S5P_OTHERS_RET_MMC | S5P_OTHERS_RET_UART);
-	__raw_writel(tmp , S5P_OTHERS);
-
 	s3c_pm_do_restore_core(s5pv210_core_save, ARRAY_SIZE(s5pv210_core_save));
 }
 
diff --git a/arch/arm/mach-s5pv210/regs-clock.h b/arch/arm/mach-s5pv210/regs-clock.h
index 4640f0f03c12..fb3eb77412db 100644
--- a/arch/arm/mach-s5pv210/regs-clock.h
+++ b/arch/arm/mach-s5pv210/regs-clock.h
@@ -188,10 +188,6 @@
 #define S5P_SLEEP_CFG_USBOSC_EN		(1 << 1)
 
 /* OTHERS Resgister */
-#define S5P_OTHERS_RET_IO		(1 << 31)
-#define S5P_OTHERS_RET_CF		(1 << 30)
-#define S5P_OTHERS_RET_MMC		(1 << 29)
-#define S5P_OTHERS_RET_UART		(1 << 28)
 #define S5P_OTHERS_USB_SIG_MASK		(1 << 16)
 
 /* S5P_DAC_CONTROL */
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 652d3eabe1e5..fdadc9cbcfce 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -24,6 +24,7 @@
 #include <linux/irqdomain.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -643,6 +644,60 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 			exynos_pinctrl_resume_bank(drvdata, bank);
 }
 
+/* Retention control for S5PV210 are located at the end of clock controller */
+#define S5P_OTHERS 0xE000
+
+#define S5P_OTHERS_RET_IO		(1 << 31)
+#define S5P_OTHERS_RET_CF		(1 << 30)
+#define S5P_OTHERS_RET_MMC		(1 << 29)
+#define S5P_OTHERS_RET_UART		(1 << 28)
+
+static void s5pv210_retention_off(struct samsung_pinctrl_drv_data *drvdata)
+{
+	void *clk_base = drvdata->retention_ctrl->priv;
+	u32 tmp;
+
+	tmp = __raw_readl(clk_base + S5P_OTHERS);
+	tmp |= (S5P_OTHERS_RET_IO | S5P_OTHERS_RET_CF | S5P_OTHERS_RET_MMC |
+		S5P_OTHERS_RET_UART);
+	__raw_writel(tmp, clk_base + S5P_OTHERS);
+}
+
+static struct samsung_retention_ctrl *
+s5pv210_retention_init(struct samsung_pinctrl_drv_data *drvdata,
+		       const struct samsung_retention_data *data)
+{
+	struct samsung_retention_ctrl *ctrl;
+	struct device_node *np;
+	void *clk_base;
+
+	ctrl = devm_kzalloc(drvdata->dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	np = of_find_compatible_node(NULL, NULL, "samsung,s5pv210-clock");
+	if (!np) {
+		pr_err("%s: failed to find clock controller DT node\n",
+			__func__);
+		return NULL;
+	}
+
+	clk_base = of_iomap(np, 0);
+	if (!clk_base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
+		return NULL;
+	}
+
+	ctrl->priv = clk_base;
+	ctrl->off = s5pv210_retention_off;
+
+	return ctrl;
+}
+
+static const struct samsung_retention_data s5pv210_retention_data __initconst = {
+	.init	 = s5pv210_retention_init,
+};
+
 /* pin banks of s5pv210 pin-controller */
 static const struct samsung_pin_bank_data s5pv210_pin_bank[] __initconst = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -690,6 +745,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
 		.eint_wkup_init = exynos_eint_wkup_init,
 		.suspend	= exynos_pinctrl_suspend,
 		.resume		= exynos_pinctrl_resume,
+		.retention_data	= &s5pv210_retention_data,
 	},
 };
 
-- 
1.9.1

[PATCH 12/12] pinctrl: samsung: Replace syscore ops with standard platform device pm_ops

[Marek Szyprowski]

Once the dependency on PMU driver (for pad retention control) has been
removed, there is no reason to use syscore_ops based suspend/resume.
This patch replaces it with standard platform device pm_ops based solution.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 72 ++++++-------------------------
 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 95a84086a2e9..48acf5908b84 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -29,7 +29,6 @@
 #include <linux/irqdomain.h>
 #include <linux/of_device.h>
 #include <linux/spinlock.h>
-#include <linux/syscore_ops.h>
 
 #include "../core.h"
 #include "pinctrl-samsung.h"
@@ -49,9 +48,6 @@
 	{ "samsung,pin-val", PINCFG_TYPE_DAT },
 };
 
-/* Global list of devices (struct samsung_pinctrl_drv_data) */
-static LIST_HEAD(drvdata_list);
-
 static unsigned int pin_base;
 
 static int samsung_get_group_count(struct pinctrl_dev *pctldev)
@@ -1081,22 +1077,18 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, drvdata);
 
-	/* Add to the global list */
-	list_add_tail(&drvdata->node, &drvdata_list);
-
 	return 0;
 }
 
 #ifdef CONFIG_PM
-
 /**
- * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device
+ * samsung_pinctrl_suspend - save pinctrl state for suspend
  *
  * Save data for all banks handled by this device.
  */
-static void samsung_pinctrl_suspend_dev(
-	struct samsung_pinctrl_drv_data *drvdata)
+static int samsung_pinctrl_suspend(struct device *dev)
 {
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
 	int i;
 
 	for (i = 0; i < drvdata->nr_banks; i++) {
@@ -1133,15 +1125,17 @@ static void samsung_pinctrl_suspend_dev(
 	if (drvdata->retention_ctrl && drvdata->retention_ctrl->on)
 		drvdata->retention_ctrl->on(drvdata);
 
+	return 0;
 }
 
 /**
- * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
+ * samsung_pinctrl_resume - restore pinctrl state from suspend
  *
  * Restore one of the banks that was saved during suspend.
  */
-static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
+static int samsung_pinctrl_resume(struct device *dev)
 {
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
 	int i;
 
 	if (drvdata->resume)
@@ -1180,48 +1174,10 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 
 	if (drvdata->retention_ctrl && drvdata->retention_ctrl->off)
 		drvdata->retention_ctrl->off(drvdata);
-}
-
-/**
- * samsung_pinctrl_suspend - save pinctrl state for suspend
- *
- * Save data for all banks across all devices.
- */
-static int samsung_pinctrl_suspend(void)
-{
-	struct samsung_pinctrl_drv_data *drvdata;
-
-	list_for_each_entry(drvdata, &drvdata_list, node) {
-		samsung_pinctrl_suspend_dev(drvdata);
-	}
-
 	return 0;
 }
-
-/**
- * samsung_pinctrl_resume - restore pinctrl state for suspend
- *
- * Restore data for all banks across all devices.
- */
-static void samsung_pinctrl_resume(void)
-{
-	struct samsung_pinctrl_drv_data *drvdata;
-
-	list_for_each_entry_reverse(drvdata, &drvdata_list, node) {
-		samsung_pinctrl_resume_dev(drvdata);
-	}
-}
-
-#else
-#define samsung_pinctrl_suspend		NULL
-#define samsung_pinctrl_resume		NULL
 #endif
 
-static struct syscore_ops samsung_pinctrl_syscore_ops = {
-	.suspend	= samsung_pinctrl_suspend,
-	.resume		= samsung_pinctrl_resume,
-};
-
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 #ifdef CONFIG_PINCTRL_EXYNOS
 	{ .compatible = "samsung,exynos3250-pinctrl",
@@ -1263,25 +1219,23 @@ static void samsung_pinctrl_resume(void)
 };
 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)
+};
+
 static struct platform_driver samsung_pinctrl_driver = {
 	.probe		= samsung_pinctrl_probe,
 	.driver = {
 		.name	= "samsung-pinctrl",
 		.of_match_table = samsung_pinctrl_dt_match,
 		.suppress_bind_attrs = true,
+		.pm = &samsung_pinctrl_pm_ops,
 	},
 };
 
 static int __init samsung_pinctrl_drv_register(void)
 {
-	/*
-	 * Register syscore ops for save/restore of registers across suspend.
-	 * It's important to ensure that this driver is running at an earlier
-	 * initcall level than any arch-specific init calls that install syscore
-	 * ops that turn off pad retention (like exynos_pm_resume).
-	 */
-	register_syscore_ops(&samsung_pinctrl_syscore_ops);
-
 	return platform_driver_register(&samsung_pinctrl_driver);
 }
 postcore_initcall(samsung_pinctrl_drv_register);
-- 
1.9.1

Re: [PATCH 00/12] Move pad retention control to Exynos pin controller driver

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 07:44:55AM +0100, Marek Szyprowski wrote:
> Hello,
> 
> This patchset is a follow-up of my work on adding runtime PM support
> to Exynos pin controller driver:
> http://www.spinics.net/lists/arm-kernel/msg550161.html
> 
> Runtime PM support itself needs a bit more discussion, so lets first focus on
> the prerequisites.
> 
> In case of Exynos pin controller driver it is a pad retention control. In
> current code it was handled by machine and PMU code and had no relation to
> what pin controller driver does. This patch series moves pad retention
> control to pin controller driver. While implmenting it, I also did a little
> cleanup of both Exynos PMU and pin controller drivers.
> 
> Patches are based on linux-next from 2017.01.16 with Exynos4415 support
> removal patch applied: https://lkml.org/lkml/2017/1/14/137

I didn't find explicit statement for dependency between the patches in
the patchset itself and also I could not find the usage of
EXYNOS_PMU_DEV_NAME by pinctrl driver.

Do the pinctrl changes depend on soc/samsung patches? If not, then
probably your future work will depend on this?

Best regards,
Krzysztof

Re: [PATCH 00/12] Move pad retention control to Exynos pin controller driver

[Krzysztof Kozlowski]

On Mon, Jan 16, 2017 at 09:23:45PM +0200, Krzysztof Kozlowski wrote:
> On Mon, Jan 16, 2017 at 07:44:55AM +0100, Marek Szyprowski wrote:
> > Hello,
> > 
> > This patchset is a follow-up of my work on adding runtime PM support
> > to Exynos pin controller driver:
> > http://www.spinics.net/lists/arm-kernel/msg550161.html
> > 
> > Runtime PM support itself needs a bit more discussion, so lets first focus on
> > the prerequisites.
> > 
> > In case of Exynos pin controller driver it is a pad retention control. In
> > current code it was handled by machine and PMU code and had no relation to
> > what pin controller driver does. This patch series moves pad retention
> > control to pin controller driver. While implmenting it, I also did a little
> > cleanup of both Exynos PMU and pin controller drivers.
> > 
> > Patches are based on linux-next from 2017.01.16 with Exynos4415 support
> > removal patch applied: https://lkml.org/lkml/2017/1/14/137
> 
> I didn't find explicit statement for dependency between the patches in
> the patchset itself and also I could not find the usage of
> EXYNOS_PMU_DEV_NAME by pinctrl driver.

Okay, now I found the usage of the function from header so this answers
the question below about dependency in the patchset itself. Anyway it
would be nice to state that explicitly.

Best regards,
Krzysztof

> 
> Do the pinctrl changes depend on soc/samsung patches? If not, then
> probably your future work will depend on this?
> 
> Best regards,
> Krzysztof