Re: [PATCH 01/15] serial: 8250: split Moxa PCIe serial board support out of 8250_pci
From: Andy Shevchenko
Date: Mon May 04 2026 - 09:31:41 EST
On Mon, May 4, 2026 at 11:49 AM Crescent Hsieh
<crescentcy.hsieh@xxxxxxxx> wrote:
>
> The Moxa PCIe multiport serial boards are currently handled as part of
> 8250_pci.c. In preparation for adding Moxa-specific UART features and
> optimizations, move the Moxa PCIe implementation into a dedicated
> driver.
>
> This introduces drivers/tty/serial/8250/8250_mxpcie.c and wires it up
> via Kconfig and Makefile, while preserving the existing probe flow and
> device IDs.
Thanks for doing this!
> This change was suggested during earlier review by Andy Shevchenko.
an earlier
Perhaps you wanted to add a link references here, something like this
This change was suggested during earlier reviews by Andy Shevchenko [1][2].
> No functional change intended.
> Link: https://lore.kernel.org/all/ZmQovC6TbDpTb3c8@surfacebook.localdomain/
> Link: https://lore.kernel.org/all/CAHp75VeDsVt0GQYUFxLM+obfmqXBPa3hM3YMsFbc26uzWZG-SQ@xxxxxxxxxxxxxx/
...and hence
Link: https://lore.kernel.org/all/ZmQovC6TbDpTb3c8@surfacebook.localdomain/
[1]
Link: https://lore.kernel.org/all/CAHp75VeDsVt0GQYUFxLM+obfmqXBPa3hM3YMsFbc26uzWZG-SQ@xxxxxxxxxxxxxx/
[2]
(note [*] references)
> Suggested-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@xxxxxxxx>
...
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/types.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
Keep above sorted alphabetically, it makes it easier to read and
follow and see if anything is missing or a leftover.
+ blank line (i.o.w. keep the below headers in a separate group as
this driver is 8250 driver)
> +#include <linux/serial_8250.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
Only the first one is needed.
> +#include <linux/8250_pci.h>
...
> +static unsigned int mxpcie8250_get_supp_rs(unsigned short device)
> +{
> + switch (device & MOXA_DEV_ID_IFACE_MASK) {
> + case 0x0000:
> + case 0x0600:
> + return MOXA_SUPP_RS232;
> + case 0x0100:
> + return MOXA_SUPP_RS232 | MOXA_SUPP_RS422 | MOXA_SUPP_RS485;
> + case 0x0300:
> + return MOXA_SUPP_RS422 | MOXA_SUPP_RS485;
> + }
> +
> + return 0;
Simply make it a default case.
> +}
> +
> +static unsigned short mxpcie8250_get_nports(unsigned short device)
> +{
> + switch (device) {
> + case PCI_DEVICE_ID_MOXA_CP116E_A_A:
> + case PCI_DEVICE_ID_MOXA_CP116E_A_B:
> + return 8;
> + }
> +
> + return FIELD_GET(MOXA_DEV_ID_NPORTS_MASK, device);
Ditto.
> +}
> +
> +static void mxpcie8250_set_interface(struct mxpcie8250 *priv,
> + unsigned int port_idx,
> + u8 mode)
> +{
> + void __iomem *uir_addr = priv->bar2_base + MOXA_UIR_OFFSET + port_idx / 2;
> + u8 cval;
> +
> + cval = ioread8(uir_addr);
> +
> + if (port_idx & 1)
% 2
With them (/2, %2) going closer to each other some compilers gain a
few bytes, see this for example
9b3cd5c7099f ("regmap: place foo / 8 and foo % 8 closer to each other").
> + cval = FIELD_MODIFY(MOXA_ODD_RS_MASK, &cval, mode);
> + else
> + cval = FIELD_MODIFY(MOXA_EVEN_RS_MASK, &cval, mode);
> +
> + iowrite8(cval, uir_addr);
> +}
...
> + priv = devm_kzalloc(dev, struct_size(priv, line, num_ports), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->supp_rs = mxpcie8250_get_supp_rs(device);
> + priv->num_ports = num_ports;
Swap these two lines (by the order) and add __counted_by() to the data
structure.
...
> + for (i = 0; i < num_ports; i++) {
Seems i is not used outside the loop, hence just
for (int i = 0; i < num_ports; i++) {
> + }
...
> +static void mxpcie8250_remove(struct pci_dev *pdev)
> +{
> + struct mxpcie8250 *priv = dev_get_drvdata(&pdev->dev);
platform_get_drvdata() IIRC
> + unsigned int i;
> +
> + for (i = 0; i < priv->num_ports; i++)
As per above.
> + serial8250_unregister_port(priv->line[i]);
> +}
--
With Best Regards,
Andy Shevchenko