On Fri, 12 Apr 2024 13:08:58 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> On Thu, 11 Apr 2024, parker@xxxxxxxxx wrote:
>
> > From: Parker Newman <pnewman@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
>
Sorry, should have added a note about fixing this and the missing argument names
below due to checkpatch.pl. I will add a note in v3.
> > * 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.
sure, good idea.
> > + 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().
Did not know it was not allowed outside the PCI subsystem. I will switch
to dev_err() etc.
> > + return rc;
> > + }
> > + }
> > +
> > for (i = 0; i < nr_ports && i < maxnr; i++) {
> > rc = board->setup(priv, pcidev, &uart, i);
> > if (rc) {
>
>
Thanks,
-Parker
