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
