RE: [PATCH v2 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

[Havalige, Thippeswamy]

Hi Bjorn,
> Whatever name/text you settle on, make sure it's in alpha order in the config
> menu seen by users.  As-is, this patch would make it:
> 
>   Xilinx AXI PCIe controller
>   Xilinx NWL PCIe controller
>   Xilinx Versal CPM PCI controller
>   Xilinx DMA PL PCIe host bridge support
> 
 > which is not in alpha order.
> 
> > +	  Say 'Y' here if you want kernel to enable support for the
> > +	  XILINX PL PCIe host bridge support, this PCIe controller
> > +	  includes DMA PL component.
> 
> > +obj-$(CONFIG_PCIE_XILINX_DMA) += pcie-xdma-pl.o
> 
> I think this filename needs to include xilinx somehow, not just "xdma".
> 
> Since the probe function calls pci_host_probe() in addition to the DMA setup,
> I guess this is a fourth Xilinx host bridge, a peer of AXI, CPM, and NWL, and
> independent of them?

- Agreed, fix it in next patch
> Is the "xdma" or ("DMA PL" as used in Kconfig) name also a peer to "CPM"
> and "NWL"?  The Kconfig text, especially, should use names that users will
> recognize.  "DMA" or "XDMA" seems a little generic.  The commit log
> mentions "Zynq" and "Ultrascale+", neither of which appears in Kconfig, so
> there are a lot of names in play here, which is confusing.
> 
> > +struct xilinx_pcie_dma {
- Agreed, fix it in next patch
> git grep "^struct .*pcie.*" drivers/pci/controller/ says the typical names are
> "<driver>_pcie".  Please do the same.
> 
> > +	void __iomem			*reg_base;
> > +	u32				irq;
> > +	struct pci_config_window	*cfg;
> > +	struct device			*dev;
> 
> Please use typical order, i.e., "dev" first, followed by "reg_base", etc.  Look at
> other drivers and make this similar.  No need to be creatively different.
- Agreed, fix it in next patch 
> > +static inline bool xilinx_pcie_dma_linkup(struct xilinx_pcie_dma
> > +*port)
> 
> Please use the *_pcie_link_up() naming scheme used elsewhere in
> drivers/pci/controller/.
- Agreed, fix it in next patch
> > +static bool xilinx_pcie_dma_valid_device(struct pci_bus *bus,
> > +unsigned int devfn)
> 
> Similarly, *_pcie_valid_device().  Lots more instances below.  Don't split the
> "pcie" from the rest of the generic parts of the name.
> 
> > +static struct pci_ecam_ops xilinx_pcie_dma_ops = {
> 
> const *_ecam_ops
- Agreed, fix it in next patch
> > +static void xilinx_mask_leg_irq(struct irq_data *data) static void
> > +xilinx_unmask_leg_irq(struct irq_data *data) static struct irq_chip
> > +xilinx_leg_irq_chip = {
> > +	.name           = "INTx",
> > +	.irq_mask       = xilinx_mask_leg_irq,
> > +	.irq_unmask     = xilinx_unmask_leg_irq,
> > +};
> 
> You use "intx" in the names below.  Please also use "intx" instead of "leg" in
> the names above.  No need for two different names for the same concept.
> 
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +	.map = xilinx_pcie_dma_intx_map,
> 
> > +	/* Enable the Bridge enable bit */
> 
> "Set the ... enable bit"
- Agreed, fix it in next patch
> > +	pcie_write(port, pcie_read(port, XILINX_PCIE_DMA_REG_RPSC) |
> 
> > +static int xilinx_pcie_dma_parse_dt(struct xilinx_pcie_dma *port,
> > +				    struct resource *bus_range)
> > +{
> > +	struct device *dev = port->dev;
> > +	int err;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct resource *res;
> 
> Weird ordering.  Suggest order of use:
- Agreed, fix it in next patch
>   struct device *dev = port->dev;
>   struct platform_device *pdev = to_platform_device(dev);
>   struct resource *res;
>   int err;
> 
> > +static int xilinx_pcie_dma_probe(struct platform_device *pdev) {
> > +	struct xilinx_pcie_dma *port;
> > +	struct device *dev = &pdev->dev;
> > +	struct pci_host_bridge *bridge;
> > +	struct resource_entry *bus;
> > +	int err;
> 
> Would order "struct device *dev" first.
- Agreed, fix it in next patch
> Bjorn