[+to Krzysztof] On Mon, Aug 18, 2025 at 03:00:00PM +0530, Shradha Todi wrote: > > On Mon, Aug 11, 2025 at 09:16:37PM +0530, Shradha Todi wrote: > > > Add host and endpoint controller driver support for FSD SoC. > > It's kind of unfortunate that the driver uses "ep" everywhere for > > struct exynos_pcie pointers. It's going to be confusing because "ep" > > is also commonly used for endpoint-related things, e.g., struct > > dw_pcie_ep pointers. Maybe it's not worth changing; I dunno. > > I did try to rename the structure and the pointers > (https://lore.kernel.org/all/20230214121333.1837-9-shradha.t@xxxxxxxxxxx/) > But the intention was different back then and so the idea was rejected. > I could add a patch to only rename the pointers to something less > confusing like "exy_pci" The patch you mention did several renames: s/to_exynos_pcie/to_samsung_pcie/ s/struct exynos_pcie/struct samsung_pcie/ s/struct exynos_pcie *ep/struct samsung_pcie *sp/ I'm only concerned about the confusion of "ep" being used both for "struct exynos_pcie *" and for "struct dw_pcie_ep *". It would still be sort of an annoying patch to do something like this: s/struct exynos_pcie *ep/struct exynos_pcie *pcie/ But 'git grep "struct .*_pcie \*.*=" drivers/pci/controller/' says using "pcie" in this way is quite common, so maybe it would be worth doing. What do you think, Krzysztof? > > > +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) { Is there a reason to restrict it to the case when *only* FSD_IRQ_MSI_ENABLE is set? Bjorn
