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:   Thu, 17 Nov 2022 10:25:13 +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,
        jay.dolan@accesio.com, cang1@live.co.uk,
        u.kleine-koenig@pengutronix.de, wander@redhat.com,
        etremblay@distech-controls.com, jk@ozlabs.org,
        biju.das.jz@bp.renesas.com, geert+renesas@glider.be,
        phil.edworthy@renesas.com, lukas@wunner.de,
        UNGLinuxDriver@microchip.com, colin.i.king@gmail.com,
        Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Subject: Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver
 for quad-uart support.
Message-ID: <Y3Xv6XOwddNq6Gmr@smile.fi.intel.com>
References: <20221117050126.2966714-1-kumaravel.thiagarajan@microchip.com>
 <20221117050126.2966714-2-kumaravel.thiagarajan@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221117050126.2966714-2-kumaravel.thiagarajan@microchip.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Thu, Nov 17, 2022 at 10:31:23AM +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.

Getting better!

...

> +struct pci1xxxx_8250 {
> +	struct pci_dev *dev;

Call it pdev to distinguish with regular struct device.

> +	unsigned int nr;
> +	void __iomem *membase;
> +	int line[];
> +};

...

> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> +	switch (dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:

> +	default:

You can even start with a default, so it will be more visible.
But the way it's now is also okay.

> +		return 1;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		return 2;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		return 3;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> +		return 4;
> +	}
> +}

...

> +	quot = (NSEC_PER_SEC / (baud * UART_BIT_SAMPLE_CNT));

Too many parentheses.

> +	*frac = (((NSEC_PER_SEC - (quot * baud * UART_BIT_SAMPLE_CNT)) /

Ditto.

> +		  UART_BIT_SAMPLE_CNT) * 255) / baud;

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +		first_offset = 256;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		first_offset = 512;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> +		first_offset = 768;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		first_offset = 256;
> +		break;

Can't it be moved to the above list?

> +	default:
> +		first_offset = 0;
> +		break;
> +	}

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		if (idx > 0)
> +			idx++;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +		if (idx > 0)
> +			idx += 2;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		if (idx > 0)
> +			idx++;
> +		break;

Can it be moved to the above list?

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +		if (idx > 1)
> +			idx++;
> +		break;

default?

> +	}

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI12000:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11010:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11101:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11400:
> +	default:
> +		irq_idx = 0;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +		irq_idx = 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +		irq_idx = 2;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> +		irq_idx = 3;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:

> +		irq_idx = idx;

This line is duplicated. I told you how to avoid duplication.
Use -1 outside of the switch-case and check that after.

> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +		if (idx > 0)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +		if (idx > 0)
> +			idx += 2;
> +		irq_idx = idx;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +		irq_idx = idx + 1;
> +		break;


> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		if (idx > 0)
> +			idx += 1;
> +		irq_idx = idx + 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		irq_idx = idx + 2;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +		if (idx > 1)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		if (idx > 0)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +		irq_idx = idx + 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> +		irq_idx = idx;
> +		break;

Try to make this entire switch-case more compact. It's possible.

> +	}

...

> +	dev = &pdev->dev;

You can do it in the definition block above, since we know that dev is always
valid at this point.

...

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

Never fails?

...

> +	for (i = 0; i < nr_ports; i++) {
> +		if (num_vectors == 4)
> +			pci1xxxx_irq_assign(priv, &uart, i);
> +
> +		rc = pci1xxxx_setup(priv, &uart, i);
> +		if (rc) {
> +			dev_warn(dev, "Failed to setup port %u\n", i);
> +			break;

If it's not a fatal error, why break? Don't you need to continue for the rest?
Otherwise use

	return dev_err_probe(...);

pattern.

> +		}
> +		priv->line[i] = serial8250_register_8250_port(&uart);
> +		if (priv->line[i] < 0) {
> +			dev_err(dev,
> +				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> +				uart.port.iobase, uart.port.irq,
> +				uart.port.iotype, priv->line[i]);
> +			break;

Ditto.

> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko