[PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Maulik Shah]

Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().

No functional impact.

Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 45b3a2763eb8..6771f5eb29e4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1012,10 +1012,8 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 
 static void msm_gpio_irq_eoi(struct irq_data *d)
 {
-	d = d->parent_data;
-
-	if (d)
-		d->chip->irq_eoi(d);
+	if (d->parent_data)
+		irq_chip_eoi_parent(d);
 }
 
 static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,

---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260514-pinctrl_msm_irq_eoi-ab736e16d411

Best regards,
--  
Maulik Shah <maulik.shah@oss.qualcomm.com>

Re: [PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Bjorn Andersson]

On Thu, May 14, 2026 at 02:08:25PM +0530, Maulik Shah wrote:
> Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().
> 
> No functional impact.
> 

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 45b3a2763eb8..6771f5eb29e4 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1012,10 +1012,8 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  
>  static void msm_gpio_irq_eoi(struct irq_data *d)
>  {
> -	d = d->parent_data;
> -
> -	if (d)
> -		d->chip->irq_eoi(d);
> +	if (d->parent_data)
> +		irq_chip_eoi_parent(d);
>  }
>  
>  static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> 
> ---
> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> change-id: 20260514-pinctrl_msm_irq_eoi-ab736e16d411
> 
> Best regards,
> --  
> Maulik Shah <maulik.shah@oss.qualcomm.com>
> 

Re: [PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Bjorn Andersson]

On Mon, May 18, 2026 at 10:20:55PM -0500, Bjorn Andersson wrote:
> On Thu, May 14, 2026 at 02:08:25PM +0530, Maulik Shah wrote:
> > Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().
> > 
> > No functional impact.
> > 
> 
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> 

On second though, I'm not sure I want to r-b this patch.

The commit message explains the action of the patch, not the reasons for
the patch. From the description I inferred that irq_chip_eoi_parent()
does implement what is open coded here, and a quick glance confirms
that.

I'm guessing that irq_chip_eoi_parent() didn't exist when
msm_gpio_irq_eoi() was written? Or was it not used for some reason?

> Regards,
> Bjorn
> 
> > Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 45b3a2763eb8..6771f5eb29e4 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -1012,10 +1012,8 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> >  
> >  static void msm_gpio_irq_eoi(struct irq_data *d)
> >  {
> > -	d = d->parent_data;
> > -
> > -	if (d)
> > -		d->chip->irq_eoi(d);
> > +	if (d->parent_data)

"I know that irq_chip_eoi_parent() will peak into d->parent_data, so
let's peek into the object first to avoid it dereferencing a NULL
pointer".

I see one other caller to irq_chip_eoi_parent() doing this, most
everyone else just register irq_chip_eoi_parent directly in the ops
struct.

Are we doing it right?

Regards,
Bjorn

> > +		irq_chip_eoi_parent(d);
> >  }
> >  
> >  static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> > 
> > ---
> > base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> > change-id: 20260514-pinctrl_msm_irq_eoi-ab736e16d411
> > 
> > Best regards,
> > --  
> > Maulik Shah <maulik.shah@oss.qualcomm.com>
> > 
> 

Re: [PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Maulik Shah (mkshah)]

On 5/19/2026 9:03 AM, Bjorn Andersson wrote:
> On Mon, May 18, 2026 at 10:20:55PM -0500, Bjorn Andersson wrote:
>> On Thu, May 14, 2026 at 02:08:25PM +0530, Maulik Shah wrote:
>>> Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().
>>>
>>> No functional impact.
>>>
>>
>> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
>>
> 
> On second though, I'm not sure I want to r-b this patch.
> 
> The commit message explains the action of the patch, not the reasons for
> the patch. From the description I inferred that irq_chip_eoi_parent()
> does implement what is open coded here, and a quick glance confirms
> that.
> 
> I'm guessing that irq_chip_eoi_parent() didn't exist when
> msm_gpio_irq_eoi() was written? Or was it not used for some reason?

irq_chip_eoi_parent() did exist before msm_gpio_irq_eoi() and msmgpio irqchip was using same
with a condition that pctrl->irq_chip.irq_eoi was only initialized to point irq_chip_eoi_parent()
for the cases where the gpio had a parent pdc/mpm irq number.

Later via [1] (as part to make irqchip immutable) pctrl->irq_chip.irq_eoi callback was updated to
use for each gpio interrupt and hence requiring the check like below inside eoi call to invoke only
if parent data (PDC/MPM) is valid.

   d = d->parent_data;
   if (d)
      d->chip->irq_eoi(d);

[1] https://lore.kernel.org/all/20220419141846.598305-8-maz@kernel.org/ 

> 
>> Regards,
>> Bjorn
>>
>>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>> ---
>>>  drivers/pinctrl/qcom/pinctrl-msm.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> index 45b3a2763eb8..6771f5eb29e4 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> @@ -1012,10 +1012,8 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>>>  
>>>  static void msm_gpio_irq_eoi(struct irq_data *d)
>>>  {
>>> -	d = d->parent_data;
>>> -
>>> -	if (d)
>>> -		d->chip->irq_eoi(d);
>>> +	if (d->parent_data)
> 
> "I know that irq_chip_eoi_parent() will peak into d->parent_data, so
> let's peek into the object first to avoid it dereferencing a NULL
> pointer".
> 
> I see one other caller to irq_chip_eoi_parent() doing this, most
> everyone else just register irq_chip_eoi_parent directly in the ops
> struct.
> 
> Are we doing it right?

Yes, because some gpios may not be wake up capable (they don't have their PDC / MPM interrupt)
and for such cases irq_domain_trim_hierarchy() makes irqd->parent_data = NULL, so this has to be
checked using if (d->parent_data) before invoking irq_chip_eoi_parent(), since internally
irq_chip_eoi_parent() de-references without a valid pointer check.

one can argue that the valid pointer check should be taken care inside irq_chip_eoi_parent()
(and all other irq_chip_*_parent() APIs) but i guess not all irqchip have cases where
irq_domain_trim_hierarchy() may be invoked and it would be overhead for them.

Thanks,
Maulik

> 
> Regards,
> Bjorn
> 
>>> +		irq_chip_eoi_parent(d);
>>>  }
>>>  
>>>  static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
>>>
>>> ---
>>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
>>> change-id: 20260514-pinctrl_msm_irq_eoi-ab736e16d411
>>>
>>> Best regards,
>>> --  
>>> Maulik Shah <maulik.shah@oss.qualcomm.com>
>>>
>>

Re: [PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Linus Walleij]

On Thu, May 14, 2026 at 10:38 AM Maulik Shah
<maulik.shah@oss.qualcomm.com> wrote:

> Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().
>
> No functional impact.
>
> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>

Please resend this with a commit log including all the reasoning
requested by Bjorn.

(Maybe that is already in my inbox, sorry then.)

Yours,
Linus Walleij