vf610: Add GPIO support

The pinctrl driver initialized the register offsets for the pins with 0. On Vybrid an offset of 0 is a valid offset for the pinctrl mux register. So far, this was solved using the ZERO_OFFSET_VALID flag which allowed offsets of 0. However, this does not allow to verify whether a pins struct imx_pmx_func was initialized or not.

Use signed offset values for register offsets and initialize those with -1 in order to detect uninitialized offset values reliable.

Signed-off-by Stefan Agner <[email protected]>

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c   | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.h   |  1 +
 drivers/pinctrl/pinctrl-vf610.c |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-imx.c +8 -0

index 0d4558b..64d1b59 100644
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+

drivers/pinctrl/pinctrl-imx.c +14 -0

+
+	return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;

drivers/pinctrl/pinctrl-imx.c +7 -0

+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
 static const struct pinmux_ops imx_pmx_ops = {
 	.get_functions_count = imx_pmx_get_funcs_count,
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
+	.gpio_request_enable = imx_pmx_gpio_request_enable,
+	.gpio_set_direction = imx_pmx_gpio_set_direction,
 	.enable = imx_pmx_enable,
 };
 

drivers/pinctrl/pinctrl-imx.c +0 -0

 	info->dev = &pdev->dev;
 
 	/* Create state holders etc for this driver */

drivers/pinctrl/pinctrl-imx.h +1 -0

index 49e55d3..8f37ca4 100644
@@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
 };
 
 #define SHARE_MUX_CONF_REG	0x1
+#define GPIO_CONTROL		0x2
 
 #define NO_MUX		0x0
 #define NO_PAD		0x0

drivers/pinctrl/pinctrl-vf610.c +1 -1

index b788e15..fdf5661 100644
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
 static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.pins = vf610_pinctrl_pads,
 	.npins = ARRAY_SIZE(vf610_pinctrl_pads),
-	.flags = SHARE_MUX_CONF_REG,
+	.flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
 };
 
 static struct of_device_id vf610_pinctrl_of_match[] = {

Signature · 1 line

2.1.0

Quoted 32 lines

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c   | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.h   |  1 +
 drivers/pinctrl/pinctrl-vf610.c |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-imx.c +13 -0

index 0d4558b..64d1b59 100644
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 314

Quoted 5 lines

drivers/pinctrl/pinctrl-imx.c +5 -0

+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);

Isn't this setup redundant at all, since imx_pmx_enable() already takes care of setting mux register including GPIO mode?

Quoted 20 lines

drivers/pinctrl/pinctrl-imx.c +20 -0

+
+	return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 338

Quoted 4 lines

drivers/pinctrl/pinctrl-imx.c +4 -0

+	if (input)
+		reg &= ~0x2;
+	else
+		reg |= 0x2;

This is all about Output Buffer Enable (OBE) bit. What about Input Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO direction as well?

Quoted 14 lines

drivers/pinctrl/pinctrl-imx.c +7 -0

+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
 static const struct pinmux_ops imx_pmx_ops = {
 	.get_functions_count = imx_pmx_get_funcs_count,
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
+	.gpio_request_enable = imx_pmx_gpio_request_enable,
+	.gpio_set_direction = imx_pmx_gpio_set_direction,
 	.enable = imx_pmx_enable,
 };
 

drivers/pinctrl/pinctrl-imx.c int imx_pinctrl_probe(struct platform_device *pdev, · line 635

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c +5 -0

@@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
+
+	/* GPIO control functions only intended for shared mux/conf register */
+	if (info->flags & GPIO_CONTROL)
+		BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
+

If this is always true, why don't we just use flag SHARE_MUX_CONF_REG and save GPIO_CONTROL? This check doesn't make too much sense to me if we choose to have a new flag for GPIO setup. IMO, we should probably either drop the GPIO_CONTROL flag or the check.

Shawn

Quoted 31 lines

drivers/pinctrl/pinctrl-imx.c +0 -0

 	info->dev = &pdev->dev;
 
 	/* Create state holders etc for this driver */

drivers/pinctrl/pinctrl-imx.h +1 -0

index 49e55d3..8f37ca4 100644
@@ -84,6 +84,7 @@ struct imx_pinctrl_soc_info {
 };
 
 #define SHARE_MUX_CONF_REG	0x1
+#define GPIO_CONTROL		0x2
 
 #define NO_MUX		0x0
 #define NO_PAD		0x0

drivers/pinctrl/pinctrl-vf610.c +1 -2

index b788e15..fdf5661 100644
@@ -299,7 +299,7 @@ static const struct pinctrl_pin_desc vf610_pinctrl_pads[] = {
 static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.pins = vf610_pinctrl_pads,
 	.npins = ARRAY_SIZE(vf610_pinctrl_pads),
-	.flags = SHARE_MUX_CONF_REG,
+	.flags = SHARE_MUX_CONF_REG | GPIO_CONTROL,
 };
 
 static struct of_device_id vf610_pinctrl_of_match[] = {
--

2.1.0

Am 2014-09-25 04:47, schrieb Shawn Guo:

Quoted 33 lines · Stefan Agner

Quoted 32 lines

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c   | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.h   |  1 +
 drivers/pinctrl/pinctrl-vf610.c |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-imx.c +13 -0

index 0d4558b..64d1b59 100644
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 314

Quoted 5 lines

drivers/pinctrl/pinctrl-imx.c +5 -0

+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);

Isn't this setup redundant at all, since imx_pmx_enable() already takes care of setting mux register including GPIO mode?

Yes currently this is redundant, when a pinmux is actually applied. What is the expected behaviour? Is a explicit pinmux necessary before we can use GPIO? If not, maybe it would make more sense to use imx_pmx_enable here to write all pinctrl settings?

Quoted 21 lines

Quoted 21 lines

drivers/pinctrl/pinctrl-imx.c +20 -0

+
+	return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 338

Quoted 4 lines

drivers/pinctrl/pinctrl-imx.c +4 -0

+	if (input)
+		reg &= ~0x2;
+	else
+		reg |= 0x2;

This is all about Output Buffer Enable (OBE) bit. What about Input Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO direction as well?

The leave the input buffer doesn't hurt, it allows to read back the value which is actually "on the wire". If a pin is hard on GND, one can actually see that.

Quoted 15 lines

Quoted 15 lines

drivers/pinctrl/pinctrl-imx.c +7 -0

+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
 static const struct pinmux_ops imx_pmx_ops = {
 	.get_functions_count = imx_pmx_get_funcs_count,
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
+	.gpio_request_enable = imx_pmx_gpio_request_enable,
+	.gpio_set_direction = imx_pmx_gpio_set_direction,
 	.enable = imx_pmx_enable,
 };
 

Quoted 15 lines

drivers/pinctrl/pinctrl-imx.c int imx_pinctrl_probe(struct platform_device *pdev, · line 635

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c +5 -0

@@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
+
+	/* GPIO control functions only intended for shared mux/conf register */
+	if (info->flags & GPIO_CONTROL)
+		BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
+

If this is always true, why don't we just use flag SHARE_MUX_CONF_REG and save GPIO_CONTROL? This check doesn't make too much sense to me if we choose to have a new flag for GPIO setup. IMO, we should probably either drop the GPIO_CONTROL flag or the check.

Well, this is always true because the vf610 driver configures both configs. But when somebody accidentally enables GPIO_CONFIG without understanding the implications... This was more meant like "don't try to use the GPIO_CONTROL just like that, its Vybird specific".

But I'm ok to remove this runtime check, maybe a comment describing the flags is more appropriate..?

Quoted 44 lines

Am 2014-09-25 04:47, schrieb Shawn Guo:

Quoted 42 lines · Stefan Agner

Quoted 27 lines

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c   | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-imx.h   |  1 +
 drivers/pinctrl/pinctrl-vf610.c |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-imx.c +8 -0

index 0d4558b..64d1b59 100644
@@ -294,10 +294,59 @@ static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 314

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +10 -0

+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);

Isn't this setup redundant at all, since imx_pmx_enable() already takes care of setting mux register including GPIO mode?

Quoted 4 lines

Yes currently this is redundant, when a pinmux is actually applied. What is the expected behaviour? Is a explicit pinmux necessary before we can use GPIO? If not, maybe it would make more sense to use imx_pmx_enable here to write all pinctrl settings?

Okay, as per Documentation/pinctrl.txt, it's required that GPIO and PINCTRL can be used as orthogonal. That said, your code does the right thing. Sorry for the noisy comment.

Quoted 31 lines

Quoted 21 lines

Quoted 21 lines

drivers/pinctrl/pinctrl-imx.c +20 -0

+
+	return 0;
+}
+
+static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+	   struct pinctrl_gpio_range *range, unsigned offset, bool input)
+{
+	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+	const struct imx_pinctrl_soc_info *info = ipctl->info;
+	const struct imx_pin_reg *pin_reg;
+	u32 reg;
+
+	if (!(info->flags & GPIO_CONTROL))
+		return -EINVAL;
+
+	pin_reg = &info->pin_regs[offset];
+	if (pin_reg->mux_reg == -1)
+		return -EINVAL;
+
+	reg = readl(ipctl->base + pin_reg->mux_reg);

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 338

Quoted 4 lines

drivers/pinctrl/pinctrl-imx.c +4 -0

+	if (input)
+		reg &= ~0x2;
+	else
+		reg |= 0x2;

This is all about Output Buffer Enable (OBE) bit. What about Input Buffer Enable (IBE) bit? Don't we need to set or clear it as per GPIO direction as well?

Quoted 3 lines

The leave the input buffer doesn't hurt, it allows to read back the value which is actually "on the wire". If a pin is hard on GND, one can actually see that.

Okay.

Quoted 31 lines

Quoted 15 lines

Quoted 15 lines

drivers/pinctrl/pinctrl-imx.c +7 -0

+	writel(reg, ipctl->base + pin_reg->mux_reg);
+
+	return 0;
+}
+
 static const struct pinmux_ops imx_pmx_ops = {
 	.get_functions_count = imx_pmx_get_funcs_count,
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
+	.gpio_request_enable = imx_pmx_gpio_request_enable,
+	.gpio_set_direction = imx_pmx_gpio_set_direction,
 	.enable = imx_pmx_enable,
 };
 

Quoted 15 lines

drivers/pinctrl/pinctrl-imx.c int imx_pinctrl_probe(struct platform_device *pdev, · line 635

Quoted 9 lines

drivers/pinctrl/pinctrl-imx.c +5 -0

@@ -579,6 +628,11 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
+
+	/* GPIO control functions only intended for shared mux/conf register */
+	if (info->flags & GPIO_CONTROL)
+		BUG_ON(!(info->flags & SHARE_MUX_CONF_REG));
+

If this is always true, why don't we just use flag SHARE_MUX_CONF_REG and save GPIO_CONTROL? This check doesn't make too much sense to me if we choose to have a new flag for GPIO setup. IMO, we should probably either drop the GPIO_CONTROL flag or the check.

Quoted 4 lines

Well, this is always true because the vf610 driver configures both configs. But when somebody accidentally enables GPIO_CONFIG without understanding the implications... This was more meant like "don't try to use the GPIO_CONTROL just like that, its Vybird specific".

But it will become a blocker if some day an i.MX controller (no flag SHARE_MUX_CONF_REG) needs to use GPIO_CONFIG.

Quoted 2 lines

But I'm ok to remove this runtime check, maybe a comment describing the flags is more appropriate..?

Sounds good.

Shawn

Am 2014-09-25 11:07, schrieb Shawn Guo:

Quoted 10 lines · Stefan Agner

Quoted 9 lines

Am 2014-09-25 04:47, schrieb Shawn Guo:

Quoted 8 lines · Stefan Agner

Quoted 7 lines

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 314

Quoted 3 lines

drivers/pinctrl/pinctrl-imx.c +3 -0

+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);

Isn't this setup redundant at all, since imx_pmx_enable() already takes care of setting mux register including GPIO mode?

Quoted 8 lines

Quoted 4 lines

Yes currently this is redundant, when a pinmux is actually applied. What is the expected behaviour? Is a explicit pinmux necessary before we can use GPIO? If not, maybe it would make more sense to use imx_pmx_enable here to write all pinctrl settings?

Okay, as per Documentation/pinctrl.txt, it's required that GPIO and PINCTRL can be used as orthogonal. That said, your code does the right thing. Sorry for the noisy comment.

I'm happy you came up with this, since its the thing which I'm must unsure whether this is right.

Right now, if you just export a random pin (which has no pinmux configured in dt), you get an error since this function fails (mux_reg == -1). But I guess we can't avoid it, we need the pinctrl framework to have a valid setting before we fiddle around with the pad.

But now, if there is a pinmux configuration in dt, but its not applied (using pinctrl-0 = ...), the export will succeed, however the GPIO will not really be usable since no sane pad configuration is not applied (input/output buffer disabled, no drive-strength)... So we might well just don't change the mux here too.

IMHO, fully orthogonal is not possible, since on Vybrid those two depend on each other. But in order to make it "more orthogonal", we maybe should force applying the full configuration in imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then only mux the pad to to GPIO...

What do you think?

Quoted 12 lines

Am 2014-09-25 11:07, schrieb Shawn Guo:

Quoted 10 lines · Stefan Agner

Quoted 9 lines

Am 2014-09-25 04:47, schrieb Shawn Guo:

Quoted 8 lines · Stefan Agner

Quoted 7 lines

Add pinmux support for GPIO for Vybrid (vf610) IOMUX controller. This is needed since direction configuration is not part of the GPIO module in Vybrid.

Signed-off-by Stefan Agner <[email protected]>

---

[snip]

Quoted 30 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

drivers/pinctrl/pinctrl-imx.c static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector, · line 314

Quoted 3 lines

drivers/pinctrl/pinctrl-imx.c +3 -0

+	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg &= ~(0x7 << 20);
+	writel(reg, ipctl->base + pin_reg->mux_reg);

Isn't this setup redundant at all, since imx_pmx_enable() already takes care of setting mux register including GPIO mode?

Quoted 8 lines

Quoted 4 lines

Yes currently this is redundant, when a pinmux is actually applied. What is the expected behaviour? Is a explicit pinmux necessary before we can use GPIO? If not, maybe it would make more sense to use imx_pmx_enable here to write all pinctrl settings?

Okay, as per Documentation/pinctrl.txt, it's required that GPIO and PINCTRL can be used as orthogonal. That said, your code does the right thing. Sorry for the noisy comment.

I'm happy you came up with this, since its the thing which I'm must unsure whether this is right.

Right now, if you just export a random pin (which has no pinmux configured in dt), you get an error since this function fails (mux_reg == -1). But I guess we can't avoid it, we need the pinctrl framework to have a valid setting before we fiddle around with the pad.

But now, if there is a pinmux configuration in dt, but its not applied (using pinctrl-0 = ...), the export will succeed, however the GPIO will not really be usable since no sane pad configuration is not applied (input/output buffer disabled, no drive-strength)... So we might well just don't change the mux here too.

Quoted 7 lines

IMHO, fully orthogonal is not possible, since on Vybrid those two depend on each other. But in order to make it "more orthogonal", we maybe should force applying the full configuration in imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then only mux the pad to to GPIO...

What do you think?

The muxing must have been done, that is correct. However, this could be by a boot loader, by some other API, etc. People maybe concerned about Linux affecting 'CAN' (or some 'mission critical' pins) muxing for instance, but want the A5 to handle GPIO.

If somewhere, somehow the pin was muxed properly and the GPIO code still works, I believe this is a win. I see that this flexibility makes it more difficult to get something that just works. I think the device trees take care of this for normal users. It might be appropriate to add a DT node that sets 'GPIO_CONTROL' and put a note in the DT-GPIO document, that pinctrl is needed for a normal case or if some external muxing is used or not needed, then 'control=false' can be set (or whatever good DT terminology)? Then the 'GPIO_CONTROL' check would be in the 'vf610_gpio_direction_input()' functions. The 'pinctrl' functions are currently compiled to stubs if that is configured out.

Then there is 'multi-machine' support with DT and compile time selects with CONFIG_PINCTRL and I don't think there is a lot of additional code or confusion?

Quoted 9 lines · On 25 Sep 2014, [email protected] wrote:

Quoted 7 lines

IMHO, fully orthogonal is not possible, since on Vybrid those two depend on each other. But in order to make it "more orthogonal", we maybe should force applying the full configuration in imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then only mux the pad to to GPIO...
What do you think?

Am 2014-09-25 18:43, schrieb Bill Pringlemeir:

Quoted 16 lines

The muxing must have been done, that is correct. However, this could be by a boot loader, by some other API, etc. People maybe concerned about Linux affecting 'CAN' (or some 'mission critical' pins) muxing for instance, but want the A5 to handle GPIO.

If somewhere, somehow the pin was muxed properly and the GPIO code still works, I believe this is a win. I see that this flexibility makes it more difficult to get something that just works. I think the device trees take care of this for normal users. It might be appropriate to add a DT node that sets 'GPIO_CONTROL' and put a note in the DT-GPIO document, that pinctrl is needed for a normal case or if some external muxing is used or not needed, then 'control=false' can be set (or whatever good DT terminology)? Then the 'GPIO_CONTROL' check would be in the 'vf610_gpio_direction_input()' functions. The 'pinctrl' functions are currently compiled to stubs if that is configured out.

Currently pinctrl is not optional for Vybrid (Kconfig mandates pinctrl by "select PINCTRL_VF610").

IMHO there is no real value having a way to mark a pin as "muxed externally" (control=false). If we use a pin, we should configure and mux that properly in Linux, and if we don't have valid configuration/mux information, we should not touch that pin.

In v2 of this patchset, the GPIO code only worked when there _is_ a pinctrl entry for that GPIO pin in the DT. This is because I need the register offset, which is provided by the entry itself. But the pad configuration is another point, which I did not consider in v2. But in v3, the GPIO code also writes the pad control.

Quoted 3 lines

Then there is 'multi-machine' support with DT and compile time selects with CONFIG_PINCTRL and I don't think there is a lot of additional code or confusion?

I don't understand that.

Quoted 27 lines

Quoted 8 lines · stefan at agner.ch

Quoted 7 lines

IMHO, fully orthogonal is not possible, since on Vybrid those two depend on each other. But in order to make it "more orthogonal", we maybe should force applying the full configuration in imx_pmx_gpio_request_enable (e.g. drive strenght etc), rather then only mux the pad to to GPIO...
What do you think?

Am 2014-09-25 18:43, schrieb Bill Pringlemeir:

Quoted 16 lines

The muxing must have been done, that is correct. However, this could be by a boot loader, by some other API, etc. People maybe concerned about Linux affecting 'CAN' (or some 'mission critical' pins) muxing for instance, but want the A5 to handle GPIO.

If somewhere, somehow the pin was muxed properly and the GPIO code still works, I believe this is a win. I see that this flexibility makes it more difficult to get something that just works. I think the device trees take care of this for normal users. It might be appropriate to add a DT node that sets 'GPIO_CONTROL' and put a note in the DT-GPIO document, that pinctrl is needed for a normal case or if some external muxing is used or not needed, then 'control=false' can be set (or whatever good DT terminology)? Then the 'GPIO_CONTROL' check would be in the 'vf610_gpio_direction_input()' functions. The 'pinctrl' functions are currently compiled to stubs if that is configured out.

Quoted 7 lines · stefan at agner.ch

Currently pinctrl is not optional for Vybrid (Kconfig mandates pinctrl by "select PINCTRL_VF610").

IMHO there is no real value having a way to mark a pin as "muxed externally" (control=false). If we use a pin, we should configure and mux that properly in Linux, and if we don't have valid configuration/mux information, we should not touch that pin.

I didn't mean per-pin, I meant for the GPIO controller as a whole. I was also looking at a IOMUX controlled by the M4 and/or a boot loader. I agree it is not possible with the mainline. This just makes it even less possible. This is great if we know we are going that route.

Quoted 6 lines

In v2 of this patchset, the GPIO code only worked when there _is_ a pinctrl entry for that GPIO pin in the DT. This is because I need the register offset, which is provided by the entry itself. But the pad configuration is another point, which I did not consider in v2. But in v3, the GPIO code also writes the pad control.

I was a little concerned about this as the pin may have pull-ups and pull-downs. We would not want to dynamically change this. However, I don't fully understand this path.

[snip]

Quoted 5 lines

Quoted 3 lines

Then there is 'multi-machine' support with DT and compile time selects with CONFIG_PINCTRL and I don't think there is a lot of additional code or confusion?

I don't understand that.

I was thinking of a modified Vybrid which would allow us to not select PINCTRL. As per Shawn's comment, it is suppose to be optional/orthogonal to the GPIO controller. If someone made the change to not select PINCTRL_VF610, then the GPIO code calls would currently compile to stubs. To get the same things with a multi-machine (Vybrid/Imx with/without pinctrl), then you could use the DT property which would avoid the pinctrl calls.

The more required modules we require for each feature, the harder it is to partition the AIPS peripherals between cores. It sort of makes the mainline Linux only useful for the VF5xx non-secure parts.

I guess another option is to make another pinctrl module for the Vybrid which uses some inter-core communications to alter the MUX configuration. This makes sense for the pinctrl and the CCM/clock modules. Each and every driver depends on them, so it is probably good for sanity to require that Linux has some version of these modules. If the 'M4' was to control the pins and/or clock, then some other modules could be written that makes some calls to that CPU (or world).

For other AIPS peripherals, it would be nice if they could be either configured out and/or set to disabled by the device tree.

drivers/gpio/gpio-vf610.c new file +14 -0

new file mode 100644
index 0000000..6649a13
@@ -0,0 +1,284 @@
+/*
+ * vf610 GPIO support through PORT and GPIO module
+ *
+ * Copyright (c) 2014 Toradex AG.
+ *
+ * Author: Stefan Agner <[email protected]>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

drivers/gpio/gpio-vf610.c new file +6 -0

+#define VF610_GPIO_PER_PORT		32
+
+struct vf610_gpio_port {
+	struct gpio_chip gc;
+	void __iomem *base;
+	void __iomem *gpio_base;

drivers/gpio/gpio-vf610.c new file +14 -0

+static int vf610_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct vf610_gpio_port *port;
+	struct resource *iores;
+	struct gpio_chip *gc;
+	int irq, ret;
+
+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);

drivers/gpio/gpio-vf610.c new file +53 -0

+
+	gc = &port->gc;
+	gc->of_node = np;
+	gc->dev = dev;
+	gc->label = "vf610-gpio",
+	gc->ngpio = VF610_GPIO_PER_PORT,
+	gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
+
+	gc->request = vf610_gpio_request,
+	gc->free = vf610_gpio_free,
+	gc->direction_input = vf610_gpio_direction_input,
+	gc->get = vf610_gpio_get,
+	gc->direction_output = vf610_gpio_direction_output,
+	gc->set = vf610_gpio_set,
+
+	ret = gpiochip_add(gc);
+	if (ret < 0)
+		return ret;
+
+	/* Clear the interrupt status register for all GPIO's */
+	vf610_gpio_writel(~0, port->base + PORT_ISFR);
+
+	ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
+				   handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(dev, "failed to add irqchip\n");
+		gpiochip_remove(gc);
+		return ret;
+	}
+	gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, irq,
+				     vf610_gpio_irq_handler);
+
+	return 0;
+}
+
+static struct platform_driver vf610_gpio_driver = {
+	.driver		= {
+		.name	= "gpio-vf610",
+		.owner	= THIS_MODULE,
+		.of_match_table = vf610_gpio_dt_ids,
+	},
+	.probe		= vf610_gpio_probe,
+};
+
+static int __init gpio_vf610_init(void)
+{
+	return platform_driver_register(&vf610_gpio_driver);
+}
+subsys_initcall(gpio_vf610_init);
+
+MODULE_AUTHOR("Stefan Agner <[email protected]>");
+MODULE_DESCRIPTION("Freescale VF610 GPIO");
+MODULE_LICENSE("GPL v2");

Signature · 1 line

2.1.0

Quoted 20 lines

drivers/gpio/gpio-vf610.c new file +14 -0

new file mode 100644
index 0000000..6649a13
@@ -0,0 +1,284 @@
+/*
+ * vf610 GPIO support through PORT and GPIO module
+ *
+ * Copyright (c) 2014 Toradex AG.
+ *
+ * Author: Stefan Agner <[email protected]>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

drivers/gpio/gpio-vf610.c line 24

Quoted 10 lines

drivers/gpio/gpio-vf610.c new file +10 -0

+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/bitops.h>
+#include <linux/gpio.h>

You might want to have the headers sort alphabetically.

drivers/gpio/gpio-vf610.c line 31

Quoted 7 lines

drivers/gpio/gpio-vf610.c new file +7 -0

+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <asm-generic/bug.h>

Why do we need this header?

drivers/gpio/gpio-vf610.c line 33

Quoted 2 lines

drivers/gpio/gpio-vf610.c new file +2 -0

+
+

One new line is enough.

Quoted 6 lines

drivers/gpio/gpio-vf610.c new file +6 -0

+#define VF610_GPIO_PER_PORT		32
+
+struct vf610_gpio_port {
+	struct gpio_chip gc;
+	void __iomem *base;
+	void __iomem *gpio_base;

drivers/gpio/gpio-vf610.c line 49

Quoted 10 lines

drivers/gpio/gpio-vf610.c new file +10 -0

+	u8 irqc[VF610_GPIO_PER_PORT];
+};
+
+#define GPIO_PDOR		0x00
+#define GPIO_PSOR		0x04
+#define GPIO_PCOR		0x08
+#define GPIO_PTOR		0x0c
+#define GPIO_PDIR		0x10
+
+#define PORT_PCR(n)		(n * 0x4)

s/n/(n)

...

Quoted 18 lines

drivers/gpio/gpio-vf610.c new file +18 -0

+static int vf610_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct vf610_gpio_port *port;
+	struct resource *iores;
+	struct gpio_chip *gc;
+	int irq, ret;
+
+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	port->base = devm_ioremap_resource(dev, iores);
+	if (IS_ERR(port->base))
+		return PTR_ERR(port->base);
+

drivers/gpio/gpio-vf610.c line 229

Quoted 6 lines

drivers/gpio/gpio-vf610.c new file +6 -0

+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	port->gpio_base = devm_ioremap_resource(dev, iores);
+	if (IS_ERR(port->gpio_base))
+		return PTR_ERR(port->gpio_base);
+
+	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

Why not platform_get_irq()?

drivers/gpio/gpio-vf610.c line 231

Quoted 3 lines

drivers/gpio/gpio-vf610.c new file +2 -0

+	if (irq == NO_IRQ)
+		return -ENODEV;

Instead of using NO_IRQ, we should check if irq is a negative value and propagate the value as the return, something like:

	if (irq < 0)
		return irq;

Quoted 57 lines

drivers/gpio/gpio-vf610.c new file +53 -1

+
+	gc = &port->gc;
+	gc->of_node = np;
+	gc->dev = dev;
+	gc->label = "vf610-gpio",
+	gc->ngpio = VF610_GPIO_PER_PORT,
+	gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
+
+	gc->request = vf610_gpio_request,
+	gc->free = vf610_gpio_free,
+	gc->direction_input = vf610_gpio_direction_input,
+	gc->get = vf610_gpio_get,
+	gc->direction_output = vf610_gpio_direction_output,
+	gc->set = vf610_gpio_set,
+
+	ret = gpiochip_add(gc);
+	if (ret < 0)
+		return ret;
+
+	/* Clear the interrupt status register for all GPIO's */
+	vf610_gpio_writel(~0, port->base + PORT_ISFR);
+
+	ret = gpiochip_irqchip_add(gc, &vf610_gpio_irq_chip, 0,
+				   handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(dev, "failed to add irqchip\n");
+		gpiochip_remove(gc);
+		return ret;
+	}
+	gpiochip_set_chained_irqchip(gc, &vf610_gpio_irq_chip, irq,
+				     vf610_gpio_irq_handler);
+
+	return 0;
+}
+
+static struct platform_driver vf610_gpio_driver = {
+	.driver		= {
+		.name	= "gpio-vf610",
+		.owner	= THIS_MODULE,
+		.of_match_table = vf610_gpio_dt_ids,
+	},
+	.probe		= vf610_gpio_probe,
+};
+
+static int __init gpio_vf610_init(void)
+{
+	return platform_driver_register(&vf610_gpio_driver);
+}
+subsys_initcall(gpio_vf610_init);
+
+MODULE_AUTHOR("Stefan Agner <[email protected]>");
+MODULE_DESCRIPTION("Freescale VF610 GPIO");
+MODULE_LICENSE("GPL v2");
--

2.1.0