On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
<kumaravel.thiagarajan@xxxxxxxxxxxxx> wrote:
>
> pci1xxxx is a PCIe switch with a multi-function endpoint on one of its
> downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
Thanks for the contribution!
Brief looking into the code I can see that you may easily reduce it by ~20%.
Think about it. You may take other examples, that are servicing PCI based
devices (8250_exar, 8250_lpss, 8250_mid) on how to shrink the code base.
...
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/tty.h>
> +#include <linux/serial_reg.h>
> +#include <linux/serial_core.h>
> +#include <linux/8250_pci.h>
> +#include <linux/serial_8250.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
Why not sorted?
Do you need all of them?
...
> + const unsigned int standard_baud_list[] = {50, 75, 110, 134, 150, 300,
> + 600, 1200, 1800, 2000, 2400, 3600,
> + 4800, 7200, 9600, 19200, 38400, 57600,
> + 115200, 125000, 136400, 150000, 166700,
> + 187500, 214300, 250000, 300000, 375000,
> + 500000, 750000, 1000000, 1500000};
Why?!
...
> + if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
No. We don't want to have this in the new drivers. There is BOTHER
which might be used instead.
> + writel((port->custom_divisor & 0x3FFFFFFF),
> + (port->membase + CLK_DIVISOR_REG));
...
> + frac = (((1000000000 - (quot * baud *
> + UART_BIT_SAMPLE_CNT)) / UART_BIT_SAMPLE_CNT)
> + * 255) / baud;
Funny indentation.
...
> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> + const struct pci_device_id *ent)
> +{
> + struct pci1xxxx_8250 *priv;
> + struct uart_8250_port uart;
> + unsigned int nr_ports, i;
> + int num_vectors = 0;
> + int rc;
> +
> + rc = pcim_enable_device(dev);
> + pci_save_state(dev);
Why is this call here?
> + if (rc)
> + return rc;
> +
> + nr_ports = pci1xxxx_get_num_ports(dev);
> +
> + priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +
> + priv->membase = pcim_iomap(dev, 0, 0);
> + priv->dev = dev;
> + priv->nr = nr_ports;
> + if (!priv)
> + return -ENOMEM;
You are dereferencing a NULL pointer before checking, how did you test
your code?
> + pci_set_master(dev);
> +
> + num_vectors = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> + if (num_vectors < 0)
> + return rc;
What does this mean?
> + memset(&uart, 0, sizeof(uart));
> + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> + uart.port.uartclk = 48000000;
> + uart.port.dev = &dev->dev;
> +
> + if (num_vectors == 4)
> + writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> + else
> + uart.port.irq = pci_irq_vector(dev, 0);
> +
> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
> + mchp_pci1xxxx_irq_assign(priv, &uart, i);
> + rc = mchp_pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_err(&dev->dev, "Failed to setup port %u\n", i);
> + break;
> + }
> + priv->line[i] = serial8250_register_8250_port(&uart);
> +
> + if (priv->line[i] < 0) {
> + dev_err(&dev->dev,
> + "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> + uart.port.iobase, uart.port.irq,
> + uart.port.iotype, priv->line[i]);
> + break;
> + }
> + }
> +
> + pci_set_drvdata(dev, priv);
> +
> + return 0;
> +}
...
> +static const struct pci_device_id pci1xxxx_pci_tbl[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
> + { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
> + { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11400) },
> + { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11414) },
> + { PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI12000) },
> + {0,}
{ } is enough
> +};
...
> +
Unneeded blank line
> +module_pci_driver(pci1xxxx_pci_driver);
...
> + [PORT_MCHP16550A] = {
> + .name = "MCHP16550A",
> + .fifo_size = 256,
> + .tx_loadsz = 256,
> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01,
> + .rxtrig_bytes = {2, 66, 130, 194},
> + .flags = UART_CAP_FIFO,
> + },
Why do you need this?
...
> +/* MCHP 16550A UART with 256 byte FIFOs */
> +#define PORT_MCHP16550A 124
...and this?
If you really need this, try to find a gap in the numbering, there are some.
--
With Best Regards,
Andy Shevchenko
