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

[Parker Newman <parker@finest.io>]

On Fri, 12 Apr 2024 13:08:58 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> 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.
>

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