diff mbox

[04/16] pinctrl: samsung: Parse pin banks from DT

Message ID 1349685556-23718-5-git-send-email-t.figa@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa Oct. 8, 2012, 8:39 a.m. UTC
Currently SoC-specific properties such as list of pin banks, register
offsets and bitfield sizes are being taken from static data structures
residing in pinctrl-exynos.c.

This patch modifies the pinctrl-samsung driver to parse all SoC-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>
---
 drivers/pinctrl/pinctrl-exynos.c  |   5 ++
 drivers/pinctrl/pinctrl-samsung.c | 169 +++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-samsung.h |  17 +++-
 3 files changed, 187 insertions(+), 4 deletions(-)

Comments

Linus Walleij Oct. 10, 2012, 7:34 a.m. UTC | #1
On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa <t.figa@samsung.com> wrote:

> Currently SoC-specific properties such as list of pin banks, register
> offsets and bitfield sizes are being taken from static data structures
> residing in pinctrl-exynos.c.
>
> This patch modifies the pinctrl-samsung driver to parse all SoC-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.

So why? Two approaches:

- Put as much info as possible into the device tree
- Put as much info as possible into the driver

The first approach is currently only used by pinctrl-single.c.

That driver is designed for the case where all info about
the hardware arrives in some description language that
can be translated into a simple DT description.

If you want to use that approach, you should use that
driver. If that driver does not work for you, then it's not
fulfilling it's purpose as a one-stop shop for simple
pin controllers entirely contained within the device tree,
and should be renamed or redesigned.

If you will end up with a hybrid approach with some
stuff in the device tree and some stuff in the code,
it's better to keep the old driver.

Yours,
Linus Walleij
Tomasz Figa Oct. 10, 2012, 8:39 a.m. UTC | #2
On Wednesday 10 of October 2012 09:34:05 Linus Walleij wrote:
> On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Currently SoC-specific properties such as list of pin banks, register
> > offsets and bitfield sizes are being taken from static data structures
> > residing in pinctrl-exynos.c.
> > 
> > This patch modifies the pinctrl-samsung driver to parse all
> > SoC-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.
> 
> So why? Two approaches:
> 
> - Put as much info as possible into the device tree
> - Put as much info as possible into the driver
> 
> The first approach is currently only used by pinctrl-single.c.
> 
> That driver is designed for the case where all info about
> the hardware arrives in some description language that
> can be translated into a simple DT description.
> 
> If you want to use that approach, you should use that
> driver. If that driver does not work for you, then it's not
> fulfilling it's purpose as a one-stop shop for simple
> pin controllers entirely contained within the device tree,
> and should be renamed or redesigned.
> 
> If you will end up with a hybrid approach with some
> stuff in the device tree and some stuff in the code,
> it's better to keep the old driver.

This will allow us to cover all the existing Samsung SoCs, starting from 
S3C24xx, through S3C64xx, S5P*, all supported Exynos SoCs and ending on any 
future SoCs using this kind of pin controller, without bloating the driver 
with hardly readable macros, lots of (often duplicated) static data and 
similar.

If there are some serious problems with this approach, just let me know and 
I will reconsider it, but if not, I'd like to keep it, because of the 
benefits it gives.

(Even if I moved all those SoC-specific data to static structures located 
in the driver, to keep the most readable way of GPIO specification in DT, I 
would have to create nodes for all banks in DT anyway and the driver would 
have to match particular nodes with their static data. I don't like this 
kind of approach.)

Best regards,
Linus Walleij Oct. 11, 2012, 1:52 p.m. UTC | #3
On Wed, Oct 10, 2012 at 10:39 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 10 of October 2012 09:34:05 Linus Walleij wrote:

>> If you will end up with a hybrid approach with some
>> stuff in the device tree and some stuff in the code,
>> it's better to keep the old driver.
>
> This will allow us to cover all the existing Samsung SoCs, starting from
> S3C24xx, through S3C64xx, S5P*, all supported Exynos SoCs and ending on any
> future SoCs using this kind of pin controller, without bloating the driver
> with hardly readable macros, lots of (often duplicated) static data and
> similar.

I do not agree with this, as you probably have realized by now...

I think it's better to use the compatible string to choose the offset
variable directly in the driver. But hey, it's just me, still.

> If there are some serious problems with this approach, just let me know and
> I will reconsider it, but if not, I'd like to keep it, because of the
> benefits it gives.

I'd like some input from Thomas Abraham before I make up my
mind about it.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index b4b58d4..e3fa263 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 c660fa5..e828a0e 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -46,6 +46,8 @@  struct pin_config {
 	{ "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN },
 };
 
+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)
 {
@@ -602,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);
@@ -778,6 +782,86 @@  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_type(struct samsung_pin_bank *bank,
+							struct device_node *np)
+{
+	struct samsung_pin_bank *type = np->data;
+	int ret;
+	u32 val;
+
+	if (type) {
+		*bank = *type;
+		return 0;
+	}
+
+	type = kzalloc(sizeof(*type), GFP_KERNEL);
+	if (!type)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "samsung,func-width", &val);
+	if (ret)
+		return ret;
+	type->func_width = val;
+
+	ret = of_property_read_u32(np, "samsung,pud-width", &val);
+	if (!ret)
+		type->pud_width = val;
+
+	ret = of_property_read_u32(np, "samsung,drv-width", &val);
+	if (!ret)
+		type->drv_width = val;
+
+	ret = of_property_read_u32(np, "samsung,conpdn-width", &val);
+	if (!ret)
+		type->conpdn_width = val;
+
+	ret = of_property_read_u32(np, "samsung,pudpdn-width", &val);
+	if (!ret)
+		type->pudpdn_width = val;
+
+	*bank = *type;
+	np->data = type;
+
+	return 0;
+}
+
+static int samsung_pinctrl_parse_dt_bank(struct samsung_pin_bank *bank,
+							struct device_node *np)
+{
+	int ret;
+	u32 val;
+	struct device_node *type_np;
+
+	type_np = of_parse_phandle(np, "samsung,bank-type", 0);
+	if (!type_np)
+		return -EINVAL;
+
+	ret = samsung_pinctrl_parse_dt_bank_type(bank, type_np);
+	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;
+
+	bank->name = np->name;
+
+	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)
@@ -785,6 +869,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) {
@@ -792,7 +884,80 @@  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 (b->eint_type == EINT_TYPE_GPIO) {
+			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;
+	ctrl->base = pin_base;
+	pin_base += ctrl->nr_pins;
+
+	return ctrl;
 }
 
 static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
@@ -860,7 +1025,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 */