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

Ilpo Järvinen
2024-04-12T10:09:06Z

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.