pinctrl: sunxi: misc improvements for gpio

PATCH New Chen-Yu Tsai 3 participants 12 messages Jul 14, 2014 – Jul 28, 2014

Subsystems (3)

GPIO Arm/Allwinner sunXi SoC support Pin control

lore

Share & tools

lore

Chen-Yu Tsai

author

2014-07-14 17:24 UTC #0 lore raw

cover pinctrl: sunxi: misc improvements for gpio

Hi,

These 2 patches are fixes / improvements to the gpio side of the sunxi pinctrl driver. They are based on pinctrl/devel (3a19805). The patches change the same lines of code, so they are sent together.

Patch 1 adds locking gpio lines when used as external interrupts. Similar patches were done by Linus and other maintainers for various platforms.

A somewhat related issue is that the sunxi pinctrl driver does not block users from requesting an already muxed pin as a gpio line. Maybe we should do some locking there as well? Are there any kernel interfaces for this? Or do we need to do it in the driver specifically for our hardware?

(I had the unfortunate experience of poking GPIOs listed in the fex files, not noticing they were used by the uart console.)

Patch 2 changes the gpio ranges registered by the pinctrl driver. Instead of just passing the pin number, we pass the pin offset, so that the range is not out of bounds (with respect to gpiochip.ngpio). This happens on sun6i/sun8i platforms for the R_PIO controllers.

As gpiochip.base currently matches the pin number base, we get the nice result that gpio numbers match pin numbers again.

AFAIK pinctrl pin numbers are device specific, so I'm wondering if we should also number them in terms of offsets, rather than absolute pin numbers. It's more of an asthetic change though. Any thoughts?

Thank you

Cheers
ChenYu

Chen-Yu Tsai (2):
pinctrl: sunxi: use gpiolib API to mark a GPIO used as an IRQ
pinctrl: sunxi: number gpio ranges starting from 0

 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Signature · 1 line

2.0.1

Chen-Yu Tsai

author

2014-07-14 17:24 UTC #1 lore raw

PATCH 1/2 pinctrl: sunxi: use gpiolib API to mark a GPIO used as an IRQ

When an IRQ is started on a GPIO line, mark this GPIO as IRQ in the gpiolib so we can keep track of the usage centrally.

Signed-off-by Chen-Yu Tsai <[email protected]>

---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/pinctrl/sunxi/pinctrl-sunxi.c +17 -0

index 47f5e1b..23c1b9f 100644
@@ -541,18 +541,33 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
 {
 	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 	struct sunxi_desc_function *func;
+	int ret;
 
 	func = sunxi_pinctrl_desc_find_function_by_pin(pctl,
 					pctl->irq_array[d->hwirq], "irq");
 	if (!func)
 		return -EINVAL;
 
+	ret = gpio_lock_as_irq(pctl->chip, pctl->irq_array[d->hwirq]);
+	if (ret) {
+		dev_err(pctl->dev, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return ret;
+	}
+
 	/* Change muxing to INT mode */
 	sunxi_pmx_set(pctl->pctl_dev, pctl->irq_array[d->hwirq], func->muxval);
 
 	return 0;
 }
 
+static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
+{
+	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
+
+	gpio_unlock_as_irq(pctl->chip, pctl->irq_array[d->hwirq]);
+}
+
 static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);

@@ -657,6 +672,7 @@ static struct irq_chip sunxi_pinctrl_edge_irq_chip = {
 	.irq_mask	= sunxi_pinctrl_irq_mask,
 	.irq_unmask	= sunxi_pinctrl_irq_unmask,
 	.irq_request_resources = sunxi_pinctrl_irq_request_resources,
+	.irq_release_resources = sunxi_pinctrl_irq_release_resources,
 	.irq_set_type	= sunxi_pinctrl_irq_set_type,
 	.flags		= IRQCHIP_SKIP_SET_WAKE,
 };

@@ -670,6 +686,7 @@ static struct irq_chip sunxi_pinctrl_level_irq_chip = {
 	.irq_enable	= sunxi_pinctrl_irq_ack_unmask,
 	.irq_disable	= sunxi_pinctrl_irq_mask,
 	.irq_request_resources = sunxi_pinctrl_irq_request_resources,
+	.irq_release_resources = sunxi_pinctrl_irq_release_resources,
 	.irq_set_type	= sunxi_pinctrl_irq_set_type,
 	.flags		= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_EOI_THREADED |
 			  IRQCHIP_EOI_IF_HANDLED,

Signature · 1 line

2.0.1

Maxime Ripard

2014-07-24 11:46 UTC #2 lore raw

PATCH 1/2 pinctrl: sunxi: use gpiolib API to mark a GPIO used as an IRQ

Hi Chen-Yu,

Sorry for the belated reply.

Quoted 4 lines · Chen-Yu Tsai

When an IRQ is started on a GPIO line, mark this GPIO as IRQ in the gpiolib so we can keep track of the usage centrally.
Signed-off-by Chen-Yu Tsai <[email protected]>

It looks fine:

Acked-by Maxime Ripard <[email protected]>

Thanks!
Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140724/c63c7f89/attachment.sig>

Linus Walleij

2014-07-28 10:11 UTC #3 lore raw

PATCH 1/2 pinctrl: sunxi: use gpiolib API to mark a GPIO used as an IRQ

In reply to Chen-Yu Tsai

Quoted 4 lines · Chen-Yu Tsai

When an IRQ is started on a GPIO line, mark this GPIO as IRQ in the gpiolib so we can keep track of the usage centrally.
Signed-off-by Chen-Yu Tsai <[email protected]>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij

Chen-Yu Tsai

author

2014-07-14 17:24 UTC #4 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

In reply to Chen-Yu Tsai

The pinctrl-sunxi driver originally used the pin number as the gpio range offset. This resulted in large, bogus gpio numbers for the new sun6i-a31-r pinctrl devices.

This patch makes the driver number the gpios ranges starting from an offset of 0, by subtracting the pin_base number from the pin number. This also makes the system-wide gpio number match the pin number.

Tested on sun8i with sysfs exported gpios.

Signed-off-by Chen-Yu Tsai <[email protected]>

---

This patch also changes the GPIO bindings for R_PIO:

    gpios = <&r_pio B N flag>;

Where B originally was the pinbank label (L or M) counted from A, with this patch it becomes (L or M) counted from its pinbank base (L).

Thus

    gpios = <&r_pio 10 11 0>; /* PL11 */

becomes

    gpios = <&r_pio 0 11 0>; /* PL11 */

IMO this is correct, as the binding shows the bank offset and pin offset within the bank for the GPIO controller. But I'm worried it might be a bit confusing.

---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/pinctrl/sunxi/pinctrl-sunxi.c +8 -5

index 23c1b9f..de4fb06 100644
@@ -507,7 +507,7 @@ static int sunxi_pinctrl_gpio_of_xlate(struct gpio_chip *gc,
 	base = PINS_PER_BANK * gpiospec->args[0];
 	pin = base + gpiospec->args[1];
 
-	if (pin > (gc->base + gc->ngpio))
+	if (pin > gc->ngpio)
 		return -EINVAL;
 
 	if (flags)

@@ -520,12 +520,13 @@ static int sunxi_pinctrl_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	struct sunxi_pinctrl *pctl = dev_get_drvdata(chip->dev);
 	struct sunxi_desc_function *desc;
+	unsigned pinnum = pctl->desc->pin_base + offset;
 	unsigned irqnum;
 
 	if (offset >= chip->ngpio)
 		return -ENXIO;
 
-	desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, offset, "irq");
+	desc = sunxi_pinctrl_desc_find_function_by_pin(pctl, pinnum, "irq");
 	if (!desc)
 		return -EINVAL;

@@ -548,7 +549,8 @@ static int sunxi_pinctrl_irq_request_resources(struct irq_data *d)
 	if (!func)
 		return -EINVAL;
 
-	ret = gpio_lock_as_irq(pctl->chip, pctl->irq_array[d->hwirq]);
+	ret = gpio_lock_as_irq(pctl->chip,
+			pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
 	if (ret) {
 		dev_err(pctl->dev, "unable to lock HW IRQ %lu for IRQ\n",
 			irqd_to_hwirq(d));

@@ -565,7 +567,8 @@ static void sunxi_pinctrl_irq_release_resources(struct irq_data *d)
 {
 	struct sunxi_pinctrl *pctl = irq_data_get_irq_chip_data(d);
 
-	gpio_unlock_as_irq(pctl->chip, pctl->irq_array[d->hwirq]);
+	gpio_unlock_as_irq(pctl->chip,
+			   pctl->irq_array[d->hwirq] - pctl->desc->pin_base);
 }
 
 static int sunxi_pinctrl_irq_set_type(struct irq_data *d, unsigned int type)

@@ -931,7 +934,7 @@ int sunxi_pinctrl_init(struct platform_device *pdev,
 		const struct sunxi_desc_pin *pin = pctl->desc->pins + i;
 
 		ret = gpiochip_add_pin_range(pctl->chip, dev_name(&pdev->dev),
-					     pin->pin.number,
+					     pin->pin.number - pctl->desc->pin_base,
 					     pin->pin.number, 1);
 		if (ret)
 			goto gpiochip_error;

Signature · 1 line

2.0.1

Maxime Ripard

2014-07-24 12:48 UTC #5 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

Quoted 11 lines · Chen-Yu Tsai

The pinctrl-sunxi driver originally used the pin number as the gpio range offset. This resulted in large, bogus gpio numbers for the new sun6i-a31-r pinctrl devices.
This patch makes the driver number the gpios ranges starting from an offset of 0, by subtracting the pin_base number from the pin number. This also makes the system-wide gpio number match the pin number.
Tested on sun8i with sysfs exported gpios.
Signed-off-by Chen-Yu Tsai <[email protected]>

Acked-by Maxime Ripard <[email protected]>

Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140724/3c18f510/attachment.sig>

Chen-Yu Tsai

author

2014-07-24 13:19 UTC #6 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

In reply to Chen-Yu Tsai

Hi Maxime,

Quoted 24 lines · Chen-Yu Tsai

The pinctrl-sunxi driver originally used the pin number as the gpio range offset. This resulted in large, bogus gpio numbers for the new sun6i-a31-r pinctrl devices.
This patch makes the driver number the gpios ranges starting from an offset of 0, by subtracting the pin_base number from the pin number. This also makes the system-wide gpio number match the pin number.
Tested on sun8i with sysfs exported gpios.
Signed-off-by Chen-Yu Tsai <[email protected]>
---
This patch also changes the GPIO bindings for R_PIO:
    gpios = <&r_pio B N flag>;
Where B originally was the pinbank label (L or M) counted from A, with this patch it becomes (L or M) counted from its pinbank base (L).
Thus
    gpios = <&r_pio 10 11 0>; /* PL11 */

Quoted 7 lines

becomes
    gpios = <&r_pio 0 11 0>; /* PL11 */
IMO this is correct, as the binding shows the bank offset and pin offset within the bank for the GPIO controller. But I'm worried it might be a bit confusing.

I see you Acked this patch, but also in your reply to my cover letter, you mentioned that you want absolute pin numbers matching the datasheets. What about the GPIO DT bindings, as I explained above?

Just double checking. Thanks.

ChenYu

Maxime Ripard

2014-07-24 16:01 UTC #7 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

Quoted 19 lines · Chen-Yu Tsai

Quoted 11 lines
This patch also changes the GPIO bindings for R_PIO:
    gpios = <&r_pio B N flag>;
Where B originally was the pinbank label (L or M) counted from A, with this patch it becomes (L or M) counted from its pinbank base (L).
Thus
    gpios = <&r_pio 10 11 0>; /* PL11 */
Quoted 7 lines
becomes
    gpios = <&r_pio 0 11 0>; /* PL11 */
IMO this is correct, as the binding shows the bank offset and pin offset within the bank for the GPIO controller. But I'm worried it might be a bit confusing.

Quoted 5 lines

I see you Acked this patch, but also in your reply to my cover letter, you mentioned that you want absolute pin numbers matching the datasheets. What about the GPIO DT bindings, as I explained above?
Just double checking. Thanks.

I'd like it to have the absolute numbers in sysfs, but the relative one in the DT. But I guess it's already what's happening, right?

Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140724/ff067b71/attachment.sig>

Chen-Yu Tsai

author

2014-07-24 16:17 UTC #8 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

Quoted 20 lines · Maxime Ripard

Quoted 19 lines · Chen-Yu Tsai
Quoted 18 lines
This patch also changes the GPIO bindings for R_PIO:
    gpios = <&r_pio B N flag>;
Where B originally was the pinbank label (L or M) counted from A, with this patch it becomes (L or M) counted from its pinbank base (L).
Thus
    gpios = <&r_pio 10 11 0>; /* PL11 */
becomes
    gpios = <&r_pio 0 11 0>; /* PL11 */
IMO this is correct, as the binding shows the bank offset and pin offset within the bank for the GPIO controller. But I'm worried it might be a bit confusing.

Quoted 8 lines

Quoted 5 lines
I see you Acked this patch, but also in your reply to my cover letter, you mentioned that you want absolute pin numbers matching the datasheets. What about the GPIO DT bindings, as I explained above?
Just double checking. Thanks.
I'd like it to have the absolute numbers in sysfs, but the relative one in the DT. But I guess it's already what's happening, right?

That's right. Just checking. :)

ChenYu

Linus Walleij

2014-07-28 10:14 UTC #9 lore raw

PATCH 2/2 pinctrl: sunxi: number gpio ranges starting from 0

In reply to Chen-Yu Tsai

Quoted 11 lines · Chen-Yu Tsai

The pinctrl-sunxi driver originally used the pin number as the gpio range offset. This resulted in large, bogus gpio numbers for the new sun6i-a31-r pinctrl devices.
This patch makes the driver number the gpios ranges starting from an offset of 0, by subtracting the pin_base number from the pin number. This also makes the system-wide gpio number match the pin number.
Tested on sun8i with sysfs exported gpios.
Signed-off-by Chen-Yu Tsai <[email protected]>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij

Linus Walleij

2014-07-22 16:33 UTC #10 lore raw

cover pinctrl: sunxi: misc improvements for gpio

In reply to Chen-Yu Tsai

I'm waiting for Maxime to look at these patches before merging.

Quoted 7 lines · Chen-Yu Tsai

These 2 patches are fixes / improvements to the gpio side of the sunxi pinctrl driver. They are based on pinctrl/devel (3a19805). The patches change the same lines of code, so they are sent together.
Patch 1 adds locking gpio lines when used as external interrupts. Similar patches were done by Linus and other maintainers for various platforms.

Nice.

Quoted 8 lines

A somewhat related issue is that the sunxi pinctrl driver does not block users from requesting an already muxed pin as a gpio line. Maybe we should do some locking there as well? Are there any kernel interfaces for this? Or do we need to do it in the driver specifically for our hardware?
(I had the unfortunate experience of poking GPIOs listed in the fex files, not noticing they were used by the uart console.)

The pin control core explicitly allows concurrent muxing to some certain function and GPIO usage (see Documentation/pinctrl.txt).

This is because some HW may allow you to e.g. "spy" on pins using it's GPIO registers, or use it from the GPIO subsystem at the same time some other way.

If this does not apply to your hardware, you can further restrictions in the driver, for example pinctrl_request_gpio() can return something negative from the pin control part of the driver, like -EBUSY.

Quoted 3 lines

AFAIK pinctrl pin numbers are device specific, so I'm wondering if we should also number them in terms of offsets, rather than absolute pin numbers. It's more of an asthetic change though. Any thoughts?

Usually I say these pin numbers should try to match what is in the data sheet so it is easy to understand and debug. Sometimes pins are numbered with letters and stuff so they rather have names than numbers, then some artificial numbering is applied, whatever is helpful.

I'm not directly familiar with the sunxi numbering system though...

Yours,
Linus Walleij

Maxime Ripard

2014-07-24 12:57 UTC #11 lore raw

cover pinctrl: sunxi: misc improvements for gpio

Quoted 11 lines · Linus Walleij

Quoted 3 lines
AFAIK pinctrl pin numbers are device specific, so I'm wondering if we should also number them in terms of offsets, rather than absolute pin numbers. It's more of an asthetic change though. Any thoughts?
Usually I say these pin numbers should try to match what is in the data sheet so it is easy to understand and debug. Sometimes pins are numbered with letters and stuff so they rather have names than numbers, then some artificial numbering is applied, whatever is helpful.
I'm not directly familiar with the sunxi numbering system though...

I'd prefer if we had an absolute number that more or less matches the one in the datasheet.

Maxime

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140724/ed4c4083/attachment.sig>