Hello Jonathan, Thank you for the review! Also, apologies for late reply. On 20-08-28 10:08:43, Jonathan Cameron wrote: [...] > > Might potentially be worth tidying up the masks as well? > Or potentially drop them given I suspect that there are no cases > in which the mask is actually doing anything... Just to confirm - you have the following constants in mind? drivers/pci/controller/pcie-rockchip.h: #define PCIE_ECAM_BUS(x) (((x) & 0xff) << 20) #define PCIE_ECAM_DEV(x) (((x) & 0x1f) << 15) #define PCIE_ECAM_FUNC(x) (((x) & 0x7) << 12) drivers/pci/controller/dwc/pcie-al.c: #define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << 12) I can move PCIE_ECAM_BUS, PCIE_ECAM_DEV and PCIE_ECAM_FUNC (as PCIE_ECAM_FUN) to the linux/pci-ecam.h file, as these seem useful, but without the masks, and then update other files to use these. We could then leverage these, for example: pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base + - (busnr_ecam << 20) + - PCIE_ECAM_DEVFN(devfn)); + PCIE_ECAM_BUS(busnr_ecam) + + PCIE_ECAM_FUN(devfn)); What do you think? Bjorn, would that be acceptable? Krzysztof
