Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type

["Chocron, Jonathan" <jonnyc@amazon.com>]

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