[RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[Tomasz Figa]

This RFC series is a work on replacing static platform-specific data in
pinctrl-samsung driver with data dynamically parsed from device tree.

It aims at reducing the SoC-specific part of the driver and thus the
amount of modifications to driver sources when adding support for next
SoCs (like Exynos4x12).

Furthermore, moving definitions of pin banks to device tree will allow
to simplify GPIO and GEINT specification to a format similar to used
previously by gpiolib-based implementation, using a phandle to the bank
and pin index inside the bank, e.g.
	gpios = <&gpa1 4 0>;
	interrupt-parent = <&gpa1>;
	interrupts = <4 0>;

Any comments are welcome.

TODO:
 - bindings documentation
 - per-bank GPIO and GEINT specification

Tomasz Figa (6):
  pinctrl: exynos: Parse wakeup-eint parameters from DT
  pinctrl: samsung: Parse pin banks from DT
  pinctrl: exynos: Remove static platform-specific data
  pinctrl: samsung: Parse bank-specific eint offset from DT
  ARM: dts: exynos4210: Remove legacy gpio nodes
  ARM: dts: exynos4210: Add platform-specific descriptions for pin
    controllers

 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi | 605 ++++++++++++++++++++++++
 arch/arm/boot/dts/exynos4210-pinctrl.dtsi       |   2 +
 arch/arm/boot/dts/exynos4210.dtsi               | 241 +---------
 drivers/pinctrl/pinctrl-exynos.c                | 124 ++---
 drivers/pinctrl/pinctrl-exynos.h                | 157 ------
 drivers/pinctrl/pinctrl-samsung.c               | 153 +++++-
 drivers/pinctrl/pinctrl-samsung.h               |  19 +-
 7 files changed, 813 insertions(+), 488 deletions(-)
 create mode 100644 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

-- 
1.7.12

[RFC 1/6] pinctrl: exynos: Parse wakeup-eint parameters from DT

[Tomasz Figa]

This patch converts the pinctrl-exynos driver to parse wakeup interrupt
count and register offsets from device tree. It reduces the amount of
static platform-specific data and facilitates adding further SoC
variants to pinctrl-samsung driver.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 0d01d19..575378a 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -435,6 +435,8 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	struct device_node *np;
 	struct exynos_weint_data *weint_data;
 	int idx, irq;
+	u32 val;
+	int ret;
 
 	for_each_child_of_node(dev->of_node, np) {
 		if (of_match_node(exynos_wkup_irq_ids, np)) {
@@ -445,6 +447,26 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	if (!wkup_np)
 		return -ENODEV;
 
+	ret = of_property_read_u32(wkup_np, "samsung,weint-count", &val);
+	if (ret)
+		return -EINVAL;
+	d->ctrl->nr_wint = val;
+
+	ret = of_property_read_u32(wkup_np, "samsung,weint-con", &val);
+	if (ret)
+		return -EINVAL;
+	d->ctrl->weint_con = val;
+
+	ret = of_property_read_u32(wkup_np, "samsung,weint-mask", &val);
+	if (ret)
+		return -EINVAL;
+	d->ctrl->weint_mask = val;
+
+	ret = of_property_read_u32(wkup_np, "samsung,weint-pend", &val);
+	if (ret)
+		return -EINVAL;
+	d->ctrl->weint_pend = val;
+
 	d->wkup_irqd = irq_domain_add_linear(wkup_np, d->ctrl->nr_wint,
 				&exynos_wkup_irqd_ops, d);
 	if (!d->gpio_irqd) {
-- 
1.7.12

[RFC 2/6] pinctrl: samsung: Parse pin banks from DT

[Tomasz Figa]

Currently platform-specific properties such as list of pin banks,
register offsets and bitfield sizes is being taken from static data
structure residing in pinctrl-exynos.c.

This patch modifies the pinctrl-samsung driver to parse all
platform-specific data from device tree, which will allow to remove the
static data structures and facilitate adding of further SoC variants to
the pinctrl-samsung driver.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c  |   5 ++
 drivers/pinctrl/pinctrl-samsung.c | 148 +++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-samsung.h |  17 ++++-
 3 files changed, 166 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 575378a..827b744 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -599,3 +599,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.label		= "exynos4210-gpio-ctrl2",
 	},
 };
+
+struct samsung_pin_ctrl_variant exynos4_pin_ctrl = {
+	.eint_gpio_init = exynos_eint_gpio_init,
+	.eint_wkup_init = exynos_eint_wkup_init,
+};
diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index dd108a9..ff1d001 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
+#include <linux/spinlock.h>
 
 #include "core.h"
 #include "pinctrl-samsung.h"
@@ -46,6 +47,10 @@ struct pin_config {
 	{ "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN },
 };
 
+DEFINE_SPINLOCK(init_lock);
+
+static unsigned int pin_base = 0;
+
 /* check if the selector is a valid pin group selector */
 static int samsung_get_group_count(struct pinctrl_dev *pctldev)
 {
@@ -599,6 +604,8 @@ static int __init samsung_pinctrl_parse_dt(struct platform_device *pdev,
 		u32 function;
 		if (of_find_property(cfg_np, "interrupt-controller", NULL))
 			continue;
+		if (of_find_property(cfg_np, "gpio-controller", NULL))
+			continue;
 
 		ret = samsung_pinctrl_parse_dt_pins(pdev, cfg_np,
 					&drvdata->pctl,	&pin_list, &npins);
@@ -775,6 +782,59 @@ static int __init samsung_gpiolib_unregister(struct platform_device *pdev,
 
 static const struct of_device_id samsung_pinctrl_dt_match[];
 
+static int samsung_pinctrl_parse_dt_bank(struct samsung_pin_bank *bank,
+							struct device_node *np)
+{
+	int ret;
+	u32 val;
+
+	ret = of_property_read_string(np, "samsung,pin-bank", &bank->name);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(np, "samsung,pctl-offset", &val);
+	if (ret)
+		return ret;
+	bank->pctl_offset = val;
+
+	ret = of_property_read_u32(np, "samsung,pin-count", &val);
+	if (ret)
+		return ret;
+	bank->nr_pins = val;
+
+	ret = of_property_read_u32(np, "samsung,func-width", &val);
+	if (ret)
+		return ret;
+	bank->func_width = val;
+
+	ret = of_property_read_u32(np, "samsung,pud-width", &val);
+	if (ret)
+		return ret;
+	bank->pud_width = val;
+
+	ret = of_property_read_u32(np, "samsung,drv-width", &val);
+	if (ret)
+		return ret;
+	bank->drv_width = val;
+
+	ret = of_property_read_u32(np, "samsung,conpdn-width", &val);
+	if (!ret)
+		bank->conpdn_width = val;
+
+	ret = of_property_read_u32(np, "samsung,pudpdn-width", &val);
+	if (!ret)
+		bank->pudpdn_width = val;
+
+	if (!of_find_property(np, "interrupt-controller", NULL)) {
+		bank->eint_type = EINT_TYPE_NONE;
+		return 0;
+	}
+
+	bank->eint_type = EINT_TYPE_GPIO;
+
+	return 0;
+}
+
 /* retrieve the soc specific data */
 static struct samsung_pin_ctrl *samsung_pinctrl_get_soc_data(
 				struct platform_device *pdev)
@@ -782,6 +842,14 @@ static struct samsung_pin_ctrl *samsung_pinctrl_get_soc_data(
 	int id;
 	const struct of_device_id *match;
 	const struct device_node *node = pdev->dev.of_node;
+	struct device_node *bank_np;
+	struct samsung_pin_ctrl *ctrl;
+	struct samsung_pin_bank *banks, *b;
+	struct samsung_pin_ctrl_variant *variant;
+	unsigned int bank_cnt = 0;
+	unsigned int eint_cnt = 0;
+	u32 val;
+	int ret;
 
 	id = of_alias_get_id(pdev->dev.of_node, "pinctrl");
 	if (id < 0) {
@@ -789,7 +857,83 @@ static struct samsung_pin_ctrl *samsung_pinctrl_get_soc_data(
 		return NULL;
 	}
 	match = of_match_node(samsung_pinctrl_dt_match, node);
-	return (struct samsung_pin_ctrl *)match->data + id;
+	variant = match->data;
+
+	for_each_child_of_node(node, bank_np) {
+		if (!of_find_property(bank_np, "gpio-controller", NULL))
+			continue;
+		++bank_cnt;
+	}
+
+	if (!bank_cnt) {
+		dev_err(&pdev->dev, "no pin banks specified\n");
+		return NULL;
+	}
+
+	ctrl = devm_kzalloc(&pdev->dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl) {
+		dev_err(&pdev->dev, "failed to allocate soc data\n");
+		return NULL;
+	}
+
+	banks = devm_kzalloc(&pdev->dev,
+			bank_cnt * sizeof(*ctrl->pin_banks), GFP_KERNEL);
+	if (!banks) {
+		dev_err(&pdev->dev, "failed to allocate pin banks\n");
+		return NULL;
+	}
+
+	b = banks;
+	for_each_child_of_node(node, bank_np) {
+		if (!of_find_property(bank_np, "gpio-controller", NULL))
+			continue;
+		if (samsung_pinctrl_parse_dt_bank(b, bank_np))
+			return NULL;
+		b->pin_base = ctrl->nr_pins;
+		ctrl->nr_pins += b->nr_pins;
+		if (of_find_property(bank_np, "interrupt-controller", NULL)) {
+			b->irq_base = eint_cnt;
+			eint_cnt += b->nr_pins;
+		}
+		++b;
+	}
+
+	if (eint_cnt) {
+		ret = of_property_read_u32(node, "samsung,geint-con", &val);
+		if (ret)
+			return NULL;
+		ctrl->geint_con = val;
+
+		ret = of_property_read_u32(node, "samsung,geint-mask", &val);
+		if (ret)
+			return NULL;
+		ctrl->geint_mask = val;
+
+		ret = of_property_read_u32(node, "samsung,geint-pend", &val);
+		if (ret)
+			return NULL;
+		ctrl->geint_pend = val;
+
+		ret = of_property_read_u32(node, "samsung,svc", &val);
+		if (ret)
+			return NULL;
+		ctrl->svc = val;
+
+		ctrl->eint_gpio_init = variant->eint_gpio_init;
+	}
+
+	ctrl->pin_banks = banks;
+	ctrl->nr_banks = bank_cnt;
+	ctrl->nr_gint = eint_cnt;
+	ctrl->label = node->name;
+	ctrl->eint_wkup_init = variant->eint_wkup_init;
+
+	spin_lock(&init_lock);
+	ctrl->base = pin_base;
+	pin_base += ctrl->nr_pins;
+	spin_unlock(&init_lock);
+
+	return ctrl;
 }
 
 static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
@@ -857,7 +1001,7 @@ static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
 
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 	{ .compatible = "samsung,pinctrl-exynos4210",
-		.data = (void *)exynos4210_pin_ctrl },
+		.data = &exynos4_pin_ctrl },
 	{},
 };
 MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index b895693..5d59ce6 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -123,7 +123,19 @@ struct samsung_pin_bank {
 	u8		pudpdn_width;
 	enum eint_type	eint_type;
 	u32		irq_base;
-	char		*name;
+	const char	*name;
+};
+
+/**
+ * struct samsung_pin_ctrl_variant: represents a pin controller variant.
+ * @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
+ *	interrupts for the controller.
+ */
+struct samsung_pin_ctrl_variant {
+	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
+	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
 };
 
 /**
@@ -168,7 +180,7 @@ struct samsung_pin_ctrl {
 
 	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
 	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
-	char		*label;
+	const char	*label;
 };
 
 /**
@@ -235,5 +247,6 @@ struct samsung_pmx_func {
 
 /* list of all exported SoC specific data */
 extern struct samsung_pin_ctrl exynos4210_pin_ctrl[];
+extern struct samsung_pin_ctrl_variant exynos4_pin_ctrl;
 
 #endif /* __PINCTRL_SAMSUNG_H */
-- 
1.7.12

[RFC 3/6] pinctrl: exynos: Remove static platform-specific data

[Tomasz Figa]

The patch "pinctrl: samsung: Parse pin banks from DT" introduced parsing
platform-specific data from device tree, so there is no need to keep the
previously used definitions in headers and source files.

This patch cleans up the pinctrl-exynos driver from unused
platform-specific data.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c  |  96 -----------------------
 drivers/pinctrl/pinctrl-exynos.h  | 157 --------------------------------------
 drivers/pinctrl/pinctrl-samsung.h |   1 -
 3 files changed, 254 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 827b744..3cbc632 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -504,102 +504,6 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	return 0;
 }
 
-/* pin banks of exynos4210 pin-controller 0 */
-static struct samsung_pin_bank exynos4210_pin_banks0[] = {
-	EXYNOS_PIN_BANK_EINTG(0x000, EXYNOS4210_GPIO_A0, "gpa0"),
-	EXYNOS_PIN_BANK_EINTG(0x020, EXYNOS4210_GPIO_A1, "gpa1"),
-	EXYNOS_PIN_BANK_EINTG(0x040, EXYNOS4210_GPIO_B, "gpb"),
-	EXYNOS_PIN_BANK_EINTG(0x060, EXYNOS4210_GPIO_C0, "gpc0"),
-	EXYNOS_PIN_BANK_EINTG(0x080, EXYNOS4210_GPIO_C1, "gpc1"),
-	EXYNOS_PIN_BANK_EINTG(0x0A0, EXYNOS4210_GPIO_D0, "gpd0"),
-	EXYNOS_PIN_BANK_EINTG(0x0C0, EXYNOS4210_GPIO_D1, "gpd1"),
-	EXYNOS_PIN_BANK_EINTG(0x0E0, EXYNOS4210_GPIO_E0, "gpe0"),
-	EXYNOS_PIN_BANK_EINTG(0x100, EXYNOS4210_GPIO_E1, "gpe1"),
-	EXYNOS_PIN_BANK_EINTG(0x120, EXYNOS4210_GPIO_E2, "gpe2"),
-	EXYNOS_PIN_BANK_EINTG(0x140, EXYNOS4210_GPIO_E3, "gpe3"),
-	EXYNOS_PIN_BANK_EINTG(0x160, EXYNOS4210_GPIO_E4, "gpe4"),
-	EXYNOS_PIN_BANK_EINTG(0x180, EXYNOS4210_GPIO_F0, "gpf0"),
-	EXYNOS_PIN_BANK_EINTG(0x1A0, EXYNOS4210_GPIO_F1, "gpf1"),
-	EXYNOS_PIN_BANK_EINTG(0x1C0, EXYNOS4210_GPIO_F2, "gpf2"),
-	EXYNOS_PIN_BANK_EINTG(0x1E0, EXYNOS4210_GPIO_F3, "gpf3"),
-};
-
-/* pin banks of exynos4210 pin-controller 1 */
-static struct samsung_pin_bank exynos4210_pin_banks1[] = {
-	EXYNOS_PIN_BANK_EINTG(0x000, EXYNOS4210_GPIO_J0, "gpj0"),
-	EXYNOS_PIN_BANK_EINTG(0x020, EXYNOS4210_GPIO_J1, "gpj1"),
-	EXYNOS_PIN_BANK_EINTG(0x040, EXYNOS4210_GPIO_K0, "gpk0"),
-	EXYNOS_PIN_BANK_EINTG(0x060, EXYNOS4210_GPIO_K1, "gpk1"),
-	EXYNOS_PIN_BANK_EINTG(0x080, EXYNOS4210_GPIO_K2, "gpk2"),
-	EXYNOS_PIN_BANK_EINTG(0x0A0, EXYNOS4210_GPIO_K3, "gpk3"),
-	EXYNOS_PIN_BANK_EINTG(0x0C0, EXYNOS4210_GPIO_L0, "gpl0"),
-	EXYNOS_PIN_BANK_EINTG(0x0E0, EXYNOS4210_GPIO_L1, "gpl1"),
-	EXYNOS_PIN_BANK_EINTG(0x100, EXYNOS4210_GPIO_L2, "gpl2"),
-	EXYNOS_PIN_BANK_EINTN(0x120, EXYNOS4210_GPIO_Y0, "gpy0"),
-	EXYNOS_PIN_BANK_EINTN(0x140, EXYNOS4210_GPIO_Y1, "gpy1"),
-	EXYNOS_PIN_BANK_EINTN(0x160, EXYNOS4210_GPIO_Y2, "gpy2"),
-	EXYNOS_PIN_BANK_EINTN(0x180, EXYNOS4210_GPIO_Y3, "gpy3"),
-	EXYNOS_PIN_BANK_EINTN(0x1A0, EXYNOS4210_GPIO_Y4, "gpy4"),
-	EXYNOS_PIN_BANK_EINTN(0x1C0, EXYNOS4210_GPIO_Y5, "gpy5"),
-	EXYNOS_PIN_BANK_EINTN(0x1E0, EXYNOS4210_GPIO_Y6, "gpy6"),
-	EXYNOS_PIN_BANK_EINTN(0xC00, EXYNOS4210_GPIO_X0, "gpx0"),
-	EXYNOS_PIN_BANK_EINTN(0xC20, EXYNOS4210_GPIO_X1, "gpx1"),
-	EXYNOS_PIN_BANK_EINTN(0xC40, EXYNOS4210_GPIO_X2, "gpx2"),
-	EXYNOS_PIN_BANK_EINTN(0xC60, EXYNOS4210_GPIO_X3, "gpx3"),
-};
-
-/* pin banks of exynos4210 pin-controller 2 */
-static struct samsung_pin_bank exynos4210_pin_banks2[] = {
-	EXYNOS_PIN_BANK_EINTN(0x000, EXYNOS4210_GPIO_Z, "gpz"),
-};
-
-/*
- * Samsung pinctrl driver data for Exynos4210 SoC. Exynos4210 SoC includes
- * three gpio/pin-mux/pinconfig controllers.
- */
-struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
-	{
-		/* pin-controller instance 0 data */
-		.pin_banks	= exynos4210_pin_banks0,
-		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks0),
-		.base		= EXYNOS4210_GPIO_A0_START,
-		.nr_pins	= EXYNOS4210_GPIOA_NR_PINS,
-		.nr_gint	= EXYNOS4210_GPIOA_NR_GINT,
-		.geint_con	= EXYNOS_GPIO_ECON_OFFSET,
-		.geint_mask	= EXYNOS_GPIO_EMASK_OFFSET,
-		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
-		.svc		= EXYNOS_SVC_OFFSET,
-		.eint_gpio_init = exynos_eint_gpio_init,
-		.label		= "exynos4210-gpio-ctrl0",
-	}, {
-		/* pin-controller instance 1 data */
-		.pin_banks	= exynos4210_pin_banks1,
-		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks1),
-		.base		= EXYNOS4210_GPIOA_NR_PINS,
-		.nr_pins	= EXYNOS4210_GPIOB_NR_PINS,
-		.nr_gint	= EXYNOS4210_GPIOB_NR_GINT,
-		.nr_wint	= 32,
-		.geint_con	= EXYNOS_GPIO_ECON_OFFSET,
-		.geint_mask	= EXYNOS_GPIO_EMASK_OFFSET,
-		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
-		.weint_con	= EXYNOS_WKUP_ECON_OFFSET,
-		.weint_mask	= EXYNOS_WKUP_EMASK_OFFSET,
-		.weint_pend	= EXYNOS_WKUP_EPEND_OFFSET,
-		.svc		= EXYNOS_SVC_OFFSET,
-		.eint_gpio_init = exynos_eint_gpio_init,
-		.eint_wkup_init = exynos_eint_wkup_init,
-		.label		= "exynos4210-gpio-ctrl1",
-	}, {
-		/* pin-controller instance 2 data */
-		.pin_banks	= exynos4210_pin_banks2,
-		.nr_banks	= ARRAY_SIZE(exynos4210_pin_banks2),
-		.base		= EXYNOS4210_GPIOA_NR_PINS +
-					EXYNOS4210_GPIOB_NR_PINS,
-		.nr_pins	= EXYNOS4210_GPIOC_NR_PINS,
-		.label		= "exynos4210-gpio-ctrl2",
-	},
-};
-
 struct samsung_pin_ctrl_variant exynos4_pin_ctrl = {
 	.eint_gpio_init = exynos_eint_gpio_init,
 	.eint_wkup_init = exynos_eint_wkup_init,
diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h
index 31d0a06..a72cfc7 100644
--- a/drivers/pinctrl/pinctrl-exynos.h
+++ b/drivers/pinctrl/pinctrl-exynos.h
@@ -17,133 +17,6 @@
  * (at your option) any later version.
  */
 
-#define EXYNOS_GPIO_START(__gpio)	((__gpio##_START) + (__gpio##_NR))
-
-#define EXYNOS4210_GPIO_A0_NR	(8)
-#define EXYNOS4210_GPIO_A1_NR	(6)
-#define EXYNOS4210_GPIO_B_NR	(8)
-#define EXYNOS4210_GPIO_C0_NR	(5)
-#define EXYNOS4210_GPIO_C1_NR	(5)
-#define EXYNOS4210_GPIO_D0_NR	(4)
-#define EXYNOS4210_GPIO_D1_NR	(4)
-#define EXYNOS4210_GPIO_E0_NR	(5)
-#define EXYNOS4210_GPIO_E1_NR	(8)
-#define EXYNOS4210_GPIO_E2_NR	(6)
-#define EXYNOS4210_GPIO_E3_NR	(8)
-#define EXYNOS4210_GPIO_E4_NR	(8)
-#define EXYNOS4210_GPIO_F0_NR	(8)
-#define EXYNOS4210_GPIO_F1_NR	(8)
-#define EXYNOS4210_GPIO_F2_NR	(8)
-#define EXYNOS4210_GPIO_F3_NR	(6)
-#define EXYNOS4210_GPIO_J0_NR	(8)
-#define EXYNOS4210_GPIO_J1_NR	(5)
-#define EXYNOS4210_GPIO_K0_NR	(7)
-#define EXYNOS4210_GPIO_K1_NR	(7)
-#define EXYNOS4210_GPIO_K2_NR	(7)
-#define EXYNOS4210_GPIO_K3_NR	(7)
-#define EXYNOS4210_GPIO_L0_NR	(8)
-#define EXYNOS4210_GPIO_L1_NR	(3)
-#define EXYNOS4210_GPIO_L2_NR	(8)
-#define EXYNOS4210_GPIO_Y0_NR	(6)
-#define EXYNOS4210_GPIO_Y1_NR	(4)
-#define EXYNOS4210_GPIO_Y2_NR	(6)
-#define EXYNOS4210_GPIO_Y3_NR	(8)
-#define EXYNOS4210_GPIO_Y4_NR	(8)
-#define EXYNOS4210_GPIO_Y5_NR	(8)
-#define EXYNOS4210_GPIO_Y6_NR	(8)
-#define EXYNOS4210_GPIO_X0_NR	(8)
-#define EXYNOS4210_GPIO_X1_NR	(8)
-#define EXYNOS4210_GPIO_X2_NR	(8)
-#define EXYNOS4210_GPIO_X3_NR	(8)
-#define EXYNOS4210_GPIO_Z_NR	(7)
-
-enum exynos4210_gpio_xa_start {
-	EXYNOS4210_GPIO_A0_START	= 0,
-	EXYNOS4210_GPIO_A1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_A0),
-	EXYNOS4210_GPIO_B_START		= EXYNOS_GPIO_START(EXYNOS4210_GPIO_A1),
-	EXYNOS4210_GPIO_C0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_B),
-	EXYNOS4210_GPIO_C1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_C0),
-	EXYNOS4210_GPIO_D0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_C1),
-	EXYNOS4210_GPIO_D1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_D0),
-	EXYNOS4210_GPIO_E0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_D1),
-	EXYNOS4210_GPIO_E1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_E0),
-	EXYNOS4210_GPIO_E2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_E1),
-	EXYNOS4210_GPIO_E3_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_E2),
-	EXYNOS4210_GPIO_E4_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_E3),
-	EXYNOS4210_GPIO_F0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_E4),
-	EXYNOS4210_GPIO_F1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_F0),
-	EXYNOS4210_GPIO_F2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_F1),
-	EXYNOS4210_GPIO_F3_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_F2),
-};
-
-enum exynos4210_gpio_xb_start {
-	EXYNOS4210_GPIO_J0_START	= 0,
-	EXYNOS4210_GPIO_J1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_J0),
-	EXYNOS4210_GPIO_K0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_J1),
-	EXYNOS4210_GPIO_K1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_K0),
-	EXYNOS4210_GPIO_K2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_K1),
-	EXYNOS4210_GPIO_K3_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_K2),
-	EXYNOS4210_GPIO_L0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_K3),
-	EXYNOS4210_GPIO_L1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_L0),
-	EXYNOS4210_GPIO_L2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_L1),
-	EXYNOS4210_GPIO_Y0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_L2),
-	EXYNOS4210_GPIO_Y1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y0),
-	EXYNOS4210_GPIO_Y2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y1),
-	EXYNOS4210_GPIO_Y3_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y2),
-	EXYNOS4210_GPIO_Y4_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y3),
-	EXYNOS4210_GPIO_Y5_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y4),
-	EXYNOS4210_GPIO_Y6_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y5),
-	EXYNOS4210_GPIO_X0_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_Y6),
-	EXYNOS4210_GPIO_X1_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_X0),
-	EXYNOS4210_GPIO_X2_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_X1),
-	EXYNOS4210_GPIO_X3_START	= EXYNOS_GPIO_START(EXYNOS4210_GPIO_X2),
-};
-
-enum exynos4210_gpio_xc_start {
-	EXYNOS4210_GPIO_Z_START		= 0,
-};
-
-#define	EXYNOS4210_GPIO_A0_IRQ		EXYNOS4210_GPIO_A0_START
-#define	EXYNOS4210_GPIO_A1_IRQ		EXYNOS4210_GPIO_A1_START
-#define	EXYNOS4210_GPIO_B_IRQ		EXYNOS4210_GPIO_B_START
-#define	EXYNOS4210_GPIO_C0_IRQ		EXYNOS4210_GPIO_C0_START
-#define	EXYNOS4210_GPIO_C1_IRQ		EXYNOS4210_GPIO_C1_START
-#define	EXYNOS4210_GPIO_D0_IRQ		EXYNOS4210_GPIO_D0_START
-#define	EXYNOS4210_GPIO_D1_IRQ		EXYNOS4210_GPIO_D1_START
-#define	EXYNOS4210_GPIO_E0_IRQ		EXYNOS4210_GPIO_E0_START
-#define	EXYNOS4210_GPIO_E1_IRQ		EXYNOS4210_GPIO_E1_START
-#define	EXYNOS4210_GPIO_E2_IRQ		EXYNOS4210_GPIO_E2_START
-#define	EXYNOS4210_GPIO_E3_IRQ		EXYNOS4210_GPIO_E3_START
-#define	EXYNOS4210_GPIO_E4_IRQ		EXYNOS4210_GPIO_E4_START
-#define	EXYNOS4210_GPIO_F0_IRQ		EXYNOS4210_GPIO_F0_START
-#define	EXYNOS4210_GPIO_F1_IRQ		EXYNOS4210_GPIO_F1_START
-#define	EXYNOS4210_GPIO_F2_IRQ		EXYNOS4210_GPIO_F2_START
-#define	EXYNOS4210_GPIO_F3_IRQ		EXYNOS4210_GPIO_F3_START
-#define	EXYNOS4210_GPIO_J0_IRQ		EXYNOS4210_GPIO_J0_START
-#define	EXYNOS4210_GPIO_J1_IRQ		EXYNOS4210_GPIO_J1_START
-#define	EXYNOS4210_GPIO_K0_IRQ		EXYNOS4210_GPIO_K0_START
-#define	EXYNOS4210_GPIO_K1_IRQ		EXYNOS4210_GPIO_K1_START
-#define	EXYNOS4210_GPIO_K2_IRQ		EXYNOS4210_GPIO_K2_START
-#define	EXYNOS4210_GPIO_K3_IRQ		EXYNOS4210_GPIO_K3_START
-#define	EXYNOS4210_GPIO_L0_IRQ		EXYNOS4210_GPIO_L0_START
-#define	EXYNOS4210_GPIO_L1_IRQ		EXYNOS4210_GPIO_L1_START
-#define	EXYNOS4210_GPIO_L2_IRQ		EXYNOS4210_GPIO_L2_START
-#define	EXYNOS4210_GPIO_Z_IRQ		EXYNOS4210_GPIO_Z_START
-
-#define EXYNOS4210_GPIOA_NR_PINS	EXYNOS_GPIO_START(EXYNOS4210_GPIO_F3)
-#define EXYNOS4210_GPIOA_NR_GINT	EXYNOS_GPIO_START(EXYNOS4210_GPIO_F3)
-#define EXYNOS4210_GPIOB_NR_PINS	EXYNOS_GPIO_START(EXYNOS4210_GPIO_X3)
-#define EXYNOS4210_GPIOB_NR_GINT	EXYNOS_GPIO_START(EXYNOS4210_GPIO_L2)
-#define EXYNOS4210_GPIOC_NR_PINS	EXYNOS_GPIO_START(EXYNOS4210_GPIO_Z)
-
-/* External GPIO and wakeup interrupt related definitions */
-#define EXYNOS_GPIO_ECON_OFFSET		0x700
-#define EXYNOS_GPIO_EMASK_OFFSET	0x900
-#define EXYNOS_GPIO_EPEND_OFFSET	0xA00
-#define EXYNOS_WKUP_ECON_OFFSET		0xE00
-#define EXYNOS_WKUP_EMASK_OFFSET	0xF00
-#define EXYNOS_WKUP_EPEND_OFFSET	0xF40
-#define EXYNOS_SVC_OFFSET		0xB08
 #define EXYNOS_EINT_FUNC		0xF
 
 /* helpers to access interrupt service register */
@@ -163,36 +36,6 @@ enum exynos4210_gpio_xc_start {
 #define EXYNOS_EINT_CON_LEN		4
 
 #define EXYNOS_EINT_MAX_PER_BANK	8
-#define EXYNOS_EINT_NR_WKUP_EINT
-
-#define EXYNOS_PIN_BANK_EINTN(reg, __gpio, id)		\
-	{						\
-		.pctl_offset	= reg,			\
-		.pin_base	= (__gpio##_START),	\
-		.nr_pins	= (__gpio##_NR),	\
-		.func_width	= 4,			\
-		.pud_width	= 2,			\
-		.drv_width	= 2,			\
-		.conpdn_width	= 2,			\
-		.pudpdn_width	= 2,			\
-		.eint_type	= EINT_TYPE_NONE,	\
-		.name		= id			\
-	}
-
-#define EXYNOS_PIN_BANK_EINTG(reg, __gpio, id)		\
-	{						\
-		.pctl_offset	= reg,			\
-		.pin_base	= (__gpio##_START),	\
-		.nr_pins	= (__gpio##_NR),	\
-		.func_width	= 4,			\
-		.pud_width	= 2,			\
-		.drv_width	= 2,			\
-		.conpdn_width	= 2,			\
-		.pudpdn_width	= 2,			\
-		.eint_type	= EINT_TYPE_GPIO,	\
-		.irq_base	= (__gpio##_IRQ),	\
-		.name		= id			\
-	}
 
 /**
  * struct exynos_geint_data: gpio eint specific data for irq_chip callbacks.
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 5d59ce6..db1907c 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -246,7 +246,6 @@ struct samsung_pmx_func {
 };
 
 /* list of all exported SoC specific data */
-extern struct samsung_pin_ctrl exynos4210_pin_ctrl[];
 extern struct samsung_pin_ctrl_variant exynos4_pin_ctrl;
 
 #endif /* __PINCTRL_SAMSUNG_H */
-- 
1.7.12

[RFC 4/6] pinctrl: samsung: Parse bank-specific eint offset from DT

[Tomasz Figa]

Some SoCs, like Exynos4x12, have non-linear layout of EINT control
registers and so current way of calculating register addresses does not
work correctly for them.

This patch adds parsing of samsung,eint-offset property from bank nodes
and uses the read values instead of calculating the offsets from bank
index.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c  | 5 ++---
 drivers/pinctrl/pinctrl-samsung.c | 5 +++++
 drivers/pinctrl/pinctrl-samsung.h | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 3cbc632..836ac36 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -146,7 +146,7 @@ static struct exynos_geint_data *exynos_get_eint_data(irq_hw_number_t hw,
 	struct samsung_pin_bank *bank = d->ctrl->pin_banks;
 	struct exynos_geint_data *eint_data;
 	unsigned int nr_banks = d->ctrl->nr_banks, idx;
-	unsigned int irq_base = 0, eint_offset = 0;
+	unsigned int irq_base = 0;
 
 	if (hw >= d->ctrl->nr_gint) {
 		dev_err(d->dev, "unsupported ext-gpio interrupt\n");
@@ -159,7 +159,6 @@ static struct exynos_geint_data *exynos_get_eint_data(irq_hw_number_t hw,
 		if ((hw >= irq_base) && (hw < (irq_base + bank->nr_pins)))
 			break;
 		irq_base += bank->nr_pins;
-		eint_offset += 4;
 	}
 
 	if (idx == nr_banks) {
@@ -175,7 +174,7 @@ static struct exynos_geint_data *exynos_get_eint_data(irq_hw_number_t hw,
 
 	eint_data->bank	= bank;
 	eint_data->pin = hw - irq_base;
-	eint_data->eint_offset = eint_offset;
+	eint_data->eint_offset = bank->eint_offset;
 	return eint_data;
 }
 
diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index ff1d001..03bf743 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -832,6 +832,11 @@ static int samsung_pinctrl_parse_dt_bank(struct samsung_pin_bank *bank,
 
 	bank->eint_type = EINT_TYPE_GPIO;
 
+	ret = of_property_read_u32(np, "samsung,eint-offset", &val);
+	if (ret)
+		return ret;
+	bank->eint_offset = val;
+
 	return 0;
 }
 
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index db1907c..72303f1 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -122,6 +122,7 @@ struct samsung_pin_bank {
 	u8		conpdn_width;
 	u8		pudpdn_width;
 	enum eint_type	eint_type;
+	u32		eint_offset;
 	u32		irq_base;
 	const char	*name;
 };
-- 
1.7.12

[RFC 5/6] ARM: dts: exynos4210: Remove legacy gpio nodes

[Tomasz Figa]

This patch removes the legacy gpio nodes as the gpio driver is going to
be replaced with the new pinctrl driver.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 229 --------------------------------------
 1 file changed, 229 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index a4bd0c9..ecbc707 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -250,233 +250,4 @@
 			interrupts = <0 36 0>;
 		};
 	};
-
-	gpio-controllers {
-		#address-cells = <1>;
-		#size-cells = <1>;
-		gpio-controller;
-		ranges;
-
-		gpa0: gpio-controller@11400000 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400000 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpa1: gpio-controller@11400020 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400020 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpb: gpio-controller@11400040 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400040 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpc0: gpio-controller@11400060 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400060 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpc1: gpio-controller@11400080 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400080 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpd0: gpio-controller@114000A0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114000A0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpd1: gpio-controller@114000C0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114000C0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpe0: gpio-controller@114000E0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114000E0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpe1: gpio-controller@11400100 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400100 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpe2: gpio-controller@11400120 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400120 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpe3: gpio-controller@11400140 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400140 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpe4: gpio-controller@11400160 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400160 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpf0: gpio-controller@11400180 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11400180 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpf1: gpio-controller@114001A0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114001A0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpf2: gpio-controller@114001C0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114001C0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpf3: gpio-controller@114001E0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x114001E0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpj0: gpio-controller@11000000 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000000 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpj1: gpio-controller@11000020 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000020 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpk0: gpio-controller@11000040 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000040 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpk1: gpio-controller@11000060 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000060 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpk2: gpio-controller@11000080 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000080 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpk3: gpio-controller@110000A0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110000A0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpl0: gpio-controller@110000C0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110000C0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpl1: gpio-controller@110000E0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110000E0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpl2: gpio-controller@11000100 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000100 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy0: gpio-controller@11000120 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000120 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy1: gpio-controller@11000140 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000140 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy2: gpio-controller@11000160 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000160 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy3: gpio-controller@11000180 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000180 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy4: gpio-controller@110001A0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110001A0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy5: gpio-controller@110001C0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110001C0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpy6: gpio-controller@110001E0 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x110001E0 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpx0: gpio-controller@11000C00 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000C00 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpx1: gpio-controller@11000C20 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000C20 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpx2: gpio-controller@11000C40 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000C40 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpx3: gpio-controller@11000C60 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x11000C60 0x20>;
-			#gpio-cells = <4>;
-		};
-
-		gpz: gpio-controller@03860000 {
-			compatible = "samsung,exynos4-gpio";
-			reg = <0x03860000 0x20>;
-			#gpio-cells = <4>;
-		};
-	};
 };
-- 
1.7.12

[RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

The patch "pinctrl: samsung: Parse pin banks from DT" introduced
platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi | 605 ++++++++++++++++++++++++
 arch/arm/boot/dts/exynos4210-pinctrl.dtsi       |   2 +
 arch/arm/boot/dts/exynos4210.dtsi               |  12 +
 3 files changed, 619 insertions(+)
 create mode 100644 arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

diff --git a/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
new file mode 100644
index 0000000..cac7f71
--- /dev/null
+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
@@ -0,0 +1,605 @@
+/*
+ * Samsung's Exynos4210 SoC pinctrl banks device tree source
+ *
+ * Copyright (c) 2012 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Samsung's Exynos4210 SoC pin banks are listed as device tree nodes
+ * in this file.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/ {
+	pinctrl@11400000 {
+		gpa0: pin-bank@0 {
+			gpio-controller;
+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x00>;
+		};
+
+		gpa1: pin-bank@1 {
+			gpio-controller;
+			samsung,pctl-offset = <0x020>;
+			samsung,pin-bank = "gpa1";
+			samsung,pin-count = <6>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x04>;
+		};
+
+		gpb: pin-bank@2 {
+			gpio-controller;
+			samsung,pctl-offset = <0x040>;
+			samsung,pin-bank = "gpb";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x08>;
+		};
+
+		gpc0: pin-bank@3 {
+			gpio-controller;
+			samsung,pctl-offset = <0x060>;
+			samsung,pin-bank = "gpc0";
+			samsung,pin-count = <5>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x0C>;
+		};
+
+		gpc1: pin-bank@4 {
+			gpio-controller;
+			samsung,pctl-offset = <0x080>;
+			samsung,pin-bank = "gpc1";
+			samsung,pin-count = <5>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x10>;
+		};
+
+		gpd0: pin-bank@5 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0A0>;
+			samsung,pin-bank = "gpd0";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x14>;
+		};
+
+		gpd1: pin-bank@6 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0C0>;
+			samsung,pin-bank = "gpd1";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x18>;
+		};
+
+		gpe0: pin-bank@7 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0E0>;
+			samsung,pin-bank = "gpe0";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x1C>;
+		};
+
+		gpe1: pin-bank@8 {
+			gpio-controller;
+			samsung,pctl-offset = <0x100>;
+			samsung,pin-bank = "gpe1";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x20>;
+		};
+
+		gpe2: pin-bank@9 {
+			gpio-controller;
+			samsung,pctl-offset = <0x120>;
+			samsung,pin-bank = "gpe2";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x24>;
+		};
+
+		gpe3: pin-bank@10 {
+			gpio-controller;
+			samsung,pctl-offset = <0x140>;
+			samsung,pin-bank = "gpe3";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x28>;
+		};
+
+		gpe4: pin-bank@11 {
+			gpio-controller;
+			samsung,pctl-offset = <0x160>;
+			samsung,pin-bank = "gpe4";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x2C>;
+		};
+
+		gpf0: pin-bank@12 {
+			gpio-controller;
+			samsung,pctl-offset = <0x180>;
+			samsung,pin-bank = "gpf0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x30>;
+		};
+
+		gpf1: pin-bank@13 {
+			gpio-controller;
+			samsung,pctl-offset = <0x1A0>;
+			samsung,pin-bank = "gpf1";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x34>;
+		};
+
+		gpf2: pin-bank@14 {
+			gpio-controller;
+			samsung,pctl-offset = <0x1C0>;
+			samsung,pin-bank = "gpf2";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x38>;
+		};
+
+		gpf3: pin-bank@15 {
+			gpio-controller;
+			samsung,pctl-offset = <0x1E0>;
+			samsung,pin-bank = "gpf3";
+			samsung,pin-count = <6>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x3C>;
+		};
+	};
+
+	pinctrl@11000000 {
+		gpj0: pin-bank@0 {
+			gpio-controller;
+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpj0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x00>;
+		};
+
+		gpj1: pin-bank@1 {
+			gpio-controller;
+			samsung,pctl-offset = <0x020>;
+			samsung,pin-bank = "gpj1";
+			samsung,pin-count = <5>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x04>;
+		};
+
+		gpk0: pin-bank@2 {
+			gpio-controller;
+			samsung,pctl-offset = <0x040>;
+			samsung,pin-bank = "gpk0";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x08>;
+		};
+
+		gpk1: pin-bank@3 {
+			gpio-controller;
+			samsung,pctl-offset = <0x060>;
+			samsung,pin-bank = "gpk1";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x0C>;
+		};
+
+		gpk2: pin-bank@4 {
+			gpio-controller;
+			samsung,pctl-offset = <0x080>;
+			samsung,pin-bank = "gpk2";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x10>;
+		};
+
+		gpk3: pin-bank@5 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0A0>;
+			samsung,pin-bank = "gpk3";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x14>;
+		};
+
+		gpl0: pin-bank@6 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0C0>;
+			samsung,pin-bank = "gpl0";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x18>;
+		};
+
+		gpl1: pin-bank@7 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0E0>;
+			samsung,pin-bank = "gpl1";
+			samsung,pin-count = <2>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x1C>;
+		};
+
+		gpl2: pin-bank@8 {
+			gpio-controller;
+			samsung,pctl-offset = <0x100>;
+			samsung,pin-bank = "gpl2";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x20>;
+		};
+
+		gpm0: pin-bank@9 {
+			gpio-controller;
+			samsung,pctl-offset = <0x260>;
+			samsung,pin-bank = "gpm0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x24>;
+		};
+
+		gpm1: pin-bank@10 {
+			gpio-controller;
+			samsung,pctl-offset = <0x280>;
+			samsung,pin-bank = "gpm1";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x28>;
+		};
+
+		gpm2: pin-bank@11 {
+			gpio-controller;
+			samsung,pctl-offset = <0x2A0>;
+			samsung,pin-bank = "gpm2";
+			samsung,pin-count = <5>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x2C>;
+		};
+
+		gpm3: pin-bank@12 {
+			gpio-controller;
+			samsung,pctl-offset = <0x2C0>;
+			samsung,pin-bank = "gpm3";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x30>;
+		};
+
+		gpm4: pin-bank@13 {
+			gpio-controller;
+			samsung,pctl-offset = <0x2E0>;
+			samsung,pin-bank = "gpm4";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+
+			interrupt-controller;
+			samsung,eint-offset = <0x34>;
+		};
+
+		gpy0: pin-bank@14 {
+			gpio-controller;
+			samsung,pctl-offset = <0x120>;
+			samsung,pin-bank = "gpy0";
+			samsung,pin-count = <6>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy1: pin-bank@15 {
+			gpio-controller;
+			samsung,pctl-offset = <0x140>;
+			samsung,pin-bank = "gpy1";
+			samsung,pin-count = <4>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy2: pin-bank@16 {
+			gpio-controller;
+			samsung,pctl-offset = <0x160>;
+			samsung,pin-bank = "gpy2";
+			samsung,pin-count = <6>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy3: pin-bank@17 {
+			gpio-controller;
+			samsung,pctl-offset = <0x180>;
+			samsung,pin-bank = "gpy3";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy4: pin-bank@18 {
+			gpio-controller;
+			samsung,pctl-offset = <0x1A0>;
+			samsung,pin-bank = "gpy4";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy5: pin-bank@19{
+			gpio-controller;
+			samsung,pctl-offset = <0x1C0>;
+			samsung,pin-bank = "gpy5";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpy6: pin-bank@20 {
+			gpio-controller;
+			samsung,pctl-offset = <0x1E0>;
+			samsung,pin-bank = "gpy6";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;
+		};
+
+		gpx0: pin-bank@21 {
+			gpio-controller;
+			samsung,pctl-offset = <0xC00>;
+			samsung,pin-bank = "gpx0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+		};
+
+		gpx1: pin-bank@22 {
+			gpio-controller;
+			samsung,pctl-offset = <0xC20>;
+			samsung,pin-bank = "gpx1";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+		};
+
+		gpx2: pin-bank@23 {
+			gpio-controller;
+			samsung,pctl-offset = <0xC40>;
+			samsung,pin-bank = "gpx2";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+		};
+
+		gpx3: pin-bank@24 {
+			gpio-controller;
+			samsung,pctl-offset = <0xC60>;
+			samsung,pin-bank = "gpx3";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+		};
+	};
+
+	pinctrl@03860000 {
+		gpz: pin-bank@0 {
+			gpio-controller;
+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpz";
+			samsung,pin-count = <7>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi
index b12cf27..94846d5 100644
--- a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi
@@ -14,6 +14,8 @@
  * published by the Free Software Foundation.
 */
 
+/include/ "exynos4210-pinctrl-banks.dtsi"
+
 / {
 	pinctrl@11400000 {
 		uart0_data: uart0-data {
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index ecbc707..0e93717 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -59,6 +59,10 @@
 		reg = <0x11400000 0x1000>;
 		interrupts = <0 47 0>;
 		interrupt-controller;
+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;
 		#interrupt-cells = <2>;
 	};
 
@@ -67,6 +71,10 @@
 		reg = <0x11000000 0x1000>;
 		interrupts = <0 46 0>;
 		interrupt-controller;
+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;
 		#interrupt-cells = <2>;
 
 		wakup_eint: wakeup-interrupt-controller {
@@ -79,6 +87,10 @@
 					<0 24 0>, <0 25 0>, <0 26 0>, <0 27 0>,
 					<0 28 0>, <0 29 0>, <0 30 0>, <0 31 0>,
 					<0 32 0>;
+			samsung,weint-count = <32>;
+			samsung,weint-con = <0xE00>;
+			samsung,weint-mask = <0xF00>;
+			samsung,weint-pend = <0xF40>;
 		};
 	};
 
-- 
1.7.12

Re: [RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[Linus Walleij]

On Thu, Sep 20, 2012 at 10:53 AM, Tomasz Figa <t.figa@samsung.com> wrote:

> This RFC series is a work on replacing static platform-specific data in
> pinctrl-samsung driver with data dynamically parsed from device tree.

Please include Stephen Warren on this series, he know his way
around pinctrl <-> devicetree better than anyone else.

(I'll look and see if I can see something that sticks out...)

Yours,
Linus Walleij

Re: [RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[Stephen Warren]

On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> This RFC series is a work on replacing static platform-specific data in
> pinctrl-samsung driver with data dynamically parsed from device tree.

Hmm. I tend to think this is exactly the opposite of the correct
direction; you end up wasting a whole ton of time during the boot
process parsing data out of the device tree only to end up with exactly
the same tables that you would have just put into the kernel anyway. Is
it really likely that future SoCs will change information such as the
width of the pullup/pulldown bitfield, but not change anything else
that's not already in this binding. If that isn't the case, the binding
won't be complete enough to describe any new features on future SoCs anyway.

> It aims at reducing the SoC-specific part of the driver and thus the
> amount of modifications to driver sources when adding support for next
> SoCs (like Exynos4x12).
> 
> Furthermore, moving definitions of pin banks to device tree will allow
> to simplify GPIO and GEINT specification to a format similar to used
> previously by gpiolib-based implementation, using a phandle to the bank
> and pin index inside the bank, e.g.
> 	gpios = <&gpa1 4 0>;
> 	interrupt-parent = <&gpa1>;
> 	interrupts = <4 0>;

I don't think those two are correlated; the GPIO specifier format could
just as easily be <bank pin> irrespective of whether the pinctrl driver
contains SoC-specific tables or not.

> Any comments are welcome.
> 
> TODO:
>  - bindings documentation

That's unfortunate; it would be the most interesting part to review. I
guess I'll try to work out the binding from the examples in patch 6.

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
> platform-specific data parsing from DT.
> 
> This patch adds all necessary nodes and properties to exynos4210 device
> tree sources.

> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

> +/ {
> +	pinctrl@11400000 {
> +		gpa0: pin-bank@0 {

If you're going to put a unit address ("@0")into the DT node name, the
node should have a reg property containing the same value, and the
parent node should have #address-cells and #size-cells properties.

However, I believe you can actually get unique node names without using
a unit address; instead name the nodes after the object they represent,
so e.g. s/pin-bank@0/gpa0/ above.

> +			gpio-controller;

You need a #gpio-cells property too.

> +			samsung,pctl-offset = <0x000>;
> +			samsung,pin-bank = "gpa0";
> +			samsung,pin-count = <8>;
> +			samsung,func-width = <4>;
> +			samsung,pud-width = <2>;
> +			samsung,drv-width = <2>;
> +			samsung,conpdn-width = <2>;
> +			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields
always be packed together into say the LSB of the registers? What if
there are gaps between the fields in a future SoC variant, or the order
of the fields in the register changes? I think you want to add either a
samsung,func-bit/samsung,func-position property for each of the fields,
or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that
point, you'd need to edit the driver anyway in order to define a new DT
property to represent the new field. Perhaps instead of having an
explicit named property per field in the register, you want a simple
list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

That would allow a completely arbitrary number of fields and layouts to
be described.

I wonder if for absolute generality you want a samsung,pin-stride
property to represent the difference in register address per pin. I
assume that's hard-coded as 4 right now.

> +			interrupt-controller;

You need a #interrupt-cells property too.

> +		gpd0: pin-bank@5 {
> +			gpio-controller;
> +			samsung,pctl-offset = <0x0A0>;

I think hex number are usually lower-case in DT, but I may be
extrapolating a generality from a limited set of examples.

> +		gpy5: pin-bank@19{

Missing a space before the { there.

> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index ecbc707..0e93717 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -59,6 +59,10 @@
>  		reg = <0x11400000 0x1000>;
>  		interrupts = <0 47 0>;
>  		interrupt-controller;
> +		samsung,geint-con = <0x700>;
> +		samsung,geint-mask = <0x900>;
> +		samsung,geint-pend = <0xA00>;
> +		samsung,svc = <0xB08>;

I assume those new properties represent register addresses within the
block. If not, I don't understand what they are.

It's unclear to me why those properties aren't all part of
exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where
the register addresses for the pinctrl registers are the same (hence can
be in a shared exynos4210-pinctrl-banks.dtsi), yet the register
addresses for the geint registers are different (hence must be in
non-shared exynos4210.dtsi)?

Do these properties interact with samsung,eint-offset at all? Oh,
perhaps these properties are defining a top-level interrupt controller
that aggregates all the banks together, whereas samsung,eint-offset is
per-bank?

Re: [RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[Tomasz Figa]

Hi Stephen,

Thanks for your comments.

On Friday 21 of September 2012 12:40:35 Stephen Warren wrote:
> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> > This RFC series is a work on replacing static platform-specific data in
> > pinctrl-samsung driver with data dynamically parsed from device tree.
> 
> Hmm. I tend to think this is exactly the opposite of the correct
> direction; you end up wasting a whole ton of time during the boot
> process parsing data out of the device tree only to end up with exactly
> the same tables that you would have just put into the kernel anyway.

Yes, I'm aware that parsing all those information from device tree won't be 
free. I have even considering simplifying the binding to something like

	samsung,pin-bank = <offset count func pud drv conpdn pudpdn>;

listing widths of fields in fixed order, with some fields allowed to be 
zero, meaning that given bank doesn't support this control. It would 
simplify the parsing to just iterating over a table under a single 
property. What do you think?

I have decided to post the original variant to get some comments earlier, 
as I already had all the rest of patches based on it.

> Is
> it really likely that future SoCs will change information such as the
> width of the pullup/pulldown bitfield, but not change anything else
> that's not already in this binding. If that isn't the case, the binding
> won't be complete enough to describe any new features on future SoCs
> anyway.

Looking at the history of Samsung SoCs and specifics of this subsystem, 
there isn't much likely to change other than the bindings already account 
for (and the binding represents whatever the driver accounts for).

> > It aims at reducing the SoC-specific part of the driver and thus the
> > amount of modifications to driver sources when adding support for next
> > SoCs (like Exynos4x12).
> > 
> > Furthermore, moving definitions of pin banks to device tree will allow
> > to simplify GPIO and GEINT specification to a format similar to used
> > previously by gpiolib-based implementation, using a phandle to the bank
> > and pin index inside the bank, e.g.
> > 
> > 	gpios = <&gpa1 4 0>;
> > 	interrupt-parent = <&gpa1>;
> > 	interrupts = <4 0>;
> 
> I don't think those two are correlated; the GPIO specifier format could
> just as easily be <bank pin> irrespective of whether the pinctrl driver
> contains SoC-specific tables or not.

Correct me if I'm wrong, but each bank needs to have its own subnode to be 
able to address pins like this. That was the starting point of the whole 
series and the idea that if all the banks (which are SoC-specific) have to 
be defined anyway, maybe it wouldn't be too bad to put all the SoC-specific 
parameters there too.

> > Any comments are welcome.
> > 
> > TODO:
> >  - bindings documentation
> 
> That's unfortunate; it would be the most interesting part to review. I
> guess I'll try to work out the binding from the examples in patch 6.

Sorry about that. I thought the examples would be sufficient. Still, I was 
focused at getting comments about the idea of moving such data to DT in 
general, not the bindings, which are most likely to change, in particular.

Best regards,
Tomasz Figa

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

Hi Stephen,

Thanks for your comments.

On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> > The patch "pinctrl: samsung: Parse pin banks from DT" introduced
> > platform-specific data parsing from DT.
> > 
> > This patch adds all necessary nodes and properties to exynos4210 device
> > tree sources.
> > 
> > +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
> > 
> > +/ {
> > +	pinctrl@11400000 {
> > +		gpa0: pin-bank@0 {
> 
> If you're going to put a unit address ("@0")into the DT node name, the
> node should have a reg property containing the same value, and the
> parent node should have #address-cells and #size-cells properties.
> 
> However, I believe you can actually get unique node names without using
> a unit address; instead name the nodes after the object they represent,
> so e.g. s/pin-bank@0/gpa0/ above.

Good point. I wasn't aware of the relation between unit address and reg 
property. I thought it was just for readability.

> 
> > +			gpio-controller;
> 
> You need a #gpio-cells property too.

It will be added in further patches that add per-bank GPIO addressing.

In this RFC, this property is just used to distinguish pin bank nodes from 
pin group nodes.
 
> > +			samsung,pctl-offset = <0x000>;
> > +			samsung,pin-bank = "gpa0";
> > +			samsung,pin-count = <8>;
> > +			samsung,func-width = <4>;
> > +			samsung,pud-width = <2>;
> > +			samsung,drv-width = <2>;
> > +			samsung,conpdn-width = <2>;
> > +			samsung,pudpdn-width = <2>;
> 
> The properties above represent the width of the fields. Must all fields
> always be packed together into say the LSB of the registers? What if
> there are gaps between the fields in a future SoC variant, or the order
> of the fields in the register changes? I think you want to add either a
> samsung,func-bit/samsung,func-position property for each of the fields,
> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
> the generic pinctrl binding used a mask for this purpose.
> 
> What if a future SoC variant adds more fields to the register? At that
> point, you'd need to edit the driver anyway in order to define a new DT
> property to represent the new field. Perhaps instead of having an
> explicit named property per field in the register, you want a simple
> list of fields:
> 
> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;
> 
> That would allow a completely arbitrary number of fields and layouts to
> be described.
> 
> I wonder if for absolute generality you want a samsung,pin-stride
> property to represent the difference in register address per pin. I
> assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more 
conservative solution would be better, like reducing the amount of 
information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

	compatible = "samsung,exynos4-pin-bank*;

and then define supported bank types in the driver. SoC-specific data would 
remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

> > +			interrupt-controller;
> You need a #interrupt-cells property too.

Just as with gpio-controller before, in this RFC this is just a mark to 
distinguish banks with interrupts from banks without interrupts.

Further patches will allow to use it properly (and will add #interrupt-
cells property too).

> > +		gpd0: pin-bank@5 {
> > +			gpio-controller;
> > +			samsung,pctl-offset = <0x0A0>;
> 
> I think hex number are usually lower-case in DT, but I may be
> extrapolating a generality from a limited set of examples.

I have seen both variants, with upper-case being more popular across 
Samsung's dts files and so I have chosen to use it. (Personally I prefer 
lower-case, though, if it does matter.)

> > +		gpy5: pin-bank@19{
> 
> Missing a space before the { there.

Right.

> > diff --git a/arch/arm/boot/dts/exynos4210.dtsi
> > b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644
> > --- a/arch/arm/boot/dts/exynos4210.dtsi
> > +++ b/arch/arm/boot/dts/exynos4210.dtsi
> > @@ -59,6 +59,10 @@
> > 
> >  		reg = <0x11400000 0x1000>;
> >  		interrupts = <0 47 0>;
> >  		interrupt-controller;
> > 
> > +		samsung,geint-con = <0x700>;
> > +		samsung,geint-mask = <0x900>;
> > +		samsung,geint-pend = <0xA00>;
> > +		samsung,svc = <0xB08>;
> 
> I assume those new properties represent register addresses within the
> block. If not, I don't understand what they are.

Yes, they do.

> It's unclear to me why those properties aren't all part of
> exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where
> the register addresses for the pinctrl registers are the same (hence can
> be in a shared exynos4210-pinctrl-banks.dtsi), yet the register
> addresses for the geint registers are different (hence must be in
> non-shared exynos4210.dtsi)?

Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. 
Other SoCs are going to have their own whatever-pinctrl-banks.dtsi.

> Do these properties interact with samsung,eint-offset at all? Oh,
> perhaps these properties are defining a top-level interrupt controller
> that aggregates all the banks together, whereas samsung,eint-offset is
> per-bank?

Yes. There is a bunch of CON registers one after another, for each bank, 
which supports interrupts and similarly for MASK and PEND. All those geint-
xxx properties define the location of first register and eint-offset 
defines location of register for particular bank.

Best regards,
Tomasz Figa

Re: [RFC 0/6] pinctrl: samsung: Remove static platform-specific data

[Stephen Warren]

On 09/21/2012 01:31 PM, Tomasz Figa wrote:
> On Friday 21 of September 2012 12:40:35 Stephen Warren wrote:
>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>> This RFC series is a work on replacing static platform-specific data in
>>> pinctrl-samsung driver with data dynamically parsed from device tree.
...
>>> It aims at reducing the SoC-specific part of the driver and thus the
>>> amount of modifications to driver sources when adding support for next
>>> SoCs (like Exynos4x12).
>>>
>>> Furthermore, moving definitions of pin banks to device tree will allow
>>> to simplify GPIO and GEINT specification to a format similar to used
>>> previously by gpiolib-based implementation, using a phandle to the bank
>>> and pin index inside the bank, e.g.
>>>
>>> 	gpios = <&gpa1 4 0>;
>>> 	interrupt-parent = <&gpa1>;
>>> 	interrupts = <4 0>;
>>
>> I don't think those two are correlated; the GPIO specifier format could
>> just as easily be <bank pin> irrespective of whether the pinctrl driver
>> contains SoC-specific tables or not.
> 
> Correct me if I'm wrong, but each bank needs to have its own subnode to be 
> able to address pins like this. That was the starting point of the whole 
> series and the idea that if all the banks (which are SoC-specific) have to 
> be defined anyway, maybe it wouldn't be too bad to put all the SoC-specific 
> parameters there too.

If you write your own custom .xlate function, I think you can do
basically anything you want; all of the following are possible, I believe:

a) Single DT node covering all banks, with GPIO specifier being <n>
where n is some linearized GPIO ID across all banks.

b) Single DT node covering all banks, with GPIO specifier being <b n>
where b is the bank number, and n is the GPIO ID within the bank (xlate
would presumably calculate "(b * MAX_GPIOS_PER_BANK) + n" and up with
the same data as in (a) above.

c) One DT node per bank, one gpio_chip registered per node, with the
GPIO specifier being <n> where n is the GPIO ID within the bank the
phandle points at.

d) One DT node per bank, all contained within a single parent DT node,
one gpio_chip registered per node (one for each bank node, and one for
the parent node), with GPIO phandles pointing at the parent node, with
GPIO specifier in the format of either (a) or (b) above, and the
top-level node's .xlate returning a different gpio_chip (for one of the
child bank-specific nodes) rather than the parent chip. At least, IIRC,
Grant Likely was going to extend a gpio_chip's .xlate to be able to
return a different chip; I'm not sure if that was implemented yet or not.

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
>>> platform-specific data parsing from DT.
>>>
>>> This patch adds all necessary nodes and properties to exynos4210 device
>>> tree sources.
>>>
>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

>>> +			samsung,pctl-offset = <0x000>;
>>> +			samsung,pin-bank = "gpa0";
>>> +			samsung,pin-count = <8>;
>>> +			samsung,func-width = <4>;
>>> +			samsung,pud-width = <2>;
>>> +			samsung,drv-width = <2>;
>>> +			samsung,conpdn-width = <2>;
>>> +			samsung,pudpdn-width = <2>;
>>
>> The properties above represent the width of the fields. Must all fields
>> always be packed together into say the LSB of the registers? What if
>> there are gaps between the fields in a future SoC variant, or the order
>> of the fields in the register changes? I think you want to add either a
>> samsung,func-bit/samsung,func-position property for each of the fields,
>> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
>> the generic pinctrl binding used a mask for this purpose.
>>
>> What if a future SoC variant adds more fields to the register? At that
>> point, you'd need to edit the driver anyway in order to define a new DT
>> property to represent the new field. Perhaps instead of having an
>> explicit named property per field in the register, you want a simple
>> list of fields:
>>
>> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
>> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;
>>
>> That would allow a completely arbitrary number of fields and layouts to
>> be described.
>>
>> I wonder if for absolute generality you want a samsung,pin-stride
>> property to represent the difference in register address per pin. I
>> assume that's hard-coded as 4 right now.
> 
> Hmm, considering so many different possible changes, maybe a more 
> conservative solution would be better, like reducing the amount of 
> information held in DT to bank type, e.g.
> 
> 	samsung,bank-type = "exynos4";
> 
> or maybe
> 
> 	compatible = "samsung,exynos4-pin-bank*;
> 
> and then define supported bank types in the driver. SoC-specific data would 
> remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Yes, removing much of the data from DT and putting it into a tiny table
in the driver makes sense to me in this case.

>>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi
>>> b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644
>>> --- a/arch/arm/boot/dts/exynos4210.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4210.dtsi
>>> @@ -59,6 +59,10 @@
>>>
>>>  		reg = <0x11400000 0x1000>;
>>>  		interrupts = <0 47 0>;
>>>  		interrupt-controller;
>>>
>>> +		samsung,geint-con = <0x700>;
>>> +		samsung,geint-mask = <0x900>;
>>> +		samsung,geint-pend = <0xA00>;
>>> +		samsung,svc = <0xB08>;
>>
>> I assume those new properties represent register addresses within the
>> block. If not, I don't understand what they are.
> 
> Yes, they do.
> 
>> It's unclear to me why those properties aren't all part of
>> exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where
>> the register addresses for the pinctrl registers are the same (hence can
>> be in a shared exynos4210-pinctrl-banks.dtsi), yet the register
>> addresses for the geint registers are different (hence must be in
>> non-shared exynos4210.dtsi)?
> 
> Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. 
> Other SoCs are going to have their own whatever-pinctrl-banks.dtsi.

OK, so my point here is: why not put all the pinctrl-related properties
into a single file, rather than spreading them across different files.
Having separate files makes sense if they can be re-used in different
places, but not if they're single-use.

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> > On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
> >> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> >>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
> >>> platform-specific data parsing from DT.
> >>> 
> >>> This patch adds all necessary nodes and properties to exynos4210
> >>> device
> >>> tree sources.
> >>> 
> >>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
> >>> 
> >>> +			samsung,pctl-offset = <0x000>;
> >>> +			samsung,pin-bank = "gpa0";
> >>> +			samsung,pin-count = <8>;
> >>> +			samsung,func-width = <4>;
> >>> +			samsung,pud-width = <2>;
> >>> +			samsung,drv-width = <2>;
> >>> +			samsung,conpdn-width = <2>;
> >>> +			samsung,pudpdn-width = <2>;
> >> 
> >> The properties above represent the width of the fields. Must all
> >> fields
> >> always be packed together into say the LSB of the registers? What if
> >> there are gaps between the fields in a future SoC variant, or the
> >> order
> >> of the fields in the register changes? I think you want to add either
> >> a
> >> samsung,func-bit/samsung,func-position property for each of the
> >> fields,
> >> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>.
> >> IIRC,
> >> the generic pinctrl binding used a mask for this purpose.
> >> 
> >> What if a future SoC variant adds more fields to the register? At that
> >> point, you'd need to edit the driver anyway in order to define a new
> >> DT
> >> property to represent the new field. Perhaps instead of having an
> >> explicit named property per field in the register, you want a simple
> >> list of fields:
> >> 
> >> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
> >> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;
> >> 
> >> That would allow a completely arbitrary number of fields and layouts
> >> to
> >> be described.
> >> 
> >> I wonder if for absolute generality you want a samsung,pin-stride
> >> property to represent the difference in register address per pin. I
> >> assume that's hard-coded as 4 right now.
> > 
> > Hmm, considering so many different possible changes, maybe a more
> > conservative solution would be better, like reducing the amount of
> > information held in DT to bank type, e.g.
> > 
> > 	samsung,bank-type = "exynos4";
> > 
> > or maybe
> > 
> > 	compatible = "samsung,exynos4-pin-bank*;
> > 
> > and then define supported bank types in the driver. SoC-specific data
> > would remain in DT, i.e. pctl-offset, pin-bank, pin-count,
> > eint-offset, etc.
> Yes, removing much of the data from DT and putting it into a tiny table
> in the driver makes sense to me in this case.

A hybrid solution came to my mind, define bank types in device tree once 
and then reference them in banks. Something like:

	pinctrl-bank-types {
		bank_off: bank-off {
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
			samsung,conpdn-width = <2>;
			samsung,pudpdn-width = <2>;
		};

		bank_alive: bank-alive {
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
		};
	};

	/* ... */

	pinctrl@12345678 {
		/* ... */
		gpa0: gpa0 {
			/* ... */
			samsung,bank-type = <&bank_off>;
			/* ... */
		};
		/* ... */
	};

This would add the possibility to define new banks types quickly, but would 
not add too much overhead.

What do you think?

> >>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi
> >>> b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644
> >>> --- a/arch/arm/boot/dts/exynos4210.dtsi
> >>> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> >>> @@ -59,6 +59,10 @@
> >>> 
> >>>  		reg = <0x11400000 0x1000>;
> >>>  		interrupts = <0 47 0>;
> >>>  		interrupt-controller;
> >>> 
> >>> +		samsung,geint-con = <0x700>;
> >>> +		samsung,geint-mask = <0x900>;
> >>> +		samsung,geint-pend = <0xA00>;
> >>> +		samsung,svc = <0xB08>;
> >> 
> >> I assume those new properties represent register addresses within the
> >> block. If not, I don't understand what they are.
> > 
> > Yes, they do.
> > 
> >> It's unclear to me why those properties aren't all part of
> >> exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where
> >> the register addresses for the pinctrl registers are the same (hence
> >> can
> >> be in a shared exynos4210-pinctrl-banks.dtsi), yet the register
> >> addresses for the geint registers are different (hence must be in
> >> non-shared exynos4210.dtsi)?
> > 
> > Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to
> > Exynos4210. Other SoCs are going to have their own
> > whatever-pinctrl-banks.dtsi.
> OK, so my point here is: why not put all the pinctrl-related properties
> into a single file, rather than spreading them across different files.
> Having separate files makes sense if they can be re-used in different
> places, but not if they're single-use.

All the definitions in device tree for pinctrl take lots of lines and so I 
though it would make it more readable if pin groups would have its own 
source file and so would pin banks.

Now that I think of it, they aren't going to be modified too much, so it 
might be better indeed to put them together in a single file.

Best regards,
Tomasz Figa

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/24/2012 03:31 PM, Tomasz Figa wrote:
> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
>> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
>>>>> platform-specific data parsing from DT.
>>>>>
>>>>> This patch adds all necessary nodes and properties to exynos4210
>>>>> device
>>>>> tree sources.
>>>>>
>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
>>>>>
>>>>> +			samsung,pctl-offset = <0x000>;
>>>>> +			samsung,pin-bank = "gpa0";
>>>>> +			samsung,pin-count = <8>;
>>>>> +			samsung,func-width = <4>;
>>>>> +			samsung,pud-width = <2>;
>>>>> +			samsung,drv-width = <2>;
>>>>> +			samsung,conpdn-width = <2>;
>>>>> +			samsung,pudpdn-width = <2>;
>>>>
>>>> The properties above represent the width of the fields. Must all
>>>> fields
>>>> always be packed together into say the LSB of the registers? What if
>>>> there are gaps between the fields in a future SoC variant, or the
>>>> order
>>>> of the fields in the register changes? I think you want to add either
>>>> a
>>>> samsung,func-bit/samsung,func-position property for each of the
>>>> fields,
>>>> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>.
>>>> IIRC,
>>>> the generic pinctrl binding used a mask for this purpose.
>>>>
>>>> What if a future SoC variant adds more fields to the register? At that
>>>> point, you'd need to edit the driver anyway in order to define a new
>>>> DT
>>>> property to represent the new field. Perhaps instead of having an
>>>> explicit named property per field in the register, you want a simple
>>>> list of fields:
>>>>
>>>> samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
>>>> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;
>>>>
>>>> That would allow a completely arbitrary number of fields and layouts
>>>> to
>>>> be described.
>>>>
>>>> I wonder if for absolute generality you want a samsung,pin-stride
>>>> property to represent the difference in register address per pin. I
>>>> assume that's hard-coded as 4 right now.
>>>
>>> Hmm, considering so many different possible changes, maybe a more
>>> conservative solution would be better, like reducing the amount of
>>> information held in DT to bank type, e.g.
>>>
>>> 	samsung,bank-type = "exynos4";
>>>
>>> or maybe
>>>
>>> 	compatible = "samsung,exynos4-pin-bank*;
>>>
>>> and then define supported bank types in the driver. SoC-specific data
>>> would remain in DT, i.e. pctl-offset, pin-bank, pin-count,
>>> eint-offset, etc.
>> Yes, removing much of the data from DT and putting it into a tiny table
>> in the driver makes sense to me in this case.
> 
> A hybrid solution came to my mind, define bank types in device tree once 
> and then reference them in banks. Something like:
> 
> 	pinctrl-bank-types {
> 		bank_off: bank-off {
> 			samsung,func-width = <4>;
> 			samsung,pud-width = <2>;
> 			samsung,drv-width = <2>;
> 			samsung,conpdn-width = <2>;
> 			samsung,pudpdn-width = <2>;
> 		};
> 
> 		bank_alive: bank-alive {
> 			samsung,func-width = <4>;
> 			samsung,pud-width = <2>;
> 			samsung,drv-width = <2>;
> 		};
> 	};
> 
> 	/* ... */
> 
> 	pinctrl@12345678 {
> 		/* ... */
> 		gpa0: gpa0 {
> 			/* ... */
> 			samsung,bank-type = <&bank_off>;
> 			/* ... */
> 		};
> 		/* ... */
> 	};
> 
> This would add the possibility to define new banks types quickly, but would 
> not add too much overhead.
> 
> What do you think?

That does solve the issue of parsing those same values over and over,
but at the cost of making the binding a lot more conceptually complex.

However, it doesn't solve any of the extensibility issues I raised such
as whether pos+size or mask would be a better representation, what about
if the fields end up being in different (separate) registers on newer
SoCs, etc.

I forget, do you actually have multiple different SoCs right now (or in
the near future where the HW design is known now for certain even if the
chip isn't available) that have different values for all these *-width
properties and hence can be represented just using this binding and
without altering the driver at all? If so, I suppose the original
binding is at least useful (although I would certainly still request to
use *-mask instead of *-width properties).

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

Hi Stephen,

On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:
> On 09/24/2012 03:31 PM, Tomasz Figa wrote:
> > On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
> >> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> >>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
> >>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> >>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
> >>>>> platform-specific data parsing from DT.
> >>>>> 
> >>>>> This patch adds all necessary nodes and properties to exynos4210
> >>>>> device
> >>>>> tree sources.
> >>>>> 
> >>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
> >>>>> 
> >>>>> +			samsung,pctl-offset = <0x000>;
> >>>>> +			samsung,pin-bank = "gpa0";
> >>>>> +			samsung,pin-count = <8>;
> >>>>> +			samsung,func-width = <4>;
> >>>>> +			samsung,pud-width = <2>;
> >>>>> +			samsung,drv-width = <2>;
> >>>>> +			samsung,conpdn-width = <2>;
> >>>>> +			samsung,pudpdn-width = <2>;
> >>>> 
> >>>> The properties above represent the width of the fields. Must all
> >>>> fields
> >>>> always be packed together into say the LSB of the registers? What if
> >>>> there are gaps between the fields in a future SoC variant, or the
> >>>> order
> >>>> of the fields in the register changes? I think you want to add
> >>>> either
> >>>> a
> >>>> samsung,func-bit/samsung,func-position property for each of the
> >>>> fields,
> >>>> or change from samsung,func-width=<4> to samsung,field-mask=<0xf>.
> >>>> IIRC,
> >>>> the generic pinctrl binding used a mask for this purpose.
> >>>> 
> >>>> What if a future SoC variant adds more fields to the register? At
> >>>> that
> >>>> point, you'd need to edit the driver anyway in order to define a new
> >>>> DT
> >>>> property to represent the new field. Perhaps instead of having an
> >>>> explicit named property per field in the register, you want a simple
> >>>> list of fields:
> >>>> 
> >>>> samsung,pin-property-names = "func", "pud", "drv", "conpdn",
> >>>> "pudpdn";
> >>>> samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;
> >>>> 
> >>>> That would allow a completely arbitrary number of fields and layouts
> >>>> to
> >>>> be described.
> >>>> 
> >>>> I wonder if for absolute generality you want a samsung,pin-stride
> >>>> property to represent the difference in register address per pin. I
> >>>> assume that's hard-coded as 4 right now.
> >>> 
> >>> Hmm, considering so many different possible changes, maybe a more
> >>> conservative solution would be better, like reducing the amount of
> >>> information held in DT to bank type, e.g.
> >>> 
> >>> 	samsung,bank-type = "exynos4";
> >>> 
> >>> or maybe
> >>> 
> >>> 	compatible = "samsung,exynos4-pin-bank*;
> >>> 
> >>> and then define supported bank types in the driver. SoC-specific data
> >>> would remain in DT, i.e. pctl-offset, pin-bank, pin-count,
> >>> eint-offset, etc.
> >> 
> >> Yes, removing much of the data from DT and putting it into a tiny
> >> table
> >> in the driver makes sense to me in this case.
> > 
> > A hybrid solution came to my mind, define bank types in device tree
> > once
> > 
> > and then reference them in banks. Something like:
> > 	pinctrl-bank-types {
> > 	
> > 		bank_off: bank-off {
> > 		
> > 			samsung,func-width = <4>;
> > 			samsung,pud-width = <2>;
> > 			samsung,drv-width = <2>;
> > 			samsung,conpdn-width = <2>;
> > 			samsung,pudpdn-width = <2>;
> > 		
> > 		};
> > 		
> > 		bank_alive: bank-alive {
> > 		
> > 			samsung,func-width = <4>;
> > 			samsung,pud-width = <2>;
> > 			samsung,drv-width = <2>;
> > 		
> > 		};
> > 	
> > 	};
> > 	
> > 	/* ... */
> > 	
> > 	pinctrl@12345678 {
> > 	
> > 		/* ... */
> > 		gpa0: gpa0 {
> > 		
> > 			/* ... */
> > 			samsung,bank-type = <&bank_off>;
> > 			/* ... */
> > 		
> > 		};
> > 		/* ... */
> > 	
> > 	};
> > 
> > This would add the possibility to define new banks types quickly, but
> > would not add too much overhead.
> > 
> > What do you think?
> 
> That does solve the issue of parsing those same values over and over,
> but at the cost of making the binding a lot more conceptually complex.
> 
> However, it doesn't solve any of the extensibility issues I raised such
> as whether pos+size or mask would be a better representation, what about
> if the fields end up being in different (separate) registers on newer
> SoCs, etc.

Hmm, could you elaborate on the idea of using mask instead of field widths? 
I don't see how this could be better and there is an additional drawback of 
having to calculate width and pos from every mask.

Anyway, back to your concern, the values that are written to the bit fields 
specified by those bindings are arbitrary SoC-specific values anyway, so 
if, for example, we get a SoC with following register layout:

bits:    7 | 6 - 4  | 3 | 2 - 0
meaning: 0 | func 1 | 0 | func 0

or

bits:    7 - 5  | 4 | 3 - 1  | 0
meaning: func 1 | 0 | func 0 | 0

we can easily define the width as 4 and use appropriate 4-bit function 
values with zeroes on reserved positions.

> I forget, do you actually have multiple different SoCs right now (or in
> the near future where the HW design is known now for certain even if the
> chip isn't available) that have different values for all these *-width
> properties and hence can be represented just using this binding and
> without altering the driver at all? If so, I suppose the original
> binding is at least useful (although I would certainly still request to
> use *-mask instead of *-width properties).

The binding I proposed covers all Samsung SoCs currently available, 
starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two 
function registers), to the whole Exynos series, including latest Exynos5.

Best regards,
--
Tomasz Figa
Samsung Poland R&D Center

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/25/2012 03:37 AM, Tomasz Figa wrote:
> Hi Stephen,
> 
> On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:
>> On 09/24/2012 03:31 PM, Tomasz Figa wrote:
>>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
>>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
>>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
>>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
>>>>>>> platform-specific data parsing from DT.
>>>>>>>
>>>>>>> This patch adds all necessary nodes and properties to exynos4210
>>>>>>> device
>>>>>>> tree sources.
>>>>>>>
>>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
>>>>>>>
>>>>>>> +			samsung,pctl-offset = <0x000>;
>>>>>>> +			samsung,pin-bank = "gpa0";
>>>>>>> +			samsung,pin-count = <8>;
>>>>>>> +			samsung,func-width = <4>;
>>>>>>> +			samsung,pud-width = <2>;
>>>>>>> +			samsung,drv-width = <2>;
>>>>>>> +			samsung,conpdn-width = <2>;
>>>>>>> +			samsung,pudpdn-width = <2>;
...
> Hmm, could you elaborate on the idea of using mask instead of field widths? 

For background: With e.g.:

samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;

How do you know if the layout is:

bits:    7-4  | 3-2 | 1-0
meaning: func | pud | drv

or:

bits:    7-6 | 5-4 | 3-0  |
meaning: drv | pud | func |

or:

bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
meaning: func  | unused | pud | unused | drv | unused

I suppose what you're saying is that for all currently extant Samsung
SoCs, there's some rule that defines this; perhaps the fields are always
in order MSB to LSB func, pud, drv, and there are never any unused bits
between the fields? If so, I suppose that's reasonable, even if it does
restrict the binding's ability to support any unanticipated future SoC
register layout changes.

> I don't see how this could be better and there is an additional drawback of 
> having to calculate width and pos from every mask.

With the DT properties just defining "width", the driver still has to
calculate the pos from every width by adding up the widths of all fields
lower in the register, right? Or, does each field always start at a
hard-coded bit position?

Anyway, you could completely avoid this question by using masks instead:

samsung,func-mask = <0xf0>;
samsung,pud-mask = <0xc>;
samsung,drv-mask = <0x3>;

The mask defines exactly which bits are included in the register field,
so it implicitly defines both the position and width of the field.

Finding the shift/size is very easy. I believe Tony Lindgren's generic
pinctrl already does this along these lines. Very roughly:

func_pos = ffs(func_mask);
func_width = ffs(~(func_mask >> func_pos));

> Anyway, back to your concern, the values that are written to the bit fields 
> specified by those bindings are arbitrary SoC-specific values anyway, so 
> if, for example, we get a SoC with following register layout:
> 
> bits:    7 | 6 - 4  | 3 | 2 - 0
> meaning: 0 | func 1 | 0 | func 0
> 
> or
> 
> bits:    7 - 5  | 4 | 3 - 1  | 0
> meaning: func 1 | 0 | func 0 | 0
> 
> we can easily define the width as 4 and use appropriate 4-bit function 
> values with zeroes on reserved positions.

The problem with that is that if the datasheet documents "func" values
of 0, 1, 2, 3, whereas your driver expects values that are shifted left
one bit in order to fit into the field, the DT would need to contain 0,
2, 4, 6. So, the DT values then don't match the documentation, which
would end up being confusing.

>> I forget, do you actually have multiple different SoCs right now (or in
>> the near future where the HW design is known now for certain even if the
>> chip isn't available) that have different values for all these *-width
>> properties and hence can be represented just using this binding and
>> without altering the driver at all? If so, I suppose the original
>> binding is at least useful (although I would certainly still request to
>> use *-mask instead of *-width properties).
> 
> The binding I proposed covers all Samsung SoCs currently available, 
> starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two 
> function registers), to the whole Exynos series, including latest Exynos5.

OK, the HW is nice and consistent then. In that case, the binding is
probably reasonable. Hopefully the HW designers are aware they shouldn't
randomly break the uniformity!

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote:
> On 09/25/2012 03:37 AM, Tomasz Figa wrote:
> > Hi Stephen,
> > 
> > On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:
> >> On 09/24/2012 03:31 PM, Tomasz Figa wrote:
> >>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
> >>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> >>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
> >>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> >>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
> >>>>>>> platform-specific data parsing from DT.
> >>>>>>> 
> >>>>>>> This patch adds all necessary nodes and properties to exynos4210
> >>>>>>> device
> >>>>>>> tree sources.
> >>>>>>> 
> >>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
> >>>>>>> 
> >>>>>>> +			samsung,pctl-offset = <0x000>;
> >>>>>>> +			samsung,pin-bank = "gpa0";
> >>>>>>> +			samsung,pin-count = <8>;
> >>>>>>> +			samsung,func-width = <4>;
> >>>>>>> +			samsung,pud-width = <2>;
> >>>>>>> +			samsung,drv-width = <2>;
> >>>>>>> +			samsung,conpdn-width = <2>;
> >>>>>>> +			samsung,pudpdn-width = <2>;
> 
> ...
> 
> > Hmm, could you elaborate on the idea of using mask instead of field
> > widths?
> For background: With e.g.:
> 
> samsung,func-width = <4>;
> samsung,pud-width = <2>;
> samsung,drv-width = <2>;
> 
> How do you know if the layout is:
> 
> bits:    7-4  | 3-2 | 1-0
> meaning: func | pud | drv
> 
> or:
> 
> bits:    7-6 | 5-4 | 3-0  |
> meaning: drv | pud | func |
> 
> or:
> 
> bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
> meaning: func  | unused | pud | unused | drv | unused
> 
> I suppose what you're saying is that for all currently extant Samsung
> SoCs, there's some rule that defines this; perhaps the fields are always
> in order MSB to LSB func, pud, drv, and there are never any unused bits
> between the fields? If so, I suppose that's reasonable, even if it does
> restrict the binding's ability to support any unanticipated future SoC
> register layout changes.

I think we have a little misunderstanding here.

All the Samsung SoCs currently available have separate registers for 
particular configuration types. Each register is used to configure all pins 
in a bank. The width field specifies how many bits are used per pin, not 
per configuration type.

> > I don't see how this could be better and there is an additional
> > drawback of having to calculate width and pos from every mask.
> 
> With the DT properties just defining "width", the driver still has to
> calculate the pos from every width by adding up the widths of all fields
> lower in the register, right? Or, does each field always start at a
> hard-coded bit position?
> 
> Anyway, you could completely avoid this question by using masks instead:
> 
> samsung,func-mask = <0xf0>;
> samsung,pud-mask = <0xc>;
> samsung,drv-mask = <0x3>;
> 
> The mask defines exactly which bits are included in the register field,
> so it implicitly defines both the position and width of the field.
> 
> Finding the shift/size is very easy. I believe Tony Lindgren's generic
> pinctrl already does this along these lines. Very roughly:
> 
> func_pos = ffs(func_mask);
> func_width = ffs(~(func_mask >> func_pos));

Right, this looks fine.

> > Anyway, back to your concern, the values that are written to the bit
> > fields specified by those bindings are arbitrary SoC-specific values
> > anyway, so if, for example, we get a SoC with following register
> > layout:
> > 
> > bits:    7 | 6 - 4  | 3 | 2 - 0
> > meaning: 0 | func 1 | 0 | func 0
> > 
> > or
> > 
> > bits:    7 - 5  | 4 | 3 - 1  | 0
> > meaning: func 1 | 0 | func 0 | 0
> > 
> > we can easily define the width as 4 and use appropriate 4-bit function
> > values with zeroes on reserved positions.
> 
> The problem with that is that if the datasheet documents "func" values
> of 0, 1, 2, 3, whereas your driver expects values that are shifted left
> one bit in order to fit into the field, the DT would need to contain 0,
> 2, 4, 6. So, the DT values then don't match the documentation, which
> would end up being confusing.
> 
> >> I forget, do you actually have multiple different SoCs right now (or
> >> in
> >> the near future where the HW design is known now for certain even if
> >> the
> >> chip isn't available) that have different values for all these *-width
> >> properties and hence can be represented just using this binding and
> >> without altering the driver at all? If so, I suppose the original
> >> binding is at least useful (although I would certainly still request
> >> to
> >> use *-mask instead of *-width properties).
> > 
> > The binding I proposed covers all Samsung SoCs currently available,
> > starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with
> > two
> > function registers), to the whole Exynos series, including latest
> > Exynos5.
> OK, the HW is nice and consistent then. In that case, the binding is
> probably reasonable. Hopefully the HW designers are aware they shouldn't
> randomly break the uniformity!

Let's hope so.

Best regards,
Tomasz Figa

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/25/2012 11:41 AM, Tomasz Figa wrote:
> On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote:
>> On 09/25/2012 03:37 AM, Tomasz Figa wrote:
>>> Hi Stephen,
>>>
>>> On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:
>>>> On 09/24/2012 03:31 PM, Tomasz Figa wrote:
>>>>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
>>>>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
>>>>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
>>>>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>>>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT" introduced
>>>>>>>>> platform-specific data parsing from DT.
>>>>>>>>>
>>>>>>>>> This patch adds all necessary nodes and properties to exynos4210
>>>>>>>>> device
>>>>>>>>> tree sources.
>>>>>>>>>
>>>>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
>>>>>>>>>
>>>>>>>>> +			samsung,pctl-offset = <0x000>;
>>>>>>>>> +			samsung,pin-bank = "gpa0";
>>>>>>>>> +			samsung,pin-count = <8>;
>>>>>>>>> +			samsung,func-width = <4>;
>>>>>>>>> +			samsung,pud-width = <2>;
>>>>>>>>> +			samsung,drv-width = <2>;
>>>>>>>>> +			samsung,conpdn-width = <2>;
>>>>>>>>> +			samsung,pudpdn-width = <2>;
>>
>> ...
>>
>>> Hmm, could you elaborate on the idea of using mask instead of field
>>> widths?
>> For background: With e.g.:
>>
>> samsung,func-width = <4>;
>> samsung,pud-width = <2>;
>> samsung,drv-width = <2>;
>>
>> How do you know if the layout is:
>>
>> bits:    7-4  | 3-2 | 1-0
>> meaning: func | pud | drv
>>
>> or:
>>
>> bits:    7-6 | 5-4 | 3-0  |
>> meaning: drv | pud | func |
>>
>> or:
>>
>> bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
>> meaning: func  | unused | pud | unused | drv | unused
>>
>> I suppose what you're saying is that for all currently extant Samsung
>> SoCs, there's some rule that defines this; perhaps the fields are always
>> in order MSB to LSB func, pud, drv, and there are never any unused bits
>> between the fields? If so, I suppose that's reasonable, even if it does
>> restrict the binding's ability to support any unanticipated future SoC
>> register layout changes.
> 
> I think we have a little misunderstanding here.
> 
> All the Samsung SoCs currently available have separate registers for 
> particular configuration types. Each register is used to configure all pins 
> in a bank. The width field specifies how many bits are used per pin, not 
> per configuration type.

Oh I see. In that case, I guess just having "width" properties is fine,
and I can see how it's much more likely this scheme would be extensible
to any future SoC that sticks with the same overall kind of register
structure.

It'd be a good idea to describe this explicitly in the binding
documentation.

BTW, how does the driver know what register addresses to use; I can see
the base for each pin controller bank is in samsung,pctl-offset, but
what describes the offset for each of the func, pud, drv, ... registers
from there? Are the offsets the same for all current Samsung SoCs?

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Tomasz Figa]

On Tuesday 25 of September 2012 12:22:03 Stephen Warren wrote:
> On 09/25/2012 11:41 AM, Tomasz Figa wrote:
> > On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote:
> >> On 09/25/2012 03:37 AM, Tomasz Figa wrote:
> >>> Hi Stephen,
> >>> 
> >>> On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:
> >>>> On 09/24/2012 03:31 PM, Tomasz Figa wrote:
> >>>>> On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
> >>>>>> On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> >>>>>>> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
> >>>>>>>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
> >>>>>>>>> The patch "pinctrl: samsung: Parse pin banks from DT"
> >>>>>>>>> introduced
> >>>>>>>>> platform-specific data parsing from DT.
> >>>>>>>>> 
> >>>>>>>>> This patch adds all necessary nodes and properties to
> >>>>>>>>> exynos4210
> >>>>>>>>> device
> >>>>>>>>> tree sources.
> >>>>>>>>> 
> >>>>>>>>> +++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi
> >>>>>>>>> 
> >>>>>>>>> +			samsung,pctl-offset = <0x000>;
> >>>>>>>>> +			samsung,pin-bank = "gpa0";
> >>>>>>>>> +			samsung,pin-count = <8>;
> >>>>>>>>> +			samsung,func-width = <4>;
> >>>>>>>>> +			samsung,pud-width = <2>;
> >>>>>>>>> +			samsung,drv-width = <2>;
> >>>>>>>>> +			samsung,conpdn-width = <2>;
> >>>>>>>>> +			samsung,pudpdn-width = <2>;
> >> 
> >> ...
> >> 
> >>> Hmm, could you elaborate on the idea of using mask instead of field
> >>> widths?
> >> 
> >> For background: With e.g.:
> >> 
> >> samsung,func-width = <4>;
> >> samsung,pud-width = <2>;
> >> samsung,drv-width = <2>;
> >> 
> >> How do you know if the layout is:
> >> 
> >> bits:    7-4  | 3-2 | 1-0
> >> meaning: func | pud | drv
> >> 
> >> or:
> >> 
> >> bits:    7-6 | 5-4 | 3-0  |
> >> meaning: drv | pud | func |
> >> 
> >> or:
> >> 
> >> bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
> >> meaning: func  | unused | pud | unused | drv | unused
> >> 
> >> I suppose what you're saying is that for all currently extant Samsung
> >> SoCs, there's some rule that defines this; perhaps the fields are
> >> always
> >> in order MSB to LSB func, pud, drv, and there are never any unused
> >> bits
> >> between the fields? If so, I suppose that's reasonable, even if it
> >> does
> >> restrict the binding's ability to support any unanticipated future SoC
> >> register layout changes.
> > 
> > I think we have a little misunderstanding here.
> > 
> > All the Samsung SoCs currently available have separate registers for
> > particular configuration types. Each register is used to configure all
> > pins in a bank. The width field specifies how many bits are used per
> > pin, not per configuration type.
> 
> Oh I see. In that case, I guess just having "width" properties is fine,
> and I can see how it's much more likely this scheme would be extensible
> to any future SoC that sticks with the same overall kind of register
> structure.
> 
> It'd be a good idea to describe this explicitly in the binding
> documentation.

OK.

> BTW, how does the driver know what register addresses to use; I can see
> the base for each pin controller bank is in samsung,pctl-offset, but
> what describes the offset for each of the func, pud, drv, ... registers
> from there? Are the offsets the same for all current Samsung SoCs?

The offsets are defined as constants in the driver.

They are the same in all cases, but the "4bit2" bank type of S3C64xx, which 
can have up to 16 pins with 4-bit function specifiers, so two registers are 
required for function configuration. In this case all the remaining 
registers are offset by 0x04.

I couldn't think about any good solution for this special case, but still, 
I haven't been thinking a lot about it, as the driver is targetted at 
current Exynos SoCs primarily.

Best regards,
Tomasz Figa

Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

[Stephen Warren]

On 09/25/2012 12:35 PM, Tomasz Figa wrote:
> On Tuesday 25 of September 2012 12:22:03 Stephen Warren wrote:
...
>> BTW, how does the driver know what register addresses to use; I can see
>> the base for each pin controller bank is in samsung,pctl-offset, but
>> what describes the offset for each of the func, pud, drv, ... registers
>> from there? Are the offsets the same for all current Samsung SoCs?
> 
> The offsets are defined as constants in the driver.
> 
> They are the same in all cases, but the "4bit2" bank type of S3C64xx, which 
> can have up to 16 pins with 4-bit function specifiers, so two registers are 
> required for function configuration. In this case all the remaining 
> registers are offset by 0x04.
> 
> I couldn't think about any good solution for this special case, but still, 
> I haven't been thinking a lot about it, as the driver is targetted at 
> current Exynos SoCs primarily.

I suppose if you always assume that the registers will appear in a
specific order, and never have gaps between them, then you can simply
always calculate the addresses as e.g.:

reg_func = reg_base
reg_pud = reg_func + round_up(num_pins / (32 / func_width))
reg_drv = reg_pud + round_up(num_pins / (32 / func_width))
...

Then, there wouldn't ever be any special cases - that calculation would
always work.

An alternative would be to put each register's address in DT rather than
just the base of the register block. It'd certainly be more
future-flexible, even if not strictly necessary.