[v6,tty-next,1/4] serial: 8250_pci: Add serial8250_pci_setup_port definition in 8250_pcilib.c
Commit Message
Move implementation of setup_port API to serial8250_pci_setup_port
Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
Changes in v6:
- Made this patch first patch of the patch series
Changes in v5:
- This is the new patch added in v5 version of this patchset
- Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
---
drivers/tty/serial/8250/8250_pci.c | 24 ++---------------
drivers/tty/serial/8250/8250_pcilib.c | 38 +++++++++++++++++++++++++++
drivers/tty/serial/8250/8250_pcilib.h | 9 +++++++
drivers/tty/serial/8250/Kconfig | 4 +++
drivers/tty/serial/8250/Makefile | 1 +
5 files changed, 54 insertions(+), 22 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
Comments
On Thu, 1 Dec 2022, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v6:
> - Made this patch first patch of the patch series
>
> Changes in v5:
> - This is the new patch added in v5 version of this patchset
> - Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
>
> ---
> diff --git a/drivers/tty/serial/8250/8250_pcilib.h b/drivers/tty/serial/8250/8250_pcilib.h
> new file mode 100644
> index 000000000000..41ef01d5c3c5
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pcilib.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 8250 PCI library header file.
> + *
> + * Copyright (C) 2001 Russell King, All Rights Reserved.
> + */
You shouldn't depend on .c file having things included for you. So
please add these:
#include "8250.h"
struct pci_dev;
Other than that,
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> +
> +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
> + unsigned int offset, int regshift);
On Thu, Dec 01, 2022 at 10:21:43AM +0530, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
We refer to the functions as func().
Moreover the grammatical period is what each end of sentence should have.
> +EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
Make it namespaced from day 1.
SERIAL_8250_PCI would be good name for symbol namespaces.
On Thu, Dec 01, 2022 at 11:06:53AM +0200, Ilpo Järvinen wrote:
> On Thu, 1 Dec 2022, Kumaravel Thiagarajan wrote:
...
> > +/*
> > + * 8250 PCI library header file.
> > + *
> > + * Copyright (C) 2001 Russell King, All Rights Reserved.
> > + */
>
> You shouldn't depend on .c file having things included for you. So
> please add these:
> #include "8250.h"
TBH I don't see how this is used here, but types.h for sure is missing.
> struct pci_dev;
+ blank line
struct uart_8250_port;
> Other than that,
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
> > +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
> > + unsigned int offset, int regshift);
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, December 1, 2022 5:42 PM
> To: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Subject: Re: [PATCH v6 tty-next 1/4] serial: 8250_pci: Add
> serial8250_pci_setup_port definition in 8250_pcilib.c
>
> > struct pci_dev;
>
> + blank line
>
> struct uart_8250_port;
Hi Andy,
Is the blank line required here?
Thanks,
Tharun Kumar P
On Tue, Dec 06, 2022 at 07:09:29PM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Thursday, December 1, 2022 5:42 PM
> > > struct pci_dev;
> >
> > + blank line
> >
> > struct uart_8250_port;
>
> Is the blank line required here?
Strictly speaking no, it's not required. But it shows the group of generic
forward declarations and specific to the topic (driver / subsystem).
@@ -24,6 +24,7 @@
#include <asm/io.h>
#include "8250.h"
+#include "8250_pcilib.h"
/*
* init function returns:
@@ -89,28 +90,7 @@ static int
setup_port(struct serial_private *priv, struct uart_8250_port *port,
u8 bar, unsigned int offset, int regshift)
{
- struct pci_dev *dev = priv->dev;
-
- if (bar >= PCI_STD_NUM_BARS)
- return -EINVAL;
-
- if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
- if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
- return -ENOMEM;
-
- port->port.iotype = UPIO_MEM;
- port->port.iobase = 0;
- port->port.mapbase = pci_resource_start(dev, bar) + offset;
- port->port.membase = pcim_iomap_table(dev)[bar] + offset;
- port->port.regshift = regshift;
- } else {
- port->port.iotype = UPIO_PORT;
- port->port.iobase = pci_resource_start(dev, bar) + offset;
- port->port.mapbase = 0;
- port->port.membase = NULL;
- port->port.regshift = 0;
- }
- return 0;
+ return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
}
/*
new file mode 100644
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 8250 PCI library.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "8250.h"
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
+ u8 bar, unsigned int offset, int regshift)
+{
+ if (bar >= PCI_STD_NUM_BARS)
+ return -EINVAL;
+
+ if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
+ if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
+ return -ENOMEM;
+
+ port->port.iotype = UPIO_MEM;
+ port->port.iobase = 0;
+ port->port.mapbase = pci_resource_start(dev, bar) + offset;
+ port->port.membase = pcim_iomap_table(dev)[bar] + offset;
+ port->port.regshift = regshift;
+ } else {
+ port->port.iotype = UPIO_PORT;
+ port->port.iobase = pci_resource_start(dev, bar) + offset;
+ port->port.mapbase = 0;
+ port->port.membase = NULL;
+ port->port.regshift = 0;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
new file mode 100644
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 8250 PCI library header file.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
+ unsigned int offset, int regshift);
@@ -132,6 +132,7 @@ config SERIAL_8250_DMA
config SERIAL_8250_PCI
tristate "8250/16550 PCI device support"
depends on SERIAL_8250 && PCI
+ select SERIAL_8250_PCILIB
default SERIAL_8250
help
This builds standard PCI serial support. You may be able to
@@ -500,6 +501,9 @@ config SERIAL_8250_MID
Intel Medfield SOC and various other Intel platforms that is not
covered by the more generic SERIAL_8250_PCI option.
+config SERIAL_8250_PCILIB
+ bool
+
config SERIAL_8250_PERICOM
tristate "Support for Pericom and Acces I/O serial ports"
default SERIAL_8250
@@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
8250_base-$(CONFIG_SERIAL_8250_DWLIB) += 8250_dwlib.o
8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
+8250_base-$(CONFIG_SERIAL_8250_PCILIB) += 8250_pcilib.o
obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
obj-$(CONFIG_SERIAL_8250_EXAR) += 8250_exar.o