On Mon, 2019-07-22 at 08:54 +0000, Gustavo Pimentel wrote:
>
> >
> > > > +static inline void al_pcie_target_bus_set(struct al_pcie
> > > > *pcie,
> > > > + u8 target_bus,
> > > > + u8 mask_target_bus)
> > > > +{
> > > > + void __iomem *cfg_control_addr;
> > > > + u32 reg;
> > > > +
> > > > + reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK,
> > > > mask_target_bus) |
> > > > + FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK,
> > > > target_bus);
> > > > +
> > > > + cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > > > > controller_base +
> > > >
> > > > + AXI_BASE_OFFSET + pcie-
> > > > >reg_offsets.ob_ctrl
> > > > +
> > > > + CFG_TARGET_BUS);
> > > > +
> > > > + writel(reg, cfg_control_addr);
> > >
> > > From what I'm seeing you commonly use writel() and readl() with a
> > > common
> > > base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> > > I'd suggest to creating a writel and readl with that offset
> > > built-in.
> > >
> >
> > I prefer to keep it generic, since in future revisions we might
> > want to
> > access regs which are not in the AXI region. You think I should add
> > wrappers which simply hide the pcie->controller_base part?
>
> I and other developers typically do that, but it's a suggestion. IMHO
> it
> helps to keep the code cleaner and more readable.
>
Added al_pcie_controller_readl/writel wrappers.
--
Thanks,
Jonathan
