PL061 GPIO: Changing _irq_chip_data with _irq_data for real irqs.

PATCH New Viresh KUMAR 2 participants 5 messages Apr 21, 2010 GPIO

Share & tools

Viresh KUMAR

author

2010-04-21 07:00 UTC #0 lore raw

PATCH 1/1 PL061 GPIO: Changing *_irq_chip_data with *_irq_data for real irqs.

PL061 driver is using set_irq_chip_data and get_irq_chip_data for real irq lines. It must be using *_irq_data functions instead. As chip_data is used by interrupt controllers also, which makes vic write at incorrect addresses.

Also, pl061_direction_output doesn't set value of gpio to value passed to it. Adding code to set value of GPIO pin to requrested value.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/gpio/pl061.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/gpio/pl061.c +4 -4

index 3ad1eeb..0a30124 100644
@@ -86,10 +86,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		return -EINVAL;
 
 	spin_lock_irqsave(&chip->lock, flags);
-	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
 	gpiodir = readb(chip->base + GPIODIR);
 	gpiodir |= 1 << offset;
 	writeb(gpiodir, chip->base + GPIODIR);
+	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
 	spin_unlock_irqrestore(&chip->lock, flags);
 
 	return 0;

@@ -203,7 +203,7 @@ static struct irq_chip pl061_irqchip = {
 
 static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
 {
-	struct list_head *chip_list = get_irq_chip_data(irq);
+	struct list_head *chip_list = get_irq_data(irq);
 	struct list_head *ptr;
 	struct pl061_gpio *chip;

@@ -296,9 +296,9 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 			goto iounmap;
 		}
 		INIT_LIST_HEAD(chip_list);
-		set_irq_chip_data(irq, chip_list);
+		set_irq_data(irq, chip_list);
 	} else
-		chip_list = get_irq_chip_data(irq);
+		chip_list = get_irq_data(irq);
 	list_add(&chip->list, chip_list);
 
 	for (i = 0; i < PL061_GPIO_NR; i++) {

Signature · 1 line

1.6.0.2

Baruch Siach

2010-04-21 07:37 UTC #1 lore raw

PATCH 1/1 PL061 GPIO: Changing *_irq_chip_data with *_irq_data for real irqs.

Hi Viresh,

Quoted 6 lines · Viresh KUMAR

PL061 driver is using set_irq_chip_data and get_irq_chip_data for real irq lines. It must be using *_irq_data functions instead. As chip_data is used by interrupt controllers also, which makes vic write at incorrect addresses.
Also, pl061_direction_output doesn't set value of gpio to value passed to it. Adding code to set value of GPIO pin to requrested value.

Please split this patch into two separate patches. The two fixes are unrelated to each other.

Quoted 11 lines

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/gpio/pl061.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/pl061.c +0 -0
index 3ad1eeb..0a30124 100644

drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 89

Quoted 4 lines

drivers/gpio/pl061.c +0 -1

@@ -86,10 +86,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		return -EINVAL;
 
 	spin_lock_irqsave(&chip->lock, flags);
-	writeb(!!value << offset, chip->base + (1 << (offset + 2)));

I'm still for keeping this line.

Quoted 18 lines

drivers/gpio/pl061.c +2 -1

 	gpiodir = readb(chip->base + GPIODIR);
 	gpiodir |= 1 << offset;
 	writeb(gpiodir, chip->base + GPIODIR);
+	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
 	spin_unlock_irqrestore(&chip->lock, flags);
 
 	return 0;

@@ -203,7 +203,7 @@ static struct irq_chip pl061_irqchip = {
 
 static void pl061_irq_handler(unsigned irq, struct irq_desc *desc)
 {
-	struct list_head *chip_list = get_irq_chip_data(irq);
+	struct list_head *chip_list = get_irq_data(irq);
 	struct list_head *ptr;
 	struct pl061_gpio *chip;

@@ -296,9 +296,9 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 			goto iounmap;

drivers/gpio/pl061.c static int __init pl061_probe(struct amba_device *dev, struct amba_id *id) · line 304

Quoted 10 lines

drivers/gpio/pl061.c +2 -2

 		}
 		INIT_LIST_HEAD(chip_list);
-		set_irq_chip_data(irq, chip_list);
+		set_irq_data(irq, chip_list);
 	} else
-		chip_list = get_irq_chip_data(irq);
+		chip_list = get_irq_data(irq);
 	list_add(&chip->list, chip_list);
 
 	for (i = 0; i < PL061_GPIO_NR; i++) {

baruch

Signature · 3 lines

                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -

Viresh KUMAR

author

2010-04-21 07:44 UTC #2 lore raw

PATCH 1/1 PL061 GPIO: Changing *_irq_chip_data with *_irq_data for real irqs.

Quoted 7 lines · Baruch Siach

Hi Viresh,
Quoted 4 lines · Viresh KUMAR
PL061 driver is using set_irq_chip_data and get_irq_chip_data for real irq lines. It must be using *_irq_data functions instead. As chip_data is used by interrupt controllers also, which makes vic write at incorrect addresses.

Quoted 5 lines

Quoted 2 lines
Also, pl061_direction_output doesn't set value of gpio to value passed to it. Adding code to set value of GPIO pin to requrested value.
Please split this patch into two separate patches. The two fixes are unrelated to each other.

OK.

Quoted 14 lines

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/gpio/pl061.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/gpio/pl061.c +0 -0

index 3ad1eeb..0a30124 100644
@@ -86,10 +86,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		return -EINVAL;

Quoted 4 lines

drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 89
Quoted 2 lines
drivers/gpio/pl061.c +0 -1
 	spin_lock_irqsave(&chip->lock, flags);
-	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
I'm still for keeping this line.

I will do that, but i am still not sure why should we keep it. Changing value before changing direction will have no effect. And if in future releases of pl061 IP, this bug is resolved then also the changing value after direction will be fine.
Is this fine?

Baruch Siach

2010-04-21 08:02 UTC #3 lore raw

PATCH 1/1 PL061 GPIO: Changing *_irq_chip_data with *_irq_data for real irqs.

Hi Viresh,

Quoted 35 lines · Viresh KUMAR

Quoted 7 lines · Baruch Siach
Hi Viresh,
Quoted 4 lines · Viresh KUMAR
PL061 driver is using set_irq_chip_data and get_irq_chip_data for real irq lines. It must be using *_irq_data functions instead. As chip_data is used by interrupt controllers also, which makes vic write at incorrect addresses.
Quoted 5 lines
Quoted 2 lines
Also, pl061_direction_output doesn't set value of gpio to value passed to it. Adding code to set value of GPIO pin to requrested value.
Please split this patch into two separate patches. The two fixes are unrelated to each other.
OK.
Quoted 14 lines
Quoted 14 lines
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/gpio/pl061.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/gpio/pl061.c +0 -0
index 3ad1eeb..0a30124 100644
@@ -86,10 +86,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		return -EINVAL;
 
Quoted 4 lines
drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 89
Quoted 2 lines
drivers/gpio/pl061.c +0 -1
 	spin_lock_irqsave(&chip->lock, flags);
-	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
I'm still for keeping this line.

Quoted 4 lines

I will do that, but i am still not sure why should we keep it. Changing value before changing direction will have no effect. And if in future releases of pl061 IP, this bug is resolved then also the changing value after direction will be fine.

Wrong. Changing the value after direction may cause the GPIO line to output the wrong value for a short period of time (between the first writeb() and the second). Preventing this is the whole point of having the value parameter in the .direction_output method, instead of calling gpio_direction_output() and then gpio_set_value().

baruch

Signature · 3 lines

                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Viresh KUMAR

author

2010-04-21 08:12 UTC #4 lore raw

PATCH 1/1 PL061 GPIO: Changing *_irq_chip_data with *_irq_data for real irqs.

Quoted 16 lines · Baruch Siach

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 11 lines
Quoted 9 lines
drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 89
Quoted 9 lines
drivers/gpio/pl061.c +0 -1
index 3ad1eeb..0a30124 100644
@@ -86,10 +86,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		return -EINVAL;
 
 	spin_lock_irqsave(&chip->lock, flags);
-	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
I'm still for keeping this line.
Quoted 4 lines
Quoted 4 lines
I will do that, but i am still not sure why should we keep it. Changing value before changing direction will have no effect. And if in future releases of pl061 IP, this bug is resolved then also the changing value after direction will be fine.

Quoted 5 lines

Wrong. Changing the value after direction may cause the GPIO line to output the wrong value for a short period of time (between the first writeb() and the second). Preventing this is the whole point of having the value parameter in the .direction_output method, instead of calling gpio_direction_output() and then gpio_set_value().

Thanks for clarifying.

regards,
viresh.