Hi Bjorn, Thanks for review comments, will fix all below comments in next patch. Regards, Thippeswamy H > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Friday, December 13, 2024 10:58 PM > To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx> > Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; > manivannan.sadhasivam@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; > conor+dt@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; jingoohan1@xxxxxxxxx; Simek, Michal > <michal.simek@xxxxxxx>; Gogada, Bharat Kumar > <bharat.kumar.gogada@xxxxxxx> > Subject: Re: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port > driver > > On Fri, Dec 13, 2024 at 12:10:35PM +0530, Thippeswamy Havalige wrote: > > Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port. > > Add space before "(". > > > +config PCIE_AMD_MDB > > + bool "AMD PCIe controller (host mode)" > > Seems too generic to describe this as "the AMD PCIe controller". > Perhaps at least "AMD MDB PCIe controller"? And/or include "Versal2"? > > > + depends on OF || COMPILE_TEST > > + depends on PCI && PCI_MSI > > + select PCIE_DW_HOST > > + help > > + Say Y here to enable PCIe controller support on AMD SoCs. The > > + PCIe controller is based on DesignWare Hardware and uses AMD > > + hardware wrappers. > > Make this help text a little more specific, too. > > > + * struct amd_mdb_pcie - PCIe port information > > + * @pci: DesignWare PCIe controller structure > > + * @mdb_base: MDB System Level Control and Status Register(SLCR) Base > > Add space before "(". > > Thanks for expanding this initialism. Capitalize it in the text of other patches so it's > obvious that it's an initialism, not a word. > > > + * @intx_domain: Legacy IRQ domain pointer > > Just say "INTx IRQ domain pointer". No point in using two terms when we use > "INTx" everywhere else. > > > + * @mdb_domain: MDB IRQ domain pointer */ struct amd_mdb_pcie { > > + struct dw_pcie pci; > > + void __iomem *mdb_base; > > + struct irq_domain *intx_domain; > > + struct irq_domain *mdb_domain; > > +}; > > > + * amd_mdb_pcie_init_port - Initialize hardware > > + * @pcie: PCIe port information > > + * @pdev: platform device > > + */ > > +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie, > > + struct platform_device *pdev) > > "pdev" is unused, why include it? > > > +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args) { > > + struct amd_mdb_pcie *pcie = args; > > + unsigned long val; > > + int i; > > + > > + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC); > > Spurious extra space. > > > +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie, > > + struct platform_device *pdev) > > +{ > > + struct dw_pcie *pci = &pcie->pci; > > + struct dw_pcie_rp *pp = &pci->pp; > > + struct device *dev = &pdev->dev; > > + struct device_node *node = dev->of_node; > > + struct device_node *pcie_intc_node; > > + > > + /* Setup INTx */ > > + pcie_intc_node = of_get_next_child(node, NULL); > > + if (!pcie_intc_node) { > > + dev_err(dev, "No PCIe Intc node found\n"); > > + return -EINVAL; > > + } > > + > > + pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32, > > + &event_domain_ops, > > + pcie); > > Fix whitespace. "pcie" would fit on the previous line. > > Bjorn