Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Christophe JAILLET]

Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :
> 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.
>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
> ---

[...]

> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> +			       int offset)
> +{
> +	struct pci_dev *dev = priv->dev;
> +
> +	if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> +		if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> +			return -ENOMEM;
> +
> +		port->port.iotype = UPIO_MEM;
> +		port->port.iobase = 0;
> +		port->port.mapbase = pci_resource_start(dev, 0) + offset;
> +		port->port.membase = pcim_iomap_table(dev)[0] + offset;

Hi,

Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines 
above in the 'if')

> +		port->port.regshift = 0;
> +	} else {
> +		port->port.iotype = UPIO_PORT;
> +		port->port.iobase = pci_resource_start(dev, 0) + offset;
> +		port->port.mapbase = 0;
> +		port->port.membase = NULL;
> +		port->port.regshift = 0;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct pci1xxxx_8250 *priv;
> +	struct uart_8250_port uart;
> +	unsigned int nr_ports, i;
> +	int num_vectors = 0;
> +	int rc;
> +
> +	rc = pcim_enable_device(dev);
> +	if (rc)
> +		return rc;
> +
> +	nr_ports = pci1xxxx_get_num_ports(dev);
> +
> +	priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->membase = pcim_iomap(dev, 0, 0);
> +	priv->dev = dev;
> +	priv->nr =  nr_ports;
> +
> +	pci_set_master(dev);
> +
> +	num_vectors  = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> +	if (num_vectors < 0)
> +		return num_vectors;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> +	uart.port.uartclk = 62500000;
> +	uart.port.dev = &dev->dev;
> +
> +	if (num_vectors == 4)
> +		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> +	else
> +		uart.port.irq = pci_irq_vector(dev, 0);
> +
> +	for (i = 0; i < nr_ports; i++) {
> +		if (num_vectors == 4)
> +			pci1xxxx_irq_assign(priv, &uart, i);
> +		priv->line[i] = -ENOSPC;
> +		rc = pci1xxxx_setup(priv, &uart, i);
> +		if (rc) {
> +			dev_err(&dev->dev, "Failed to setup port %u\n", i);
> +			break;
> +		}
> +		priv->line[i] = serial8250_register_8250_port(&uart);

In case of error, this should be undone in an error handling path in the 
probe, as done in the remove() function below.

If we break, we still continue and return success. But the last 
priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove() 
is called?

> +
> +		if (priv->line[i] < 0) {
> +			dev_err(&dev->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;
> +		}
> +	}
> +
> +	pci_set_drvdata(dev, priv);
> +
> +	return 0;
> +}
> +
> +static void pci1xxxx_serial_remove(struct pci_dev *dev)
> +{
> +	struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < priv->nr; i++) {
> +		if (priv->line[i] >= 0)
> +			serial8250_unregister_port(priv->line[i]);
> +	}
> +}
> +

[...]