On Fri, Dec 16, 2022 at 10:56:54AM +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 const 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 */
One space or TAB before /* will increase readability.
> +};
...
> + ret = serial8250_pci_setup_port(pdev, port, 0, port_idx * 256, 0);
Actually isn't 0x100 better (show that there is an offset rather than
a value of an register)?
> + if (ret < 0)
> + return ret;
...
> +static unsigned int pci1xxxx_get_max_port(int subsys_dev)
> +{
> + int i = 0;
What the point to assign this one?
Actually, better is
unsigned int = MAX_PORTS;
> + if (subsys_dev < ARRAY_SIZE(logical_to_physical_port_idx))
> + for (i = MAX_PORTS - 1; i >= 0; i--)
while (i--) {
> + if (logical_to_physical_port_idx[subsys_dev][i] != -1)
> + return logical_to_physical_port_idx[subsys_dev][i] + 1;
}
(Note missinng {} in the above code. Does checkpatch complain on this?)
> +
> + if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414)
> + return 1;
> +
> + return 4;
> +}
...
> + num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd, PCI_IRQ_ALL_TYPES);
> + if (num_vectors < 0) {
> + pci_iounmap(pdev, priv->membase);
Here is inconsistency on how you interpret pci_*() calls when
pcim_enable_device() has been used. I.e. for IRQ you don't
deallocate resources explicitly (yes, it's done automatically
anyway), but you explicitly call pci_iounmap(). Choose a single
approach for all of them.
> + return num_vectors;
> + }
...
> +static_assert((ARRAY_SIZE(logical_to_physical_port_idx) ==
> + PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3 + 1));
Can be still one line.
--
With Best Regards,
Andy Shevchenko
