Received-SPF: pass (google.com: domain of linux-kernel+bounces-142470-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 12 Apr 2024 13:08:58 +0300 (EEST)
To: parker@finest.io
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
    Jiri Slaby <jirislaby@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
    linux-serial <linux-serial@vger.kernel.org>, 
    Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v2 4/7] serial: exar: add optional board_setup function
In-Reply-To: <69677365f4c753cbe6b5c028f530db49e6712ab3.1712863999.git.pnewman@connecttech.com>
Message-ID: <385ccf17-199e-13cb-e003-8b583252cab8@linux.intel.com>
References: <cover.1712863999.git.pnewman@connecttech.com> <69677365f4c753cbe6b5c028f530db49e6712ab3.1712863999.git.pnewman@connecttech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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.