[PATCH 0/4] serial: sc16is7xx: split into core and I2C/SPI parts

[PATCH 0/4] serial: sc16is7xx: split into core and I2C/SPI parts

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hello,
this patch series splits the SPI/I2C parts for the sc16is7xx driver into
separate source files (and separate I2C/SPI drivers).

These changes are based on suggestions made by Andy Shevchenko
following this discussion:

Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/

The patches are multiple, simply to facilitate the review process. In the end,
they could be merged into a single patch.

This patch series depends on the the following patch (in linux-next/stable):
    commit f7b487648986 ("lib/find: add atomic find_bit() primitives")

I have tested the changes on a custom board with two SC16IS752 DUART over
a SPI interface using a Variscite IMX8MN NANO SOM. The four UARTs are
configured in RS-485 mode.

I did not test the changes on a real SC16is7xx using the I2C interface. But
I slighly modified the driver to be able to simulate an I2C device using
i2c-stub. I was then able to instantiate a virtual I2C device without
disturbing existing connection/communication between real SPI devices on
/dev/ttySC0 and /dev/ttySC2 (using a loopback cable).

Thank you.

Hugo Villeneuve (4):
  serial: sc16is7xx: simplify and improve Kconfig help text
  serial: sc16is7xx: split into core and I2C/SPI parts (core)
  serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)
  serial: sc16is7xx: split into core and I2C/SPI parts
    (sc16is7xx_regcfg)

 drivers/tty/serial/Kconfig         |  41 +++--
 drivers/tty/serial/Makefile        |   4 +-
 drivers/tty/serial/sc16is7xx.c     | 242 ++++++-----------------------
 drivers/tty/serial/sc16is7xx.h     |  41 +++++
 drivers/tty/serial/sc16is7xx_i2c.c |  74 +++++++++
 drivers/tty/serial/sc16is7xx_spi.c |  97 ++++++++++++
 6 files changed, 279 insertions(+), 220 deletions(-)
 create mode 100644 drivers/tty/serial/sc16is7xx.h
 create mode 100644 drivers/tty/serial/sc16is7xx_i2c.c
 create mode 100644 drivers/tty/serial/sc16is7xx_spi.c


base-commit: 52b56990d214c7403b20f691ac61861a37c0f0db
-- 
2.39.2

[PATCH 1/4] serial: sc16is7xx: simplify and improve Kconfig help text

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Simplify and improve Kconfig help text for SC16IS7XX driver:

Especially get rid of the confusing:
    "If required say y, and say n to..."
in each of the I2C and SPI portions.

Capitalize SPI keyword for consistency.

Display list of supported ICs each on a separate line (and aligned) to
facilitate locating a specific IC model.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/Kconfig | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index ffcf4882b25f..48087e34ff52 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1028,9 +1028,18 @@ config SERIAL_SC16IS7XX
 	select SERIAL_CORE
 	depends on (SPI_MASTER && !I2C) || I2C
 	help
-	  This selects support for SC16IS7xx serial ports.
-	  Supported ICs are SC16IS740, SC16IS741, SC16IS750, SC16IS752,
-	  SC16IS760 and SC16IS762. Select supported buses using options below.
+	  Core driver for NXP SC16IS7xx serial ports.
+	  Supported ICs are:
+
+	    SC16IS740
+	    SC16IS741
+	    SC16IS750
+	    SC16IS752
+	    SC16IS760
+	    SC16IS762
+
+	  You also must select at least one of I2C and SPI interface
+	  drivers below.
 
 config SERIAL_SC16IS7XX_I2C
 	bool "SC16IS7xx for I2C interface"
@@ -1041,9 +1050,7 @@ config SERIAL_SC16IS7XX_I2C
 	default y
 	help
 	  Enable SC16IS7xx driver on I2C bus,
-	  If required say y, and say n to i2c if not required,
-	  Enabled by default to support oldconfig.
-	  You must select at least one bus for the driver to be built.
+	  enabled by default to support oldconfig.
 
 config SERIAL_SC16IS7XX_SPI
 	bool "SC16IS7xx for spi interface"
@@ -1052,10 +1059,7 @@ config SERIAL_SC16IS7XX_SPI
 	select SERIAL_SC16IS7XX_CORE if SERIAL_SC16IS7XX
 	select REGMAP_SPI if SPI_MASTER
 	help
-	  Enable SC16IS7xx driver on SPI bus,
-	  If required say y, and say n to spi if not required,
-	  This is additional support to existing driver.
-	  You must select at least one bus for the driver to be built.
+	  Enable SC16IS7xx driver on SPI bus.
 
 config SERIAL_TIMBERDALE
 	tristate "Support for timberdale UART"
-- 
2.39.2

[PATCH 2/4] serial: sc16is7xx: split into core and I2C/SPI parts (core)

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Split the common code from sc16is7xx driver and move the I2C and SPI bus
parts into interface-specific source files.

sc16is7xx becomes the core functions which can support multiple bus
interfaces like I2C and SPI.

No functional change intended.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/Kconfig         |  19 +--
 drivers/tty/serial/Makefile        |   4 +-
 drivers/tty/serial/sc16is7xx.c     | 223 +++++------------------------
 drivers/tty/serial/sc16is7xx.h     |  41 ++++++
 drivers/tty/serial/sc16is7xx_i2c.c |  71 +++++++++
 drivers/tty/serial/sc16is7xx_spi.c |  94 ++++++++++++
 6 files changed, 248 insertions(+), 204 deletions(-)
 create mode 100644 drivers/tty/serial/sc16is7xx.h
 create mode 100644 drivers/tty/serial/sc16is7xx_i2c.c
 create mode 100644 drivers/tty/serial/sc16is7xx_spi.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 48087e34ff52..40343fdf7896 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1020,13 +1020,10 @@ config SERIAL_SCCNXP_CONSOLE
 	help
 	  Support for console on SCCNXP serial ports.
 
-config SERIAL_SC16IS7XX_CORE
-	tristate
-
 config SERIAL_SC16IS7XX
 	tristate "SC16IS7xx serial support"
 	select SERIAL_CORE
-	depends on (SPI_MASTER && !I2C) || I2C
+	depends on SPI_MASTER || I2C
 	help
 	  Core driver for NXP SC16IS7xx serial ports.
 	  Supported ICs are:
@@ -1042,22 +1039,18 @@ config SERIAL_SC16IS7XX
 	  drivers below.
 
 config SERIAL_SC16IS7XX_I2C
-	bool "SC16IS7xx for I2C interface"
+	tristate "SC16IS7xx for I2C interface"
 	depends on SERIAL_SC16IS7XX
 	depends on I2C
-	select SERIAL_SC16IS7XX_CORE if SERIAL_SC16IS7XX
-	select REGMAP_I2C if I2C
-	default y
+	select REGMAP_I2C
 	help
-	  Enable SC16IS7xx driver on I2C bus,
-	  enabled by default to support oldconfig.
+	  Enable SC16IS7xx driver on I2C bus.
 
 config SERIAL_SC16IS7XX_SPI
-	bool "SC16IS7xx for spi interface"
+	tristate "SC16IS7xx for SPI interface"
 	depends on SERIAL_SC16IS7XX
 	depends on SPI_MASTER
-	select SERIAL_SC16IS7XX_CORE if SERIAL_SC16IS7XX
-	select REGMAP_SPI if SPI_MASTER
+	select REGMAP_SPI
 	help
 	  Enable SC16IS7xx driver on SPI bus.
 
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index b25e9b54a660..9f51b933915a 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -75,7 +75,9 @@ obj-$(CONFIG_SERIAL_SA1100)		+= sa1100.o
 obj-$(CONFIG_SERIAL_SAMSUNG)		+= samsung_tty.o
 obj-$(CONFIG_SERIAL_SB1250_DUART)	+= sb1250-duart.o
 obj-$(CONFIG_SERIAL_SCCNXP)		+= sccnxp.o
-obj-$(CONFIG_SERIAL_SC16IS7XX_CORE)	+= sc16is7xx.o
+obj-$(CONFIG_SERIAL_SC16IS7XX_SPI)	+= sc16is7xx_spi.o
+obj-$(CONFIG_SERIAL_SC16IS7XX_I2C)	+= sc16is7xx_i2c.o
+obj-$(CONFIG_SERIAL_SC16IS7XX)		+= sc16is7xx.o
 obj-$(CONFIG_SERIAL_SH_SCI)		+= sh-sci.o
 obj-$(CONFIG_SERIAL_SIFIVE)		+= sifive.o
 obj-$(CONFIG_SERIAL_SPRD)		+= sprd_serial.o
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 929206a9a6e1..5b53c88b7133 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1,19 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * SC16IS7xx tty serial driver - Copyright (C) 2014 GridPoint
- * Author: Jon Ringle <jringle@gridpoint.com>
+ * SC16IS7xx tty serial driver - common code
  *
- *  Based on max310x.c, by Alexander Shiyan <shc_work@mail.ru>
+ * Copyright (C) 2014 GridPoint
+ * Author: Jon Ringle <jringle@gridpoint.com>
+ * Based on max310x.c, by Alexander Shiyan <shc_work@mail.ru>
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/export.h>
 #include <linux/gpio/driver.h>
-#include <linux/i2c.h>
+#include <linux/kthread.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/property.h>
@@ -22,14 +22,13 @@
 #include <linux/serial.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
-#include <linux/spi/spi.h>
 #include <linux/uaccess.h>
 #include <linux/units.h>
 #include <uapi/linux/sched/types.h>
 
-#define SC16IS7XX_NAME			"sc16is7xx"
+#include "sc16is7xx.h"
+
 #define SC16IS7XX_MAX_DEVS		8
-#define SC16IS7XX_MAX_PORTS		2 /* Maximum number of UART ports per IC. */
 
 /* SC16IS7XX register definitions */
 #define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
@@ -302,16 +301,9 @@
 
 
 /* Misc definitions */
-#define SC16IS7XX_SPI_READ_BIT		BIT(7)
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_GPIOS_PER_BANK	4
 
-struct sc16is7xx_devtype {
-	char	name[10];
-	int	nr_gpio;
-	int	nr_uart;
-};
-
 #define SC16IS7XX_RECONF_MD		(1 << 0)
 #define SC16IS7XX_RECONF_IER		(1 << 1)
 #define SC16IS7XX_RECONF_RS485		(1 << 2)
@@ -492,35 +484,40 @@ static void sc16is7xx_stop_rx(struct uart_port *port)
 	sc16is7xx_ier_clear(port, SC16IS7XX_IER_RDI_BIT);
 }
 
-static const struct sc16is7xx_devtype sc16is74x_devtype = {
+const struct sc16is7xx_devtype sc16is74x_devtype = {
 	.name		= "SC16IS74X",
 	.nr_gpio	= 0,
 	.nr_uart	= 1,
 };
+EXPORT_SYMBOL_GPL(sc16is74x_devtype);
 
-static const struct sc16is7xx_devtype sc16is750_devtype = {
+const struct sc16is7xx_devtype sc16is750_devtype = {
 	.name		= "SC16IS750",
 	.nr_gpio	= 8,
 	.nr_uart	= 1,
 };
+EXPORT_SYMBOL_GPL(sc16is750_devtype);
 
-static const struct sc16is7xx_devtype sc16is752_devtype = {
+const struct sc16is7xx_devtype sc16is752_devtype = {
 	.name		= "SC16IS752",
 	.nr_gpio	= 8,
 	.nr_uart	= 2,
 };
+EXPORT_SYMBOL_GPL(sc16is752_devtype);
 
-static const struct sc16is7xx_devtype sc16is760_devtype = {
+const struct sc16is7xx_devtype sc16is760_devtype = {
 	.name		= "SC16IS760",
 	.nr_gpio	= 8,
 	.nr_uart	= 1,
 };
+EXPORT_SYMBOL_GPL(sc16is760_devtype);
 
-static const struct sc16is7xx_devtype sc16is762_devtype = {
+const struct sc16is7xx_devtype sc16is762_devtype = {
 	.name		= "SC16IS762",
 	.nr_gpio	= 8,
 	.nr_uart	= 2,
 };
+EXPORT_SYMBOL_GPL(sc16is762_devtype);
 
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 {
@@ -1463,9 +1460,8 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
 };
 
-static int sc16is7xx_probe(struct device *dev,
-			   const struct sc16is7xx_devtype *devtype,
-			   struct regmap *regmaps[], int irq)
+int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
+		    struct regmap *regmaps[], int irq)
 {
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	unsigned int val;
@@ -1657,8 +1653,9 @@ static int sc16is7xx_probe(struct device *dev,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sc16is7xx_probe);
 
-static void sc16is7xx_remove(struct device *dev)
+void sc16is7xx_remove(struct device *dev)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 	int i;
@@ -1680,8 +1677,9 @@ static void sc16is7xx_remove(struct device *dev)
 
 	clk_disable_unprepare(s->clk);
 }
+EXPORT_SYMBOL_GPL(sc16is7xx_remove);
 
-static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
+const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
 	{ .compatible = "nxp,sc16is740",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is741",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is750",	.data = &sc16is750_devtype, },
@@ -1690,9 +1688,10 @@ static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
 	{ .compatible = "nxp,sc16is762",	.data = &sc16is762_devtype, },
 	{ }
 };
+EXPORT_SYMBOL_GPL(sc16is7xx_dt_ids);
 MODULE_DEVICE_TABLE(of, sc16is7xx_dt_ids);
 
-static struct regmap_config regcfg = {
+struct regmap_config sc16is7xx_regcfg = {
 	.reg_bits = 5,
 	.pad_bits = 3,
 	.val_bits = 8,
@@ -1705,8 +1704,9 @@ static struct regmap_config regcfg = {
 	.max_raw_write = SC16IS7XX_FIFO_SIZE,
 	.max_register = SC16IS7XX_EFCR_REG,
 };
+EXPORT_SYMBOL_GPL(sc16is7xx_regcfg);
 
-static const char *sc16is7xx_regmap_name(u8 port_id)
+const char *sc16is7xx_regmap_name(u8 port_id)
 {
 	switch (port_id) {
 	case 0:	return "port0";
@@ -1716,184 +1716,27 @@ static const char *sc16is7xx_regmap_name(u8 port_id)
 		return NULL;
 	}
 }
+EXPORT_SYMBOL_GPL(sc16is7xx_regmap_name);
 
-static unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id)
+unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id)
 {
 	/* CH1,CH0 are at bits 2:1. */
 	return port_id << 1;
 }
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
-static int sc16is7xx_spi_probe(struct spi_device *spi)
-{
-	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
-	unsigned int i;
-	int ret;
-
-	/* Setup SPI bus */
-	spi->bits_per_word	= 8;
-	/* For all variants, only mode 0 is supported */
-	if ((spi->mode & SPI_MODE_X_MASK) != SPI_MODE_0)
-		return dev_err_probe(&spi->dev, -EINVAL, "Unsupported SPI mode\n");
-
-	spi->mode		= spi->mode ? : SPI_MODE_0;
-	spi->max_speed_hz	= spi->max_speed_hz ? : 4 * HZ_PER_MHZ;
-	ret = spi_setup(spi);
-	if (ret)
-		return ret;
-
-	devtype = spi_get_device_match_data(spi);
-	if (!devtype)
-		return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n");
-
-	for (i = 0; i < devtype->nr_uart; i++) {
-		regcfg.name = sc16is7xx_regmap_name(i);
-		/*
-		 * If read_flag_mask is 0, the regmap code sets it to a default
-		 * of 0x80. Since we specify our own mask, we must add the READ
-		 * bit ourselves:
-		 */
-		regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i) |
-			SC16IS7XX_SPI_READ_BIT;
-		regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
-		regmaps[i] = devm_regmap_init_spi(spi, &regcfg);
-	}
-
-	return sc16is7xx_probe(&spi->dev, devtype, regmaps, spi->irq);
-}
-
-static void sc16is7xx_spi_remove(struct spi_device *spi)
-{
-	sc16is7xx_remove(&spi->dev);
-}
-
-static const struct spi_device_id sc16is7xx_spi_id_table[] = {
-	{ "sc16is74x",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is740",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is741",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is750",	(kernel_ulong_t)&sc16is750_devtype, },
-	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
-	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
-	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
-	{ }
-};
-
-MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table);
-
-static struct spi_driver sc16is7xx_spi_uart_driver = {
-	.driver = {
-		.name		= SC16IS7XX_NAME,
-		.of_match_table	= sc16is7xx_dt_ids,
-	},
-	.probe		= sc16is7xx_spi_probe,
-	.remove		= sc16is7xx_spi_remove,
-	.id_table	= sc16is7xx_spi_id_table,
-};
-#endif
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
-static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
-{
-	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
-	unsigned int i;
-
-	devtype = i2c_get_match_data(i2c);
-	if (!devtype)
-		return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");
-
-	for (i = 0; i < devtype->nr_uart; i++) {
-		regcfg.name = sc16is7xx_regmap_name(i);
-		regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i);
-		regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
-		regmaps[i] = devm_regmap_init_i2c(i2c, &regcfg);
-	}
-
-	return sc16is7xx_probe(&i2c->dev, devtype, regmaps, i2c->irq);
-}
-
-static void sc16is7xx_i2c_remove(struct i2c_client *client)
-{
-	sc16is7xx_remove(&client->dev);
-}
-
-static const struct i2c_device_id sc16is7xx_i2c_id_table[] = {
-	{ "sc16is74x",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is740",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is741",	(kernel_ulong_t)&sc16is74x_devtype, },
-	{ "sc16is750",	(kernel_ulong_t)&sc16is750_devtype, },
-	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
-	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
-	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, sc16is7xx_i2c_id_table);
-
-static struct i2c_driver sc16is7xx_i2c_uart_driver = {
-	.driver = {
-		.name		= SC16IS7XX_NAME,
-		.of_match_table	= sc16is7xx_dt_ids,
-	},
-	.probe		= sc16is7xx_i2c_probe,
-	.remove		= sc16is7xx_i2c_remove,
-	.id_table	= sc16is7xx_i2c_id_table,
-};
-
-#endif
+EXPORT_SYMBOL_GPL(sc16is7xx_regmap_port_mask);
 
 static int __init sc16is7xx_init(void)
 {
-	int ret;
-
-	ret = uart_register_driver(&sc16is7xx_uart);
-	if (ret) {
-		pr_err("Registering UART driver failed\n");
-		return ret;
-	}
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
-	ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
-	if (ret < 0) {
-		pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
-		goto err_i2c;
-	}
-#endif
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
-	ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
-	if (ret < 0) {
-		pr_err("failed to init sc16is7xx spi --> %d\n", ret);
-		goto err_spi;
-	}
-#endif
-	return ret;
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
-err_spi:
-#endif
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
-	i2c_del_driver(&sc16is7xx_i2c_uart_driver);
-err_i2c:
-#endif
-	uart_unregister_driver(&sc16is7xx_uart);
-	return ret;
+	return uart_register_driver(&sc16is7xx_uart);
 }
 module_init(sc16is7xx_init);
 
 static void __exit sc16is7xx_exit(void)
 {
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
-	i2c_del_driver(&sc16is7xx_i2c_uart_driver);
-#endif
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
-	spi_unregister_driver(&sc16is7xx_spi_uart_driver);
-#endif
 	uart_unregister_driver(&sc16is7xx_uart);
 }
 module_exit(sc16is7xx_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Jon Ringle <jringle@gridpoint.com>");
-MODULE_DESCRIPTION("SC16IS7XX serial driver");
+MODULE_DESCRIPTION("SC16IS7xx tty serial core driver");
diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h
new file mode 100644
index 000000000000..410f34b1005c
--- /dev/null
+++ b/drivers/tty/serial/sc16is7xx.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/* SC16IS7xx SPI/I2C tty serial driver */
+
+#ifndef _SC16IS7XX_H_
+#define _SC16IS7XX_H_
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/serial_core.h>
+#include <linux/types.h>
+
+#define SC16IS7XX_NAME		"sc16is7xx"
+#define SC16IS7XX_MAX_PORTS	2 /* Maximum number of UART ports per IC. */
+
+struct sc16is7xx_devtype {
+	char	name[10];
+	int	nr_gpio;
+	int	nr_uart;
+};
+
+extern struct regmap_config sc16is7xx_regcfg;
+
+extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];
+
+extern const struct sc16is7xx_devtype sc16is74x_devtype;
+extern const struct sc16is7xx_devtype sc16is750_devtype;
+extern const struct sc16is7xx_devtype sc16is752_devtype;
+extern const struct sc16is7xx_devtype sc16is760_devtype;
+extern const struct sc16is7xx_devtype sc16is762_devtype;
+
+const char *sc16is7xx_regmap_name(u8 port_id);
+
+unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id);
+
+int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
+		    struct regmap *regmaps[], int irq);
+
+void sc16is7xx_remove(struct device *dev);
+
+#endif /* _SC16IS7XX_H_ */
diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
new file mode 100644
index 000000000000..70f0c329cc13
--- /dev/null
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* SC16IS7xx I2C interface driver */
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "sc16is7xx.h"
+
+static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
+{
+	const struct sc16is7xx_devtype *devtype;
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
+	unsigned int i;
+
+	devtype = i2c_get_match_data(i2c);
+	if (!devtype)
+		return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");
+
+	for (i = 0; i < devtype->nr_uart; i++) {
+		sc16is7xx_regcfg.name = sc16is7xx_regmap_name(i);
+		sc16is7xx_regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i);
+		sc16is7xx_regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
+		regmaps[i] = devm_regmap_init_i2c(i2c, &sc16is7xx_regcfg);
+	}
+
+	return sc16is7xx_probe(&i2c->dev, devtype, regmaps, i2c->irq);
+}
+
+static void sc16is7xx_i2c_remove(struct i2c_client *client)
+{
+	sc16is7xx_remove(&client->dev);
+}
+
+static const struct i2c_device_id sc16is7xx_i2c_id_table[] = {
+	{ "sc16is74x",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is740",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is741",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is750",	(kernel_ulong_t)&sc16is750_devtype, },
+	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
+	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
+	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sc16is7xx_i2c_id_table);
+
+static struct i2c_driver sc16is7xx_i2c_driver = {
+	.driver = {
+		.name		= SC16IS7XX_NAME,
+		.of_match_table	= sc16is7xx_dt_ids,
+	},
+	.probe		= sc16is7xx_i2c_probe,
+	.remove		= sc16is7xx_i2c_remove,
+	.id_table	= sc16is7xx_i2c_id_table,
+};
+
+static int __init sc16is7xx_i2c_init(void)
+{
+	return i2c_add_driver(&sc16is7xx_i2c_driver);
+}
+module_init(sc16is7xx_i2c_init);
+
+static void __exit sc16is7xx_i2c_exit(void)
+{
+	i2c_del_driver(&sc16is7xx_i2c_driver);
+}
+module_exit(sc16is7xx_i2c_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");
diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
new file mode 100644
index 000000000000..3942fc1b7455
--- /dev/null
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* SC16IS7xx SPI interface driver */
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#include "sc16is7xx.h"
+
+/* SPI definitions */
+#define SC16IS7XX_SPI_READ_BIT	BIT(7)
+
+static int sc16is7xx_spi_probe(struct spi_device *spi)
+{
+	const struct sc16is7xx_devtype *devtype;
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
+	unsigned int i;
+	int ret;
+
+	/* Setup SPI bus */
+	spi->bits_per_word	= 8;
+	/* For all variants, only mode 0 is supported */
+	if ((spi->mode & SPI_MODE_X_MASK) != SPI_MODE_0)
+		return dev_err_probe(&spi->dev, -EINVAL, "Unsupported SPI mode\n");
+
+	spi->mode		= spi->mode ? : SPI_MODE_0;
+	spi->max_speed_hz	= spi->max_speed_hz ? : 4 * HZ_PER_MHZ;
+	ret = spi_setup(spi);
+	if (ret)
+		return ret;
+
+	devtype = spi_get_device_match_data(spi);
+	if (!devtype)
+		return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n");
+
+	for (i = 0; i < devtype->nr_uart; i++) {
+		sc16is7xx_regcfg.name = sc16is7xx_regmap_name(i);
+		/*
+		 * If read_flag_mask is 0, the regmap code sets it to a default
+		 * of 0x80. Since we specify our own mask, we must add the READ
+		 * bit ourselves:
+		 */
+		sc16is7xx_regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i) |
+			SC16IS7XX_SPI_READ_BIT;
+		sc16is7xx_regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
+		regmaps[i] = devm_regmap_init_spi(spi, &sc16is7xx_regcfg);
+	}
+
+	return sc16is7xx_probe(&spi->dev, devtype, regmaps, spi->irq);
+}
+
+static void sc16is7xx_spi_remove(struct spi_device *spi)
+{
+	sc16is7xx_remove(&spi->dev);
+}
+
+static const struct spi_device_id sc16is7xx_spi_id_table[] = {
+	{ "sc16is74x",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is740",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is741",	(kernel_ulong_t)&sc16is74x_devtype, },
+	{ "sc16is750",	(kernel_ulong_t)&sc16is750_devtype, },
+	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
+	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
+	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table);
+
+static struct spi_driver sc16is7xx_spi_driver = {
+	.driver = {
+		.name		= SC16IS7XX_NAME,
+		.of_match_table	= sc16is7xx_dt_ids,
+	},
+	.probe		= sc16is7xx_spi_probe,
+	.remove		= sc16is7xx_spi_remove,
+	.id_table	= sc16is7xx_spi_id_table,
+};
+
+static int __init sc16is7xx_spi_init(void)
+{
+	return spi_register_driver(&sc16is7xx_spi_driver);
+}
+module_init(sc16is7xx_spi_init);
+
+static void __exit sc16is7xx_spi_exit(void)
+{
+	spi_unregister_driver(&sc16is7xx_spi_driver);
+}
+module_exit(sc16is7xx_spi_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SC16IS7xx SPI interface driver");
-- 
2.39.2

[PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Before, sc16is7xx_lines was checked for a free (zero) bit first, and then
later it was set only if UART port registration succeeded.

Now that sc16is7xx_lines is shared for the I2C and SPI drivers, make sure
it is reserved and modified atomically, and use a new variable to hold the
status of UART port regisration.

Remove need to check for previous port registration in sc16is7xx_remove(),
because if sc16is7xx_probe() succeeded, we are guaranteed to have
successfully registered both ports.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5b53c88b7133..5073ebfc45bf 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1468,6 +1468,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
+	bool port_registered[SC16IS7XX_MAX_PORTS];
 
 	for (i = 0; i < devtype->nr_uart; i++)
 		if (IS_ERR(regmaps[i]))
@@ -1533,8 +1534,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
-		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
-							SC16IS7XX_MAX_DEVS);
+		port_registered[i] = false;
+
+		s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
+						     SC16IS7XX_MAX_DEVS);
 		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
 			ret = -ERANGE;
 			goto out_ports;
@@ -1584,7 +1587,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		if (ret)
 			goto out_ports;
 
-		set_bit(s->p[i].port.line, sc16is7xx_lines);
+		port_registered[i] = true;
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
@@ -1642,9 +1645,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 #endif
 
 out_ports:
-	for (i = 0; i < devtype->nr_uart; i++)
-		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
+	for (i = 0; i < devtype->nr_uart; i++) {
+		clear_bit(s->p[i].port.line, sc16is7xx_lines);
+		if (port_registered[i])
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+	}
 
 	kthread_stop(s->kworker_task);
 
@@ -1667,8 +1672,8 @@ void sc16is7xx_remove(struct device *dev)
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
 		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
-		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
-			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+		clear_bit(s->p[i].port.line, sc16is7xx_lines);
+		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
-- 
2.39.2

[PATCH 4/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_regcfg)

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Since each I2C/SPI probe function can modify sc16is7xx_regcfg at the same
time, change structure to be constant and do the required modifications on
a local copy.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c     |  2 +-
 drivers/tty/serial/sc16is7xx.h     |  2 +-
 drivers/tty/serial/sc16is7xx_i2c.c | 11 +++++++----
 drivers/tty/serial/sc16is7xx_spi.c | 11 +++++++----
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5073ebfc45bf..706c8b18250b 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1696,7 +1696,7 @@ const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
 EXPORT_SYMBOL_GPL(sc16is7xx_dt_ids);
 MODULE_DEVICE_TABLE(of, sc16is7xx_dt_ids);
 
-struct regmap_config sc16is7xx_regcfg = {
+const struct regmap_config sc16is7xx_regcfg = {
 	.reg_bits = 5,
 	.pad_bits = 3,
 	.val_bits = 8,
diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h
index 410f34b1005c..8d03357e35c7 100644
--- a/drivers/tty/serial/sc16is7xx.h
+++ b/drivers/tty/serial/sc16is7xx.h
@@ -19,7 +19,7 @@ struct sc16is7xx_devtype {
 	int	nr_uart;
 };
 
-extern struct regmap_config sc16is7xx_regcfg;
+extern const struct regmap_config sc16is7xx_regcfg;
 
 extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];
 
diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
index 70f0c329cc13..5667c56cf2aa 100644
--- a/drivers/tty/serial/sc16is7xx_i2c.c
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -12,17 +12,20 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
 {
 	const struct sc16is7xx_devtype *devtype;
 	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
+	struct regmap_config regcfg;
 	unsigned int i;
 
 	devtype = i2c_get_match_data(i2c);
 	if (!devtype)
 		return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");
 
+	memcpy(&regcfg, &sc16is7xx_regcfg, sizeof(struct regmap_config));
+
 	for (i = 0; i < devtype->nr_uart; i++) {
-		sc16is7xx_regcfg.name = sc16is7xx_regmap_name(i);
-		sc16is7xx_regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i);
-		sc16is7xx_regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
-		regmaps[i] = devm_regmap_init_i2c(i2c, &sc16is7xx_regcfg);
+		regcfg.name = sc16is7xx_regmap_name(i);
+		regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i);
+		regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
+		regmaps[i] = devm_regmap_init_i2c(i2c, &regcfg);
 	}
 
 	return sc16is7xx_probe(&i2c->dev, devtype, regmaps, i2c->irq);
diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
index 3942fc1b7455..55c1d4ad83f5 100644
--- a/drivers/tty/serial/sc16is7xx_spi.c
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -16,6 +16,7 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
 {
 	const struct sc16is7xx_devtype *devtype;
 	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
+	struct regmap_config regcfg;
 	unsigned int i;
 	int ret;
 
@@ -35,17 +36,19 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
 	if (!devtype)
 		return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n");
 
+	memcpy(&regcfg, &sc16is7xx_regcfg, sizeof(struct regmap_config));
+
 	for (i = 0; i < devtype->nr_uart; i++) {
-		sc16is7xx_regcfg.name = sc16is7xx_regmap_name(i);
+		regcfg.name = sc16is7xx_regmap_name(i);
 		/*
 		 * If read_flag_mask is 0, the regmap code sets it to a default
 		 * of 0x80. Since we specify our own mask, we must add the READ
 		 * bit ourselves:
 		 */
-		sc16is7xx_regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i) |
+		regcfg.read_flag_mask = sc16is7xx_regmap_port_mask(i) |
 			SC16IS7XX_SPI_READ_BIT;
-		sc16is7xx_regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
-		regmaps[i] = devm_regmap_init_spi(spi, &sc16is7xx_regcfg);
+		regcfg.write_flag_mask = sc16is7xx_regmap_port_mask(i);
+		regmaps[i] = devm_regmap_init_spi(spi, &regcfg);
 	}
 
 	return sc16is7xx_probe(&spi->dev, devtype, regmaps, spi->irq);
-- 
2.39.2

Re: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)

[kernel test robot]

Hi Hugo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 52b56990d214c7403b20f691ac61861a37c0f0db]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-simplify-and-improve-Kconfig-help-text/20240207-061642
base:   52b56990d214c7403b20f691ac61861a37c0f0db
patch link:    https://lore.kernel.org/r/20240206214208.2141067-4-hugo%40hugovil.com
patch subject: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)
config: i386-buildonly-randconfig-003-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080139.jQSw8VKg-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402080139.jQSw8VKg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402080139.jQSw8VKg-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/tty/serial/sc16is7xx.c:1539:23: error: call to undeclared function 'find_and_set_bit'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1539 |                 s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
         |                                     ^
   drivers/tty/serial/sc16is7xx.c:1539:23: note: did you mean 'test_and_set_bit'?
   include/asm-generic/bitops/instrumented-atomic.h:68:29: note: 'test_and_set_bit' declared here
      68 | static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
         |                             ^
   1 error generated.


vim +/find_and_set_bit +1539 drivers/tty/serial/sc16is7xx.c

  1462	
  1463	int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
  1464			    struct regmap *regmaps[], int irq)
  1465	{
  1466		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1467		unsigned int val;
  1468		u32 uartclk = 0;
  1469		int i, ret;
  1470		struct sc16is7xx_port *s;
  1471		bool port_registered[SC16IS7XX_MAX_PORTS];
  1472	
  1473		for (i = 0; i < devtype->nr_uart; i++)
  1474			if (IS_ERR(regmaps[i]))
  1475				return PTR_ERR(regmaps[i]);
  1476	
  1477		/*
  1478		 * This device does not have an identification register that would
  1479		 * tell us if we are really connected to the correct device.
  1480		 * The best we can do is to check if communication is at all possible.
  1481		 *
  1482		 * Note: regmap[0] is used in the probe function to access registers
  1483		 * common to all channels/ports, as it is guaranteed to be present on
  1484		 * all variants.
  1485		 */
  1486		ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
  1487		if (ret < 0)
  1488			return -EPROBE_DEFER;
  1489	
  1490		/* Alloc port structure */
  1491		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1492		if (!s) {
  1493			dev_err(dev, "Error allocating port structure\n");
  1494			return -ENOMEM;
  1495		}
  1496	
  1497		/* Always ask for fixed clock rate from a property. */
  1498		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1499	
  1500		s->clk = devm_clk_get_optional(dev, NULL);
  1501		if (IS_ERR(s->clk))
  1502			return PTR_ERR(s->clk);
  1503	
  1504		ret = clk_prepare_enable(s->clk);
  1505		if (ret)
  1506			return ret;
  1507	
  1508		freq = clk_get_rate(s->clk);
  1509		if (freq == 0) {
  1510			if (uartclk)
  1511				freq = uartclk;
  1512			if (pfreq)
  1513				freq = *pfreq;
  1514			if (freq)
  1515				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1516			else
  1517				return -EINVAL;
  1518		}
  1519	
  1520		s->devtype = devtype;
  1521		dev_set_drvdata(dev, s);
  1522	
  1523		kthread_init_worker(&s->kworker);
  1524		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1525					      "sc16is7xx");
  1526		if (IS_ERR(s->kworker_task)) {
  1527			ret = PTR_ERR(s->kworker_task);
  1528			goto out_clk;
  1529		}
  1530		sched_set_fifo(s->kworker_task);
  1531	
  1532		/* reset device, purging any pending irq / data */
  1533		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
  1534			     SC16IS7XX_IOCONTROL_SRESET_BIT);
  1535	
  1536		for (i = 0; i < devtype->nr_uart; ++i) {
  1537			port_registered[i] = false;
  1538	
> 1539			s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
  1540							     SC16IS7XX_MAX_DEVS);
  1541			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1542				ret = -ERANGE;
  1543				goto out_ports;
  1544			}
  1545	
  1546			/* Initialize port data */
  1547			s->p[i].port.dev	= dev;
  1548			s->p[i].port.irq	= irq;
  1549			s->p[i].port.type	= PORT_SC16IS7XX;
  1550			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1551			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1552			s->p[i].port.iobase	= i;
  1553			/*
  1554			 * Use all ones as membase to make sure uart_configure_port() in
  1555			 * serial_core.c does not abort for SPI/I2C devices where the
  1556			 * membase address is not applicable.
  1557			 */
  1558			s->p[i].port.membase	= (void __iomem *)~0;
  1559			s->p[i].port.iotype	= UPIO_PORT;
  1560			s->p[i].port.uartclk	= freq;
  1561			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1562			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1563			s->p[i].port.ops	= &sc16is7xx_ops;
  1564			s->p[i].old_mctrl	= 0;
  1565			s->p[i].regmap		= regmaps[i];
  1566	
  1567			mutex_init(&s->p[i].efr_lock);
  1568	
  1569			ret = uart_get_rs485_mode(&s->p[i].port);
  1570			if (ret)
  1571				goto out_ports;
  1572	
  1573			/* Disable all interrupts */
  1574			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1575			/* Disable TX/RX */
  1576			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1577					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1578					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1579	
  1580			/* Initialize kthread work structs */
  1581			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1582			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1583			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1584	
  1585			/* Register port */
  1586			ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1587			if (ret)
  1588				goto out_ports;
  1589	
  1590			port_registered[i] = true;
  1591	
  1592			/* Enable EFR */
  1593			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1594					     SC16IS7XX_LCR_CONF_MODE_B);
  1595	
  1596			regcache_cache_bypass(regmaps[i], true);
  1597	
  1598			/* Enable write access to enhanced features */
  1599			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1600					     SC16IS7XX_EFR_ENABLE_BIT);
  1601	
  1602			regcache_cache_bypass(regmaps[i], false);
  1603	
  1604			/* Restore access to general registers */
  1605			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1606	
  1607			/* Go to suspend mode */
  1608			sc16is7xx_power(&s->p[i].port, 0);
  1609		}
  1610	
  1611		sc16is7xx_setup_irda_ports(s);
  1612	
  1613		ret = sc16is7xx_setup_mctrl_ports(s, regmaps[0]);
  1614		if (ret)
  1615			goto out_ports;
  1616	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)

[kernel test robot]

Hi Hugo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 52b56990d214c7403b20f691ac61861a37c0f0db]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-simplify-and-improve-Kconfig-help-text/20240207-061642
base:   52b56990d214c7403b20f691ac61861a37c0f0db
patch link:    https://lore.kernel.org/r/20240206214208.2141067-4-hugo%40hugovil.com
patch subject: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)
config: i386-randconfig-001-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080730.RUiQXpMA-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402080730.RUiQXpMA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402080730.RUiQXpMA-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1539:37: error: implicit declaration of function 'find_and_set_bit'; did you mean 'test_and_set_bit'? [-Werror=implicit-function-declaration]
    1539 |                 s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
         |                                     ^~~~~~~~~~~~~~~~
         |                                     test_and_set_bit
   cc1: some warnings being treated as errors


vim +1539 drivers/tty/serial/sc16is7xx.c

  1462	
  1463	int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
  1464			    struct regmap *regmaps[], int irq)
  1465	{
  1466		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1467		unsigned int val;
  1468		u32 uartclk = 0;
  1469		int i, ret;
  1470		struct sc16is7xx_port *s;
  1471		bool port_registered[SC16IS7XX_MAX_PORTS];
  1472	
  1473		for (i = 0; i < devtype->nr_uart; i++)
  1474			if (IS_ERR(regmaps[i]))
  1475				return PTR_ERR(regmaps[i]);
  1476	
  1477		/*
  1478		 * This device does not have an identification register that would
  1479		 * tell us if we are really connected to the correct device.
  1480		 * The best we can do is to check if communication is at all possible.
  1481		 *
  1482		 * Note: regmap[0] is used in the probe function to access registers
  1483		 * common to all channels/ports, as it is guaranteed to be present on
  1484		 * all variants.
  1485		 */
  1486		ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
  1487		if (ret < 0)
  1488			return -EPROBE_DEFER;
  1489	
  1490		/* Alloc port structure */
  1491		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1492		if (!s) {
  1493			dev_err(dev, "Error allocating port structure\n");
  1494			return -ENOMEM;
  1495		}
  1496	
  1497		/* Always ask for fixed clock rate from a property. */
  1498		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1499	
  1500		s->clk = devm_clk_get_optional(dev, NULL);
  1501		if (IS_ERR(s->clk))
  1502			return PTR_ERR(s->clk);
  1503	
  1504		ret = clk_prepare_enable(s->clk);
  1505		if (ret)
  1506			return ret;
  1507	
  1508		freq = clk_get_rate(s->clk);
  1509		if (freq == 0) {
  1510			if (uartclk)
  1511				freq = uartclk;
  1512			if (pfreq)
  1513				freq = *pfreq;
  1514			if (freq)
  1515				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1516			else
  1517				return -EINVAL;
  1518		}
  1519	
  1520		s->devtype = devtype;
  1521		dev_set_drvdata(dev, s);
  1522	
  1523		kthread_init_worker(&s->kworker);
  1524		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1525					      "sc16is7xx");
  1526		if (IS_ERR(s->kworker_task)) {
  1527			ret = PTR_ERR(s->kworker_task);
  1528			goto out_clk;
  1529		}
  1530		sched_set_fifo(s->kworker_task);
  1531	
  1532		/* reset device, purging any pending irq / data */
  1533		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
  1534			     SC16IS7XX_IOCONTROL_SRESET_BIT);
  1535	
  1536		for (i = 0; i < devtype->nr_uart; ++i) {
  1537			port_registered[i] = false;
  1538	
> 1539			s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
  1540							     SC16IS7XX_MAX_DEVS);
  1541			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1542				ret = -ERANGE;
  1543				goto out_ports;
  1544			}
  1545	
  1546			/* Initialize port data */
  1547			s->p[i].port.dev	= dev;
  1548			s->p[i].port.irq	= irq;
  1549			s->p[i].port.type	= PORT_SC16IS7XX;
  1550			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1551			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1552			s->p[i].port.iobase	= i;
  1553			/*
  1554			 * Use all ones as membase to make sure uart_configure_port() in
  1555			 * serial_core.c does not abort for SPI/I2C devices where the
  1556			 * membase address is not applicable.
  1557			 */
  1558			s->p[i].port.membase	= (void __iomem *)~0;
  1559			s->p[i].port.iotype	= UPIO_PORT;
  1560			s->p[i].port.uartclk	= freq;
  1561			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1562			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1563			s->p[i].port.ops	= &sc16is7xx_ops;
  1564			s->p[i].old_mctrl	= 0;
  1565			s->p[i].regmap		= regmaps[i];
  1566	
  1567			mutex_init(&s->p[i].efr_lock);
  1568	
  1569			ret = uart_get_rs485_mode(&s->p[i].port);
  1570			if (ret)
  1571				goto out_ports;
  1572	
  1573			/* Disable all interrupts */
  1574			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1575			/* Disable TX/RX */
  1576			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1577					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1578					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1579	
  1580			/* Initialize kthread work structs */
  1581			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1582			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1583			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1584	
  1585			/* Register port */
  1586			ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1587			if (ret)
  1588				goto out_ports;
  1589	
  1590			port_registered[i] = true;
  1591	
  1592			/* Enable EFR */
  1593			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1594					     SC16IS7XX_LCR_CONF_MODE_B);
  1595	
  1596			regcache_cache_bypass(regmaps[i], true);
  1597	
  1598			/* Enable write access to enhanced features */
  1599			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1600					     SC16IS7XX_EFR_ENABLE_BIT);
  1601	
  1602			regcache_cache_bypass(regmaps[i], false);
  1603	
  1604			/* Restore access to general registers */
  1605			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1606	
  1607			/* Go to suspend mode */
  1608			sc16is7xx_power(&s->p[i].port, 0);
  1609		}
  1610	
  1611		sc16is7xx_setup_irda_ports(s);
  1612	
  1613		ret = sc16is7xx_setup_mctrl_ports(s, regmaps[0]);
  1614		if (ret)
  1615			goto out_ports;
  1616	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)

[Hugo Villeneuve]

On Tue,  6 Feb 2024 16:42:07 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Before, sc16is7xx_lines was checked for a free (zero) bit first, and then
> later it was set only if UART port registration succeeded.
> 
> Now that sc16is7xx_lines is shared for the I2C and SPI drivers, make sure
> it is reserved and modified atomically, and use a new variable to hold the
> status of UART port regisration.
> 
> Remove need to check for previous port registration in sc16is7xx_remove(),
> because if sc16is7xx_probe() succeeded, we are guaranteed to have
> successfully registered both ports.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 5b53c88b7133..5073ebfc45bf 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1468,6 +1468,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	u32 uartclk = 0;
>  	int i, ret;
>  	struct sc16is7xx_port *s;
> +	bool port_registered[SC16IS7XX_MAX_PORTS];
>  
>  	for (i = 0; i < devtype->nr_uart; i++)
>  		if (IS_ERR(regmaps[i]))
> @@ -1533,8 +1534,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		     SC16IS7XX_IOCONTROL_SRESET_BIT);
>  
>  	for (i = 0; i < devtype->nr_uart; ++i) {
> -		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
> -							SC16IS7XX_MAX_DEVS);
> +		port_registered[i] = false;

I just realised that this will not work. All port_registered[]
members need to be initialized before entering the loop.

I will add this (for V2) before the loop:

  memset(port_registered, 0, sizeof(port_registered));

Hugo Villeneuve


> +
> +		s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
> +						     SC16IS7XX_MAX_DEVS);
>  		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
>  			ret = -ERANGE;
>  			goto out_ports;
> @@ -1584,7 +1587,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		if (ret)
>  			goto out_ports;
>  
> -		set_bit(s->p[i].port.line, sc16is7xx_lines);
> +		port_registered[i] = true;
>  
>  		/* Enable EFR */
>  		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
> @@ -1642,9 +1645,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  #endif
>  
>  out_ports:
> -	for (i = 0; i < devtype->nr_uart; i++)
> -		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
> +	for (i = 0; i < devtype->nr_uart; i++) {
> +		clear_bit(s->p[i].port.line, sc16is7xx_lines);
> +		if (port_registered[i])
>  			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> +	}
>  
>  	kthread_stop(s->kworker_task);
>  
> @@ -1667,8 +1672,8 @@ void sc16is7xx_remove(struct device *dev)
>  
>  	for (i = 0; i < s->devtype->nr_uart; i++) {
>  		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
> -		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
> -			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> +		clear_bit(s->p[i].port.line, sc16is7xx_lines);
> +		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
>  		sc16is7xx_power(&s->p[i].port, 0);
>  	}
>  
> -- 
> 2.39.2
> 
>