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

[PATCH v3 0/5] 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 changes are split into multiple patches to facilitate the review process.
In the end, some of them could be merged into a single patch.

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.

Link: [v1] https://lore.kernel.org/lkml/20240206214208.2141067-1-hugo@hugovil.com
      [v2] https://lore.kernel.org/lkml/20240307161828.118495-1-hugo@hugovil.com

Changes for V2:
- Move port_registered[] init before the for loop to prevent bug if
  aborting probe when i = 0
- Since patch f7b487648986 ("lib/find: add atomic find_bit() primitives") has
  been dropped from linux-next/master, rework patch 2 (sc16is7xx_lines)
  without find_and_set_bit.

Changes for V3:
- Simplify sc16is7xx_lines allocation by using the IDA framework

Hugo Villeneuve (5):
  serial: sc16is7xx: unconditionally clear line bit in
    sc16is7xx_remove()
  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     | 255 ++++++-----------------------
 drivers/tty/serial/sc16is7xx.h     |  41 +++++
 drivers/tty/serial/sc16is7xx_i2c.c |  74 +++++++++
 drivers/tty/serial/sc16is7xx_spi.c |  97 +++++++++++
 6 files changed, 288 insertions(+), 224 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: 39cd87c4eb2b893354f3b850f916353f2658ae6f
-- 
2.39.2

[PATCH v3 1/5] serial: sc16is7xx: unconditionally clear line bit in sc16is7xx_remove()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

There is no need to check for previous port registration in
sc16is7xx_remove() because if sc16is7xx_probe() succeeded, we are
guaranteed to have successfully registered both ports. We can thus
unconditionally clear the line allocation bit in sc16is7xx_lines.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 929206a9a6e11..eec6fa8b0da79 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1670,8 +1670,8 @@ static 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 v3 2/5] 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 ffcf4882b25f9..48087e34ff527 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 v3 3/5] 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 48087e34ff527..40343fdf78964 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 b25e9b54a6609..9f51b933915a5 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 eec6fa8b0da79..05e5cbc0f9e8b 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 0000000000000..410f34b1005c3
--- /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 0000000000000..70f0c329cc133
--- /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 0000000000000..3942fc1b7455a
--- /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

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

[Andy Shevchenko]

On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> 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.

...

> -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

Is it?

>         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.

Hmm... What I was thinking about is more like dropping
 the SERIAL_SC16IS7XX and having I2C/SPI to select the core.

See many examples under drivers/iio on how it's done.

...

> +EXPORT_SYMBOL_GPL(sc16is74x_devtype);

Is it namespaced? Please make sure we are not polluting the global
namespace with these.

...

> +#ifndef _SC16IS7XX_H_
> +#define _SC16IS7XX_H_
> +
> +#include <linux/device.h>

Not used (by this file).

> +#include <linux/mod_devicetable.h>

> +#include <linux/regmap.h>

> +#include <linux/serial_core.h>

Not used.

> +#include <linux/types.h>

> +extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];

No __maybe_unused. Just have it always be added.

> +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);

Will require forward declaration

#include ...

struct device;

> +#endif /* _SC16IS7XX_H_ */

...

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

Follow the IWYU principle (include what you use).

...

> +               return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");

+ dev_printk.h

...

> +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);

This is now module_i2c_driver().

...

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");

+ MODULE_IMPORT_NS()

...

> +++ b/drivers/tty/serial/sc16is7xx_spi.c

Similar/same comments as per i2c counterpart.

-- 
With Best Regards,
Andy Shevchenko

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

[Hugo Villeneuve]

On Tue, 2 Apr 2024 22:40:07 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

Hi Andy,

> On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > 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.
> 
> ...
> 
> > -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
> 
> Is it?

See discussion below, but I would remove the SPI/I2C depends. And I
would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.

> 
> >         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.
> 
> Hmm... What I was thinking about is more like dropping
>  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> 
> See many examples under drivers/iio on how it's done.

Ok, I found this example:
bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")

In it, the SPI part doesn't select the core, but depends on it, similar
to what I have done. I find this approach more interesting for
embedded systems as you can enable/disable I2C or SPI part if you
need only one interface.

> 
> ...
> 
> > +EXPORT_SYMBOL_GPL(sc16is74x_devtype);
> 
> Is it namespaced? Please make sure we are not polluting the global
> namespace with these.

Ok, will add namespace support to all exports.


> 
> ...
> 
> > +#ifndef _SC16IS7XX_H_
> > +#define _SC16IS7XX_H_
> > +
> > +#include <linux/device.h>
> 
> Not used (by this file).

I was assuming that this file was for "struct device"?


> 
> > +#include <linux/mod_devicetable.h>
> 
> > +#include <linux/regmap.h>
> 
> > +#include <linux/serial_core.h>
> 
> Not used.

Ok, will drop for V4.


> 
> > +#include <linux/types.h>
> 
> > +extern const struct of_device_id __maybe_unused sc16is7xx_dt_ids[];
> 
> No __maybe_unused. Just have it always be added.

Ok.


> 
> > +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);
> 
> Will require forward declaration
> 
> #include ...
> 
> struct device;

Isn't it provided by <linux/device.h> ?


> 
> > +#endif /* _SC16IS7XX_H_ */
> 
> ...
> 
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> 
> Follow the IWYU principle (include what you use).

Ok, I tried to follow it, I do think those 4 includes are required in
this file, no?

and maybe I can add <linux/string.h> for memcpy().


> 
> ...
> 
> > +               return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");
> 
> + dev_printk.h

Ok.


> 
> ...
> 
> > +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);
> 
> This is now module_i2c_driver().

Ok. Great, simplifies things.


> 
> ...
> 
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");
> 
> + MODULE_IMPORT_NS()

Ok.


> 
> ...
> 
> > +++ b/drivers/tty/serial/sc16is7xx_spi.c
> 
> Similar/same comments as per i2c counterpart.

Ok.


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 


-- 
Hugo Villeneuve

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

[Andy Shevchenko]

On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 2 Apr 2024 22:40:07 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

...

> > > -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
> >
> > Is it?
>
> See discussion below, but I would remove the SPI/I2C depends. And I
> would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.
>
> >
> > >         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.
> >
> > Hmm... What I was thinking about is more like dropping
> >  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> >
> > See many examples under drivers/iio on how it's done.
>
> Ok, I found this example:
> bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")
>
> In it, the SPI part doesn't select the core, but depends on it, similar
> to what I have done. I find this approach more interesting for
> embedded systems as you can enable/disable I2C or SPI part if you
> need only one interface.

Yes, but what I mean is to have i2c/spi symbols visible and if user
selects one of them or both the core is automatically being selected.

...

> > > +#include <linux/device.h>
> >
> > Not used (by this file).
>
> I was assuming that this file was for "struct device"?

But it does not use it. It uses an opaque pointer, for which the
forward declaration is enough to have.

...

> > > +void sc16is7xx_remove(struct device *dev);
> >
> > Will require forward declaration
> >
> > #include ...
> >
> > struct device;
>
> Isn't it provided by <linux/device.h> ?

But why? Have you looked into device.h? It's a mess. You don't need it
in this header.

...

> > Follow the IWYU principle (include what you use).
>
> Ok, I tried to follow it, I do think those 4 includes are required in
> this file, no?

I haven't deeply checked, I believe for the next version you will
provide a better list.

> and maybe I can add <linux/string.h> for memcpy().

For sure, yes.

--
With Best Regards,
Andy Shevchenko

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

[Hugo Villeneuve]

On Wed, 3 Apr 2024 20:44:05 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 2 Apr 2024 22:40:07 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> ...
> 
> > > > -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
> > >
> > > Is it?
> >
> > See discussion below, but I would remove the SPI/I2C depends. And I
> > would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.
> >
> > >
> > > >         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.
> > >
> > > Hmm... What I was thinking about is more like dropping
> > >  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> > >
> > > See many examples under drivers/iio on how it's done.
> >
> > Ok, I found this example:
> > bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")
> >
> > In it, the SPI part doesn't select the core, but depends on it, similar
> > to what I have done. I find this approach more interesting for
> > embedded systems as you can enable/disable I2C or SPI part if you
> > need only one interface.
> 
> Yes, but what I mean is to have i2c/spi symbols visible and if user
> selects one of them or both the core is automatically being selected.

Ok, I see. But a drawback of doing this is that for each interface
(I2C/SPI), you would need to list (duplicate) all the devices
supported. For now, that list is only in one place,
for the generic SERIAL_SC16IS7XX_CORE section:

...
config SERIAL_SC16IS7XX_CORE
	tristate "SC16IS7xx serial support"
	select SERIAL_CORE
	help
	  Core driver for NXP SC16IS7xx serial ports.
	  Supported ICs are:

	    SC16IS740
	    SC16IS741
	    SC16IS750
...


> 
> ...
> 
> > > > +#include <linux/device.h>
> > >
> > > Not used (by this file).
> >
> > I was assuming that this file was for "struct device"?
> 
> But it does not use it. It uses an opaque pointer, for which the
> forward declaration is enough to have.
> 
> ...
> 
> > > > +void sc16is7xx_remove(struct device *dev);
> > >
> > > Will require forward declaration
> > >
> > > #include ...
> > >
> > > struct device;
> >
> > Isn't it provided by <linux/device.h> ?
> 
> But why? Have you looked into device.h? It's a mess. You don't need it
> in this header.

Yes I have looked at it, and saw that the forward declaration of
"struct device" opaque pointer is in it, and this is why I was
including it. But I will remove it if you wish.


> 
> ...
> 
> > > Follow the IWYU principle (include what you use).
> >
> > Ok, I tried to follow it, I do think those 4 includes are required in
> > this file, no?
> 
> I haven't deeply checked, I believe for the next version you will
> provide a better list.

Ok

> 
> > and maybe I can add <linux/string.h> for memcpy().
> 
> For sure, yes.

Ok.

Thanks for your comments.

Hugo.

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

[Andy Shevchenko]

On Wed, Apr 3, 2024 at 8:59 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Wed, 3 Apr 2024 20:44:05 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 2 Apr 2024 22:40:07 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

...

> > > > > -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
> > > >
> > > > Is it?
> > >
> > > See discussion below, but I would remove the SPI/I2C depends. And I
> > > would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.
> > >
> > > >
> > > > >         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.
> > > >
> > > > Hmm... What I was thinking about is more like dropping
> > > >  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> > > >
> > > > See many examples under drivers/iio on how it's done.
> > >
> > > Ok, I found this example:
> > > bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")
> > >
> > > In it, the SPI part doesn't select the core, but depends on it, similar
> > > to what I have done. I find this approach more interesting for
> > > embedded systems as you can enable/disable I2C or SPI part if you
> > > need only one interface.
> >
> > Yes, but what I mean is to have i2c/spi symbols visible and if user
> > selects one of them or both the core is automatically being selected.
>
> Ok, I see. But a drawback of doing this is that for each interface
> (I2C/SPI), you would need to list (duplicate) all the devices
> supported. For now, that list is only in one place,
> for the generic SERIAL_SC16IS7XX_CORE section:
>
> ...
> config SERIAL_SC16IS7XX_CORE
>         tristate "SC16IS7xx serial support"
>         select SERIAL_CORE
>         help
>           Core driver for NXP SC16IS7xx serial ports.
>           Supported ICs are:
>
>             SC16IS740
>             SC16IS741
>             SC16IS750

Hmm... How do the other drivers have this separation? So, check
several examples (and check _the recently added_) in IIO and follow
that. If they have CORE visible, follow it.

...

> > > Isn't it provided by <linux/device.h> ?
> >
> > But why? Have you looked into device.h? It's a mess. You don't need it
> > in this header.
>
> Yes I have looked at it, and saw that the forward declaration of
> "struct device" opaque pointer is in it, and this is why I was
> including it. But I will remove it if you wish.

This file doesn't use the struct device, it uses an opaque pointer. In
C for this the forward declaration is enough, no need to include a
whole mess from device.h.

-- 
With Best Regards,
Andy Shevchenko

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

[Hugo Villeneuve]

On Wed, 3 Apr 2024 21:04:29 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 3, 2024 at 8:59 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Wed, 3 Apr 2024 20:44:05 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Wed, Apr 3, 2024 at 7:35 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > On Tue, 2 Apr 2024 22:40:07 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> ...
> 
> > > > > > -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
> > > > >
> > > > > Is it?
> > > >
> > > > See discussion below, but I would remove the SPI/I2C depends. And I
> > > > would rename SERIAL_SC16IS7XX to SERIAL_SC16IS7XX_CORE.
> > > >
> > > > >
> > > > > >         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.
> > > > >
> > > > > Hmm... What I was thinking about is more like dropping
> > > > >  the SERIAL_SC16IS7XX and having I2C/SPI to select the core.
> > > > >
> > > > > See many examples under drivers/iio on how it's done.
> > > >
> > > > Ok, I found this example:
> > > > bf96f6e80cef ("iio: accel: kxsd9: Split out SPI transport")
> > > >
> > > > In it, the SPI part doesn't select the core, but depends on it, similar
> > > > to what I have done. I find this approach more interesting for
> > > > embedded systems as you can enable/disable I2C or SPI part if you
> > > > need only one interface.
> > >
> > > Yes, but what I mean is to have i2c/spi symbols visible and if user
> > > selects one of them or both the core is automatically being selected.
> >
> > Ok, I see. But a drawback of doing this is that for each interface
> > (I2C/SPI), you would need to list (duplicate) all the devices
> > supported. For now, that list is only in one place,
> > for the generic SERIAL_SC16IS7XX_CORE section:
> >
> > ...
> > config SERIAL_SC16IS7XX_CORE
> >         tristate "SC16IS7xx serial support"
> >         select SERIAL_CORE
> >         help
> >           Core driver for NXP SC16IS7xx serial ports.
> >           Supported ICs are:
> >
> >             SC16IS740
> >             SC16IS741
> >             SC16IS750
> 
> Hmm... How do the other drivers have this separation? So, check
> several examples (and check _the recently added_) in IIO and follow
> that. If they have CORE visible, follow it.

In iio/accel, there is a roughly equal mix of everything. But all
examples that automatically select CORE do not list any specific
variants, probably because of the maintenance burden that this would
impose like I mentioned.

The most recent one is bmi088, which has only one KConfig
selection available, with I2C/SPI parts automatically selected, and
with the benefit of only having to list all variants once. The only
minor drawback of course is having to (almost always) compile an
interface (I2C or SPI) which won't be used.

I don't mind going this way for V4.


> 
> ...
> 
> > > > Isn't it provided by <linux/device.h> ?
> > >
> > > But why? Have you looked into device.h? It's a mess. You don't need it
> > > in this header.
> >
> > Yes I have looked at it, and saw that the forward declaration of
> > "struct device" opaque pointer is in it, and this is why I was
> > including it. But I will remove it if you wish.
> 
> This file doesn't use the struct device, it uses an opaque pointer. In
> C for this the forward declaration is enough, no need to include a
> whole mess from device.h.

Ok.

Hugo.


> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 


-- 
Hugo Villeneuve

[PATCH v3 4/5] 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, there is a
possibility that the two drivers can simultaneously try to reserve the same
line bit at the same time.

To prevent this, make sure line allocation is reserved atomically, and use
a new variable to hold the status of UART port regisration.

Now that we no longer need to search if a bit is set, it is now possible
to simplify sc16is7xx_lines allocation by using the IDA framework.

Link: https://lore.kernel.org/all/20231212150302.a9ec5d085a4ba65e89ca41af@hugovil.com/
Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 05e5cbc0f9e8b..58ab6b16860b8 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -341,7 +341,7 @@ struct sc16is7xx_port {
 	struct sc16is7xx_one		p[];
 };
 
-static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
+static DEFINE_IDA(sc16is7xx_lines);
 
 static struct uart_driver sc16is7xx_uart = {
 	.owner		= THIS_MODULE,
@@ -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]))
@@ -1532,13 +1533,19 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
+	/* Mark each port line and status as uninitialised. */
 	for (i = 0; i < devtype->nr_uart; ++i) {
-		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
-							SC16IS7XX_MAX_DEVS);
-		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
-			ret = -ERANGE;
+		s->p[i].port.line = SC16IS7XX_MAX_DEVS;
+		port_registered[i] = false;
+	}
+
+	for (i = 0; i < devtype->nr_uart; ++i) {
+		ret = ida_alloc_max(&sc16is7xx_lines,
+				    SC16IS7XX_MAX_DEVS - 1, GFP_KERNEL);
+		if (ret < 0)
 			goto out_ports;
-		}
+
+		s->p[i].port.line = ret;
 
 		/* Initialize port data */
 		s->p[i].port.dev	= dev;
@@ -1584,7 +1591,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 +1649,12 @@ 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++) {
+		if (s->p[i].port.line < SC16IS7XX_MAX_DEVS)
+			ida_free(&sc16is7xx_lines, s->p[i].port.line);
+		if (port_registered[i])
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+	}
 
 	kthread_stop(s->kworker_task);
 
@@ -1667,7 +1677,7 @@ 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);
-		clear_bit(s->p[i].port.line, sc16is7xx_lines);
+		ida_free(&sc16is7xx_lines, s->p[i].port.line);
 		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
-- 
2.39.2

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

[Andy Shevchenko]

On Tue, Apr 2, 2024 at 8:45 PM 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, there is a
> possibility that the two drivers can simultaneously try to reserve the same
> line bit at the same time.
>
> To prevent this, make sure line allocation is reserved atomically, and use
> a new variable to hold the status of UART port regisration.

registration

> Now that we no longer need to search if a bit is set, it is now possible
> to simplify sc16is7xx_lines allocation by using the IDA framework.

...

> -static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
> +static DEFINE_IDA(sc16is7xx_lines);

Don't we need to replace bitmap.h with idr.h with this change in place?

-- 
With Best Regards,
Andy Shevchenko

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

[Hugo Villeneuve]

On Tue, 2 Apr 2024 22:28:25 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 2, 2024 at 8:45 PM 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, there is a
> > possibility that the two drivers can simultaneously try to reserve the same
> > line bit at the same time.
> >
> > To prevent this, make sure line allocation is reserved atomically, and use
> > a new variable to hold the status of UART port regisration.
> 
> registration

Hi Andy,
will fix for V4.

> 
> > Now that we no longer need to search if a bit is set, it is now possible
> > to simplify sc16is7xx_lines allocation by using the IDA framework.
> 
> ...
> 
> > -static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
> > +static DEFINE_IDA(sc16is7xx_lines);
> 
> Don't we need to replace bitmap.h with idr.h with this change in place?

Yes, but I will replace bitops.h with idr.h in V4 (bitmap.h was not
included).

While at it, I will include an additional patch to replace inlude of 
<uapi/linux/sched/types.h> with <linux/sched.h>.

Thank you,
Hugo.

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

[Andy Shevchenko]

On Tue, Apr 2, 2024 at 10:51 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 2 Apr 2024 22:28:25 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

...

> > > -static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
> > > +static DEFINE_IDA(sc16is7xx_lines);
> >
> > Don't we need to replace bitmap.h with idr.h with this change in place?
>
> Yes, but I will replace bitops.h with idr.h in V4 (bitmap.h was not
> included).

Yep, that's what I meant.

...

> While at it, I will include an additional patch to replace inlude of
> <uapi/linux/sched/types.h> with <linux/sched.h>.

Sounds good to me.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/5] 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 58ab6b16860b8..9d2d527dd7039 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1701,7 +1701,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 410f34b1005c3..8d03357e35c7e 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 70f0c329cc133..5667c56cf2aac 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 3942fc1b7455a..55c1d4ad83f5d 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 v3 0/5] serial: sc16is7xx: split into core and I2C/SPI parts

[Andy Shevchenko]

On Tue, Apr 2, 2024 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> 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 changes are split into multiple patches to facilitate the review process.
> In the end, some of them could be merged into a single patch.
>
> 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

slightly

> 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).

For patches 1,2,5
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko