RE: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux