[PATCH v1 0/7] serial: 8250_platform: refactor and clean up a bit

[PATCH v1 0/7] serial: 8250_platform: refactor and clean up a bit

[Andy Shevchenko]

Recent patch against this driver has been added without my review.
It was technically my fault, however I was on long vacation.
So, instead of blaming anyone, this small series to tidy up the things
as I think would look and be maintainable better.

Andy Shevchenko (7):
  serial: 8250_platform: Remove duplicate mapping
  serial: 8250_platform: Don't shadow error from
    serial8250_register_8250_port()
  serial: 8250_platform: Use same check for ACPI in the whole driver
  serial: 8250_platform: Tidy up ACPI ID table
  serial: 8250_platform: Switch to use platform_get_mem_or_io()
  serial: 8250_platform: Refactor serial8250_probe()
  serial: 8250_platform: Unify comment style

 drivers/tty/serial/8250/8250_platform.c | 107 +++++++++++++-----------
 1 file changed, 58 insertions(+), 49 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/7] serial: 8250_platform: Remove duplicate mapping

[Andy Shevchenko]

UPF_IOREMAP is for serial core to map the resource on behalf of the
driver. No need to perform this explicitly in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 73002faaae4b..a0e71b03015a 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -142,12 +142,6 @@ static int serial8250_platform_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (uart.port.mapbase) {
-		uart.port.membase = devm_ioremap(dev, uart.port.mapbase, uart.port.mapsize);
-		if (!uart.port.membase)
-			return -ENOMEM;
-	}
-
 	/*
 	 * The previous call may not set iotype correctly when reg-io-width
 	 * property is absent and it doesn't support IO port resource.
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/7] serial: 8250_platform: Don't shadow error from serial8250_register_8250_port()

[Andy Shevchenko]

Don't shadow error from serial8250_register_8250_port() and return it as is.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index a0e71b03015a..ffa2b7b690d3 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -150,7 +150,7 @@ static int serial8250_platform_probe(struct platform_device *pdev)
 
 	line = serial8250_register_8250_port(&uart);
 	if (line < 0)
-		return -ENODEV;
+		return line;
 
 	return 0;
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/7] serial: 8250_platform: Use same check for ACPI in the whole driver

[Andy Shevchenko]

Use has_acpi_companion() as 8250_core does to unify this across
the driver modules.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index ffa2b7b690d3..937eea3ebdea 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -165,14 +165,13 @@ static int serial8250_probe(struct platform_device *dev)
 	struct plat_serial8250_port *p = dev_get_platdata(&dev->dev);
 	struct uart_8250_port uart;
 	int ret, i, irqflag = 0;
-	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
 
 	/*
 	 * Probe platform UART devices defined using standard hardware
 	 * discovery mechanism like ACPI or DT. Support only ACPI based
 	 * serial device for now.
 	 */
-	if (!p && is_acpi_node(fwnode))
+	if (!p && has_acpi_companion(&dev->dev))
 		return serial8250_platform_probe(dev);
 
 	memset(&uart, 0, sizeof(uart));
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/7] serial: 8250_platform: Tidy up ACPI ID table

[Andy Shevchenko]

Tidy up ACPI ID table:
- remove explicit driver_data initializer
- drop comma in the terminator entry
- use C comment style

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 937eea3ebdea..38c083e90a7d 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -261,8 +261,8 @@ static int serial8250_resume(struct platform_device *dev)
 }
 
 static const struct acpi_device_id acpi_platform_serial_table[] = {
-	{ "RSCV0003", 0 }, // RISC-V Generic 16550A UART
-	{ },
+	{ "RSCV0003" }, /* RISC-V Generic 16550A UART */
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table);
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 5/7] serial: 8250_platform: Switch to use platform_get_mem_or_io()

[Andy Shevchenko]

Switch to use new platform_get_mem_or_io() instead of home grown analogue.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 38c083e90a7d..4a26bab8c9bd 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -113,21 +113,23 @@ static int serial8250_platform_probe(struct platform_device *pdev)
 	unsigned char iotype;
 	int ret, line;
 
-	regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (regs) {
+	regs = platform_get_mem_or_io(pdev, 0);
+	if (!regs)
+		return dev_err_probe(dev, -EINVAL, "no registers defined\n");
+
+	switch (resource_type(regs)) {
+	case IORESOURCE_IO:
 		uart.port.iobase = regs->start;
 		iotype = UPIO_PORT;
-	} else {
-		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (!regs) {
-			dev_err(dev, "no registers defined\n");
-			return -EINVAL;
-		}
-
+		break;
+	case IORESOURCE_MEM:
 		uart.port.mapbase = regs->start;
 		uart.port.mapsize = resource_size(regs);
 		uart.port.flags = UPF_IOREMAP;
 		iotype = UPIO_MEM;
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	/* Default clock frequency*/
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 6/7] serial: 8250_platform: Refactor serial8250_probe()

[Andy Shevchenko]

Make it clear that it supports two cases, pure platform device and ACPI.
With this in mind, split serial8250_probe() to two functions and rename
the ACPI case accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 43 ++++++++++++++++---------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 4a26bab8c9bd..f4a0731ff134 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -105,7 +105,7 @@ void __init serial8250_isa_init_ports(void)
 /*
  * Generic 16550A platform devices
  */
-static int serial8250_platform_probe(struct platform_device *pdev)
+static int serial8250_probe_acpi(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct uart_8250_port uart = { };
@@ -157,25 +157,11 @@ static int serial8250_platform_probe(struct platform_device *pdev)
 	return 0;
 }
 
-/*
- * Register a set of serial devices attached to a platform device.  The
- * list is terminated with a zero flags entry, which means we expect
- * all entries to have at least UPF_BOOT_AUTOCONF set.
- */
-static int serial8250_probe(struct platform_device *dev)
+static int serial8250_probe_platform(struct platform_device *dev, struct plat_serial8250_port *p)
 {
-	struct plat_serial8250_port *p = dev_get_platdata(&dev->dev);
 	struct uart_8250_port uart;
 	int ret, i, irqflag = 0;
 
-	/*
-	 * Probe platform UART devices defined using standard hardware
-	 * discovery mechanism like ACPI or DT. Support only ACPI based
-	 * serial device for now.
-	 */
-	if (!p && has_acpi_companion(&dev->dev))
-		return serial8250_platform_probe(dev);
-
 	memset(&uart, 0, sizeof(uart));
 
 	if (share_irqs)
@@ -219,6 +205,31 @@ static int serial8250_probe(struct platform_device *dev)
 	return 0;
 }
 
+/*
+ * Register a set of serial devices attached to a platform device.  The
+ * list is terminated with a zero flags entry, which means we expect
+ * all entries to have at least UPF_BOOT_AUTOCONF set.
+ */
+static int serial8250_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct plat_serial8250_port *p;
+
+	p = dev_get_platdata(dev);
+	if (p)
+		return serial8250_probe_platform(pdev, p);
+
+	/*
+	 * Probe platform UART devices defined using standard hardware
+	 * discovery mechanism like ACPI or DT. Support only ACPI based
+	 * serial device for now.
+	 */
+	if (has_acpi_companion(dev))
+		return serial8250_probe_acpi(pdev);
+
+	return 0;
+}
+
 /*
  * Remove serial ports registered against a platform device.
  */
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 7/7] serial: 8250_platform: Unify comment style

[Andy Shevchenko]

Unify comment style and fix indentation in some cases.
While at it, add that it supports ACPI enumerated non-PNP devices.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_platform.c | 35 ++++++++++++++-----------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index f4a0731ff134..30b4d12ab05b 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -2,7 +2,9 @@
 /*
  *  Universal/legacy platform driver for 8250/16550-type serial ports
  *
- *  Supports: ISA-compatible 8250/16550 ports
+ *  Supports:
+ *	      ISA-compatible 8250/16550 ports
+ *	      ACPI 8250/16550 ports
  *	      PNP 8250/16550 ports
  *	      "serial8250" platform devices
  */
@@ -24,9 +26,9 @@
 
 /*
  * Configuration:
- *    share_irqs   Whether we pass IRQF_SHARED to request_irq().
+ * share_irqs:     Whether we pass IRQF_SHARED to request_irq().
  *                 This option is unsafe when used on edge-triggered interrupts.
- * skip_txen_test  Force skip of txen test at init time.
+ * skip_txen_test: Force skip of txen test at init time.
  */
 unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
 unsigned int skip_txen_test;
@@ -63,9 +65,9 @@ static void __init __serial8250_isa_init_ports(void)
 		nr_uarts = UART_NR;
 
 	/*
-	 * Set up initial isa ports based on nr_uart module param, or else
+	 * Set up initial ISA ports based on nr_uart module param, or else
 	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
-	 * need to increase nr_uarts when setting up the initial isa ports.
+	 * need to increase nr_uarts when setting up the initial ISA ports.
 	 */
 	for (i = 0; i < nr_uarts; i++)
 		serial8250_setup_port(i);
@@ -132,11 +134,12 @@ static int serial8250_probe_acpi(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Default clock frequency*/
+	/* default clock frequency */
 	uart.port.uartclk = 1843200;
 	uart.port.type = PORT_16550A;
 	uart.port.dev = &pdev->dev;
 	uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
+
 	ret = uart_read_and_validate_port_properties(&uart.port);
 	/* no interrupt -> fall back to polling */
 	if (ret == -ENXIO)
@@ -206,8 +209,8 @@ static int serial8250_probe_platform(struct platform_device *dev, struct plat_se
 }
 
 /*
- * Register a set of serial devices attached to a platform device.  The
- * list is terminated with a zero flags entry, which means we expect
+ * Register a set of serial devices attached to a platform device.
+ * The list is terminated with a zero flags entry, which means we expect
  * all entries to have at least UPF_BOOT_AUTOCONF set.
  */
 static int serial8250_probe(struct platform_device *pdev)
@@ -292,7 +295,7 @@ static struct platform_driver serial8250_isa_driver = {
 
 /*
  * This "device" covers _all_ ISA 8250-compatible serial devices listed
- * in the table in include/asm/serial.h
+ * in the table in include/asm/serial.h.
  */
 struct platform_device *serial8250_isa_devs;
 
@@ -321,8 +324,7 @@ static int __init serial8250_init(void)
 	if (ret)
 		goto unreg_uart_drv;
 
-	serial8250_isa_devs = platform_device_alloc("serial8250",
-						    PLAT8250_DEV_LEGACY);
+	serial8250_isa_devs = platform_device_alloc("serial8250", PLAT8250_DEV_LEGACY);
 	if (!serial8250_isa_devs) {
 		ret = -ENOMEM;
 		goto unreg_pnp;
@@ -361,7 +363,7 @@ static void __exit serial8250_exit(void)
 	/*
 	 * This tells serial8250_unregister_port() not to re-register
 	 * the ports (thereby making serial8250_isa_driver permanently
-	 * in use.)
+	 * in use).
 	 */
 	serial8250_isa_devs = NULL;
 
@@ -394,12 +396,13 @@ MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
 
 #ifdef CONFIG_SERIAL_8250_DEPRECATED_OPTIONS
 #ifndef MODULE
-/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
- * working as well for the module options so we don't break people.  We
+/*
+ * This module was renamed to 8250_core in 3.7. Keep the old "8250" name
+ * working as well for the module options so we don't break people. We
  * need to keep the names identical and the convenient macros will happily
  * refuse to let us do that by failing the build with redefinition errors
- * of global variables.  So we stick them inside a dummy function to avoid
- * those conflicts.  The options still get parsed, and the redefined
+ * of global variables. So we stick them inside a dummy function to avoid
+ * those conflicts. The options still get parsed, and the redefined
  * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.
  *
  * This is hacky.  I'm sorry.
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 0/7] serial: 8250_platform: refactor and clean up a bit

[Sunil V L]

On Mon, Aug 12, 2024 at 06:47:02PM +0300, Andy Shevchenko wrote:
> Recent patch against this driver has been added without my review.
> It was technically my fault, however I was on long vacation.
> So, instead of blaming anyone, this small series to tidy up the things
> as I think would look and be maintainable better.
> 
> Andy Shevchenko (7):
>   serial: 8250_platform: Remove duplicate mapping
>   serial: 8250_platform: Don't shadow error from
>     serial8250_register_8250_port()
>   serial: 8250_platform: Use same check for ACPI in the whole driver
>   serial: 8250_platform: Tidy up ACPI ID table
>   serial: 8250_platform: Switch to use platform_get_mem_or_io()
>   serial: 8250_platform: Refactor serial8250_probe()
>   serial: 8250_platform: Unify comment style
> 
>  drivers/tty/serial/8250/8250_platform.c | 107 +++++++++++++-----------
>  1 file changed, 58 insertions(+), 49 deletions(-)
> 
Nice!. Thank you very much!.

For the series:

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Tested-by: Sunil V L <sunilvl@ventanamicro.com>

Thanks!
Sunil