Re: [PATCH v5 4/6] soc: mfd: add ASPEED AST2600 PCIe BMC device driver
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-05 12:30:06
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 [off-list ref] 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>.
quoted hunk ↗ jump to hunk
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 @@
[ ... ]
quoted hunk ↗ jump to hunk
+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. [ ... ]
quoted hunk ↗ jump to hunk
+static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev, + struct aspeed_pci_bmc_dev *pci_bmc_dev) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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. [ ... ]
quoted hunk ↗ jump to hunk
+static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, + const struct pci_device_id *ent) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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/cover.1785932211.git.gregoire.layet@9elements.com?part=4