[PATCH 00/15] serial: sc16is7xx: register access fixes and improvements

[PATCH 00/15] serial: sc16is7xx: register access fixes and improvements

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hello,
this patch series brings some fixes and improvements to the register
accesses for the sc16is7xx driver.

It also adds some minor improvements, and cleanups to uniformize code
style accross the driver.

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 change on a SC16is7xx using I2C interface, as my custom
board is only using SPI.

Thank you.

Hugo Villeneuve (15):
  serial: sc16is7xx: remove useless enable of enhanced features
  serial: sc16is7xx: rename LCR macros to better reflect usage
  serial: sc16is7xx: rename EFR mutex with generic name
  serial: sc16is7xx: define common register access function
  serial: sc16is7xx: use guards for simple mutex locks
  serial: sc16is7xx: use dev_err_probe() instead of dev_err()
  serial: sc16is7xx: declare SPR/TLR/XOFF2 register as volatile
  serial: sc16is7xx: move port/channel init to separate function
  serial: sc16is7xx: Kconfig: allow building with COMPILE_TEST
  serial: sc16is7xx: use KBUILD_MODNAME
  serial: sc16is7xx: remove empty line
  serial: sc16is7xx: add missing space between macro args (checkpatch)
  serial: sc16is7xx: change incorrect indentation
  serial: sc16is7xx: reformat comments
  serial: sc16is7xx: add/improve comments

 drivers/tty/serial/Kconfig         |   2 +-
 drivers/tty/serial/sc16is7xx.c     | 275 ++++++++++++++---------------
 drivers/tty/serial/sc16is7xx.h     |   1 -
 drivers/tty/serial/sc16is7xx_i2c.c |   4 +-
 drivers/tty/serial/sc16is7xx_spi.c |   4 +-
 5 files changed, 139 insertions(+), 147 deletions(-)


base-commit: f4abab350840d58d69814c6993736f03ac27df83
-- 
2.39.5

[PATCH 01/15] serial: sc16is7xx: remove useless enable of enhanced features

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Commit 43c51bb573aa ("sc16is7xx: make sure device is in suspend once
probed") permanently enabled access to the enhanced features in
sc16is7xx_probe(), and it is never disabled after that.

Therefore, remove useless re-enable of enhanced features in
sc16is7xx_set_baud().

Fixes: 43c51bb573aa ("sc16is7xx: make sure device is in suspend once probed")
Cc: stable@vger.kernel.org
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 1a2c4c14f6aac..c7435595dce13 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -588,13 +588,6 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 		div /= prescaler;
 	}
 
-	/* Enable enhanced features */
-	sc16is7xx_efr_lock(port);
-	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
-			      SC16IS7XX_EFR_ENABLE_BIT,
-			      SC16IS7XX_EFR_ENABLE_BIT);
-	sc16is7xx_efr_unlock(port);
-
 	/* If bit MCR_CLKSEL is set, the divide by 4 prescaler is activated. */
 	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
 			      SC16IS7XX_MCR_CLKSEL_BIT,
-- 
2.39.5

[PATCH 02/15] serial: sc16is7xx: rename LCR macros to better reflect usage

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

There is no reference to CONF_MODE_A or CONF_MODE_B in the manufacturer's
datasheet.

Rename register set configuration macros for the LCR register, to better
show their intended usage to select either the Special register set, or the
Enhanced register set.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index c7435595dce13..330d95446f1d7 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -149,10 +149,12 @@
 #define SC16IS7XX_LCR_WORD_LEN_6	(0x01)
 #define SC16IS7XX_LCR_WORD_LEN_7	(0x02)
 #define SC16IS7XX_LCR_WORD_LEN_8	(0x03)
-#define SC16IS7XX_LCR_CONF_MODE_A	SC16IS7XX_LCR_DLAB_BIT /* Special
-								* reg set */
-#define SC16IS7XX_LCR_CONF_MODE_B	0xBF                   /* Enhanced
-								* reg set */
+#define SC16IS7XX_LCR_REG_SET_SPECIAL	SC16IS7XX_LCR_DLAB_BIT /* Special
+								* reg set
+								*/
+#define SC16IS7XX_LCR_REG_SET_ENHANCED	0xBF                   /* Enhanced
+								* reg set
+								*/
 
 /* MCR register bits */
 #define SC16IS7XX_MCR_DTR_BIT		BIT(0)   /* DTR complement
@@ -442,7 +444,7 @@ static void sc16is7xx_efr_lock(struct uart_port *port)
 	one->old_lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
 
 	/* Enable access to Enhanced register set */
-	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_CONF_MODE_B);
+	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_REG_SET_ENHANCED);
 
 	/* Disable cache updates when writing to EFR registers */
 	regcache_cache_bypass(one->regmap, true);
@@ -598,7 +600,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 	/* Backup LCR and access special register set (DLL/DLH) */
 	lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
-			     SC16IS7XX_LCR_CONF_MODE_A);
+			     SC16IS7XX_LCR_REG_SET_SPECIAL);
 
 	/* Write the new divisor */
 	regcache_cache_bypass(one->regmap, true);
@@ -1650,7 +1652,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
-				     SC16IS7XX_LCR_CONF_MODE_B);
+				     SC16IS7XX_LCR_REG_SET_ENHANCED);
 
 		regcache_cache_bypass(regmaps[i], true);
 
-- 
2.39.5

[PATCH 03/15] serial: sc16is7xx: rename EFR mutex with generic name

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

This mutex is used as a lock when accessing registers that share the same
address space, not necessarily EFR registers.

For example, address 0x06 is shared by MSR, TCR and XOFF1 registers,
independently of EFR.

Rename the mutex with a more generic name to avoid misinterpreting its
usage.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 330d95446f1d7..26b34f23ed5fe 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -330,7 +330,7 @@ struct sc16is7xx_one_config {
 struct sc16is7xx_one {
 	struct uart_port		port;
 	struct regmap			*regmap;
-	struct mutex			efr_lock; /* EFR registers access */
+	struct mutex			lock; /* For registers sharing same address space. */
 	struct kthread_work		tx_work;
 	struct kthread_work		reg_work;
 	struct kthread_delayed_work	ms_work;
@@ -438,7 +438,7 @@ static void sc16is7xx_efr_lock(struct uart_port *port)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
-	mutex_lock(&one->efr_lock);
+	mutex_lock(&one->lock);
 
 	/* Backup content of LCR. */
 	one->old_lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
@@ -460,7 +460,7 @@ static void sc16is7xx_efr_unlock(struct uart_port *port)
 	/* Restore original content of LCR */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, one->old_lcr);
 
-	mutex_unlock(&one->efr_lock);
+	mutex_unlock(&one->lock);
 }
 
 static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
@@ -595,7 +595,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 			      SC16IS7XX_MCR_CLKSEL_BIT,
 			      prescaler == 1 ? 0 : SC16IS7XX_MCR_CLKSEL_BIT);
 
-	mutex_lock(&one->efr_lock);
+	mutex_lock(&one->lock);
 
 	/* Backup LCR and access special register set (DLL/DLH) */
 	lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
@@ -611,7 +611,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 	/* Restore LCR and access to general register set */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
 
-	mutex_unlock(&one->efr_lock);
+	mutex_unlock(&one->lock);
 
 	return DIV_ROUND_CLOSEST((clk / prescaler) / 16, div);
 }
@@ -758,7 +758,7 @@ static void sc16is7xx_update_mlines(struct sc16is7xx_one *one)
 	unsigned long flags;
 	unsigned int status, changed;
 
-	lockdep_assert_held_once(&one->efr_lock);
+	lockdep_assert_held_once(&one->lock);
 
 	status = sc16is7xx_get_hwmctrl(port);
 	changed = status ^ one->old_mctrl;
@@ -789,7 +789,7 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 	struct uart_port *port = &s->p[portno].port;
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
-	mutex_lock(&one->efr_lock);
+	mutex_lock(&one->lock);
 
 	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
 	if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
@@ -836,7 +836,7 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 	}
 
 out_port_irq:
-	mutex_unlock(&one->efr_lock);
+	mutex_unlock(&one->lock);
 
 	return rc;
 }
@@ -880,9 +880,9 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
 	    (port->rs485.delay_rts_before_send > 0))
 		msleep(port->rs485.delay_rts_before_send);
 
-	mutex_lock(&one->efr_lock);
+	mutex_lock(&one->lock);
 	sc16is7xx_handle_tx(port);
-	mutex_unlock(&one->efr_lock);
+	mutex_unlock(&one->lock);
 }
 
 static void sc16is7xx_reconf_rs485(struct uart_port *port)
@@ -949,9 +949,9 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
 	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
 
 	if (one->port.state) {
-		mutex_lock(&one->efr_lock);
+		mutex_lock(&one->lock);
 		sc16is7xx_update_mlines(one);
-		mutex_unlock(&one->efr_lock);
+		mutex_unlock(&one->lock);
 
 		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
 	}
@@ -1625,7 +1625,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		s->p[i].old_mctrl	= 0;
 		s->p[i].regmap		= regmaps[i];
 
-		mutex_init(&s->p[i].efr_lock);
+		mutex_init(&s->p[i].lock);
 
 		ret = uart_get_rs485_mode(&s->p[i].port);
 		if (ret)
-- 
2.39.5

[PATCH 04/15] serial: sc16is7xx: define common register access function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Rename lock/unlock functions to make it more generic and applicable to both
the Enhanced register set and the Special register set.

Use this new generic function when accessing the Special register set in
sc16is7xx_set_baud(), and when accessing the Enhanced register set in
sc16is7xx_set_termios() and sc16is7xx_probe().

This helps readability and also avoid to make future mistakes when
accessing these obfuscated registers.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 26b34f23ed5fe..72e4c4f80f7f5 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -421,20 +421,24 @@ static void sc16is7xx_power(struct uart_port *port, int on)
 }
 
 /*
- * In an amazing feat of design, the Enhanced Features Register (EFR)
- * shares the address of the Interrupt Identification Register (IIR).
- * Access to EFR is switched on by writing a magic value (0xbf) to the
- * Line Control Register (LCR). Any interrupt firing during this time will
- * see the EFR where it expects the IIR to be, leading to
+ * In an amazing feat of design, the enhanced register set shares the
+ * addresses 0x02 and 0x04-0x07 with the general register set.
+ * The special register set also shares the addresses 0x00-0x01 with the
+ * general register set.
+ *
+ * Access to the enhanced or special register set is enabled by writing a magic
+ * value to the Line Control Register (LCR). When enhanced register set access
+ * is enabled, for example, any interrupt firing during this time will see the
+ * EFR where it expects the IIR to be, leading to
  * "Unexpected interrupt" messages.
  *
- * Prevent this possibility by claiming a mutex while accessing the EFR,
- * and claiming the same mutex from within the interrupt handler. This is
- * similar to disabling the interrupt, but that doesn't work because the
- * bulk of the interrupt processing is run as a workqueue job in thread
- * context.
+ * Prevent this possibility by claiming a mutex when access to the enhanced
+ * or special register set is enabled, and claiming the same mutex from within
+ * the interrupt handler. This is similar to disabling the interrupt, but that
+ * doesn't work because the bulk of the interrupt processing is run as a
+ * workqueue job in thread context.
  */
-static void sc16is7xx_efr_lock(struct uart_port *port)
+static void sc16is7xx_regs_lock(struct uart_port *port, u8 register_set)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
@@ -443,18 +447,18 @@ static void sc16is7xx_efr_lock(struct uart_port *port)
 	/* Backup content of LCR. */
 	one->old_lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
 
-	/* Enable access to Enhanced register set */
-	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, SC16IS7XX_LCR_REG_SET_ENHANCED);
+	/* Enable access to the desired register set */
+	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, register_set);
 
-	/* Disable cache updates when writing to EFR registers */
+	/* Disable cache updates when writing to non-general registers */
 	regcache_cache_bypass(one->regmap, true);
 }
 
-static void sc16is7xx_efr_unlock(struct uart_port *port)
+static void sc16is7xx_regs_unlock(struct uart_port *port)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
-	/* Re-enable cache updates when writing to normal registers */
+	/* Re-enable cache updates when writing to general registers */
 	regcache_cache_bypass(one->regmap, false);
 
 	/* Restore original content of LCR */
@@ -580,8 +584,6 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
  */
 static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 {
-	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
-	u8 lcr;
 	unsigned int prescaler = 1;
 	unsigned long clk = port->uartclk, div = clk / 16 / baud;
 
@@ -595,23 +597,15 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 			      SC16IS7XX_MCR_CLKSEL_BIT,
 			      prescaler == 1 ? 0 : SC16IS7XX_MCR_CLKSEL_BIT);
 
-	mutex_lock(&one->lock);
-
-	/* Backup LCR and access special register set (DLL/DLH) */
-	lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
-	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
-			     SC16IS7XX_LCR_REG_SET_SPECIAL);
+	/* Access special register set (DLL/DLH) */
+	sc16is7xx_regs_lock(port, SC16IS7XX_LCR_REG_SET_SPECIAL);
 
 	/* Write the new divisor */
-	regcache_cache_bypass(one->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_DLH_REG, div / 256);
 	sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
-	regcache_cache_bypass(one->regmap, false);
 
-	/* Restore LCR and access to general register set */
-	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
-
-	mutex_unlock(&one->lock);
+	/* Restore access to general register set */
+	sc16is7xx_regs_unlock(port);
 
 	return DIV_ROUND_CLOSEST((clk / prescaler) / 16, div);
 }
@@ -1108,12 +1102,12 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
 
 	/* Update EFR registers */
-	sc16is7xx_efr_lock(port);
+	sc16is7xx_regs_lock(port, SC16IS7XX_LCR_REG_SET_ENHANCED);
 	sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
 	sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
 	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
 			      SC16IS7XX_EFR_FLOWCTRL_BITS, flow);
-	sc16is7xx_efr_unlock(port);
+	sc16is7xx_regs_unlock(port);
 
 	/* Get baud rate generator configuration */
 	baud = uart_get_baud_rate(port, termios, old,
@@ -1631,6 +1625,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		if (ret)
 			goto out_ports;
 
+		/* Enable access to general register set */
+		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
+
 		/* Disable all interrupts */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
 		/* Disable TX/RX */
@@ -1650,20 +1647,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 
 		port_registered[i] = true;
 
-		/* Enable EFR */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
-				     SC16IS7XX_LCR_REG_SET_ENHANCED);
-
-		regcache_cache_bypass(regmaps[i], true);
-
+		sc16is7xx_regs_lock(&s->p[i].port, SC16IS7XX_LCR_REG_SET_ENHANCED);
 		/* Enable write access to enhanced features */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
 				     SC16IS7XX_EFR_ENABLE_BIT);
-
-		regcache_cache_bypass(regmaps[i], false);
-
-		/* Restore access to general registers */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
+		sc16is7xx_regs_unlock(&s->p[i].port);
 
 		/* Go to suspend mode */
 		sc16is7xx_power(&s->p[i].port, 0);
-- 
2.39.5

[PATCH 05/15] serial: sc16is7xx: use guards for simple mutex locks

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Guards can help to make the code more readable, so use them wherever they
do so.

In many places, labels and 'ret' locals are eliminated completely.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

---

Based on patch by Jiri Slaby (SUSE) <jirislaby@kernel.org>:
commit("serial: use guards for simple mutex locks")
---
 drivers/tty/serial/sc16is7xx.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 72e4c4f80f7f5..7af09535a1563 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -783,13 +783,11 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 	struct uart_port *port = &s->p[portno].port;
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
-	mutex_lock(&one->lock);
+	guard(mutex)(&one->lock);
 
 	iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
-	if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
-		rc = false;
-		goto out_port_irq;
-	}
+	if (iir & SC16IS7XX_IIR_NO_INT_BIT)
+		return false;
 
 	iir &= SC16IS7XX_IIR_ID_MASK;
 
@@ -829,9 +827,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 		break;
 	}
 
-out_port_irq:
-	mutex_unlock(&one->lock);
-
 	return rc;
 }
 
@@ -874,9 +869,8 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
 	    (port->rs485.delay_rts_before_send > 0))
 		msleep(port->rs485.delay_rts_before_send);
 
-	mutex_lock(&one->lock);
+	guard(mutex)(&one->lock);
 	sc16is7xx_handle_tx(port);
-	mutex_unlock(&one->lock);
 }
 
 static void sc16is7xx_reconf_rs485(struct uart_port *port)
@@ -943,9 +937,8 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
 	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
 
 	if (one->port.state) {
-		mutex_lock(&one->lock);
+		guard(mutex)(&one->lock);
 		sc16is7xx_update_mlines(one);
-		mutex_unlock(&one->lock);
 
 		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
 	}
-- 
2.39.5

[PATCH 06/15] serial: sc16is7xx: use dev_err_probe() instead of dev_err()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

This simplifies code and standardizes the error output.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7af09535a1563..4384804a4e228 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1528,10 +1528,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 
 	/* Alloc port structure */
 	s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
-	if (!s) {
-		dev_err(dev, "Error allocating port structure\n");
-		return -ENOMEM;
-	}
+	if (!s)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Error allocating port structure\n");
 
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &uartclk);
-- 
2.39.5

[PATCH 07/15] serial: sc16is7xx: declare SPR/TLR/XOFF2 register as volatile

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

SPR shares the same address space as TLR and XOFF2. If SPR or TLR were to
be used eventually, this could lead to incorrect read value from the cache.

Prevent this potential bug by declaring SPR/TLR/XOFF2 as volatile.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 4384804a4e228..96ea308013912 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -545,6 +545,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 	case SC16IS7XX_IIR_REG:
 	case SC16IS7XX_LSR_REG:
 	case SC16IS7XX_MSR_REG:
+	case SC16IS7XX_SPR_REG: /* Shared address space with TLR & XOFF2 */
 	case SC16IS7XX_TXLVL_REG:
 	case SC16IS7XX_RXLVL_REG:
 	case SC16IS7XX_IOSTATE_REG:
-- 
2.39.5

[PATCH 08/15] serial: sc16is7xx: move port/channel init to separate function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

The sc16is7xx_probe() function is very long. To improve readability, move
port/channel initialization to a separate function.

No functional change intended.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 96ea308013912..406dd7cf81fe3 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1500,6 +1500,75 @@ static int sc16is7xx_reset(struct device *dev, struct regmap *regmap)
 	return 0;
 }
 
+static int sc16is7xx_setup_channel(struct sc16is7xx_one *one, int i,
+				   bool *port_registered)
+{
+	struct uart_port *port = &one->port;
+	int ret;
+
+	ret = ida_alloc_max(&sc16is7xx_lines, SC16IS7XX_MAX_DEVS - 1, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	port->line = ret;
+
+	/* Initialize port data */
+	port->type	= PORT_SC16IS7XX;
+	port->fifosize	= SC16IS7XX_FIFO_SIZE;
+	port->flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
+	port->iobase	= i;
+	/*
+	 * Use all ones as membase to make sure uart_configure_port() in
+	 * serial_core.c does not abort for SPI/I2C devices where the
+	 * membase address is not applicable.
+	 */
+	port->membase	= (void __iomem *)~0;
+	port->iotype	= UPIO_PORT;
+	port->rs485_config = sc16is7xx_config_rs485;
+	port->rs485_supported = sc16is7xx_rs485_supported;
+	port->ops	= &sc16is7xx_ops;
+	one->old_mctrl	= 0;
+
+	mutex_init(&one->lock);
+
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		return ret;
+
+	/* Enable access to general register set */
+	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, 0x00);
+
+	/* Disable all interrupts */
+	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, 0);
+	/* Disable TX/RX */
+	sc16is7xx_port_write(port, SC16IS7XX_EFCR_REG,
+			     SC16IS7XX_EFCR_RXDISABLE_BIT |
+			     SC16IS7XX_EFCR_TXDISABLE_BIT);
+
+	/* Initialize kthread work structs */
+	kthread_init_work(&one->tx_work, sc16is7xx_tx_proc);
+	kthread_init_work(&one->reg_work, sc16is7xx_reg_proc);
+	kthread_init_delayed_work(&one->ms_work, sc16is7xx_ms_proc);
+
+	/* Register port */
+	ret = uart_add_one_port(&sc16is7xx_uart, port);
+	if (ret)
+		return ret;
+
+	*port_registered = true;
+
+	sc16is7xx_regs_lock(port, SC16IS7XX_LCR_REG_SET_ENHANCED);
+	/* Enable write access to enhanced features */
+	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
+			     SC16IS7XX_EFR_ENABLE_BIT);
+	sc16is7xx_regs_unlock(port);
+
+	/* Go to suspend mode */
+	sc16is7xx_power(port, 0);
+
+	return 0;
+}
+
 int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		    struct regmap *regmaps[], int irq)
 {
@@ -1584,70 +1653,14 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	}
 
 	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;
 		s->p[i].port.irq	= irq;
-		s->p[i].port.type	= PORT_SC16IS7XX;
-		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
-		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
-		s->p[i].port.iobase	= i;
-		/*
-		 * Use all ones as membase to make sure uart_configure_port() in
-		 * serial_core.c does not abort for SPI/I2C devices where the
-		 * membase address is not applicable.
-		 */
-		s->p[i].port.membase	= (void __iomem *)~0;
-		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
-		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
-		s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
-		s->p[i].port.ops	= &sc16is7xx_ops;
-		s->p[i].old_mctrl	= 0;
 		s->p[i].regmap		= regmaps[i];
 
-		mutex_init(&s->p[i].lock);
-
-		ret = uart_get_rs485_mode(&s->p[i].port);
+		ret = sc16is7xx_setup_channel(&s->p[i], i, &port_registered[i]);
 		if (ret)
 			goto out_ports;
-
-		/* Enable access to general register set */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
-
-		/* Disable all interrupts */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
-		/* Disable TX/RX */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
-				     SC16IS7XX_EFCR_RXDISABLE_BIT |
-				     SC16IS7XX_EFCR_TXDISABLE_BIT);
-
-		/* Initialize kthread work structs */
-		kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
-		kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
-		kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
-
-		/* Register port */
-		ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
-		if (ret)
-			goto out_ports;
-
-		port_registered[i] = true;
-
-		sc16is7xx_regs_lock(&s->p[i].port, SC16IS7XX_LCR_REG_SET_ENHANCED);
-		/* Enable write access to enhanced features */
-		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
-				     SC16IS7XX_EFR_ENABLE_BIT);
-		sc16is7xx_regs_unlock(&s->p[i].port);
-
-		/* Go to suspend mode */
-		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
 	sc16is7xx_setup_irda_ports(s);
-- 
2.39.5

[PATCH 09/15] serial: sc16is7xx: Kconfig: allow building with COMPILE_TEST

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add COMPILE_TEST as an option to allow test building the driver.

Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lore.kernel.org/all/20240604083159.d984dd08741396ea4ca46418@hugovil.com/raw
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 8523ccfb364dd..9a90654dccd78 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1044,7 +1044,7 @@ config SERIAL_SCCNXP_CONSOLE
 
 config SERIAL_SC16IS7XX
 	tristate "NXP SC16IS7xx UART support"
-	depends on SPI_MASTER || I2C
+	depends on SPI_MASTER || I2C || COMPILE_TEST
 	select SERIAL_CORE
 	select SERIAL_SC16IS7XX_SPI if SPI_MASTER
 	select SERIAL_SC16IS7XX_I2C if I2C
-- 
2.39.5

[PATCH 10/15] serial: sc16is7xx: use KBUILD_MODNAME

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

There is no need to redefine the driver name. Use KBUILD_MODNAME and get
rid of DRV_NAME altogether.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 406dd7cf81fe3..d1a9fa26e9bdb 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -360,7 +360,7 @@ static DEFINE_IDA(sc16is7xx_lines);
 
 static struct uart_driver sc16is7xx_uart = {
 	.owner		= THIS_MODULE,
-	.driver_name    = SC16IS7XX_NAME,
+	.driver_name    = KBUILD_MODNAME,
 	.dev_name	= "ttySC",
 	.nr		= SC16IS7XX_MAX_DEVS,
 };
@@ -1810,4 +1810,4 @@ module_exit(sc16is7xx_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Jon Ringle <jringle@gridpoint.com>");
-MODULE_DESCRIPTION("SC16IS7xx tty serial core driver");
+MODULE_DESCRIPTION(KBUILD_MODNAME " tty serial core driver");
diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h
index afb784eaee45b..9c584d6d35932 100644
--- a/drivers/tty/serial/sc16is7xx.h
+++ b/drivers/tty/serial/sc16is7xx.h
@@ -8,7 +8,6 @@
 #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;
diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
index cd7de9e057b85..699376c3b3a54 100644
--- a/drivers/tty/serial/sc16is7xx_i2c.c
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -52,7 +52,7 @@ MODULE_DEVICE_TABLE(i2c, sc16is7xx_i2c_id_table);
 
 static struct i2c_driver sc16is7xx_i2c_driver = {
 	.driver = {
-		.name		= SC16IS7XX_NAME,
+		.name		= KBUILD_MODNAME,
 		.of_match_table	= sc16is7xx_dt_ids,
 	},
 	.probe		= sc16is7xx_i2c_probe,
@@ -63,5 +63,5 @@ static struct i2c_driver sc16is7xx_i2c_driver = {
 module_i2c_driver(sc16is7xx_i2c_driver);
 
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("SC16IS7xx I2C interface driver");
+MODULE_DESCRIPTION(KBUILD_MODNAME " interface driver");
 MODULE_IMPORT_NS("SERIAL_NXP_SC16IS7XX");
diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
index 20d736b657b17..7e76d0e38da7d 100644
--- a/drivers/tty/serial/sc16is7xx_spi.c
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -75,7 +75,7 @@ MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table);
 
 static struct spi_driver sc16is7xx_spi_driver = {
 	.driver = {
-		.name		= SC16IS7XX_NAME,
+		.name		= KBUILD_MODNAME,
 		.of_match_table	= sc16is7xx_dt_ids,
 	},
 	.probe		= sc16is7xx_spi_probe,
@@ -86,5 +86,5 @@ static struct spi_driver sc16is7xx_spi_driver = {
 module_spi_driver(sc16is7xx_spi_driver);
 
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("SC16IS7xx SPI interface driver");
+MODULE_DESCRIPTION(KBUILD_MODNAME " interface driver");
 MODULE_IMPORT_NS("SERIAL_NXP_SC16IS7XX");
-- 
2.39.5

[PATCH 11/15] serial: sc16is7xx: remove empty line

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Remove empty line inavertently added in commit d5078509c8b0
("serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()").

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index d1a9fa26e9bdb..a05be92f7e776 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -834,7 +834,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
 	bool keep_polling;
-
 	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
 
 	do {
-- 
2.39.5

[PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Fix the following checkpatch error:

    ERROR: space required after that ',' (ctx:VxV)
    +#define to_sc16is7xx_one(p,e)...

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 a05be92f7e776..9d04d665ec9ab 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
 	.nr		= SC16IS7XX_MAX_DEVS,
 };
 
-#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
+#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
 
 static u8 sc16is7xx_port_read(struct uart_port *port, u8 reg)
 {
-- 
2.39.5

[PATCH 13/15] serial: sc16is7xx: change incorrect indentation

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Change incorrect indentation level introduced in commit 9eb90d57b55a
("sc16is7xx: Add flag to activate IrDA mode").

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 9d04d665ec9ab..31d43fc857187 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1181,7 +1181,7 @@ static int sc16is7xx_startup(struct uart_port *port)
 	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
 			      SC16IS7XX_MCR_IRDA_BIT,
 			      one->irda_mode ?
-				SC16IS7XX_MCR_IRDA_BIT : 0);
+			      SC16IS7XX_MCR_IRDA_BIT : 0);
 
 	/* Enable the Rx and Tx FIFO */
 	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
-- 
2.39.5

[PATCH 14/15] serial: sc16is7xx: reformat comments

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add missing space at end of comments and reformat to have uniform style.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 31d43fc857187..b3fbe9459303a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -81,11 +81,14 @@
 /* IER register bits */
 #define SC16IS7XX_IER_RDI_BIT		BIT(0)   /* Enable RX data interrupt */
 #define SC16IS7XX_IER_THRI_BIT		BIT(1)   /* Enable TX holding register
-						  * interrupt */
+						  * interrupt
+						  */
 #define SC16IS7XX_IER_RLSI_BIT		BIT(2)   /* Enable RX line status
-						  * interrupt */
+						  * interrupt
+						  */
 #define SC16IS7XX_IER_MSI_BIT		BIT(3)   /* Enable Modem status
-						  * interrupt */
+						  * interrupt
+						  */
 
 /* IER register bits - write only if (EFR[4] == 1) */
 #define SC16IS7XX_IER_SLEEP_BIT		BIT(4)   /* Enable Sleep mode */
@@ -255,7 +258,8 @@
 
 /* EFCR register bits */
 #define SC16IS7XX_EFCR_9BIT_MODE_BIT	BIT(0)   /* Enable 9-bit or Multidrop
-						  * mode (RS485) */
+						  * mode (RS485)
+						  */
 #define SC16IS7XX_EFCR_RXDISABLE_BIT	BIT(1)   /* Disable receiver */
 #define SC16IS7XX_EFCR_TXDISABLE_BIT	BIT(2)   /* Disable transmitter */
 #define SC16IS7XX_EFCR_AUTO_RS485_BIT	BIT(4)   /* Auto RS485 RTS direction */
@@ -1152,7 +1156,7 @@ static int sc16is7xx_startup(struct uart_port *port)
 
 	sc16is7xx_power(port, 1);
 
-	/* Reset FIFOs*/
+	/* Reset FIFOs */
 	val = SC16IS7XX_FCR_RXRESET_BIT | SC16IS7XX_FCR_TXRESET_BIT;
 	sc16is7xx_port_write(port, SC16IS7XX_FCR_REG, val);
 	udelay(5);
-- 
2.39.5

[PATCH 15/15] serial: sc16is7xx: add/improve comments

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Indicate why lock needs to be asserted when accessing
MSR register, as this is not immediately obvious when looking at this
register in the device datasheet.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b3fbe9459303a..09f03edf0e5cb 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -164,7 +164,7 @@
 						  * - only on 75x/76x
 						  */
 #define SC16IS7XX_MCR_RTS_BIT		BIT(1)   /* RTS complement */
-#define SC16IS7XX_MCR_TCRTLR_BIT	BIT(2)   /* TCR/TLR register enable */
+#define SC16IS7XX_MCR_TCRTLR_BIT	BIT(2)   /* TCR/TLR registers enable */
 #define SC16IS7XX_MCR_LOOP_BIT		BIT(4)   /* Enable loopback test mode */
 #define SC16IS7XX_MCR_XONANY_BIT	BIT(5)   /* Enable Xon Any
 						  * - write enabled
@@ -545,10 +545,10 @@ EXPORT_SYMBOL_GPL(sc16is762_devtype);
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
-	case SC16IS7XX_RHR_REG:
-	case SC16IS7XX_IIR_REG:
-	case SC16IS7XX_LSR_REG:
-	case SC16IS7XX_MSR_REG:
+	case SC16IS7XX_RHR_REG: /* Shared address space with THR & DLL */
+	case SC16IS7XX_IIR_REG: /* Shared address space with FCR & EFR */
+	case SC16IS7XX_LSR_REG: /* Shared address space with XON2 */
+	case SC16IS7XX_MSR_REG: /* Shared address space with TCR & XOFF1 */
 	case SC16IS7XX_SPR_REG: /* Shared address space with TLR & XOFF2 */
 	case SC16IS7XX_TXLVL_REG:
 	case SC16IS7XX_RXLVL_REG:
@@ -757,6 +757,7 @@ static void sc16is7xx_update_mlines(struct sc16is7xx_one *one)
 	unsigned long flags;
 	unsigned int status, changed;
 
+	/* Lock required as MSR address is shared with TCR and XOFF1. */
 	lockdep_assert_held_once(&one->lock);
 
 	status = sc16is7xx_get_hwmctrl(port);
-- 
2.39.5

RE: [PATCH 13/15] serial: sc16is7xx: change incorrect indentation

[Maarten Brock]

Hi Hugo,

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Change incorrect indentation level introduced in commit 9eb90d57b55a
> ("sc16is7xx: Add flag to activate IrDA mode").

> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1181,7 +1181,7 @@ static int sc16is7xx_startup(struct uart_port *port)
>  	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
>  			      SC16IS7XX_MCR_IRDA_BIT,
>  			      one->irda_mode ?
> -				SC16IS7XX_MCR_IRDA_BIT : 0);
> +			      SC16IS7XX_MCR_IRDA_BIT : 0);

Are you sure you want to unindent this?
To me it now looks as if both "one->irda_mode" and "SC16IS7XX_MCR_IRDA_BIT : 0" are parameters.
And why not fix this by placing both on one line? Is 76 characters already over the line length limit?

Kind regards,
Maarten Brock

Re: [PATCH 13/15] serial: sc16is7xx: change incorrect indentation

[Hugo Villeneuve]

Hi Maarten,

On Fri, 26 Sep 2025 09:18:38 +0000
Maarten Brock <Maarten.Brock@sttls.nl> wrote:

> Hi Hugo,
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Change incorrect indentation level introduced in commit 9eb90d57b55a
> > ("sc16is7xx: Add flag to activate IrDA mode").
> 
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1181,7 +1181,7 @@ static int sc16is7xx_startup(struct uart_port *port)
> >  	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
> >  			      SC16IS7XX_MCR_IRDA_BIT,
> >  			      one->irda_mode ?
> > -				SC16IS7XX_MCR_IRDA_BIT : 0);
> > +			      SC16IS7XX_MCR_IRDA_BIT : 0);
> 
> Are you sure you want to unindent this?
> To me it now looks as if both "one->irda_mode" and "SC16IS7XX_MCR_IRDA_BIT : 0" are parameters.
> And why not fix this by placing both on one line? Is 76 characters already over the line length limit?

Excellent suggestion, will look much better. Will do it in V2.

-- 
Hugo Villeneuve

Re: [PATCH 05/15] serial: sc16is7xx: use guards for simple mutex locks

[Jiri Slaby]

On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
...
> @@ -829,9 +827,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>   		break;
>   	}
>   
> -out_port_irq:
> -	mutex_unlock(&one->lock);
> -
>   	return rc;

No need for rc now AFAICT.

>   }
>   
> @@ -874,9 +869,8 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
>   	    (port->rs485.delay_rts_before_send > 0))
>   		msleep(port->rs485.delay_rts_before_send);
>   
> -	mutex_lock(&one->lock);
> +	guard(mutex)(&one->lock);
>   	sc16is7xx_handle_tx(port);
> -	mutex_unlock(&one->lock);
>   }
>   
>   static void sc16is7xx_reconf_rs485(struct uart_port *port)
> @@ -943,9 +937,8 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
>   	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
>   
>   	if (one->port.state) {
> -		mutex_lock(&one->lock);
> +		guard(mutex)(&one->lock);
>   		sc16is7xx_update_mlines(one);
> -		mutex_unlock(&one->lock);
>   
>   		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);

Now the lock is held till here. R U sure it is OK?

thanks,
-- 
js
suse labs

Re: [PATCH 06/15] serial: sc16is7xx: use dev_err_probe() instead of dev_err()

[Jiri Slaby]

On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> This simplifies code and standardizes the error output.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7af09535a1563..4384804a4e228 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1528,10 +1528,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>   
>   	/* Alloc port structure */
>   	s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
> -	if (!s) {
> -		dev_err(dev, "Error allocating port structure\n");
> -		return -ENOMEM;
> -	}
> +	if (!s)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Error allocating port structure\n");

This does not work as you'd expect:
         case -ENOMEM:
                 /* Don't print anything on -ENOMEM, there's already 
enough output */
                 break;

thanks,
-- 
js
suse labs

Re: [PATCH 11/15] serial: sc16is7xx: remove empty line

[Jiri Slaby]

On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Remove empty line inavertently added in commit d5078509c8b0
> ("serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()").
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index d1a9fa26e9bdb..a05be92f7e776 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -834,7 +834,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
>   static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>   {
>   	bool keep_polling;
> -
>   	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;

And remove the cast and switch the two definitions, so we have a 
reversed xmas tree ;).

>   	do {


-- 
js
suse labs

Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Jiri Slaby]

On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Fix the following checkpatch error:
> 
>      ERROR: space required after that ',' (ctx:VxV)
>      +#define to_sc16is7xx_one(p,e)...
> 
> 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 a05be92f7e776..9d04d665ec9ab 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
>   	.nr		= SC16IS7XX_MAX_DEVS,
>   };
>   
> -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))

Or perhaps make it type-safe and more obvious by switching it to an inline?

-- 
js
suse labs

Re: [PATCH 14/15] serial: sc16is7xx: reformat comments

[Jiri Slaby]

On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Add missing space at end of comments and reformat to have uniform style.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 31d43fc857187..b3fbe9459303a 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -81,11 +81,14 @@
>   /* IER register bits */
>   #define SC16IS7XX_IER_RDI_BIT		BIT(0)   /* Enable RX data interrupt */
>   #define SC16IS7XX_IER_THRI_BIT		BIT(1)   /* Enable TX holding register
> -						  * interrupt */
> +						  * interrupt
> +						  */

I am not forcing this, but maybe put it on one line, if it fits into 100 
columns? I believe people are using 100 columns nowadays (me included).


-- 
js
suse labs

Re: [PATCH 05/15] serial: sc16is7xx: use guards for simple mutex locks

[Hugo Villeneuve]

Hi Jiri,

On Mon, 29 Sep 2025 08:09:12 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> ...
> > @@ -829,9 +827,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >   		break;
> >   	}
> >   
> > -out_port_irq:
> > -	mutex_unlock(&one->lock);
> > -
> >   	return rc;
> 
> No need for rc now AFAICT.

You are right, I will remove it in v2.

> 
> >   }
> >   
> > @@ -874,9 +869,8 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
> >   	    (port->rs485.delay_rts_before_send > 0))
> >   		msleep(port->rs485.delay_rts_before_send);
> >   
> > -	mutex_lock(&one->lock);
> > +	guard(mutex)(&one->lock);
> >   	sc16is7xx_handle_tx(port);
> > -	mutex_unlock(&one->lock);
> >   }
> >   
> >   static void sc16is7xx_reconf_rs485(struct uart_port *port)
> > @@ -943,9 +937,8 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
> >   	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
> >   
> >   	if (one->port.state) {
> > -		mutex_lock(&one->lock);
> > +		guard(mutex)(&one->lock);
> >   		sc16is7xx_update_mlines(one);
> > -		mutex_unlock(&one->lock);
> >   
> >   		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
> 
> Now the lock is held till here. R U sure it is OK?

Now that you mention it, I am sure its not OK :)

I will restore this one to the original lock/unlock code in V2.

Thank you,
Hugo.

Re: [PATCH 06/15] serial: sc16is7xx: use dev_err_probe() instead of dev_err()

[Hugo Villeneuve]

Hi Jiri,

On Mon, 29 Sep 2025 08:10:17 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > This simplifies code and standardizes the error output.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >   drivers/tty/serial/sc16is7xx.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 7af09535a1563..4384804a4e228 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1528,10 +1528,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
> >   
> >   	/* Alloc port structure */
> >   	s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
> > -	if (!s) {
> > -		dev_err(dev, "Error allocating port structure\n");
> > -		return -ENOMEM;
> > -	}
> > +	if (!s)
> > +		return dev_err_probe(dev, -ENOMEM,
> > +				     "Error allocating port structure\n");
> 
> This does not work as you'd expect:
>          case -ENOMEM:
>                  /* Don't print anything on -ENOMEM, there's already 
> enough output */
>                  break;

Ok, I will simply remove the original dev_err() call.

Hugo.

Re: [PATCH 11/15] serial: sc16is7xx: remove empty line

[Hugo Villeneuve]

Hi Jiri,

On Mon, 29 Sep 2025 08:14:33 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Remove empty line inavertently added in commit d5078509c8b0
> > ("serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()").
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >   drivers/tty/serial/sc16is7xx.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index d1a9fa26e9bdb..a05be92f7e776 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -834,7 +834,6 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
> >   static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
> >   {
> >   	bool keep_polling;
> > -
> >   	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> 
> And remove the cast and switch the two definitions, so we have a 
> reversed xmas tree ;).

Good idea, I will do it in V2.

Hugo.

Re: [PATCH 05/15] serial: sc16is7xx: use guards for simple mutex locks

[Jiri Slaby]

On 30. 09. 25, 2:27, Hugo Villeneuve wrote:
>>> @@ -943,9 +937,8 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
>>>    	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
>>>    
>>>    	if (one->port.state) {
>>> -		mutex_lock(&one->lock);
>>> +		guard(mutex)(&one->lock);
>>>    		sc16is7xx_update_mlines(one);
>>> -		mutex_unlock(&one->lock);
>>>    
>>>    		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
>>
>> Now the lock is held till here. R U sure it is OK?
> 
> Now that you mention it, I am sure its not OK :)
> 
> I will restore this one to the original lock/unlock code in V2.

Or use a scoped lock ;).

-- 
js
suse labs

Re: [PATCH 05/15] serial: sc16is7xx: use guards for simple mutex locks

[Hugo Villeneuve]

Hi Jiri,

On Tue, 30 Sep 2025 06:01:35 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 30. 09. 25, 2:27, Hugo Villeneuve wrote:
> >>> @@ -943,9 +937,8 @@ static void sc16is7xx_ms_proc(struct kthread_work *ws)
> >>>    	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
> >>>    
> >>>    	if (one->port.state) {
> >>> -		mutex_lock(&one->lock);
> >>> +		guard(mutex)(&one->lock);
> >>>    		sc16is7xx_update_mlines(one);
> >>> -		mutex_unlock(&one->lock);
> >>>    
> >>>    		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
> >>
> >> Now the lock is held till here. R U sure it is OK?
> > 
> > Now that you mention it, I am sure its not OK :)
> > 
> > I will restore this one to the original lock/unlock code in V2.
> 
> Or use a scoped lock ;).

Cool, will use that.

Thank you,
Hugo.

Re: [PATCH 14/15] serial: sc16is7xx: reformat comments

[Hugo Villeneuve]

Hi Jiri,

On Mon, 29 Sep 2025 08:18:01 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Add missing space at end of comments and reformat to have uniform style.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >   drivers/tty/serial/sc16is7xx.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 31d43fc857187..b3fbe9459303a 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -81,11 +81,14 @@
> >   /* IER register bits */
> >   #define SC16IS7XX_IER_RDI_BIT		BIT(0)   /* Enable RX data interrupt */
> >   #define SC16IS7XX_IER_THRI_BIT		BIT(1)   /* Enable TX holding register
> > -						  * interrupt */
> > +						  * interrupt
> > +						  */
> 
> I am not forcing this, but maybe put it on one line, if it fits into 100 
> columns? I believe people are using 100 columns nowadays (me included).

Yes good idea. And since the goal of this commit was to try to
uniformize all comments accross the driver, I will probably reformat
most of them to improve readability and follow same convention.

Hugo.

-- 
Hugo Villeneuve

Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Hugo Villeneuve]

Hi Jiri,

On Mon, 29 Sep 2025 08:15:56 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Fix the following checkpatch error:
> > 
> >      ERROR: space required after that ',' (ctx:VxV)
> >      +#define to_sc16is7xx_one(p,e)...
> > 
> > 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 a05be92f7e776..9d04d665ec9ab 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> >   	.nr		= SC16IS7XX_MAX_DEVS,
> >   };
> >   
> > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> 
> Or perhaps make it type-safe and more obvious by switching it to an inline?

Not easy to do, because this macro is also used with the second
argument "e" not always being used with the same member name. At the
same time, this is what makes this macro more complex than it should
be. I will convert it to an inline and simplify it by removing the
second argument (and of course adapt the code where the new, simpler,
inline function no longer works).

The end result improves readability a lot.

Thank you,
Hugo.

-- 
Hugo Villeneuve

Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Greg KH]

On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> Hi Jiri,
> 
> On Mon, 29 Sep 2025 08:15:56 +0200
> Jiri Slaby <jirislaby@kernel.org> wrote:
> 
> > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > Fix the following checkpatch error:
> > > 
> > >      ERROR: space required after that ',' (ctx:VxV)
> > >      +#define to_sc16is7xx_one(p,e)...
> > > 
> > > 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 a05be92f7e776..9d04d665ec9ab 100644
> > > --- a/drivers/tty/serial/sc16is7xx.c
> > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > >   };
> > >   
> > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > 
> > Or perhaps make it type-safe and more obvious by switching it to an inline?
> 
> Not easy to do, because this macro is also used with the second
> argument "e" not always being used with the same member name. At the
> same time, this is what makes this macro more complex than it should
> be. I will convert it to an inline and simplify it by removing the
> second argument (and of course adapt the code where the new, simpler,
> inline function no longer works).

Please don't use an inline fuction for container_of() as you will just
need to change it later in the future when you really want to use
container_of_const() instead :)

thanks,

greg k-h

Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Hugo Villeneuve]

Hi Greg,

On Wed, 1 Oct 2025 07:16:06 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> > Hi Jiri,
> > 
> > On Mon, 29 Sep 2025 08:15:56 +0200
> > Jiri Slaby <jirislaby@kernel.org> wrote:
> > 
> > > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > Fix the following checkpatch error:
> > > > 
> > > >      ERROR: space required after that ',' (ctx:VxV)
> > > >      +#define to_sc16is7xx_one(p,e)...
> > > > 
> > > > 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 a05be92f7e776..9d04d665ec9ab 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > > >   };
> > > >   
> > > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > > 
> > > Or perhaps make it type-safe and more obvious by switching it to an inline?
> > 
> > Not easy to do, because this macro is also used with the second
> > argument "e" not always being used with the same member name. At the
> > same time, this is what makes this macro more complex than it should
> > be. I will convert it to an inline and simplify it by removing the
> > second argument (and of course adapt the code where the new, simpler,
> > inline function no longer works).
> 
> Please don't use an inline fuction for container_of() as you will just
> need to change it later in the future when you really want to use
> container_of_const() instead :)

Noted. I will simplify the macro and leave it as a macro then.

Would you suggest to also convert container_of to
container_of_const in this patch series?

Hugo.

Re: [PATCH 12/15] serial: sc16is7xx: add missing space between macro args (checkpatch)

[Greg KH]

On Wed, Oct 01, 2025 at 09:29:20AM -0400, Hugo Villeneuve wrote:
> Hi Greg,
> 
> On Wed, 1 Oct 2025 07:16:06 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Sep 30, 2025 at 04:08:28PM -0400, Hugo Villeneuve wrote:
> > > Hi Jiri,
> > > 
> > > On Mon, 29 Sep 2025 08:15:56 +0200
> > > Jiri Slaby <jirislaby@kernel.org> wrote:
> > > 
> > > > On 24. 09. 25, 17:37, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > 
> > > > > Fix the following checkpatch error:
> > > > > 
> > > > >      ERROR: space required after that ',' (ctx:VxV)
> > > > >      +#define to_sc16is7xx_one(p,e)...
> > > > > 
> > > > > 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 a05be92f7e776..9d04d665ec9ab 100644
> > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > @@ -365,7 +365,7 @@ static struct uart_driver sc16is7xx_uart = {
> > > > >   	.nr		= SC16IS7XX_MAX_DEVS,
> > > > >   };
> > > > >   
> > > > > -#define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > > +#define to_sc16is7xx_one(p, e)	((container_of((p), struct sc16is7xx_one, e)))
> > > > 
> > > > Or perhaps make it type-safe and more obvious by switching it to an inline?
> > > 
> > > Not easy to do, because this macro is also used with the second
> > > argument "e" not always being used with the same member name. At the
> > > same time, this is what makes this macro more complex than it should
> > > be. I will convert it to an inline and simplify it by removing the
> > > second argument (and of course adapt the code where the new, simpler,
> > > inline function no longer works).
> > 
> > Please don't use an inline fuction for container_of() as you will just
> > need to change it later in the future when you really want to use
> > container_of_const() instead :)
> 
> Noted. I will simplify the macro and leave it as a macro then.
> 
> Would you suggest to also convert container_of to
> container_of_const in this patch series?

For now, no, it's not needed as I do not think this structure is ever
marked as const, is it?

I have a long-term plan to move container_of() to be
container_of_const(), but that's a kernel-wide thing, and not relevant
here.  I was just trying to point out that when inlining container_of(),
it can actually cause problems.

thanks,

greg k-h