[PATCH v1 0/7] serial: 8250_ni: Clean up the driver

[PATCH v1 0/7] serial: 8250_ni: Clean up the driver

[Andy Shevchenko]

The newly introduced driver inherited almost all same issues
that 8250_platform had and others. Clean up the driver accordingly.

Andy Shevchenko (7):
  serial: 8250_ni: Switch to use uart_read_port_properties()
  serial: 8250_ni: Remove duplicate mapping
  serial: 8250_ni: Switch to use platform_get_mem_or_io()
  serial: 8250_ni: Remove unneeded conditionals
  serial: 8250_ni: use serial_port_in()/serial_port_out() helpers
  serial: 8250_ni: Switch to use dev_err_probe()
  serial: 8250_ni: Tidy up ACPI ID table

 drivers/tty/serial/8250/8250_ni.c | 89 +++++++++++++------------------
 drivers/tty/serial/8250/Kconfig   |  2 +-
 2 files changed, 38 insertions(+), 53 deletions(-)

-- 
2.47.2

[PATCH v1 1/7] serial: 8250_ni: Switch to use uart_read_port_properties()

[Andy Shevchenko]

Since we have now a common helper to read port properties
use it instead of sparse home grown solution.

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

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index b10a42d2ad63..03e838f440be 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -285,7 +285,6 @@ static int ni16550_probe(struct platform_device *pdev)
 	const char *portmode;
 	bool rs232_property;
 	int ret;
-	int irq;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -293,10 +292,6 @@ static int ni16550_probe(struct platform_device *pdev)
 
 	spin_lock_init(&uart.port.lock);
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	ret = ni16550_get_regs(pdev, &uart.port);
 	if (ret < 0)
 		return ret;
@@ -307,10 +302,7 @@ static int ni16550_probe(struct platform_device *pdev)
 	info = device_get_match_data(dev);
 
 	uart.port.dev		= dev;
-	uart.port.irq		= irq;
-	uart.port.irqflags	= IRQF_SHARED;
-	uart.port.flags		= UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
-					| UPF_FIXED_PORT | UPF_FIXED_TYPE;
+	uart.port.flags		= UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE;
 	uart.port.startup	= ni16550_port_startup;
 	uart.port.shutdown	= ni16550_port_shutdown;
 
@@ -332,12 +324,16 @@ static int ni16550_probe(struct platform_device *pdev)
 	/*
 	 * Declaration of the base clock frequency can come from one of:
 	 * - static declaration in this driver (for older ACPI IDs)
-	 * - a "clock-frquency" ACPI
+	 * - a "clock-frequency" ACPI
 	 */
 	if (info->uartclk)
 		uart.port.uartclk = info->uartclk;
-	if (device_property_read_u32(dev, "clock-frequency",
-				     &uart.port.uartclk)) {
+
+	ret = uart_read_port_properties(&uart.port);
+	if (ret)
+		return ret;
+
+	if (!uart.port.uartclk) {
 		data->clk = devm_clk_get_enabled(dev, NULL);
 		if (!IS_ERR(data->clk))
 			uart.port.uartclk = clk_get_rate(data->clk);
-- 
2.47.2

[PATCH v1 2/7] serial: 8250_ni: 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_ni.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index 03e838f440be..562f7f29e209 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -239,11 +239,6 @@ static int ni16550_get_regs(struct platform_device *pdev,
 		port->mapsize = resource_size(regs);
 		port->flags |= UPF_IOREMAP;
 
-		port->membase = devm_ioremap(&pdev->dev, port->mapbase,
-					     port->mapsize);
-		if (!port->membase)
-			return -ENOMEM;
-
 		return 0;
 	}
 
-- 
2.47.2

[PATCH v1 3/7] serial: 8250_ni: 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_ni.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index 562f7f29e209..2dc510c0a5ef 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -224,26 +224,26 @@ static int ni16550_get_regs(struct platform_device *pdev,
 {
 	struct resource *regs;
 
-	regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (regs) {
+	regs = platform_get_mem_or_io(pdev, 0);
+	if (!regs)
+		return dev_err_probe(&pdev->dev, -EINVAL, "no registers defined\n");
+
+	switch (resource_type(regs)) {
+	case IORESOURCE_IO:
 		port->iotype = UPIO_PORT;
 		port->iobase = regs->start;
 
 		return 0;
-	}
-
-	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (regs) {
+	case IORESOURCE_MEM:
 		port->iotype = UPIO_MEM;
 		port->mapbase = regs->start;
 		port->mapsize = resource_size(regs);
 		port->flags |= UPF_IOREMAP;
 
 		return 0;
+	default:
+		return -EINVAL;
 	}
-
-	dev_err(&pdev->dev, "no registers defined\n");
-	return -EINVAL;
 }
 
 /*
-- 
2.47.2

[PATCH v1 4/7] serial: 8250_ni: Remove unneeded conditionals

[Andy Shevchenko]

It doesn't matter if the properties are supplied or not in
the struct ni16550_device_info as default in any case is 0.
Hence there is no need to check for them being set.

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

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index 2dc510c0a5ef..8bb8bb7bb4f2 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -275,7 +275,7 @@ static int ni16550_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct uart_8250_port uart = {};
 	unsigned int txfifosz, rxfifosz;
-	unsigned int prescaler = 0;
+	unsigned int prescaler;
 	struct ni16550_data *data;
 	const char *portmode;
 	bool rs232_property;
@@ -321,8 +321,7 @@ static int ni16550_probe(struct platform_device *pdev)
 	 * - static declaration in this driver (for older ACPI IDs)
 	 * - a "clock-frequency" ACPI
 	 */
-	if (info->uartclk)
-		uart.port.uartclk = info->uartclk;
+	uart.port.uartclk = info->uartclk;
 
 	ret = uart_read_port_properties(&uart.port);
 	if (ret)
@@ -340,11 +339,9 @@ static int ni16550_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	if (info->prescaler)
-		prescaler = info->prescaler;
+	prescaler = info->prescaler;
 	device_property_read_u32(dev, "clock-prescaler", &prescaler);
-
-	if (prescaler != 0) {
+	if (prescaler) {
 		uart.port.set_mctrl = ni16550_set_mctrl;
 		ni16550_config_prescaler(&uart, (u8)prescaler);
 	}
-- 
2.47.2

[PATCH v1 5/7] serial: 8250_ni: use serial_port_in()/serial_port_out() helpers

[Andy Shevchenko]

There are serial_port_in()/serial_port_out() helpers to be used
instead of direct p->serial_in()/p->serial_out().

Use them in various 8250 drivers.

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

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index 8bb8bb7bb4f2..15bee1b7dc2a 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -90,10 +90,10 @@ static int ni16550_disable_transceivers(struct uart_port *port)
 {
 	u8 pcr;
 
-	pcr = port->serial_in(port, NI16550_PCR_OFFSET);
+	pcr = serial_port_in(port, NI16550_PCR_OFFSET);
 	pcr &= ~NI16550_PCR_TXVR_ENABLE_BIT;
 	dev_dbg(port->dev, "disable transceivers: write pcr: 0x%02x\n", pcr);
-	port->serial_out(port, NI16550_PCR_OFFSET, pcr);
+	serial_port_out(port, NI16550_PCR_OFFSET, pcr);
 
 	return 0;
 }
@@ -105,7 +105,7 @@ static int ni16550_rs485_config(struct uart_port *port,
 	struct uart_8250_port *up = container_of(port, struct uart_8250_port, port);
 	u8 pcr;
 
-	pcr = serial_in(up, NI16550_PCR_OFFSET);
+	pcr = serial_port_in(port, NI16550_PCR_OFFSET);
 	pcr &= ~NI16550_PCR_WIRE_MODE_MASK;
 
 	if ((rs485->flags & SER_RS485_MODE_RS422) ||
@@ -120,7 +120,7 @@ static int ni16550_rs485_config(struct uart_port *port,
 	}
 
 	dev_dbg(port->dev, "config rs485: write pcr: 0x%02x, acr: %02x\n", pcr, up->acr);
-	serial_out(up, NI16550_PCR_OFFSET, pcr);
+	serial_port_out(port, NI16550_PCR_OFFSET, pcr);
 	serial_icr_write(up, UART_ACR, up->acr);
 
 	return 0;
-- 
2.47.2

[PATCH v1 6/7] serial: 8250_ni: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

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

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index 15bee1b7dc2a..c66bfc56838e 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -333,11 +333,8 @@ static int ni16550_probe(struct platform_device *pdev)
 			uart.port.uartclk = clk_get_rate(data->clk);
 	}
 
-	if (!uart.port.uartclk) {
-		dev_err(dev, "unable to determine clock frequency!\n");
-		ret = -ENODEV;
-		goto err;
-	}
+	if (!uart.port.uartclk)
+		return dev_err_probe(dev, -ENODEV, "unable to determine clock frequency!\n");
 
 	prescaler = info->prescaler;
 	device_property_read_u32(dev, "clock-prescaler", &prescaler);
@@ -381,14 +378,11 @@ static int ni16550_probe(struct platform_device *pdev)
 
 	ret = serial8250_register_8250_port(&uart);
 	if (ret < 0)
-		goto err;
+		return ret;
 	data->line = ret;
 
 	platform_set_drvdata(pdev, data);
 	return 0;
-
-err:
-	return ret;
 }
 
 static void ni16550_remove(struct platform_device *pdev)
-- 
2.47.2

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

[Andy Shevchenko]

Tidy up ACPI ID table:
- drop ACPI_PTR() and hence replace acpi.h with mod_devicetable.h et al.
- drop comma in the terminator entry

With that done, extend compile test coverage.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_ni.c | 15 +++++++++------
 drivers/tty/serial/8250/Kconfig   |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ni.c b/drivers/tty/serial/8250/8250_ni.c
index c66bfc56838e..b0e44fb00b3a 100644
--- a/drivers/tty/serial/8250/8250_ni.c
+++ b/drivers/tty/serial/8250/8250_ni.c
@@ -10,14 +10,18 @@
  * Copyright 2012-2023 National Instruments Corporation
  */
 
-#include <linux/acpi.h>
 #include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/init.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
-#include <linux/clk.h>
+#include <linux/serial_core.h>
+#include <linux/types.h>
 
 #include "8250.h"
 
@@ -392,7 +396,6 @@ static void ni16550_remove(struct platform_device *pdev)
 	serial8250_unregister_port(data->line);
 }
 
-#ifdef CONFIG_ACPI
 /* NI 16550 RS-485 Interface */
 static const struct ni16550_device_info nic7750 = {
 	.uartclk = 33333333,
@@ -417,20 +420,20 @@ static const struct ni16550_device_info nic7a69 = {
 	.uartclk = 29629629,
 	.prescaler = 0x09,
 };
+
 static const struct acpi_device_id ni16550_acpi_match[] = {
 	{ "NIC7750",	(kernel_ulong_t)&nic7750 },
 	{ "NIC7772",	(kernel_ulong_t)&nic7772 },
 	{ "NIC792B",	(kernel_ulong_t)&nic792b },
 	{ "NIC7A69",	(kernel_ulong_t)&nic7a69 },
-	{ },
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, ni16550_acpi_match);
-#endif
 
 static struct platform_driver ni16550_driver = {
 	.driver = {
 		.name = "ni16550",
-		.acpi_match_table = ACPI_PTR(ni16550_acpi_match),
+		.acpi_match_table = ni16550_acpi_match,
 	},
 	.probe = ni16550_probe,
 	.remove = ni16550_remove,
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 9be9760886dc..63fedac1bbae 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -565,7 +565,7 @@ config SERIAL_8250_BCM7271
 config SERIAL_8250_NI
 	tristate "NI 16550 based serial port"
 	depends on SERIAL_8250
-	depends on (X86 && ACPI) || COMPILE_TEST
+	depends on X86 || COMPILE_TEST
 	help
 	  This driver supports the integrated serial ports on National
 	  Instruments (NI) controller hardware. This is required for all NI
-- 
2.47.2

Re: [PATCH v1 0/7] serial: 8250_ni: Clean up the driver

[Chaitanya Vadrevu]

> The newly introduced driver inherited almost all same issues
> that 8250_platform had and others. Clean up the driver accordingly.
> 
> Andy Shevchenko (7):
>   serial: 8250_ni: Switch to use uart_read_port_properties()
>   serial: 8250_ni: Remove duplicate mapping
>   serial: 8250_ni: Switch to use platform_get_mem_or_io()
>   serial: 8250_ni: Remove unneeded conditionals
>   serial: 8250_ni: use serial_port_in()/serial_port_out() helpers
>   serial: 8250_ni: Switch to use dev_err_probe()
>   serial: 8250_ni: Tidy up ACPI ID table
> 
>  drivers/tty/serial/8250/8250_ni.c | 89 +++++++++++++------------------
>  drivers/tty/serial/8250/Kconfig   |  2 +-
>  2 files changed, 38 insertions(+), 53 deletions(-)

Thanks for the patches! They look good and tested fine.

For the series,

Tested-by: Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>
Reviewed-by: Chaitanya Vadrevu <chaitanya.vadrevu@emerson.com>

Thanks,
Chaitanya