Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 30 Aug 2022 22:53:31 +0300 | Subject | Re: [PATCH v1 tty-next 1/2] 8250: microchip: pci1xxxx: Add driver for the quad-uart function in the multi-function endpoint of pci1xxxx device. |
| |
On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com> 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
| |