> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Wednesday, August 31, 2022 1:24 AM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@xxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>; Uwe
> Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>; Johan Hovold
> <johan@xxxxxxxxxx>; Wander Lairson Costa <wander@xxxxxxxxxx>; Eric
> Tremblay <etremblay@xxxxxxxxxxxxxxxxxxxx>; Maciej W. Rozycki
> <macro@xxxxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Jeremy Kerr <jk@xxxxxxxxxx>; Phil Edworthy <phil.edworthy@xxxxxxxxxxx>;
> Lukas Wunner <lukas@xxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; open list:SERIAL DRIVERS <linux-
> serial@xxxxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>
> 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@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?
Ok. I will review and modify this if possible
>
> ...
>
> > + 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?!
The standard baud rates are handled within serial8250_do_set_termios which is invoked from
within mchp_pci1xxxx_set_termios in first place. Hence if it matches with any of the standard baudrates,
it can return immediately.
>
> ...
>
> > + 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.
Ok. I will modify this.
>
> > + 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.
Ok. I will modify this.
>
> ...
>
> > +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?
I think it should not be required. I will remove this, test with the device and fix this.
>
> > + 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?
Ok. I will modify this.
>
> > + 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?
It is a mistake. I will replace the num_vectors variable with rc.
>
> > + 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
Ok. I will modify this.
>
> > +};
>
> ...
>
> > +
>
> Unneeded blank line
Ok. I will modify this.
>
> > +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?
I think this is required because of the difference in the values of the FIFO trigger levels and FIFO depth.
I will review 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.
Sure. I will review this.
>
> --
> With Best Regards,
> Andy Shevchenko
