Re: [PATCH v12 1/2] serial: exar: split out the exar code from 8250_pci

On Mon, Jan 30, 2017 at 12:22 AM, Sudip Mukherjee
[off-list ref] wrote:

From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.

Looks almost perfect, just few finishing strokes below and take my
Reviewed-by: Andy Shevchenko <redacted>

+/**
+ * struct exar8250_board - board information
+ * @num_ports: number of serial ports
+ * @reg_shift: describes UART register mapping in PCI memory
+ */
+struct exar8250_board {
+       unsigned int num_ports;
+       unsigned int reg_shift;
+       bool has_slave;
+       int     (*setup)(struct exar8250 *, struct pci_dev *,

+                        const struct exar8250_board *,

This is not needed. See below.

+                        struct uart_8250_port *, int);
+       void    (*exit)(struct pci_dev *pcidev);
+};

+struct exar8250 {
+       unsigned int            nr;

+       struct exar8250_board   *board;

...since you have this one and continuing below...

+       int                     line[0];
+};

+static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+                        const struct exar8250_board *board, int idx,
+                        unsigned int offset, struct uart_8250_port *port)
+{

const struct exar8250_board *board = priv->board;

+       unsigned int bar = 0;
+
+       port->port.iotype = UPIO_MEM;
+       port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
+       port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;
+       port->port.regshift = board->reg_shift;
+
+       return 0;
+}

+static int
+pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,

+                      const struct exar8250_board *board,

Redundant.

+                      struct uart_8250_port *port, int idx)
+{
+       unsigned int offset = idx * 0x200;
+       unsigned int baud = 1843200;
+
+       port->port.uartclk = baud * 16;
+       return default_setup(priv, pcidev, board, idx, offset, port);
+}
+
+static int
+pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+                  const struct exar8250_board *board,

Ditto.

+                  struct uart_8250_port *port, int idx)
+{
+       unsigned int offset = idx * 0x200;
+       unsigned int baud = 921600;
+
+       port->port.uartclk = baud * 16;
+       return default_setup(priv, pcidev, board, idx, offset, port);
+}

+static int
+pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,

+                  const struct exar8250_board *board,

Ditto.

+                  struct uart_8250_port *port, int idx)
+{

const struct exar8250_board *board = priv->board;

+       unsigned int offset = idx * 0x400;
+       unsigned int baud = 7812500;

+static int
+exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
+{
+       unsigned int nr_ports, i, bar = 0, maxnr;
+       struct exar8250_board *board;
+       struct uart_8250_port uart;
+       struct exar8250 *priv;
+       int rc;
+

+       board = (struct exar8250_board *)ent->driver_data;

if (!board)
 return -EINVAL;

board now is mandatory to have.

+       rc = pcim_enable_device(pcidev);
+       if (rc)
+               return rc;
+
+       maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
+
+       nr_ports = board->num_ports ? board->num_ports : pcidev->device & 0x0f;
+
+       priv = devm_kzalloc(&pcidev->dev, sizeof(*priv) +
+                           sizeof(unsigned int) * nr_ports,
+                           GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+

+       priv->board = board;

Yes! This what allows you to get rid of the above.

+static const struct exar8250_board pbn_connect = {
+       .setup          = pci_connect_tech_setup,
+};

Yep! You got the idea.

+
+static const struct exar8250_board pbn_exar_ibm_saturn = {
+       .num_ports      = 1,
+       .setup          = pci_xr17c154_setup,
+};

-- 
With Best Regards,
Andy Shevchenko