Messages in this thread |  | | Date | Sat, 10 Dec 2022 23:14:50 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v8 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for quad-uart support |
| |
On Sun, Dec 11, 2022 at 07:17:28AM +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.
...
> +static int pci1xxxx_get_max_port(int subsys_dev) > +{ > + static int max_port[] = { > + 1,/* PCI12000 PCI11010 PCI11101 PCI11400 */
I would put the commas in between in the comment, or is it the name of a single product?
> + 4,/* PCI4p */ > + 3,/* PCI3p012 */ > + 4,/* PCI3p013 */ > + 4,/* PCI3p023 */ > + 4,/* PCI3p123 */ > + 2,/* PCI2p01 */ > + 3,/* PCI2p02 */ > + 4,/* PCI2p03 */ > + 3,/* PCI2p12 */ > + 4,/* PCI2p13 */ > + 4,/* PCI2p23 */ > + 1,/* PCI1p0 */ > + 2,/* PCI1p1 */ > + 3,/* PCI1p2 */ > + 4,/* PCI1p3 */ > + };
If you move this outside of the function you may use static_assert(), see below why.
> + if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3) > + if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414) > + return max_port[0]; > + else > + return 4; > + else > + return max_port[subsys_dev];
Too many redundant 'else'.
if (subsys_dev <= PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3) return max_port[subsys_dev];
(however better to compare with your size of the array)
if (subsys_dev <= ARRAY_SIZE(max_port)) return max_port[subsys_dev];
(in this case you can make sure it is the same as the above using static_assert(), so it won't compile otherwise)
if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414) return max_port[0];
return 4;
> +}
...
> +static int pci1xxxx_logical_to_physical_port_translate(int subsys_dev, int port) > +{ > + static int logical_to_physical_port_idx[][MAX_PORTS] = { > + {0, 1, 2, 3},/* PCI12000 PCI11010 PCI11101 PCI11400 PCI11414 */ > + {0, 1, 2, 3},/* PCI4p */ > + {0, 1, 2, -1},/* PCI3p012 */ > + {0, 1, 3, -1},/* PCI3p013 */ > + {0, 2, 3, -1},/* PCI3p023 */ > + {1, 2, 3, -1},/* PCI3p123 */ > + {0, 1, -1, -1},/* PCI2p01 */ > + {0, 2, -1, -1},/* PCI2p02 */ > + {0, 3, -1, -1},/* PCI2p03 */ > + {1, 2, -1, -1},/* PCI2p12 */ > + {1, 3, -1, -1},/* PCI2p13 */ > + {2, 3, -1, -1},/* PCI2p23 */ > + {0, -1, -1, -1},/* PCI1p0 */ > + {1, -1, -1, -1},/* PCI1p1 */ > + {2, -1, -1, -1},/* PCI1p2 */ > + {3, -1, -1, -1},/* PCI1p3 */ > + }; > + > + if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3) > + return logical_to_physical_port_idx[0][port]; > + else > + return logical_to_physical_port_idx[subsys_dev][port];
Similar comments as per above function.
> +}
...
> + priv->membase = pcim_iomap(pdev, 0, 0);
You issued a new version of the series without settling on this. As you said there will be no hardware that uses IO ports, why do you need pci_iomap()? I guess what you may use pci_ioremap_bar().
> + if (!priv->membase) > + return -ENOMEM;
...
> + priv->pdev = pdev;
> + subsys_dev = priv->pdev->subsystem_device;
Why use priv?
> + priv->nr = nr_ports; > + pci_set_master(pdev); > + max_vec_reqd = pci1xxxx_get_max_port(subsys_dev);
The above needs a bit of reshuffling and perhaps a blank line or lines. Make it ordered and grouped more logically.
...
> + num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd, > + PCI_IRQ_ALL_TYPES);
I would leave this on a single line (you have such a long lines already in your code).
> + if (num_vectors < 0) > + return num_vectors;
...
> +static const struct pci_device_id pci1xxxx_pci_tbl[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11010) }, > + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11101) }, > + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11400) }, > + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11414) }, > + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI12000) },
Can be simplified a bit by PCI_VDEVICE().
> + {} > +};
-- With Best Regards, Andy Shevchenko
|  |