Re: [PATCH v5 4/6] soc: mfd: add ASPEED AST2600 PCIe BMC device driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver violates MFD subsystem guidelines by using the MFD API for a single-function device and dynamically generating its `mfd_cell` arrays.
- [Low] The commit message prefix and driver structure naming do not adhere to MFD subsystem standards.
--

--- Patch [4]: [PATCH v5 4/6] soc: mfd: add ASPEED AST2600 PCIe BMC device driver ---
commit 7554e65cc1603a70fab3089c5b74e52105ba9e5b
Author: Grégoire Layet <[email protected]>

soc: mfd: add ASPEED AST2600 PCIe BMC device driver

Add support for UART over PCIe between ASPEED AST2600 BMC and host.
This is the host-side driver.

> soc: mfd: add ASPEED AST2600 PCIe BMC device driver

[Severity: Low]
This isn't a bug, but does the commit message prefix and capitalization follow
the MFD subsystem guidelines? The standard format expects
mfd: <Driver>: <Capitalized description>.

> diff --git a/drivers/mfd/aspeed-pci-core.c b/drivers/mfd/aspeed-pci-core.c
> new file mode 100644
> index 0000000000000..ad6732133ec57
> --- /dev/null
> +++ b/drivers/mfd/aspeed-pci-core.c
> @@ -0,0 +1,120 @@

[ ... ]

> +static const unsigned int vuart_msi_index[ASPEED_BMC_VUART] = { 16, 17 };
> +static const u16 vuart_port_addr[ASPEED_BMC_VUART] = { 0x3f8, 0x2f8 };
> +
> +struct aspeed_pci_bmc_dev {
> +	struct plat_serial8250_port uart[ASPEED_BMC_VUART + 1];
> +	struct mfd_cell cell;
> +};

[Severity: Low]
This isn't a bug, but does this structure name follow the MFD naming
conventions? The guidelines recommend avoiding generic suffixes like "dev" and
instead preferring just the device name.

[ ... ]

> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev,
> +					     struct aspeed_pci_bmc_dev *pci_bmc_dev)
> +{

[ ... ]

> +	for (i = 0; i < ASPEED_BMC_VUART; i++) {
> +		port = &pci_bmc_dev->uart[i];
> +
> +		/* ASPEED BMC device shift addresses by 2 to the left */
> +		vuart_ioport = vuart_port_addr[i] << 2;
> +
> +		port->mapbase = bar + vuart_ioport;
> +		port->uartclk = 115200 * 16;
> +		port->irq = pci_irq_vector(pdev, vuart_msi_index[i]);
> +		port->iotype = UPIO_MEM32;
> +		port->type = PORT_16550A;
> +		port->flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> +		port->regshift = 2;
> +	}
> +
> +	pci_bmc_dev->cell = (struct mfd_cell) {
> +		.name		= "serial8250",
> +		.platform_data	= pci_bmc_dev->uart,
> +		.pdata_size	= sizeof(pci_bmc_dev->uart),
> +	};
> +
> +	return 0;
> +}

[Severity: Medium]
Is it safe to generate the mfd_cell dynamically here? The MFD subsystem
guidelines explicitly forbid creating local dynamic copies of mfd_cell arrays,
requiring them to be static const instead to avoid memory safety hazards.

[ ... ]

> +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev,
> +					    const struct pci_device_id *ent)
> +{

[ ... ]

> +	aspeed_pci_bmc_device_setup_vuart(pdev, pci_bmc_dev);
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				    &pci_bmc_dev->cell, 1, &pdev->resource[1],
> +				    0, NULL);
> +}

[Severity: Medium]
Should the MFD API be used here for a device with only a single function? The
code registers only one child (serial8250), which violates the MFD subsystem
rules that prohibit using the MFD API for single-function devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4