RE: [PATCH v8 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for quad-uart support

[Tharunkumar.Pasumarthi]

> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Sunday, December 11, 2022 2:45 AM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v8 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for
> quad-uart support
> 
> > +             4,/* PCI1p3 */
> > +     };
> 
> If you move this outside of the function you may use static_assert(), see
> below why.

If you move this outside of the function -> Do you suggest to move the array outside
the function and make it global?

> 
>         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)

I am not getting this. You suggest doing something like this:
static_assert(subsys_dev <= ARRAY_SIZE(max_port)) ?

> > +     priv->membase = pcim_iomap(pdev, 0, 0);
> 
> 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;

Okay, I will use pci_ioremap_bar(pdev, 0)

> > +     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.

Okay. I will do something like this:
	pci_set_master(pdev);
	<NL>
	priv->pdev = pdev;
	priv->nr = nr_ports;
	<NL>
	subsys_dev = priv->pdev->subsystem_device;
	max_vec_reqd = pci1xxxx_get_max_port(subsys_dev);
	<NL>
	num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd, PCI_IRQ_ALL_TYPES);
	if (num_vectors < 0)
		return num_vectors;

> Can be simplified a bit by PCI_VDEVICE().

Okay.

Thanks,
Tharun Kumar P