[PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Mario Limonciello (AMD)]

There is an irqd_to_hwirq() intended to get the hwirq number. Switch
all use to it.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v2:
 * Declare a variable on stack in all functions using irqd_to_hwirq()
---
 drivers/pinctrl/pinctrl-amd.c | 40 ++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 127eeb0104d85..2dac5c71eb008 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -383,14 +383,15 @@ static void amd_gpio_irq_enable(struct irq_data *d)
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	gpiochip_enable_irq(gc, d->hwirq);
+	gpiochip_enable_irq(gc, hwirq);
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 	pin_reg |= BIT(INTERRUPT_ENABLE_OFF);
 	pin_reg |= BIT(INTERRUPT_MASK_OFF);
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 }
 
@@ -400,15 +401,16 @@ static void amd_gpio_irq_disable(struct irq_data *d)
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 	pin_reg &= ~BIT(INTERRUPT_ENABLE_OFF);
 	pin_reg &= ~BIT(INTERRUPT_MASK_OFF);
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	gpiochip_disable_irq(gc, d->hwirq);
+	gpiochip_disable_irq(gc, hwirq);
 }
 
 static void amd_gpio_irq_mask(struct irq_data *d)
@@ -417,11 +419,12 @@ static void amd_gpio_irq_mask(struct irq_data *d)
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 	pin_reg &= ~BIT(INTERRUPT_MASK_OFF);
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 }
 
@@ -431,11 +434,12 @@ static void amd_gpio_irq_unmask(struct irq_data *d)
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 	pin_reg |= BIT(INTERRUPT_MASK_OFF);
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 }
 
@@ -446,20 +450,21 @@ static int amd_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
 	u32 wake_mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 	int err;
 
 	pm_pr_dbg("Setting wake for GPIO %lu to %s\n",
-		   d->hwirq, str_enable_disable(on));
+		   hwirq, str_enable_disable(on));
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 
 	if (on)
 		pin_reg |= wake_mask;
 	else
 		pin_reg &= ~wake_mask;
 
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	if (on)
@@ -495,9 +500,10 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	pin_reg = readl(gpio_dev->base + (d->hwirq)*4);
+	pin_reg = readl(gpio_dev->base + hwirq * 4);
 
 	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_RISING:
@@ -563,10 +569,10 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	pin_reg_irq_en = pin_reg;
 	pin_reg_irq_en |= mask;
 	pin_reg_irq_en &= ~BIT(INTERRUPT_MASK_OFF);
-	writel(pin_reg_irq_en, gpio_dev->base + (d->hwirq)*4);
-	while ((readl(gpio_dev->base + (d->hwirq)*4) & mask) != mask)
+	writel(pin_reg_irq_en, gpio_dev->base + hwirq * 4);
+	while ((readl(gpio_dev->base + hwirq * 4) & mask) != mask)
 		continue;
-	writel(pin_reg, gpio_dev->base + (d->hwirq)*4);
+	writel(pin_reg, gpio_dev->base + hwirq * 4);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	return ret;
-- 
2.43.0

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Andy Shevchenko]

On Thu, Aug 21, 2025 at 09:49:11AM -0500, Mario Limonciello (AMD) wrote:
> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
> all use to it.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

>  	pm_pr_dbg("Setting wake for GPIO %lu to %s\n",
> -		   d->hwirq, str_enable_disable(on));
> +		   hwirq, str_enable_disable(on));

Not related to this series, just wondering if it makes sense to have this to be
in IRQ chip core (OTOH, it will not tell about GPIO per se, just about an IRQ
chip).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Mario Limonciello]

On 8/21/25 10:42 AM, Andy Shevchenko wrote:
> On Thu, Aug 21, 2025 at 09:49:11AM -0500, Mario Limonciello (AMD) wrote:
>> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
>> all use to it.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> 
> ...
> 
>>   	pm_pr_dbg("Setting wake for GPIO %lu to %s\n",
>> -		   d->hwirq, str_enable_disable(on));
>> +		   hwirq, str_enable_disable(on));
> 
> Not related to this series, just wondering if it makes sense to have this to be
> in IRQ chip core (OTOH, it will not tell about GPIO per se, just about an IRQ
> chip).
> 

It might be a bit noisy to be in the core.  If it turns out that such a 
message is valuable outside of debugging pinctrl-amd, no opposition to 
moving it though in the future.

The existing pm_pr_dbg() messages used by pinctrl-amd have been 
immensely helpful to find firmware bugs.

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Linus Walleij]

On Thu, Aug 21, 2025 at 4:49 PM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:

> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
> all use to it.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>

Hm where is this supposed to be applied...
My tree?

It doesn't currently apply to devel.

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Mario Limonciello]

On 8/21/25 5:16 PM, Linus Walleij wrote:
> On Thu, Aug 21, 2025 at 4:49 PM Mario Limonciello (AMD)
> <superm1@kernel.org> wrote:
> 
>> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
>> all use to it.
>>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> 
> Hm where is this supposed to be applied...
> My tree?
> 
> It doesn't currently apply to devel.
> 
> Yours,
> Linus Walleij

It was based off the commit that you picked up earlier this week.  It 
changes lines in that based upon Andy's feedback.

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Linus Walleij]

On Fri, Aug 22, 2025 at 2:30 AM Mario Limonciello <superm1@kernel.org> wrote:
> On 8/21/25 5:16 PM, Linus Walleij wrote:
> > On Thu, Aug 21, 2025 at 4:49 PM Mario Limonciello (AMD)
> > <superm1@kernel.org> wrote:
> >
> >> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
> >> all use to it.
> >>
> >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> >
> > Hm where is this supposed to be applied...
> > My tree?
> >
> > It doesn't currently apply to devel.
> >
> > Yours,
> > Linus Walleij
>
> It was based off the commit that you picked up earlier this week.  It
> changes lines in that based upon Andy's feedback.

Aha the single line pm_pr_debug() change. I applied that in
fixes because it was just a oneliner and came with the other patch
which was a fix.

But this multiline non-regression can't go into fixes.

I will move both of these over to -next since neither is
a technical regression.

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Andy Shevchenko]

Fri, Aug 22, 2025 at 08:33:55AM +0200, Linus Walleij kirjoitti:
> On Fri, Aug 22, 2025 at 2:30 AM Mario Limonciello <superm1@kernel.org> wrote:
> > On 8/21/25 5:16 PM, Linus Walleij wrote:
> > > On Thu, Aug 21, 2025 at 4:49 PM Mario Limonciello (AMD)
> > > <superm1@kernel.org> wrote:
> > >
> > >> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
> > >> all use to it.
> > >>
> > >> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > >
> > > Hm where is this supposed to be applied...
> > > My tree?
> > >
> > > It doesn't currently apply to devel.
> > >
> > > Yours,
> > > Linus Walleij
> >
> > It was based off the commit that you picked up earlier this week.  It
> > changes lines in that based upon Andy's feedback.
> 
> Aha the single line pm_pr_debug() change. I applied that in
> fixes because it was just a oneliner and came with the other patch
> which was a fix.
> 
> But this multiline non-regression can't go into fixes.

True, but fixes may go to the next if they are needed for dependent changes.
That's why there are three ways how to handle this:
- merge fixes to devel (That's how Mark Brown does, for example)
- merge next -rcX to devel (That's how Greg KH does, for example)
- cherry-pick fixes to devel (That's how I had done, when I was PDx86 maintainer).

- (4th one) wait till next release and proceed as usual (sometimes done by some
  maintainers depending on the case at hand)

I believe we already had this discussion once or twice in the past for the
similar cases.

> I will move both of these over to -next since neither is
> a technical regression.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] pinctrl: amd: Don't access irq_data's hwirq member directly

[Mario Limonciello]

On 8/22/2025 1:33 AM, Linus Walleij wrote:
> On Fri, Aug 22, 2025 at 2:30 AM Mario Limonciello <superm1@kernel.org> wrote:
>> On 8/21/25 5:16 PM, Linus Walleij wrote:
>>> On Thu, Aug 21, 2025 at 4:49 PM Mario Limonciello (AMD)
>>> <superm1@kernel.org> wrote:
>>>
>>>> There is an irqd_to_hwirq() intended to get the hwirq number. Switch
>>>> all use to it.
>>>>
>>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>
>>> Hm where is this supposed to be applied...
>>> My tree?
>>>
>>> It doesn't currently apply to devel.
>>>
>>> Yours,
>>> Linus Walleij
>>
>> It was based off the commit that you picked up earlier this week.  It
>> changes lines in that based upon Andy's feedback.
> 
> Aha the single line pm_pr_debug() change. I applied that in
> fixes because it was just a oneliner and came with the other patch
> which was a fix.
> 
> But this multiline non-regression can't go into fixes.
> 
> I will move both of these over to -next since neither is
> a technical regression.
> 
> Yours,
> Linus Walleij

Sounds good, thanks!