> > > > +static irqreturn_t fsd_pcie_irq_handler(int irq, void *arg)
> > > > +{
> > > > + u32 val;
> > > > + struct exynos_pcie *ep = arg;
> > > > + struct dw_pcie *pci = &ep->pci;
> > > > + struct dw_pcie_rp *pp = &pci->pp;
> > > > +
> > > > + val = readl(ep->elbi_base + FSD_IRQ2_STS);
> > > > + if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
> > > > + val &= FSD_IRQ_MSI_ENABLE;
> > > > + writel(val, ep->elbi_base + FSD_IRQ2_STS);
> > >
> > > This looks weird because FSD_IRQ_MSI_ENABLE sounds like an *enable*
> > > bit, but here you're treating it as a *status* bit.
> > >
> > > As far as I can tell, you set FSD_IRQ_MSI_ENABLE once at probe-time in
> > > fsd_pcie_msi_init(), then you clear it here in an IRQ handler, and it
> > > will never be set again. That seems wrong; am I missing something?
> >
> > Actually the status IRQ and enable IRQ registers are different offsets
> > but the bit position for MSI remains same in both cases so I just reused
> > the macro.
>
> Ah, that's what I missed, thanks! At probe-time, fsd_pcie_msi_init()
> enables it in FSD_IRQ2_EN. Here you clear it in FSD_IRQ2_STS.
>
> > But I understand that it's confusing so I will add another
> > macro for FSD_IRQ_MSI_STATUS or just rename the macro to
> > FSD_IRQ_MSI to re-use.
>
> Using the same name just because a similar bit happens to be at the
> same position in two different registers is definitely confusing. I
> think it will be better to have two macros, one for FSD_IRQ2_STS and
> another for FSD_IRQ2_EN, e.g.,
>
> #define FSD_IRQ2_STS 0x008
> #define FSD_IRQ2_STS_MSI BIT(17)
> #define FSD_IRQ2_EN 0x018
> #define FSD_IRQ2_EN_MSI BIT(17)
>
> Another question about the test:
>
> if ((val & FSD_IRQ_MSI_ENABLE) == FSD_IRQ_MSI_ENABLE) {
>
> This assumes there are no other bits in FSD_IRQ2_STS that could be
> set. I would have expected a test like this:
>
> if (val & FSD_IRQ_MSI_ENABLE) {
>
Thanks for pointing this out. FSD_IRQ_MSI_ENABLE is a single-bit, so there
is no functional difference in the two statements. I didn't have a specific
reason for using "== FSD_IRQ_MSI_ENABLE".
But I see that "val & FSD_IRQ_MSI_ENABLE" would have been the more
standard way to write this. I will update this for clarity.
> Is there a reason to restrict it to the case when *only*
> FSD_IRQ_MSI_ENABLE is set?
>
> Bjorn
