diff mbox

[09/14] pinctrl/abx500: use direct IRQ defines

Message ID 1360093715-6348-10-git-send-email-linus.walleij@stericsson.com (mailing list archive)
State New, archived
Headers show

Commit Message

Linus Walleij Feb. 5, 2013, 7:48 p.m. UTC 
From: Linus Walleij <linus.walleij@linaro.org>

Make it harder to do mistakes by introducing the actual
defined ABx500 IRQ number into the IRQ cluster definitions.
Deduct cluster offset from the GPIO offset to make each
cluster coherent.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/pinctrl-ab8500.c |  6 +++---
 drivers/pinctrl/pinctrl-ab8505.c | 10 +++++-----
 drivers/pinctrl/pinctrl-ab8540.c |  6 +++---
 drivers/pinctrl/pinctrl-ab9540.c |  8 ++++----
 drivers/pinctrl/pinctrl-abx500.c |  3 +--
 5 files changed, 16 insertions(+), 17 deletions(-)

Comments

Stephen Warren Feb. 7, 2013, 12:13 a.m. UTC | #1 
On 02/05/2013 12:48 PM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Make it harder to do mistakes by introducing the actual
> defined ABx500 IRQ number into the IRQ cluster definitions.
> Deduct cluster offset from the GPIO offset to make each
> cluster coherent.

Shouldn't this patch be squashed into the previous patch to avoid churn?

>  static struct abx500_pinctrl_soc_data ab9540_soc = {

> @@ -273,8 +273,7 @@ static int abx500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)

> -			hwirq = gpio + cluster->to_irq;
> -
> +			hwirq = gpio - cluster->start + cluster->to_irq;
>  			return irq_create_mapping(pct->parent->domain, hwirq);

In particular, this change implies that the previous patch was simply
incorrect, although I haven't really thought about it in detail.
Stephen Warren Feb. 7, 2013, 5:59 p.m. UTC | #2 
On 02/07/2013 02:01 AM, Lee Jones wrote:
> I don't see myself on cc. Was that intentional?

The original patch was that way; I assume git send-email only CC'd you
on patches written by you.

> I quite like the idea of this.
> 
> Stephen,
> 
> It doesn't mean the other patch was wrong, it just transfers the math.

Ah, I see. The issue is that the code below clearly calculates the hwirq
differently, and it wasn't immediately obvious that this part of the
patch for example:

>  struct abx500_gpio_irq_cluster ab8500_gpio_irq_cluster[] = {
> -	GPIO_IRQ_CLUSTER(6,  13, 34),
> -	GPIO_IRQ_CLUSTER(24, 25, 24),
> -	GPIO_IRQ_CLUSTER(36, 41, 14),
> +	GPIO_IRQ_CLUSTER(6,  13, AB8500_INT_GPIO6R),
> +	GPIO_IRQ_CLUSTER(24, 25, AB8500_INT_GPIO24R),
> +	GPIO_IRQ_CLUSTER(36, 41, AB8500_INT_GPIO36R),
>  };

... actually changes the values in the table (AB8500_INT_GPIO6R is 40,
so when using that value, you need to subtract of the value 6 for the
base to get the original 34).

> I wouldn't squash it into mine. I like the transition and the
> possibility to revert it if there's been some mistake.
> 
> (not to say there is one, but just in case.)
> 
> Sent from my mobile Linux device.
> 
> On Feb 7, 2013 12:14 AM, "Stephen Warren" <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     On 02/05/2013 12:48 PM, Linus Walleij wrote:
>     > From: Linus Walleij <linus.walleij@linaro.org
>     <mailto:linus.walleij@linaro.org>>
>     >
>     > Make it harder to do mistakes by introducing the actual
>     > defined ABx500 IRQ number into the IRQ cluster definitions.
>     > Deduct cluster offset from the GPIO offset to make each
>     > cluster coherent.
> 
>     Shouldn't this patch be squashed into the previous patch to avoid churn?
> 
>     >  static struct abx500_pinctrl_soc_data ab9540_soc = {
> 
>     > @@ -273,8 +273,7 @@ static int abx500_gpio_to_irq(struct gpio_chip
>     *chip, unsigned offset)
> 
>     > -                     hwirq = gpio + cluster->to_irq;
>     > -
>     > +                     hwirq = gpio - cluster->start + cluster->to_irq;
>     >                       return
>     irq_create_mapping(pct->parent->domain, hwirq);
> 
>     In particular, this change implies that the previous patch was simply
>     incorrect, although I haven't really thought about it in detail.
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-kernel" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto:majordomo@vger.kernel.org>
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     Please read the FAQ at  http://www.tux.org/lkml/
>
Linus Walleij Feb. 7, 2013, 7:10 p.m. UTC | #3 
On Thu, Feb 7, 2013 at 10:01 AM, Lee Jones <lee.jones@linaro.org> wrote:

> I don't see myself on cc. Was that intentional?

No, it was my mistake...

> I quite like the idea of this.

Can I take it as an ACK? :-)

> Stephen,
>
> It doesn't mean the other patch was wrong, it just transfers the math.
>
> I wouldn't squash it into mine. I like the transition and the possibility to
> revert it if there's been some mistake.
>
> (not to say there is one, but just in case.)

I don't think it's wrong, just different. I changed
it after trying to subtract the offset in my head back
and forth a few times and screwed up the maths.

Yours,
Linus Walleij
Lee Jones Feb. 8, 2013, 8:25 a.m. UTC | #4 
On Thu, 07 Feb 2013, Stephen Warren wrote:

> On 02/07/2013 02:01 AM, Lee Jones wrote:
> > I don't see myself on cc. Was that intentional?
> 
> The original patch was that way; I assume git send-email only CC'd you
> on patches written by you.

No, I didn't send this patch at all.

I was asking Linus if he ment to CC me, as I thought I should have been.

> > I quite like the idea of this.
> > 
> > Stephen,
> > 
> > It doesn't mean the other patch was wrong, it just transfers the math.
> 
> Ah, I see. The issue is that the code below clearly calculates the hwirq
> differently, and it wasn't immediately obvious that this part of the
> patch for example:
> 
> >  struct abx500_gpio_irq_cluster ab8500_gpio_irq_cluster[] = {
> > -	GPIO_IRQ_CLUSTER(6,  13, 34),
> > -	GPIO_IRQ_CLUSTER(24, 25, 24),
> > -	GPIO_IRQ_CLUSTER(36, 41, 14),
> > +	GPIO_IRQ_CLUSTER(6,  13, AB8500_INT_GPIO6R),
> > +	GPIO_IRQ_CLUSTER(24, 25, AB8500_INT_GPIO24R),
> > +	GPIO_IRQ_CLUSTER(36, 41, AB8500_INT_GPIO36R),
> >  };
> 
> ... actually changes the values in the table (AB8500_INT_GPIO6R is 40,
> so when using that value, you need to subtract of the value 6 for the
> base to get the original 34).

Yes, I see how that may of looked if you didn't see the other change.

So you're happy?

> > I wouldn't squash it into mine. I like the transition and the
> > possibility to revert it if there's been some mistake.
> > 
> > (not to say there is one, but just in case.)
> > 
> > Sent from my mobile Linux device.
> > 
> > On Feb 7, 2013 12:14 AM, "Stephen Warren" <swarren@wwwdotorg.org
> > <mailto:swarren@wwwdotorg.org>> wrote:
> > 
> >     On 02/05/2013 12:48 PM, Linus Walleij wrote:
> >     > From: Linus Walleij <linus.walleij@linaro.org
> >     <mailto:linus.walleij@linaro.org>>
> >     >
> >     > Make it harder to do mistakes by introducing the actual
> >     > defined ABx500 IRQ number into the IRQ cluster definitions.
> >     > Deduct cluster offset from the GPIO offset to make each
> >     > cluster coherent.
> > 
> >     Shouldn't this patch be squashed into the previous patch to avoid churn?
> > 
> >     >  static struct abx500_pinctrl_soc_data ab9540_soc = {
> > 
> >     > @@ -273,8 +273,7 @@ static int abx500_gpio_to_irq(struct gpio_chip
> >     *chip, unsigned offset)
> > 
> >     > -                     hwirq = gpio + cluster->to_irq;
> >     > -
> >     > +                     hwirq = gpio - cluster->start + cluster->to_irq;
> >     >                       return
> >     irq_create_mapping(pct->parent->domain, hwirq);
> > 
> >     In particular, this change implies that the previous patch was simply
> >     incorrect, although I haven't really thought about it in detail.
> > 
>
Stephen Warren Feb. 8, 2013, 5:06 p.m. UTC | #5 
On 02/08/2013 01:25 AM, Lee Jones wrote:
> On Thu, 07 Feb 2013, Stephen Warren wrote:
> 
>> On 02/07/2013 02:01 AM, Lee Jones wrote:
>>> I don't see myself on cc. Was that intentional?
>>
>> The original patch was that way; I assume git send-email only CC'd you
>> on patches written by you.
> 
> No, I didn't send this patch at all.
> 
> I was asking Linus if he ment to CC me, as I thought I should have been.
> 
>>> I quite like the idea of this.
>>>
>>> Stephen,
>>>
>>> It doesn't mean the other patch was wrong, it just transfers the math.
>>
>> Ah, I see. The issue is that the code below clearly calculates the hwirq
>> differently, and it wasn't immediately obvious that this part of the
>> patch for example:
>>
>>>  struct abx500_gpio_irq_cluster ab8500_gpio_irq_cluster[] = {
>>> -	GPIO_IRQ_CLUSTER(6,  13, 34),
>>> -	GPIO_IRQ_CLUSTER(24, 25, 24),
>>> -	GPIO_IRQ_CLUSTER(36, 41, 14),
>>> +	GPIO_IRQ_CLUSTER(6,  13, AB8500_INT_GPIO6R),
>>> +	GPIO_IRQ_CLUSTER(24, 25, AB8500_INT_GPIO24R),
>>> +	GPIO_IRQ_CLUSTER(36, 41, AB8500_INT_GPIO36R),
>>>  };
>>
>> ... actually changes the values in the table (AB8500_INT_GPIO6R is 40,
>> so when using that value, you need to subtract of the value 6 for the
>> base to get the original 34).
> 
> Yes, I see how that may of looked if you didn't see the other change.
> 
> So you're happy?

Yes.
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-ab8500.c b/drivers/pinctrl/pinctrl-ab8500.c
index 42675ee..3b471d8 100644
--- a/drivers/pinctrl/pinctrl-ab8500.c
+++ b/drivers/pinctrl/pinctrl-ab8500.c
@@ -456,9 +456,9 @@  struct alternate_functions ab8500_alternate_functions[AB8500_GPIO_MAX_NUMBER + 1
  *	GPIO36 to GPIO41
  */
 struct abx500_gpio_irq_cluster ab8500_gpio_irq_cluster[] = {
-	GPIO_IRQ_CLUSTER(6,  13, 34),
-	GPIO_IRQ_CLUSTER(24, 25, 24),
-	GPIO_IRQ_CLUSTER(36, 41, 14),
+	GPIO_IRQ_CLUSTER(6,  13, AB8500_INT_GPIO6R),
+	GPIO_IRQ_CLUSTER(24, 25, AB8500_INT_GPIO24R),
+	GPIO_IRQ_CLUSTER(36, 41, AB8500_INT_GPIO36R),
 };
 
 static struct abx500_pinctrl_soc_data ab8500_soc = {
diff --git a/drivers/pinctrl/pinctrl-ab8505.c b/drivers/pinctrl/pinctrl-ab8505.c
index f8075c6..3a4238e 100644
--- a/drivers/pinctrl/pinctrl-ab8505.c
+++ b/drivers/pinctrl/pinctrl-ab8505.c
@@ -349,11 +349,11 @@  struct alternate_functions ab8505_alternate_functions[AB8505_GPIO_MAX_NUMBER + 1
  *	GPIO52 to GPIO53
  */
 struct abx500_gpio_irq_cluster ab8505_gpio_irq_cluster[] = {
-	GPIO_IRQ_CLUSTER(10, 11, 34),
-	GPIO_IRQ_CLUSTER(13, 13, 34),
-	GPIO_IRQ_CLUSTER(40, 41, 14),
-	GPIO_IRQ_CLUSTER(50, 50, 63),
-	GPIO_IRQ_CLUSTER(52, 53, 63),
+	GPIO_IRQ_CLUSTER(10, 11, AB8500_INT_GPIO10R),
+	GPIO_IRQ_CLUSTER(13, 13, AB8500_INT_GPIO13R),
+	GPIO_IRQ_CLUSTER(40, 41, AB8500_INT_GPIO40R),
+	GPIO_IRQ_CLUSTER(50, 50, AB9540_INT_GPIO50R),
+	GPIO_IRQ_CLUSTER(52, 53, AB9540_INT_GPIO52R),
 };
 
 static struct abx500_pinctrl_soc_data ab8505_soc = {
diff --git a/drivers/pinctrl/pinctrl-ab8540.c b/drivers/pinctrl/pinctrl-ab8540.c
index ac2e135..8ee1e8d 100644
--- a/drivers/pinctrl/pinctrl-ab8540.c
+++ b/drivers/pinctrl/pinctrl-ab8540.c
@@ -377,9 +377,9 @@  static struct pullud ab8540_pullud = {
  *	GPIO51 to GPIO54
  */
 struct abx500_gpio_irq_cluster ab8540_gpio_irq_cluster[] = {
-	GPIO_IRQ_CLUSTER(43, 43, 126),
-	GPIO_IRQ_CLUSTER(44, 44, 127),
-	GPIO_IRQ_CLUSTER(51, 54, 63),
+	GPIO_IRQ_CLUSTER(43, 43, AB8540_INT_GPIO43F),
+	GPIO_IRQ_CLUSTER(44, 44, AB8540_INT_GPIO44F),
+	GPIO_IRQ_CLUSTER(51, 54, AB9540_INT_GPIO51R),
 };
 
 static struct abx500_pinctrl_soc_data ab8540_soc = {
diff --git a/drivers/pinctrl/pinctrl-ab9540.c b/drivers/pinctrl/pinctrl-ab9540.c
index a169e5b..7610bd0 100644
--- a/drivers/pinctrl/pinctrl-ab9540.c
+++ b/drivers/pinctrl/pinctrl-ab9540.c
@@ -455,10 +455,10 @@  struct alternate_functions ab9540alternate_functions[AB9540_GPIO_MAX_NUMBER + 1]
 };
 
 struct abx500_gpio_irq_cluster ab9540_gpio_irq_cluster[] = {
-	GPIO_IRQ_CLUSTER(10, 13, 34),
-	GPIO_IRQ_CLUSTER(24, 25, 24),
-	GPIO_IRQ_CLUSTER(40, 41, 14),
-	GPIO_IRQ_CLUSTER(50, 54, 63),
+	GPIO_IRQ_CLUSTER(10, 13, AB8500_INT_GPIO10R),
+	GPIO_IRQ_CLUSTER(24, 25, AB8500_INT_GPIO24R),
+	GPIO_IRQ_CLUSTER(40, 41, AB8500_INT_GPIO40R),
+	GPIO_IRQ_CLUSTER(50, 54, AB9540_INT_GPIO50R),
 };
 
 static struct abx500_pinctrl_soc_data ab9540_soc = {
diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
index ded8f21..a0d324b 100644
--- a/drivers/pinctrl/pinctrl-abx500.c
+++ b/drivers/pinctrl/pinctrl-abx500.c
@@ -273,8 +273,7 @@  static int abx500_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 			 * To solve this quandry, we have placed the read-in values
 			 * into the cluster information table.
 			 */
-			hwirq = gpio + cluster->to_irq;
-
+			hwirq = gpio - cluster->start + cluster->to_irq;
 			return irq_create_mapping(pct->parent->domain, hwirq);
 		}
 	}