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
