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

[PATCH v4 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 slightly 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
      [v3] https://lore.kernel.org/lkml/20240402174353.256627-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

Changes for V4:
Changes after Andy S. review comments:
- Fix commit description
- Fix includes for IWYU
- Add Reviewed-by tag (Andy) for patches 1 and 5
- Add exported symbols to SERIAL_NXP_SC16IS7XX namespace
- Fix Kconfig deps
- Keep Kconfig SERIAL_SC16IS7XX_CORE
- Convert to a single Kconfig entry and automatically selects
  I2C and SPI interfaces if available.
- Merge patches 2 and 3
- Add NXP and UART keywords to Kconfig description
- Remove __maybe_unused from of_device_id
- Simplify I2C init/exit with module_i2c_driver()
- Simplify SPI init/exit with module_spi_driver()

Hugo Villeneuve (5):
  serial: sc16is7xx: add proper sched.h include for sched_set_fifo()
  serial: sc16is7xx: unconditionally clear line bit in
    sc16is7xx_remove()
  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         |  47 ++----
 drivers/tty/serial/Makefile        |   2 +
 drivers/tty/serial/sc16is7xx.c     | 261 +++++++----------------------
 drivers/tty/serial/sc16is7xx.h     |  41 +++++
 drivers/tty/serial/sc16is7xx_i2c.c |  67 ++++++++
 drivers/tty/serial/sc16is7xx_spi.c |  90 ++++++++++
 6 files changed, 277 insertions(+), 231 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: fec50db7033ea478773b159e0e2efb135270e3b7
-- 
2.39.2

[PATCH v4 1/5] serial: sc16is7xx: add proper sched.h include for sched_set_fifo()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Replace incorrect include with the proper one for sched_set_fifo()
declaration.

Fixes: 28d2f209cd16 ("sched,serial: Convert to sched_set_fifo()")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 929206a9a6e11..34edc80859d5c 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/sched.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/tty.h>
@@ -25,7 +26,6 @@
 #include <linux/spi/spi.h>
 #include <linux/uaccess.h>
 #include <linux/units.h>
-#include <uapi/linux/sched/types.h>
 
 #define SC16IS7XX_NAME			"sc16is7xx"
 #define SC16IS7XX_MAX_DEVS		8
-- 
2.39.2

[PATCH v4 2/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>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
---
 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 34edc80859d5c..b971fd2a9a77e 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 v4 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 changes intended.

Also simplify and improve Kconfig entries.
  - Capitalize SPI keyword for consistency
  - Display list of supported ICs each on a separate line (and aligned) to
    facilitate locating a specific IC model
  - Add Manufacturer name at start of description string
  - Add UART keyword to description string

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/Kconfig         |  47 +++---
 drivers/tty/serial/Makefile        |   2 +
 drivers/tty/serial/sc16is7xx.c     | 225 +++++------------------------
 drivers/tty/serial/sc16is7xx.h     |  41 ++++++
 drivers/tty/serial/sc16is7xx_i2c.c |  64 ++++++++
 drivers/tty/serial/sc16is7xx_spi.c |  87 +++++++++++
 6 files changed, 248 insertions(+), 218 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 ffcf4882b25f9..9b5a65ea8ede3 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1021,41 +1021,30 @@ config SERIAL_SCCNXP_CONSOLE
 	  Support for console on SCCNXP serial ports.
 
 config SERIAL_SC16IS7XX_CORE
-	tristate
-
-config SERIAL_SC16IS7XX
-	tristate "SC16IS7xx serial support"
+	tristate "NXP SC16IS7xx UART support"
 	select SERIAL_CORE
-	depends on (SPI_MASTER && !I2C) || I2C
+	select SERIAL_SC16IS7XX_SPI if SPI_MASTER
+	select SERIAL_SC16IS7XX_I2C if 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 UARTs.
+	  Supported ICs are:
+
+	    SC16IS740
+	    SC16IS741
+	    SC16IS750
+	    SC16IS752
+	    SC16IS760
+	    SC16IS762
+
+	  The driver supports both I2C and SPI interfaces.
 
 config SERIAL_SC16IS7XX_I2C
-	bool "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
-	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.
+	tristate
+	select REGMAP_I2C
 
 config SERIAL_SC16IS7XX_SPI
-	bool "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
-	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.
+	tristate
+	select REGMAP_SPI
 
 config SERIAL_TIMBERDALE
 	tristate "Support for timberdale UART"
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index b25e9b54a6609..faa45f2b8bb05 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -76,6 +76,8 @@ 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_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 b971fd2a9a77e..51fb1c95d3826 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1,19 +1,22 @@
 // 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
+#undef DEFAULT_SYMBOL_NAMESPACE
+#define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX
 
 #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>
@@ -21,15 +24,15 @@
 #include <linux/sched.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
+#include <linux/string.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
-#include <linux/spi/spi.h>
 #include <linux/uaccess.h>
 #include <linux/units.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 +305,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 +488,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 +1464,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 +1657,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 +1681,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 +1692,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 +1708,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 +1720,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..2ee3ce83d95a4
--- /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/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#define SC16IS7XX_NAME		"sc16is7xx"
+#define SC16IS7XX_MAX_PORTS	2 /* Maximum number of UART ports per IC. */
+
+struct device;
+
+struct sc16is7xx_devtype {
+	char	name[10];
+	int	nr_gpio;
+	int	nr_uart;
+};
+
+extern struct regmap_config sc16is7xx_regcfg;
+
+extern const struct of_device_id 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..de51d1675abfd
--- /dev/null
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* SC16IS7xx I2C interface driver */
+
+#include <linux/dev_printk.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/string.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,
+};
+
+module_i2c_driver(sc16is7xx_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");
+MODULE_IMPORT_NS(SERIAL_NXP_SC16IS7XX);
diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
new file mode 100644
index 0000000000000..f110c4e6dce63
--- /dev/null
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* SC16IS7xx SPI interface driver */
+
+#include <linux/dev_printk.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/string.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,
+};
+
+module_spi_driver(sc16is7xx_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SC16IS7xx SPI interface driver");
+MODULE_IMPORT_NS(SERIAL_NXP_SC16IS7XX);
-- 
2.39.2

[PATCH v4 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 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.

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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 51fb1c95d3826..1eb440f0c9cd0 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -10,12 +10,12 @@
 #undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX
 
-#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/idr.h>
 #include <linux/kthread.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -345,7 +345,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,
@@ -1472,6 +1472,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]))
@@ -1536,13 +1537,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;
@@ -1588,7 +1595,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,
@@ -1646,9 +1653,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);
 
@@ -1671,7 +1681,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

[PATCH v4 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>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
---
 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 1eb440f0c9cd0..1e99ddd70b376 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1705,7 +1705,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 2ee3ce83d95a4..afb784eaee45b 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 sc16is7xx_dt_ids[];
 
diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
index de51d1675abfd..3ed47c306d855 100644
--- a/drivers/tty/serial/sc16is7xx_i2c.c
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -14,17 +14,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 f110c4e6dce63..73df36f8a7fd8 100644
--- a/drivers/tty/serial/sc16is7xx_spi.c
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -18,6 +18,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;
 
@@ -37,17 +38,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 v4 3/5] serial: sc16is7xx: split into core and I2C/SPI parts (core)

[Geert Uytterhoeven]

Hi Hugo,

On Tue, Apr 9, 2024 at 5:48 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 changes intended.
>
> Also simplify and improve Kconfig entries.
>   - Capitalize SPI keyword for consistency
>   - Display list of supported ICs each on a separate line (and aligned) to
>     facilitate locating a specific IC model
>   - Add Manufacturer name at start of description string
>   - Add UART keyword to description string
>
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Thanks for your patch, which is now commit d49216438139bca0
("serial: sc16is7xx: split into core and I2C/SPI parts (core)")
in tty-next (next-20240415 and later).

> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1021,41 +1021,30 @@ config SERIAL_SCCNXP_CONSOLE
>           Support for console on SCCNXP serial ports.
>
>  config SERIAL_SC16IS7XX_CORE
> -       tristate
> -
> -config SERIAL_SC16IS7XX
> -       tristate "SC16IS7xx serial support"
> +       tristate "NXP SC16IS7xx UART support"

Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
so arch/mips/configs/cu1??0-neo_defconfig needs to updated.

>         select SERIAL_CORE
> -       depends on (SPI_MASTER && !I2C) || I2C
> +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> +       select SERIAL_SC16IS7XX_I2C if I2C

So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
subdriver can no longer be disabled?  According to
https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
you did want to support that?

>         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 UARTs.
> +         Supported ICs are:
> +
> +           SC16IS740
> +           SC16IS741
> +           SC16IS750
> +           SC16IS752
> +           SC16IS760
> +           SC16IS762
> +
> +         The driver supports both I2C and SPI interfaces.
>
>  config SERIAL_SC16IS7XX_I2C
> -       bool "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
> -       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.
> +       tristate
> +       select REGMAP_I2C
>
>  config SERIAL_SC16IS7XX_SPI
> -       bool "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
> -       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.
> +       tristate
> +       select REGMAP_SPI
>
>  config SERIAL_TIMBERDALE
>         tristate "Support for timberdale UART"

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Tue, Apr 23, 2024 at 12:01 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Apr 9, 2024 at 5:48 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 changes intended.
> >
> > Also simplify and improve Kconfig entries.
> >   - Capitalize SPI keyword for consistency
> >   - Display list of supported ICs each on a separate line (and aligned) to
> >     facilitate locating a specific IC model
> >   - Add Manufacturer name at start of description string
> >   - Add UART keyword to description string
> >
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Thanks for your patch, which is now commit d49216438139bca0
> ("serial: sc16is7xx: split into core and I2C/SPI parts (core)")
> in tty-next (next-20240415 and later).
>
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1021,41 +1021,30 @@ config SERIAL_SCCNXP_CONSOLE
> >           Support for console on SCCNXP serial ports.
> >
> >  config SERIAL_SC16IS7XX_CORE
> > -       tristate
> > -
> > -config SERIAL_SC16IS7XX
> > -       tristate "SC16IS7xx serial support"
> > +       tristate "NXP SC16IS7xx UART support"
>
> Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> so arch/mips/configs/cu1??0-neo_defconfig needs to updated.

Or just rename SERIAL_SC16IS7XX_CORE back to SERIAL_SC16IS7XX.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Andy Shevchenko]

On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

...

> So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> subdriver can no longer be disabled?  According to
> https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> you did want to support that?

I believe it has been taken from one of the IIO drivers as an example.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Apr 23, 2024 at 1:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Apr 23, 2024 at 12:01 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

...

> Or just rename SERIAL_SC16IS7XX_CORE back to SERIAL_SC16IS7XX.

Since we do not know how many configurations elsewhere rely on this
and we have a real use case this suggestion seems plausible to me.

-- 
With Best Regards,
Andy Shevchenko

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

[Geert Uytterhoeven]

Hi Andy,

On Tue, Apr 23, 2024 at 12:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:

> > > -config SERIAL_SC16IS7XX
> > > -       tristate "SC16IS7xx serial support"
> > > +       tristate "NXP SC16IS7xx UART support"
> >
> > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
>
>         select SERIAL_CORE
> -       depends on (SPI_MASTER && !I2C) || I2C
> +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> +       select SERIAL_SC16IS7XX_I2C if I2C
>
> > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > subdriver can no longer be disabled?  According to
> > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > you did want to support that?
>
> I believe it has been taken from one of the IIO drivers as an example.

Looks like a bad example to follow:
  1. The driver question now pops up if both I2C and SPI_MASTER
     are disabled,
  2. What if SERIAL_SC16IS7XX_CORE is builtin, but I2C and/or
     SPI_MASTER are modular?

I believe the only way to fix that is by letting the sub-drivers select the
core driver, like before.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Hugo Villeneuve]

On Tue, 23 Apr 2024 12:01:23 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Hugo,
> 
> On Tue, Apr 9, 2024 at 5:48 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 changes intended.
> >
> > Also simplify and improve Kconfig entries.
> >   - Capitalize SPI keyword for consistency
> >   - Display list of supported ICs each on a separate line (and aligned) to
> >     facilitate locating a specific IC model
> >   - Add Manufacturer name at start of description string
> >   - Add UART keyword to description string
> >
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Thanks for your patch, which is now commit d49216438139bca0
> ("serial: sc16is7xx: split into core and I2C/SPI parts (core)")
> in tty-next (next-20240415 and later).
> 
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1021,41 +1021,30 @@ config SERIAL_SCCNXP_CONSOLE
> >           Support for console on SCCNXP serial ports.
> >
> >  config SERIAL_SC16IS7XX_CORE
> > -       tristate
> > -
> > -config SERIAL_SC16IS7XX
> > -       tristate "SC16IS7xx serial support"
> > +       tristate "NXP SC16IS7xx UART support"
> 
> Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> so arch/mips/configs/cu1??0-neo_defconfig needs to updated.

Hi Geert,
yes you are right, sorry I missed that.

> 
> >         select SERIAL_CORE
> > -       depends on (SPI_MASTER && !I2C) || I2C
> > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > +       select SERIAL_SC16IS7XX_I2C if I2C
> 
> So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> subdriver can no longer be disabled?  According to
> https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> you did want to support that?

Just to clarify, SPI subdriver will be disabled if
SPI_MASTER is disabled, even if I2C is enabled, and vice versa.

It was not my original intention, V1 of the patch offered the
possibility to enable/disable each subdriver individually, but Andy
pointed out that was maybe not the standard/usual/recommended way of
defining it, and to look into what other subsystems were doing,
especially iio. After some research, I settled on this approach as a
compromise.


Hugo.

> 
> >         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 UARTs.
> > +         Supported ICs are:
> > +
> > +           SC16IS740
> > +           SC16IS741
> > +           SC16IS750
> > +           SC16IS752
> > +           SC16IS760
> > +           SC16IS762
> > +
> > +         The driver supports both I2C and SPI interfaces.
> >
> >  config SERIAL_SC16IS7XX_I2C
> > -       bool "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
> > -       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.
> > +       tristate
> > +       select REGMAP_I2C
> >
> >  config SERIAL_SC16IS7XX_SPI
> > -       bool "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
> > -       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.
> > +       tristate
> > +       select REGMAP_SPI
> >
> >  config SERIAL_TIMBERDALE
> >         tristate "Support for timberdale UART"
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

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

[Hugo Villeneuve]

On Tue, 23 Apr 2024 13:37:37 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 23, 2024 at 1:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 23, 2024 at 12:01 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> ...
> 
> > Or just rename SERIAL_SC16IS7XX_CORE back to SERIAL_SC16IS7XX.
> 
> Since we do not know how many configurations elsewhere rely on this
> and we have a real use case this suggestion seems plausible to me.

Agreed.

I will look into it.

Hugo.

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

[Hugo Villeneuve]

On Tue, 23 Apr 2024 15:11:12 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

Hi Geert,

> Hi Andy,
> 
> On Tue, Apr 23, 2024 at 12:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> > > > -config SERIAL_SC16IS7XX
> > > > -       tristate "SC16IS7xx serial support"
> > > > +       tristate "NXP SC16IS7xx UART support"
> > >
> > > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
> >
> >         select SERIAL_CORE
> > -       depends on (SPI_MASTER && !I2C) || I2C
> > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > +       select SERIAL_SC16IS7XX_I2C if I2C
> >
> > > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > > subdriver can no longer be disabled?  According to
> > > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > > you did want to support that?
> >
> > I believe it has been taken from one of the IIO drivers as an example.
> 
> Looks like a bad example to follow:
>   1. The driver question now pops up if both I2C and SPI_MASTER
>      are disabled,

True.

V3 originally had this:

>  config SERIAL_SC16IS7XX
>         tristate "SC16IS7xx serial support"
>         select SERIAL_CORE
> -       depends on (SPI_MASTER && !I2C) || I2C
> +       depends on SPI_MASTER || I2C

And Andy commented "Is it?", which I probably misinterpreted as I should
not list them as dependencies.

Reintroducing "depends on SPI_MASTER || I2C" fixes this issue.


>   2. What if SERIAL_SC16IS7XX_CORE is builtin, but I2C and/or
>      SPI_MASTER are modular?

a) SERIAL_SC16IS7XX builtin and I2C modular:
CONFIG_SERIAL_SC16IS7XX=y
CONFIG_SERIAL_SC16IS7XX_I2C=m
CONFIG_SERIAL_SC16IS7XX_SPI=y

SPI_MASTER is only boolean and cannot be modular.

Hugo.


> 
> I believe the only way to fix that is by letting the sub-drivers select the
> core driver, like before.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
Hugo Villeneuve

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

[Hugo Villeneuve]

On Tue, 23 Apr 2024 09:14:10 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Tue, 23 Apr 2024 12:01:23 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > Hi Hugo,
> > 
> > On Tue, Apr 9, 2024 at 5:48 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 changes intended.
> > >
> > > Also simplify and improve Kconfig entries.
> > >   - Capitalize SPI keyword for consistency
> > >   - Display list of supported ICs each on a separate line (and aligned) to
> > >     facilitate locating a specific IC model
> > >   - Add Manufacturer name at start of description string
> > >   - Add UART keyword to description string
> > >
> > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Thanks for your patch, which is now commit d49216438139bca0
> > ("serial: sc16is7xx: split into core and I2C/SPI parts (core)")
> > in tty-next (next-20240415 and later).
> > 
> > > --- a/drivers/tty/serial/Kconfig
> > > +++ b/drivers/tty/serial/Kconfig
> > > @@ -1021,41 +1021,30 @@ config SERIAL_SCCNXP_CONSOLE
> > >           Support for console on SCCNXP serial ports.
> > >
> > >  config SERIAL_SC16IS7XX_CORE
> > > -       tristate
> > > -
> > > -config SERIAL_SC16IS7XX
> > > -       tristate "SC16IS7xx serial support"
> > > +       tristate "NXP SC16IS7xx UART support"
> > 
> > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
> 
> Hi Geert,
> yes you are right, sorry I missed that.
> 
> > 
> > >         select SERIAL_CORE
> > > -       depends on (SPI_MASTER && !I2C) || I2C
> > > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > > +       select SERIAL_SC16IS7XX_I2C if I2C
> > 
> > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > subdriver can no longer be disabled?  According to
> > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > you did want to support that?
> 
> Just to clarify, SPI subdriver will be disabled if
> SPI_MASTER is disabled, even if I2C is enabled, and vice versa.
> 
> It was not my original intention, V1 of the patch offered the
> possibility to enable/disable each subdriver individually, but Andy
> pointed out that was maybe not the standard/usual/recommended way of
> defining it, and to look into what other subsystems were doing,
> especially iio. After some research, I settled on this approach as a
> compromise.

Hi Andy and Geert,
if you are ok with what I explained, I will send a patch.

Hugo.


> 
> 
> Hugo.
> 
> > 
> > >         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 UARTs.
> > > +         Supported ICs are:
> > > +
> > > +           SC16IS740
> > > +           SC16IS741
> > > +           SC16IS750
> > > +           SC16IS752
> > > +           SC16IS760
> > > +           SC16IS762
> > > +
> > > +         The driver supports both I2C and SPI interfaces.
> > >
> > >  config SERIAL_SC16IS7XX_I2C
> > > -       bool "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
> > > -       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.
> > > +       tristate
> > > +       select REGMAP_I2C
> > >
> > >  config SERIAL_SC16IS7XX_SPI
> > > -       bool "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
> > > -       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.
> > > +       tristate
> > > +       select REGMAP_SPI
> > >
> > >  config SERIAL_TIMBERDALE
> > >         tristate "Support for timberdale UART"
> > 
> > Gr{oetje,eeting}s,
> > 
> >                         Geert
> > 
> > -- 
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> > 
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds
> > 
> 
> 


-- 
Hugo Villeneuve

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

[Geert Uytterhoeven]

On Tue, Apr 23, 2024 at 3:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Apr 23, 2024 at 12:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> > > > -config SERIAL_SC16IS7XX
> > > > -       tristate "SC16IS7xx serial support"
> > > > +       tristate "NXP SC16IS7xx UART support"
> > >
> > > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
> >
> >         select SERIAL_CORE
> > -       depends on (SPI_MASTER && !I2C) || I2C
> > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > +       select SERIAL_SC16IS7XX_I2C if I2C
> >
> > > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > > subdriver can no longer be disabled?  According to
> > > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > > you did want to support that?
> >
> > I believe it has been taken from one of the IIO drivers as an example.
>
> Looks like a bad example to follow:
>   1. The driver question now pops up if both I2C and SPI_MASTER
>      are disabled,
>   2. What if SERIAL_SC16IS7XX_CORE is builtin, but I2C and/or
>      SPI_MASTER are modular?
>
> I believe the only way to fix that is by letting the sub-drivers select the
> core driver, like before.

FTR, this issue is now upstream.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Hugo Villeneuve]

On Thu, 23 May 2024 09:33:36 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Tue, Apr 23, 2024 at 3:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 23, 2024 at 12:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > > > > -config SERIAL_SC16IS7XX
> > > > > -       tristate "SC16IS7xx serial support"
> > > > > +       tristate "NXP SC16IS7xx UART support"
> > > >
> > > > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > > > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
> > >
> > >         select SERIAL_CORE
> > > -       depends on (SPI_MASTER && !I2C) || I2C
> > > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > > +       select SERIAL_SC16IS7XX_I2C if I2C
> > >
> > > > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > > > subdriver can no longer be disabled?  According to
> > > > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > > > you did want to support that?
> > >
> > > I believe it has been taken from one of the IIO drivers as an example.
> >
> > Looks like a bad example to follow:
> >   1. The driver question now pops up if both I2C and SPI_MASTER
> >      are disabled,
> >   2. What if SERIAL_SC16IS7XX_CORE is builtin, but I2C and/or
> >      SPI_MASTER are modular?
> >
> > I believe the only way to fix that is by letting the sub-drivers select the
> > core driver, like before.
> 
> FTR, this issue is now upstream.

Hi Geert,
I replied to you and Andy a few weeks ago about this (multiple emails with suggestions/explanations), and I even asked if you were satisfied with what I proposed, but never got anything from you, so I am still waiting on feedback to send a patch to fix this:

https://lore.kernel.org/all/20240430090333.5c5f029553cabcdf699310cb@hugovil.com/

Hugo.

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

[Geert Uytterhoeven]

Hi Hugo,

On Fri, May 24, 2024 at 1:56 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Thu, 23 May 2024 09:33:36 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 23, 2024 at 3:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 23, 2024 at 12:37 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Apr 23, 2024 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Tue, Apr 9, 2024 at 5:48 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > >
> > > > > > -config SERIAL_SC16IS7XX
> > > > > > -       tristate "SC16IS7xx serial support"
> > > > > > +       tristate "NXP SC16IS7xx UART support"
> > > > >
> > > > > Hence this replaces SERIAL_SC16IS7XX_CORE by SERIAL_SC16IS7XX,
> > > > > so arch/mips/configs/cu1??0-neo_defconfig needs to updated.
> > > >
> > > >         select SERIAL_CORE
> > > > -       depends on (SPI_MASTER && !I2C) || I2C
> > > > +       select SERIAL_SC16IS7XX_SPI if SPI_MASTER
> > > > +       select SERIAL_SC16IS7XX_I2C if I2C
> > > >
> > > > > So if SPI_MASTER or I2C is enabled, the corresponding SERIAL_SC16IS7XX_*
> > > > > subdriver can no longer be disabled?  According to
> > > > > https://lore.kernel.org/all/20240403123501.8ef5c99f65a40ca2c10f635a@hugovil.com/
> > > > > you did want to support that?
> > > >
> > > > I believe it has been taken from one of the IIO drivers as an example.
> > >
> > > Looks like a bad example to follow:
> > >   1. The driver question now pops up if both I2C and SPI_MASTER
> > >      are disabled,
> > >   2. What if SERIAL_SC16IS7XX_CORE is builtin, but I2C and/or
> > >      SPI_MASTER are modular?
> > >
> > > I believe the only way to fix that is by letting the sub-drivers select the
> > > core driver, like before.
> >
> > FTR, this issue is now upstream.

> I replied to you and Andy a few weeks ago about this (multiple emails with suggestions/explanations), and I even asked if you were satisfied with what I proposed, but never got anything from you, so I am still waiting on feedback to send a patch to fix this:
>
> https://lore.kernel.org/all/20240430090333.5c5f029553cabcdf699310cb@hugovil.com/

Sorry, I indeed forgot to reply there.

Please use two visible symbols (for I2C and SPI), which select
the invisible SERIAL_SC16IS7XX_CORE.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds