Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

From: Andy Shevchenko
Date: Thu Nov 17 2022 - 03:25:36 EST


On Thu, Nov 17, 2022 at 10:31:23AM +0530, Kumaravel Thiagarajan 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.

Getting better!

...

> +struct pci1xxxx_8250 {
> + struct pci_dev *dev;

Call it pdev to distinguish with regular struct device.

> + unsigned int nr;
> + void __iomem *membase;
> + int line[];
> +};

...

> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> + switch (dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:

> + default:

You can even start with a default, so it will be more visible.
But the way it's now is also okay.

> + return 1;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + return 2;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + return 3;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> + return 4;
> + }
> +}

...

> + quot = (NSEC_PER_SEC / (baud * UART_BIT_SAMPLE_CNT));

Too many parentheses.

> + *frac = (((NSEC_PER_SEC - (quot * baud * UART_BIT_SAMPLE_CNT)) /

Ditto.

> + UART_BIT_SAMPLE_CNT) * 255) / baud;

...

> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + first_offset = 256;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + first_offset = 512;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> + first_offset = 768;
> + break;

> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + first_offset = 256;
> + break;

Can't it be moved to the above list?

> + default:
> + first_offset = 0;
> + break;
> + }

...

> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + if (idx > 0)
> + idx++;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + if (idx > 0)
> + idx += 2;
> + break;

> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + if (idx > 0)
> + idx++;
> + break;

Can it be moved to the above list?

> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + if (idx > 1)
> + idx++;
> + break;

default?

> + }

...

> + switch (priv->dev->subsystem_device) {
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> + case PCI_SUBDEVICE_ID_EFAR_PCI12000:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11010:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11101:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11400:
> + default:
> + irq_idx = 0;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> + irq_idx = 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> + irq_idx = 2;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> + irq_idx = 3;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:

> + irq_idx = idx;

This line is duplicated. I told you how to avoid duplication.
Use -1 outside of the switch-case and check that after.

> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> + if (idx > 0)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> + if (idx > 0)
> + idx += 2;
> + irq_idx = idx;
> + break;

> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> + irq_idx = idx + 1;
> + break;


> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> + if (idx > 0)
> + idx += 1;
> + irq_idx = idx + 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> + irq_idx = idx + 2;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> + if (idx > 1)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> + if (idx > 0)
> + idx++;
> + irq_idx = idx;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> + irq_idx = idx + 1;
> + break;
> + case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> + case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> + irq_idx = idx;
> + break;

Try to make this entire switch-case more compact. It's possible.

> + }

...

> + dev = &pdev->dev;

You can do it in the definition block above, since we know that dev is always
valid at this point.

...

> + priv->membase = pcim_iomap(pdev, 0, 0);

Never fails?

...

> + for (i = 0; i < nr_ports; i++) {
> + if (num_vectors == 4)
> + pci1xxxx_irq_assign(priv, &uart, i);
> +
> + rc = pci1xxxx_setup(priv, &uart, i);
> + if (rc) {
> + dev_warn(dev, "Failed to setup port %u\n", i);
> + break;

If it's not a fatal error, why break? Don't you need to continue for the rest?
Otherwise use

return dev_err_probe(...);

pattern.

> + }
> + priv->line[i] = serial8250_register_8250_port(&uart);
> + if (priv->line[i] < 0) {
> + dev_err(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;

Ditto.

> + }
> + }

--
With Best Regards,
Andy Shevchenko