[PATCH 0/2] pinctrl: sunxi: misc improvements for gpio

[PATCH 0/2] pinctrl: sunxi: misc improvements for gpio

[Chen-Yu Tsai]

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(-)

-- 
2.0.1

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

[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 <wens@csie.org>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 47f5e1b..23c1b9f 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -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,
-- 
2.0.1

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

[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 <wens@csie.org>
---

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(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 23c1b9f..de4fb06 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -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;
-- 
2.0.1

[PATCH 0/2] pinctrl: sunxi: misc improvements for gpio

[Linus Walleij]

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

On Mon, Jul 14, 2014 at 7:24 PM, Chen-Yu Tsai <wens@csie.org> wrote:

> 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.

> 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.

> 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

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

[Maxime Ripard]

Hi Chen-Yu,

Sorry for the belated reply.

On Tue, Jul 15, 2014 at 01:24:36AM +0800, Chen-Yu Tsai wrote:
> 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 <wens@csie.org>

It looks fine:
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
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>

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

[Maxime Ripard]

On Tue, Jul 15, 2014 at 01:24:37AM +0800, Chen-Yu Tsai wrote:
> 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 <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
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>

[PATCH 0/2] pinctrl: sunxi: misc improvements for gpio

[Maxime Ripard]

On Tue, Jul 22, 2014 at 06:33:10PM +0200, Linus Walleij wrote:
> > 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

-- 
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>

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

[Chen-Yu Tsai]

Hi Maxime,

On Tue, Jul 15, 2014 at 1:24 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> 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 <wens@csie.org>
> ---
>
> 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.

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

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

[Maxime Ripard]

On Thu, Jul 24, 2014 at 09:19:18PM +0800, Chen-Yu Tsai wrote:
> > 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.
> 
> 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

-- 
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>

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

[Chen-Yu Tsai]

On Fri, Jul 25, 2014 at 12:01 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Jul 24, 2014 at 09:19:18PM +0800, Chen-Yu Tsai wrote:
>> > 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.
>>
>> 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

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

[Linus Walleij]

On Mon, Jul 14, 2014 at 7:24 PM, Chen-Yu Tsai <wens@csie.org> wrote:

> 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 <wens@csie.org>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Jul 14, 2014 at 7:24 PM, Chen-Yu Tsai <wens@csie.org> wrote:

> 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 <wens@csie.org>

Patch applied with Maxime's ACK.

Yours,
Linus Walleij