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.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux