Re: [PATCH v2 4/7] serial: exar: add optional board_setup function

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Adds an optional "board_setup" function pointer to struct
> exar8250_board. This gets called once during probe prior to setting up
> the ports.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 9915a99cb7c6..b30f3855652a 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -143,7 +143,7 @@
>   *
>   * MPIO		Port	Function
>   * ----		----	--------
> - * 0		2 	Mode bit 0
> + * 0		2	Mode bit 0

Unrelated change.

>   * 1		2	Mode bit 1
>   * 2		2	Terminate bus
>   * 3		-	<reserved>
> @@ -179,22 +179,24 @@ struct exar8250_platform {
>  	int (*rs485_config)(struct uart_port *port, struct ktermios *termios,
>  			    struct serial_rs485 *rs485);
>  	const struct serial_rs485 *rs485_supported;
> -	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
> -	void (*unregister_gpio)(struct uart_8250_port *);
> +	int (*register_gpio)(struct pci_dev *pcidev, struct uart_8250_port *port);
> +	void (*unregister_gpio)(struct uart_8250_port *port);
>  };
> 
>  /**
>   * struct exar8250_board - board information
>   * @num_ports: number of serial ports
>   * @reg_shift: describes UART register mapping in PCI memory
> - * @setup: quirk run at ->probe() stage
> + * @board_setup: quirk run once at ->probe() stage before setting up ports
> + * @setup: quirk run at ->probe() stage for each port
>   * @exit: quirk run at ->remove() stage
>   */
>  struct exar8250_board {
>  	unsigned int num_ports;
>  	unsigned int reg_shift;
> -	int	(*setup)(struct exar8250 *, struct pci_dev *,
> -			 struct uart_8250_port *, int);
> +	int     (*board_setup)(struct exar8250 *priv);
> +	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
> +			 struct uart_8250_port *port, int idx);
>  	void	(*exit)(struct pci_dev *pcidev);
>  };
> 
> @@ -966,6 +968,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
>  	if (rc)
>  		return rc;
> 
> +	if (board->board_setup) {
> +		rc = board->board_setup(priv);

Could this be called board_init() as having both ->board_setup() and 
->setup() is bit confusing.

> +		if (rc) {
> +			pci_err(pcidev,
> +				"failed to setup serial board: %d\n", rc);

pci_err() belongs to pci subsystem. Please use dev_err() or return 
dev_err_probe().

> +			return rc;
> +		}
> +	}
> +
>  	for (i = 0; i < nr_ports && i < maxnr; i++) {
>  		rc = board->setup(priv, pcidev, &uart, i);
>  		if (rc) {


-- 
 i.