Messages in this thread | | | Date | Thu, 17 Nov 2022 10:25:13 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver for quad-uart support. |
| |
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
| |