Re: [PATCH 04/35] irqchip/qcom-pdc: Replace pdc_version global with a function pointer

[Mukesh Ojha]

On Fri, Apr 10, 2026 at 09:43:10PM -0500, Bjorn Andersson wrote:
> On Sat, Apr 11, 2026 at 12:10:41AM +0530, Mukesh Ojha wrote:
> > Now that the two enable paths are separate functions, replace the
> > pdc_version global with a __pdc_enable_intr function pointer. The
> > pointer is assigned once at probe time based on the version register,
> > moving the version comparison out of the interrupt enable/disable hot
> > path entirely.
> 
> That's what the patch does, but why?

I thought, it was odd to compare against the version every time during
enable/disable instead of clearing the path to take at probe time itself.
however, I don't have data to prove how hot this path is ?

> > 
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/irqchip/qcom-pdc.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> > index 21e2b4b884ee..734576cdce0c 100644
> > --- a/drivers/irqchip/qcom-pdc.c
> > +++ b/drivers/irqchip/qcom-pdc.c
> > @@ -51,7 +51,7 @@ static void __iomem *pdc_base;
> >  static void __iomem *pdc_prev_base;
> >  static struct pdc_pin_region *pdc_region;
> >  static int pdc_region_cnt;
> > -static unsigned int pdc_version;
> > +static void (*__pdc_enable_intr)(int pin_out, bool on);
> >  static bool pdc_x1e_quirk;
> >  
> >  static void pdc_base_reg_write(void __iomem *base, int reg, u32 i, u32 val)
> > @@ -123,14 +123,6 @@ static void pdc_enable_intr_cfg(int pin_out, bool on)
> >  	pdc_reg_write(IRQ_i_CFG, pin_out, enable);
> >  }
> >  
> > -static void __pdc_enable_intr(int pin_out, bool on)
> > -{
> > -	if (pdc_version < PDC_VERSION_3_2)
> > -		pdc_enable_intr_bank(pin_out, on);
> > -	else
> > -		pdc_enable_intr_cfg(pin_out, on);
> 
> This style is comfortable to read.

Agree, code readingwise, this looks easier..

> 
> > -}
> > -
> >  static void pdc_enable_intr(struct irq_data *d, bool on)
> >  {
> >  	unsigned long flags;
> > @@ -400,7 +392,8 @@ static int qcom_pdc_probe(struct platform_device *pdev, struct device_node *pare
> >  		goto fail;
> >  	}
> >  
> > -	pdc_version = pdc_reg_read(PDC_VERSION_REG, 0);
> > +	__pdc_enable_intr = (pdc_reg_read(PDC_VERSION_REG, 0) < PDC_VERSION_3_2) ?
> > +			pdc_enable_intr_bank : pdc_enable_intr_cfg;
> 
> This style is a mess.
> 
> Regards,
> Bjorn
> 
> >  
> >  	parent_domain = irq_find_host(parent);
> >  	if (!parent_domain) {
> > -- 
> > 2.53.0
> > 

-- 
-Mukesh Ojha