Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Dec 2022 22:01:22 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
Cc:     linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        gregkh@linuxfoundation.org, jirislaby@kernel.org,
        ilpo.jarvinen@linux.intel.com, macro@orcam.me.uk, cang1@live.co.uk,
        colin.i.king@gmail.com, phil.edworthy@renesas.com,
        biju.das.jz@bp.renesas.com, geert+renesas@glider.be,
        lukas@wunner.de, u.kleine-koenig@pengutronix.de, wander@redhat.com,
        etremblay@distech-controls.com, jk@ozlabs.org,
        Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Subject: Re: [PATCH v7 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for
 quad-uart support
Message-ID: <Y5DxEszrq0rXVqvl@smile.fi.intel.com>
References: <20221207235305.695541-1-kumaravel.thiagarajan@microchip.com>
 <20221207235305.695541-3-kumaravel.thiagarajan@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221207235305.695541-3-kumaravel.thiagarajan@microchip.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Dec 08, 2022 at 05:23:03AM +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_physical_port(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 */

You can make columns nicely indented, but it is up to you,

		...
		{0,  1,  2,  3}, /* PCI4p */
		...
		{1,  2,  3, -1}, /* PCI3p123 */
		...
		{2,  3, -1, -1}, /* PCI2p23 */
		{0, -1, -1, -1}, /* PCI1p0 */
		...

> +	};
> +
> +	return logical_to_physical_port_idx[subsys_dev][port];
> +}

...

> +	priv->membase = pcim_iomap(pdev, 0, 0);

Is any of those card can have an IO bar (instead of MEM)?

> +	if (!priv->membase)
> +		return -ENOMEM;

...

> +	if (num_vectors == 4)
> +		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI,
> +		       priv->membase + UART_PCI_CTRL_REG);

If you set this unconditionally when num_vectors < 4, would it still work?

> +	else
> +		uart.port.irq = pci_irq_vector(pdev, 0);

I would move this to the below...

...

> +		if (num_vectors == 4)
> +			uart.port.irq = pci_irq_vector(priv->pdev, port_idx);

...here as IRQ assignment to be seen at one place.

-- 
With Best Regards,
Andy Shevchenko