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

[Bjorn Helgaas <helgaas@kernel.org>]

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