2015-07-31 12:49:26

by Rabin Vincent

[permalink] [raw]
Subject: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

If the driver has specified its own irq_{request/release}_resources()
functions, don't override them. The gpio-etraxfs driver will use this.

Signed-off-by: Rabin Vincent <[email protected]>
---
drivers/gpio/gpiolib.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d..6865874 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -636,8 +636,12 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
gpiochip->irqchip = NULL;
return -EINVAL;
}
- irqchip->irq_request_resources = gpiochip_irq_reqres;
- irqchip->irq_release_resources = gpiochip_irq_relres;
+
+ if (!irqchip->irq_request_resources &&
+ !irqchip->irq_release_resources) {
+ irqchip->irq_request_resources = gpiochip_irq_reqres;
+ irqchip->irq_release_resources = gpiochip_irq_relres;
+ }

/*
* Prepare the mapping since the irqchip shall be orthogonal to
--
2.1.4


2015-07-31 12:49:24

by Rabin Vincent

[permalink] [raw]
Subject: [PATCH 2/2] gpio: etraxfs: add interrupt support

On ETRAX FS, all pins on the first port (and only the first port) have
interrupt support.

On ARTPEC-3, all pins on all ports have interrupt support. However,
there are only eight interrupts. Each of the interrupts is associated
with a group of pins and for each interrupt the one pin from the group
which will trigger it can be selected.

Signed-off-by: Rabin Vincent <[email protected]>
---
drivers/gpio/Kconfig | 1 +
drivers/gpio/gpio-etraxfs.c | 259 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 253 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8f1fe73..c0f176a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -172,6 +172,7 @@ config GPIO_ETRAXFS
depends on CRIS || COMPILE_TEST
depends on OF
select GPIO_GENERIC
+ select GPIOLIB_IRQCHIP
help
Say yes here to support the GPIO controller on Axis ETRAX FS SoCs.

diff --git a/drivers/gpio/gpio-etraxfs.c b/drivers/gpio/gpio-etraxfs.c
index 7276891..92e065e 100644
--- a/drivers/gpio/gpio-etraxfs.c
+++ b/drivers/gpio/gpio-etraxfs.c
@@ -1,8 +1,10 @@
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
#include <linux/of_gpio.h>
#include <linux/io.h>
+#include <linux/interrupt.h>
#include <linux/platform_device.h>
#include <linux/basic_mmio_gpio.h>

@@ -13,6 +15,7 @@
#define ETRAX_FS_rw_intr_mask 16
#define ETRAX_FS_rw_ack_intr 20
#define ETRAX_FS_r_intr 24
+#define ETRAX_FS_r_masked_intr 28
#define ETRAX_FS_rw_pb_dout 32
#define ETRAX_FS_r_pb_din 36
#define ETRAX_FS_rw_pb_oe 40
@@ -36,6 +39,37 @@
#define ARTPEC3_rw_pc_dout 92
#define ARTPEC3_rw_pc_oe 96
#define ARTPEC3_r_pd_din 116
+#define ARTPEC3_rw_intr_cfg 120
+#define ARTPEC3_rw_intr_pins 124
+#define ARTPEC3_rw_intr_mask 128
+#define ARTPEC3_rw_ack_intr 132
+#define ARTPEC3_r_masked_intr 140
+
+#define GIO_CFG_OFF 0
+#define GIO_CFG_HI 1
+#define GIO_CFG_LO 2
+#define GIO_CFG_SET 3
+#define GIO_CFG_POSEDGE 5
+#define GIO_CFG_NEGEDGE 6
+#define GIO_CFG_ANYEDGE 7
+
+struct etraxfs_gpio_info;
+
+struct etraxfs_gpio_block {
+ spinlock_t lock;
+ u32 mask;
+ u32 cfg;
+ u32 pins;
+ unsigned int group[8];
+
+ void __iomem *regs;
+ const struct etraxfs_gpio_info *info;
+};
+
+struct etraxfs_gpio_chip {
+ struct bgpio_chip bgc;
+ struct etraxfs_gpio_block *block;
+};

struct etraxfs_gpio_port {
const char *label;
@@ -48,6 +82,12 @@ struct etraxfs_gpio_port {
struct etraxfs_gpio_info {
unsigned int num_ports;
const struct etraxfs_gpio_port *ports;
+
+ unsigned int rw_ack_intr;
+ unsigned int rw_intr_mask;
+ unsigned int rw_intr_cfg;
+ unsigned int rw_intr_pins;
+ unsigned int r_masked_intr;
};

static const struct etraxfs_gpio_port etraxfs_gpio_etraxfs_ports[] = {
@@ -91,6 +131,10 @@ static const struct etraxfs_gpio_port etraxfs_gpio_etraxfs_ports[] = {
static const struct etraxfs_gpio_info etraxfs_gpio_etraxfs = {
.num_ports = ARRAY_SIZE(etraxfs_gpio_etraxfs_ports),
.ports = etraxfs_gpio_etraxfs_ports,
+ .rw_ack_intr = ETRAX_FS_rw_ack_intr,
+ .rw_intr_mask = ETRAX_FS_rw_intr_mask,
+ .rw_intr_cfg = ETRAX_FS_rw_intr_cfg,
+ .r_masked_intr = ETRAX_FS_r_masked_intr,
};

static const struct etraxfs_gpio_port etraxfs_gpio_artpec3_ports[] = {
@@ -125,8 +169,18 @@ static const struct etraxfs_gpio_port etraxfs_gpio_artpec3_ports[] = {
static const struct etraxfs_gpio_info etraxfs_gpio_artpec3 = {
.num_ports = ARRAY_SIZE(etraxfs_gpio_artpec3_ports),
.ports = etraxfs_gpio_artpec3_ports,
+ .rw_ack_intr = ARTPEC3_rw_ack_intr,
+ .rw_intr_mask = ARTPEC3_rw_intr_mask,
+ .rw_intr_cfg = ARTPEC3_rw_intr_cfg,
+ .r_masked_intr = ARTPEC3_r_masked_intr,
+ .rw_intr_pins = ARTPEC3_rw_intr_pins,
};

+static unsigned int etraxfs_gpio_chip_to_port(struct gpio_chip *gc)
+{
+ return gc->label[0] - 'A';
+}
+
static int etraxfs_gpio_of_xlate(struct gpio_chip *gc,
const struct of_phandle_args *gpiospec,
u32 *flags)
@@ -135,7 +189,7 @@ static int etraxfs_gpio_of_xlate(struct gpio_chip *gc,
* Port numbers are A to E, and the properties are integers, so we
* specify them as 0xA - 0xE.
*/
- if (gc->label[0] - 'A' + 0xA != gpiospec->args[2])
+ if (etraxfs_gpio_chip_to_port(gc) + 0xA != gpiospec->args[2])
return -EINVAL;

return of_gpio_simple_xlate(gc, gpiospec, flags);
@@ -153,13 +207,159 @@ static const struct of_device_id etraxfs_gpio_of_table[] = {
{},
};

+static unsigned int etraxfs_gpio_to_group_irq(unsigned int gpio)
+{
+ return gpio % 8;
+}
+
+static unsigned int etraxfs_gpio_to_group_pin(struct etraxfs_gpio_chip *chip,
+ unsigned int gpio)
+{
+ return 4 * etraxfs_gpio_chip_to_port(&chip->bgc.gc) + gpio / 8;
+}
+
+static void etraxfs_gpio_irq_ack(struct irq_data *d)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+
+ writel(BIT(grpirq), block->regs + block->info->rw_ack_intr);
+}
+
+static void etraxfs_gpio_irq_mask(struct irq_data *d)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+
+ spin_lock(&block->lock);
+ block->mask &= ~BIT(grpirq);
+ writel(block->mask, block->regs + block->info->rw_intr_mask);
+ spin_unlock(&block->lock);
+}
+
+static void etraxfs_gpio_irq_unmask(struct irq_data *d)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+
+ spin_lock(&block->lock);
+ block->mask |= BIT(grpirq);
+ writel(block->mask, block->regs + block->info->rw_intr_mask);
+ spin_unlock(&block->lock);
+}
+
+static int etraxfs_gpio_irq_set_type(struct irq_data *d, u32 type)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+ u32 cfg;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ cfg = GIO_CFG_POSEDGE;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ cfg = GIO_CFG_NEGEDGE;
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ cfg = GIO_CFG_ANYEDGE;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ cfg = GIO_CFG_LO;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ cfg = GIO_CFG_HI;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock(&block->lock);
+ block->cfg &= ~(0x7 << (grpirq * 3));
+ block->cfg |= (cfg << (grpirq * 3));
+ writel(block->cfg, block->regs + block->info->rw_intr_cfg);
+ spin_unlock(&block->lock);
+
+ return 0;
+}
+
+static int etraxfs_gpio_irq_request_resources(struct irq_data *d)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+ int ret = -EBUSY;
+
+ spin_lock(&block->lock);
+ if (block->group[grpirq])
+ goto out;
+
+ ret = gpiochip_lock_as_irq(&chip->bgc.gc, d->hwirq);
+ if (ret)
+ goto out;
+
+ block->group[grpirq] = d->irq;
+ if (block->info->rw_intr_pins) {
+ unsigned int pin = etraxfs_gpio_to_group_pin(chip, d->hwirq);
+
+ block->pins &= ~(0xf << (grpirq * 4));
+ block->pins |= (pin << (grpirq * 4));
+
+ writel(block->pins, block->regs + block->info->rw_intr_pins);
+ }
+
+out:
+ spin_unlock(&block->lock);
+ return ret;
+}
+
+static void etraxfs_gpio_irq_release_resources(struct irq_data *d)
+{
+ struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct etraxfs_gpio_block *block = chip->block;
+ unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
+
+ spin_lock(&block->lock);
+ block->group[grpirq] = 0;
+ gpiochip_unlock_as_irq(&chip->bgc.gc, d->hwirq);
+ spin_unlock(&block->lock);
+}
+
+static struct irq_chip etraxfs_gpio_irq_chip = {
+ .name = "gpio-etraxfs",
+ .irq_ack = etraxfs_gpio_irq_ack,
+ .irq_mask = etraxfs_gpio_irq_mask,
+ .irq_unmask = etraxfs_gpio_irq_unmask,
+ .irq_set_type = etraxfs_gpio_irq_set_type,
+ .irq_request_resources = etraxfs_gpio_irq_request_resources,
+ .irq_release_resources = etraxfs_gpio_irq_release_resources,
+};
+
+static irqreturn_t etraxfs_gpio_interrupt(int irq, void *dev_id)
+{
+ struct etraxfs_gpio_block *block = dev_id;
+ unsigned long intr = readl(block->regs + block->info->r_masked_intr);
+ int bit;
+
+ for_each_set_bit(bit, &intr, 8)
+ generic_handle_irq(block->group[bit]);
+
+ return IRQ_RETVAL(intr & 0xff);
+}
+
static int etraxfs_gpio_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
const struct etraxfs_gpio_info *info;
const struct of_device_id *match;
- struct bgpio_chip *chips;
- struct resource *res;
+ struct etraxfs_gpio_block *block;
+ struct etraxfs_gpio_chip *chips;
+ struct resource *res, *irq;
+ bool allportsirq = false;
void __iomem *regs;
int ret;
int i;
@@ -179,14 +379,44 @@ static int etraxfs_gpio_probe(struct platform_device *pdev)
if (!chips)
return -ENOMEM;

+ irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!irq)
+ return -EINVAL;
+
+ block = devm_kzalloc(dev, sizeof(*block), GFP_KERNEL);
+ if (!block)
+ return -ENOMEM;
+
+ spin_lock_init(&block->lock);
+
+ block->regs = regs;
+ block->info = info;
+
+ writel(0, block->regs + info->rw_intr_mask);
+ writel(0, block->regs + info->rw_intr_cfg);
+ if (info->rw_intr_pins) {
+ allportsirq = true;
+ writel(0, block->regs + info->rw_intr_pins);
+ }
+
+ ret = devm_request_irq(dev, irq->start, etraxfs_gpio_interrupt,
+ IRQF_SHARED, dev_name(dev), block);
+ if (ret) {
+ dev_err(dev, "Unable to request irq %d\n", ret);
+ return ret;
+ }
+
for (i = 0; i < info->num_ports; i++) {
- struct bgpio_chip *bgc = &chips[i];
+ struct etraxfs_gpio_chip *chip = &chips[i];
+ struct bgpio_chip *bgc = &chip->bgc;
const struct etraxfs_gpio_port *port = &info->ports[i];
unsigned long flags = BGPIOF_READ_OUTPUT_REG_SET;
void __iomem *dat = regs + port->din;
void __iomem *set = regs + port->dout;
void __iomem *dirout = regs + port->oe;

+ chip->block = block;
+
if (dirout == set) {
dirout = set = NULL;
flags = BGPIOF_NO_OUTPUT;
@@ -195,8 +425,11 @@ static int etraxfs_gpio_probe(struct platform_device *pdev)
ret = bgpio_init(bgc, dev, 4,
dat, set, NULL, dirout, NULL,
flags);
- if (ret)
- return ret;
+ if (ret) {
+ dev_err(dev, "Unable to init port %s\n",
+ port->label);
+ continue;
+ }

bgc->gc.ngpio = port->ngpio;
bgc->gc.label = port->label;
@@ -206,9 +439,21 @@ static int etraxfs_gpio_probe(struct platform_device *pdev)
bgc->gc.of_xlate = etraxfs_gpio_of_xlate;

ret = gpiochip_add(&bgc->gc);
- if (ret)
+ if (ret) {
dev_err(dev, "Unable to register port %s\n",
bgc->gc.label);
+ continue;
+ }
+
+ if (i > 0 && !allportsirq)
+ continue;
+
+ ret = gpiochip_irqchip_add(&bgc->gc, &etraxfs_gpio_irq_chip, 0,
+ handle_level_irq, IRQ_TYPE_NONE);
+ if (ret) {
+ dev_err(dev, "Unable to add irqchip to port %s\n",
+ bgc->gc.label);
+ }
}

return 0;
--
2.1.4

2015-07-31 14:54:34

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

On 07/31/2015 03:48 PM, Rabin Vincent wrote:
> If the driver has specified its own irq_{request/release}_resources()
> functions, don't override them. The gpio-etraxfs driver will use this.
>
> Signed-off-by: Rabin Vincent <[email protected]>
> ---
> drivers/gpio/gpiolib.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bf4bd1d..6865874 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -636,8 +636,12 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> gpiochip->irqchip = NULL;
> return -EINVAL;
> }
> - irqchip->irq_request_resources = gpiochip_irq_reqres;
> - irqchip->irq_release_resources = gpiochip_irq_relres;
> +
> + if (!irqchip->irq_request_resources &&
> + !irqchip->irq_release_resources) {
> + irqchip->irq_request_resources = gpiochip_irq_reqres;
> + irqchip->irq_release_resources = gpiochip_irq_relres;
> + }

I think, it will be better to handle req/rel cases separately.

>
> /*
> * Prepare the mapping since the irqchip shall be orthogonal to
>

Nice, thanks. I need the same actually, but I propose to make
gpiochip_irq_reqres/gpiochip_irq_relres public also, so drivers can re-use them.

--
regards,
-grygorii

2015-08-03 08:53:51

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:

> If the driver has specified its own irq_{request/release}_resources()
> functions, don't override them. The gpio-etraxfs driver will use this.
>
> Signed-off-by: Rabin Vincent <[email protected]>

Perfectly reasonable given the usecase in patch 2/2. Patch applied.

Yours,
Linus Walleij

2015-08-03 08:56:20

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

On Fri, Jul 31, 2015 at 4:54 PM, Grygorii Strashko
<[email protected]> wrote:
> On 07/31/2015 03:48 PM, Rabin Vincent wrote:

>> + if (!irqchip->irq_request_resources &&
>> + !irqchip->irq_release_resources) {
>> + irqchip->irq_request_resources = gpiochip_irq_reqres;
>> + irqchip->irq_release_resources = gpiochip_irq_relres;
>> + }
>
> I think, it will be better to handle req/rel cases separately.

No, I think that could be dangerous. The semantics of the both
functions are intertwined, if we change something in the core
we may break drivers.

It would be better with a mechanism saying "also do this
on irq_request/release resource" so a secondary vtable
for these two. Where the latter would be optional per-callback.

That way the ETRAXFS does not need to reimplement
irq locking.

I'll see what I can come up with.

Yours,
Linus Walleij

2015-08-03 08:58:41

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/2] gpio: etraxfs: add interrupt support

On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:

> On ETRAX FS, all pins on the first port (and only the first port) have
> interrupt support.
>
> On ARTPEC-3, all pins on all ports have interrupt support. However,
> there are only eight interrupts. Each of the interrupts is associated
> with a group of pins and for each interrupt the one pin from the group
> which will trigger it can be selected.
>
> Signed-off-by: Rabin Vincent <[email protected]>

Patch applied, kudos for using the IRQ helpers!

> +static int etraxfs_gpio_irq_request_resources(struct irq_data *d)
> +{
> + struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + struct etraxfs_gpio_block *block = chip->block;
> + unsigned int grpirq = etraxfs_gpio_to_group_irq(d->hwirq);
> + int ret = -EBUSY;
> +
> + spin_lock(&block->lock);
> + if (block->group[grpirq])
> + goto out;
> +
> + ret = gpiochip_lock_as_irq(&chip->bgc.gc, d->hwirq);
> + if (ret)
> + goto out;

Some duplicate code for locking the irqs, so I'll see if I can
refactor something.

Yours,
Linus Walleij

2015-08-03 09:34:37

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/2] gpio: etraxfs: add interrupt support

On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:

> On ETRAX FS, all pins on the first port (and only the first port) have
> interrupt support.
>
> On ARTPEC-3, all pins on all ports have interrupt support. However,
> there are only eight interrupts. Each of the interrupts is associated
> with a group of pins and for each interrupt the one pin from the group
> which will trigger it can be selected.
>
> Signed-off-by: Rabin Vincent <[email protected]>

Hm wait now I get confused ... probably tripping over my own shoelaces
as usual but help me here:

> +static void etraxfs_gpio_irq_ack(struct irq_data *d)
> +{
> + struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);

I don't see how this works in the irqchip functions.

Usually the chip data is the struct gpio_chip when using the
GPIOLIB_IRQCHIP, then we use some container_of to boil
out the containing struct, like:

static inline struct extraxfs_gpio_chip *to_etraxfs(struct gpio_chip *chip)
{
return container_of(chip, struct etraxfs_gpio_chip, bgc.gc);
}

So it would be:

struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct etraxfs_gpio_chip *chip = to_etraxfs(gc);

so how did you manage to replace that pointer with a pointer
to your struct etraxfs_gpio_chip?

> + ret = gpiochip_irqchip_add(&bgc->gc, &etraxfs_gpio_irq_chip, 0,
> + handle_level_irq, IRQ_TYPE_NONE);

Because this sets the irqdomain host_data to gc, then the
irqdomain .map function sets the chip data to the same.

Yours,
Linus Walleij

2015-08-03 17:30:49

by Rabin Vincent

[permalink] [raw]
Subject: Re: [PATCH 2/2] gpio: etraxfs: add interrupt support

On Mon, Aug 03, 2015 at 11:34:23AM +0200, Linus Walleij wrote:
> On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:
> > +static void etraxfs_gpio_irq_ack(struct irq_data *d)
> > +{
> > + struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> I don't see how this works in the irqchip functions.

The offset of the gpio_chip embedded in etraxfs_gpio_chip is zero.
There are a couple of other gpio drivers doing it the same way, but
container_of() would certainly be more robust.

2015-08-13 08:57:53

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/2] gpio: etraxfs: add interrupt support

On Mon, Aug 3, 2015 at 7:30 PM, Rabin Vincent <[email protected]> wrote:
> On Mon, Aug 03, 2015 at 11:34:23AM +0200, Linus Walleij wrote:
>> On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:
>> > +static void etraxfs_gpio_irq_ack(struct irq_data *d)
>> > +{
>> > + struct etraxfs_gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> I don't see how this works in the irqchip functions.
>
> The offset of the gpio_chip embedded in etraxfs_gpio_chip is zero.
> There are a couple of other gpio drivers doing it the same way, but
> container_of() would certainly be more robust.

Aha hehe yeah that seems a bit fragile, I should take a sweep and
fix them all I guess.

Yours,
Linus Walleij

2015-08-17 08:40:11

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

On Mon, Aug 3, 2015 at 10:53 AM, Linus Walleij <[email protected]> wrote:
> On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:
>
>> If the driver has specified its own irq_{request/release}_resources()
>> functions, don't override them. The gpio-etraxfs driver will use this.
>>
>> Signed-off-by: Rabin Vincent <[email protected]>
>
> Perfectly reasonable given the usecase in patch 2/2. Patch applied.

So for drivers currently using GPIOLIB_IRQCHIP the calbacks were
always overridden, even if they supplied their own?

Hence after this change, that's no longer the case, and pinctrl-at91.c
will use its own, which are identical to the generic ones, modulo the bug fix
in 5b76e79c77264899 ("gpiolib: irqchip: prevent driver unloading if gpio is
used as irq only"). Oops...

I already wrote an untested patch to convert pinctrl-at91 to the generic
ones, shall I send that right away?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2015-08-17 13:23:26

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/2] gpio: don't override irq_*_resources() callbacks

On Mon, Aug 17, 2015 at 10:40 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Mon, Aug 3, 2015 at 10:53 AM, Linus Walleij <[email protected]> wrote:
>> On Fri, Jul 31, 2015 at 2:48 PM, Rabin Vincent <[email protected]> wrote:
>>
>>> If the driver has specified its own irq_{request/release}_resources()
>>> functions, don't override them. The gpio-etraxfs driver will use this.
>>>
>>> Signed-off-by: Rabin Vincent <[email protected]>
>>
>> Perfectly reasonable given the usecase in patch 2/2. Patch applied.
>
> So for drivers currently using GPIOLIB_IRQCHIP the calbacks were
> always overridden, even if they supplied their own?

Yeah, I guess we just assumed noone supplied their own and
expected them to be nulled out.

> Hence after this change, that's no longer the case, and pinctrl-at91.c
> will use its own, which are identical to the generic ones, modulo the bug fix
> in 5b76e79c77264899 ("gpiolib: irqchip: prevent driver unloading if gpio is
> used as irq only"). Oops...
>
> I already wrote an untested patch to convert pinctrl-at91 to the generic
> ones, shall I send that right away?

Please test it first, but yeah :)

Yours,
Linus Walleij