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
