Messages in this thread | | | Date | Thu, 15 Dec 2022 19:42:32 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v9 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for quad-uart support |
| |
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
| |