2024-04-11 20:26:39

by Parker Newman

[permalink] [raw]
Subject: [PATCH v2 4/7] serial: exar: add optional board_setup function

From: Parker Newman <[email protected]>

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 <[email protected]>
---
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
* 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);
+ if (rc) {
+ pci_err(pcidev,
+ "failed to setup serial board: %d\n", rc);
+ return rc;
+ }
+ }
+
for (i = 0; i < nr_ports && i < maxnr; i++) {
rc = board->setup(priv, pcidev, &uart, i);
if (rc) {
--
2.43.2



2024-04-12 11:13:00

by Ilpo Järvinen

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

On Thu, 11 Apr 2024, [email protected] wrote:

> From: Parker Newman <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.


2024-04-12 13:51:32

by Parker Newman

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

On Fri, 12 Apr 2024 13:08:58 +0300 (EEST)
Ilpo Järvinen <[email protected]> wrote:

> On Thu, 11 Apr 2024, [email protected] wrote:
>
> > From: Parker Newman <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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