From: Hugo Villeneuve <[email protected]>
Hello,
this patch series brings a few fixes, clean-ups and improvements to the
sc16is7xx driver.
Some of the patches have been suggested by Andy Shevchenko following this
dicussion:
Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
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 (18):
serial: sc16is7xx: fix segfault when removing driver
serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of
probe error
serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()
serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()
serial: sc16is7xx: use DECLARE_BITMAP for sc16is7xx_lines bitfield
serial: sc16is7xx: use spi_get_device_match_data()
serial: sc16is7xx: use i2c_get_match_data()
serial: sc16is7xx: add driver name to struct uart_driver
serial: sc16is7xx: add macro for max number of UART ports
serial: sc16is7xx: use HZ_PER_MHZ macro to improve readability
serial: sc16is7xx: add explicit return for some switch default cases
serial: sc16is7xx: replace hardcoded divisor value with BIT() macro
serial: sc16is7xx: use in_range() for DT properties bound checks
serial: sc16is7xx: drop unneeded MODULE_ALIAS
serial: sc16is7xx: pass R/W buffer in FIFO functions
serial: sc16is7xx: reorder code to remove prototype declarations
serial: sc16is7xx: refactor EFR lock
serial: sc16is7xx: fix whitespace in sc16is7xx_startup() comments
drivers/tty/serial/sc16is7xx.c | 392 ++++++++++++++++-----------------
1 file changed, 191 insertions(+), 201 deletions(-)
base-commit: 43f012df3c1e979966524f79b5371fde6545488a
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Use preferred i2c_get_match_data() instead of device_get_match_data()
and i2c_client_get_device_id() to get the driver match data.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 8cbf54d0a16c..97a97a4bd71a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1798,17 +1798,14 @@ MODULE_ALIAS("spi:sc16is7xx");
#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
{
- const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
const struct sc16is7xx_devtype *devtype;
struct regmap *regmaps[2];
unsigned int i;
- if (i2c->dev.of_node) {
- devtype = device_get_match_data(&i2c->dev);
- if (!devtype)
- return -ENODEV;
- } else {
- devtype = (struct sc16is7xx_devtype *)id->driver_data;
+ devtype = (struct sc16is7xx_devtype *)i2c_get_match_data(i2c);
+ if (!devtype) {
+ dev_err(&i2c->dev, "Failed to match device\n");
+ return -ENODEV;
}
for (i = 0; i < devtype->nr_uart; i++) {
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Simplify and improve readability by replacing while(1) loop with
do {} while, and by using the keep_polling variable as the exit
condition, making it more explicit.
Fixes: 834449872105 ("sc16is7xx: Fix for multi-channel stall")
Cc: [email protected]
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b2d0f6d307bd..8a038a9be09e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -782,17 +782,17 @@ 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;
- while (1) {
- bool keep_polling = false;
+ do {
+ keep_polling = false;
int i;
for (i = 0; i < s->devtype->nr_uart; ++i)
keep_polling |= sc16is7xx_port_irq(s, i);
- if (!keep_polling)
- break;
- }
+ } while (keep_polling);
return IRQ_HANDLED;
}
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Move/reorder some functions to remove sc16is7xx_ier_set() and
sc16is7xx_stop_tx() prototypes declarations.
No functional change.
sc16is7xx_ier_set() was introduced in
commit cc4c1d05eb10 ("sc16is7xx: Properly resume TX after stop")
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 75 ++++++++++++++++------------------
1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 3322507ab18e..9154fd75134a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -358,9 +358,6 @@ static struct uart_driver sc16is7xx_uart = {
.nr = SC16IS7XX_MAX_DEVS,
};
-static void sc16is7xx_ier_set(struct uart_port *port, u8 bit);
-static void sc16is7xx_stop_tx(struct uart_port *port);
-
#define to_sc16is7xx_one(p,e) ((container_of((p), struct sc16is7xx_one, e)))
static u8 sc16is7xx_port_read(struct uart_port *port, u8 reg)
@@ -416,6 +413,42 @@ static void sc16is7xx_power(struct uart_port *port, int on)
on ? 0 : SC16IS7XX_IER_SLEEP_BIT);
}
+static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
+{
+ struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ lockdep_assert_held_once(&port->lock);
+
+ one->config.flags |= SC16IS7XX_RECONF_IER;
+ one->config.ier_mask |= bit;
+ one->config.ier_val &= ~bit;
+ kthread_queue_work(&s->kworker, &one->reg_work);
+}
+
+static void sc16is7xx_ier_set(struct uart_port *port, u8 bit)
+{
+ struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ lockdep_assert_held_once(&port->lock);
+
+ one->config.flags |= SC16IS7XX_RECONF_IER;
+ one->config.ier_mask |= bit;
+ one->config.ier_val |= bit;
+ kthread_queue_work(&s->kworker, &one->reg_work);
+}
+
+static void sc16is7xx_stop_tx(struct uart_port *port)
+{
+ sc16is7xx_ier_clear(port, SC16IS7XX_IER_THRI_BIT);
+}
+
+static void sc16is7xx_stop_rx(struct uart_port *port)
+{
+ sc16is7xx_ier_clear(port, SC16IS7XX_IER_RDI_BIT);
+}
+
static const struct sc16is7xx_devtype sc16is74x_devtype = {
.name = "SC16IS74X",
.nr_gpio = 0,
@@ -867,42 +900,6 @@ static void sc16is7xx_reg_proc(struct kthread_work *ws)
sc16is7xx_reconf_rs485(&one->port);
}
-static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
-{
- struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
- struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
-
- lockdep_assert_held_once(&port->lock);
-
- one->config.flags |= SC16IS7XX_RECONF_IER;
- one->config.ier_mask |= bit;
- one->config.ier_val &= ~bit;
- kthread_queue_work(&s->kworker, &one->reg_work);
-}
-
-static void sc16is7xx_ier_set(struct uart_port *port, u8 bit)
-{
- struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
- struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
-
- lockdep_assert_held_once(&port->lock);
-
- one->config.flags |= SC16IS7XX_RECONF_IER;
- one->config.ier_mask |= bit;
- one->config.ier_val |= bit;
- kthread_queue_work(&s->kworker, &one->reg_work);
-}
-
-static void sc16is7xx_stop_tx(struct uart_port *port)
-{
- sc16is7xx_ier_clear(port, SC16IS7XX_IER_THRI_BIT);
-}
-
-static void sc16is7xx_stop_rx(struct uart_port *port)
-{
- sc16is7xx_ier_clear(port, SC16IS7XX_IER_RDI_BIT);
-}
-
static void sc16is7xx_ms_proc(struct kthread_work *ws)
{
struct sc16is7xx_one *one = to_sc16is7xx_one(ws, ms_work.work);
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Add missing space at end of comments.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
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 ab68ee346ec6..a8a78d8f7fcf 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1139,7 +1139,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.2
From: Hugo Villeneuve <[email protected]>
Move common code for EFR lock/unlock of mutex into functions for code reuse
and clarity.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 104 ++++++++++++++++++---------------
1 file changed, 56 insertions(+), 48 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 9154fd75134a..ab68ee346ec6 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -333,6 +333,7 @@ struct sc16is7xx_one {
struct sc16is7xx_one_config config;
bool irda_mode;
unsigned int old_mctrl;
+ u8 old_lcr; /* Value before EFR access. */
};
struct sc16is7xx_port {
@@ -413,6 +414,49 @@ static void sc16is7xx_power(struct uart_port *port, int on)
on ? 0 : SC16IS7XX_IER_SLEEP_BIT);
}
+/* 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
+ * "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.
+ */
+static void sc16is7xx_efr_lock(struct uart_port *port)
+{
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ mutex_lock(&one->efr_lock);
+
+ /* 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_CONF_MODE_B);
+
+ /* Disable cache updates when writing to EFR registers */
+ regcache_cache_bypass(one->regmap, true);
+}
+
+static void sc16is7xx_efr_unlock(struct uart_port *port)
+{
+ struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+ /* Re-enable cache updates when writing to normal registers */
+ regcache_cache_bypass(one->regmap, false);
+
+ /* Restore original content of LCR */
+ sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, one->old_lcr);
+
+ mutex_unlock(&one->efr_lock);
+}
+
static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
{
struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
@@ -523,45 +567,19 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
div /= 4;
}
- /* In an amazing feat of design, the Enhanced Features Register shares
- * the address of the Interrupt Identification Register, and is
- * switched in by writing a magic value (0xbf) to the Line Control
- * Register. 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.
- */
- mutex_lock(&one->efr_lock);
-
- lcr = sc16is7xx_port_read(port, SC16IS7XX_LCR_REG);
-
- /* Open the LCR divisors for configuration */
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
- SC16IS7XX_LCR_CONF_MODE_B);
-
/* Enable enhanced features */
- regcache_cache_bypass(one->regmap, true);
+ sc16is7xx_efr_lock(port);
sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
SC16IS7XX_EFR_ENABLE_BIT,
SC16IS7XX_EFR_ENABLE_BIT);
-
- regcache_cache_bypass(one->regmap, false);
-
- /* Put LCR back to the normal mode */
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
-
- mutex_unlock(&one->efr_lock);
+ sc16is7xx_efr_unlock(port);
sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
SC16IS7XX_MCR_CLKSEL_BIT,
prescaler);
- /* Open the LCR divisors for configuration */
+ /* 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);
@@ -571,7 +589,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
regcache_cache_bypass(one->regmap, false);
- /* Put LCR back to the normal mode */
+ /* Restore LCR and access to general register set */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
return DIV_ROUND_CLOSEST(clk / 16, div);
@@ -1049,17 +1067,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
if (!(termios->c_cflag & CREAD))
port->ignore_status_mask |= SC16IS7XX_LSR_BRK_ERROR_MASK;
- /* As above, claim the mutex while accessing the EFR. */
- mutex_lock(&one->efr_lock);
-
- sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
- SC16IS7XX_LCR_CONF_MODE_B);
-
/* Configure flow control */
- regcache_cache_bypass(one->regmap, true);
- sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
- sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
-
port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
if (termios->c_cflag & CRTSCTS) {
flow |= SC16IS7XX_EFR_AUTOCTS_BIT |
@@ -1071,16 +1079,16 @@ static void sc16is7xx_set_termios(struct uart_port *port,
if (termios->c_iflag & IXOFF)
flow |= SC16IS7XX_EFR_SWFLOW1_BIT;
- sc16is7xx_port_update(port,
- SC16IS7XX_EFR_REG,
- SC16IS7XX_EFR_FLOWCTRL_BITS,
- flow);
- regcache_cache_bypass(one->regmap, false);
-
/* Update LCR register */
sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
- mutex_unlock(&one->efr_lock);
+ /* Update EFR registers */
+ sc16is7xx_efr_lock(port);
+ 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);
/* Get baud rate generator configuration */
baud = uart_get_baud_rate(port, termios, old,
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Allows to simplify code by removing the break statement in the default
switch/case in some functions.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d5eec2d0e94..feb50d9271ac 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -460,10 +460,8 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
case SC16IS7XX_IOCONTROL_REG:
return true;
default:
- break;
+ return false;
}
-
- return false;
}
static bool sc16is7xx_regmap_precious(struct device *dev, unsigned int reg)
@@ -472,10 +470,8 @@ static bool sc16is7xx_regmap_precious(struct device *dev, unsigned int reg)
case SC16IS7XX_RHR_REG:
return true;
default:
- break;
+ return false;
}
-
- return false;
}
static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
sc16is7xx_port_irq() from looping multiple times when there was still
interrupts to serve. It simply changed the do {} while(1) loop to a
do {} while(0) loop, which makes the loop itself now obsolete.
Clean the code by removing this obsolete do {} while(0) loop.
Fixes: 834449872105 ("sc16is7xx: Fix for multi-channel stall")
Cc: [email protected]
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 85 ++++++++++++++++------------------
1 file changed, 41 insertions(+), 44 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b92fd01cfeec..b2d0f6d307bd 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -724,58 +724,55 @@ static void sc16is7xx_update_mlines(struct sc16is7xx_one *one)
static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
{
bool rc = true;
+ unsigned int iir, rxlen;
struct uart_port *port = &s->p[portno].port;
struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
mutex_lock(&one->efr_lock);
- do {
- unsigned int iir, rxlen;
+ iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
+ if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
+ rc = false;
+ goto out_port_irq;
+ }
- iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
- if (iir & SC16IS7XX_IIR_NO_INT_BIT) {
- rc = false;
- goto out_port_irq;
- }
+ iir &= SC16IS7XX_IIR_ID_MASK;
- iir &= SC16IS7XX_IIR_ID_MASK;
-
- switch (iir) {
- case SC16IS7XX_IIR_RDI_SRC:
- case SC16IS7XX_IIR_RLSE_SRC:
- case SC16IS7XX_IIR_RTOI_SRC:
- case SC16IS7XX_IIR_XOFFI_SRC:
- rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
-
- /*
- * There is a silicon bug that makes the chip report a
- * time-out interrupt but no data in the FIFO. This is
- * described in errata section 18.1.4.
- *
- * When this happens, read one byte from the FIFO to
- * clear the interrupt.
- */
- if (iir == SC16IS7XX_IIR_RTOI_SRC && !rxlen)
- rxlen = 1;
-
- if (rxlen)
- sc16is7xx_handle_rx(port, rxlen, iir);
- break;
+ switch (iir) {
+ case SC16IS7XX_IIR_RDI_SRC:
+ case SC16IS7XX_IIR_RLSE_SRC:
+ case SC16IS7XX_IIR_RTOI_SRC:
+ case SC16IS7XX_IIR_XOFFI_SRC:
+ rxlen = sc16is7xx_port_read(port, SC16IS7XX_RXLVL_REG);
+
+ /*
+ * There is a silicon bug that makes the chip report a
+ * time-out interrupt but no data in the FIFO. This is
+ * described in errata section 18.1.4.
+ *
+ * When this happens, read one byte from the FIFO to
+ * clear the interrupt.
+ */
+ if (iir == SC16IS7XX_IIR_RTOI_SRC && !rxlen)
+ rxlen = 1;
+
+ if (rxlen)
+ sc16is7xx_handle_rx(port, rxlen, iir);
+ break;
/* CTSRTS interrupt comes only when CTS goes inactive */
- case SC16IS7XX_IIR_CTSRTS_SRC:
- case SC16IS7XX_IIR_MSI_SRC:
- sc16is7xx_update_mlines(one);
- break;
- case SC16IS7XX_IIR_THRI_SRC:
- sc16is7xx_handle_tx(port);
- break;
- default:
- dev_err_ratelimited(port->dev,
- "ttySC%i: Unexpected interrupt: %x",
- port->line, iir);
- break;
- }
- } while (0);
+ case SC16IS7XX_IIR_CTSRTS_SRC:
+ case SC16IS7XX_IIR_MSI_SRC:
+ sc16is7xx_update_mlines(one);
+ break;
+ case SC16IS7XX_IIR_THRI_SRC:
+ sc16is7xx_handle_tx(port);
+ break;
+ default:
+ dev_err_ratelimited(port->dev,
+ "ttySC%i: Unexpected interrupt: %x",
+ port->line, iir);
+ break;
+ }
out_port_irq:
mutex_unlock(&one->efr_lock);
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Improves code readability by making it more obvious that it is a MHz value.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 29844e2eefc5..7d5eec2d0e94 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -24,6 +24,7 @@
#include <linux/tty_flip.h>
#include <linux/spi/spi.h>
#include <linux/uaccess.h>
+#include <linux/units.h>
#include <uapi/linux/sched/types.h>
#define SC16IS7XX_NAME "sc16is7xx"
@@ -1741,7 +1742,7 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
spi->bits_per_word = 8;
/* only supports mode 0 on SC16IS762 */
spi->mode = spi->mode ? : SPI_MODE_0;
- spi->max_speed_hz = spi->max_speed_hz ? : 15000000;
+ spi->max_speed_hz = spi->max_speed_hz ? : 15 * HZ_PER_MHZ;
ret = spi_setup(spi);
if (ret)
return ret;
--
2.39.2
From: Hugo Villeneuve <[email protected]>
To better show why the limit is what it is, since we have only 16 bits for
the divisor.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
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 feb50d9271ac..133538f91390 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -486,7 +486,7 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
u8 prescaler = 0;
unsigned long clk = port->uartclk, div = clk / 16 / baud;
- if (div > 0xffff) {
+ if (div >= BIT(16)) {
prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
div /= 4;
}
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Improve code readability and efficiency by using in_range() when checking
device tree properties bound.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 133538f91390..29089b11f6f1 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -14,6 +14,7 @@
#include <linux/device.h>
#include <linux/gpio/driver.h>
#include <linux/i2c.h>
+#include <linux/minmax.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/property.h>
@@ -1398,7 +1399,7 @@ static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s)
struct device *dev = s->p[0].port.dev;
count = device_property_count_u32(dev, "irda-mode-ports");
- if (count < 0 || count > SC16IS7XX_MAX_PORTS)
+ if (!in_range(count, 0, SC16IS7XX_MAX_PORTS + 1))
return;
ret = device_property_read_u32_array(dev, "irda-mode-ports",
@@ -1425,7 +1426,7 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
struct device *dev = s->p[0].port.dev;
count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
- if (count < 0 || count > SC16IS7XX_MAX_PORTS)
+ if (!in_range(count, 0, SC16IS7XX_MAX_PORTS + 1))
return 0;
ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
--
2.39.2
On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Simplify and improve readability by replacing while(1) loop with
> do {} while, and by using the keep_polling variable as the exit
> condition, making it more explicit.
...
> + bool keep_polling;
> +
Stray blank line. Otherwise LGTM.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:51PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Use preferred i2c_get_match_data() instead of device_get_match_data()
> and i2c_client_get_device_id() to get the driver match data.
As per SPI patch.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
> sc16is7xx_port_irq() from looping multiple times when there was still
> interrupts to serve. It simply changed the do {} while(1) loop to a
> do {} while(0) loop, which makes the loop itself now obsolete.
>
> Clean the code by removing this obsolete do {} while(0) loop.
I'm just wondering if you used --histogram diff algo when prepared the patches.
If no, use that by default.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:54PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Improves code readability by making it more obvious that it is a MHz value.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:56PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> To better show why the limit is what it is, since we have only 16 bits for
> the divisor.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:55PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Allows to simplify code by removing the break statement in the default
> switch/case in some functions.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:57PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Improve code readability and efficiency by using in_range() when checking
> device tree properties bound.
...
> count = device_property_count_u32(dev, "irda-mode-ports");
> - if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> + if (!in_range(count, 0, SC16IS7XX_MAX_PORTS + 1))
> return;
Okay, looking at this, it becomes uglier than initial code,
means my suggestion was not good. Please, drop this patch.
--
With Best Regards,
Andy Shevchenko
On Wed, 20 Dec 2023 17:42:42 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Simplify and improve readability by replacing while(1) loop with
> > do {} while, and by using the keep_polling variable as the exit
> > condition, making it more explicit.
>
> ...
>
> > + bool keep_polling;
>
> > +
>
> Stray blank line. Otherwise LGTM.
Yes, and I just realized I should also change:
do {
keep_polling = false;
int i;
...
to:
do {
int i;
keep_polling = false;
...
Hugo Villeneuve
On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Move/reorder some functions to remove sc16is7xx_ier_set() and
> sc16is7xx_stop_tx() prototypes declarations.
>
> No functional change.
>
> sc16is7xx_ier_set() was introduced in
> commit cc4c1d05eb10 ("sc16is7xx: Properly resume TX after stop")
Missing period after ). Otherwise LGTM.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:19:02PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Add missing space at end of comments.
...
> - /* Reset FIFOs*/
> + /* Reset FIFOs */
> val = SC16IS7XX_FCR_RXRESET_BIT | SC16IS7XX_FCR_TXRESET_BIT;
> sc16is7xx_port_write(port, SC16IS7XX_FCR_REG, val);
> udelay(5);
You can combine this with other comment style cleanups and spelling fixes
(if any). I.o.w. proof-read the code and check if there are any issues
besides noted ones.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Move common code for EFR lock/unlock of mutex into functions for code reuse
> and clarity.
...
> @@ -333,6 +333,7 @@ struct sc16is7xx_one {
> struct sc16is7xx_one_config config;
> bool irda_mode;
> unsigned int old_mctrl;
> + u8 old_lcr; /* Value before EFR access. */
> };
Have you run `pahole`?
I believe with
unsigned int old_mctrl;
u8 old_lcr; /* Value before EFR access. */
bool irda_mode;
layout it will take less memory.
...
> +/* In an amazing feat of design, the Enhanced Features Register (EFR)
/*
* This is NOT the style we use for multi-line
* comments in the serial subsystem. On contrary
* this comment can be used as a proper example.
* (Yes, I noticed it's an old comment, but take
* a chance to fix it.)
*/
> + * 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
> + * "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.
> + */
...
> + sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
> + SC16IS7XX_LCR_CONF_MODE_B);
One line. (Yes, 81 character, but readability is as good as before.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 19, 2023 at 12:18:44PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Hello,
> this patch series brings a few fixes, clean-ups and improvements to the
> sc16is7xx driver.
>
> Some of the patches have been suggested by Andy Shevchenko following this
> dicussion:
>
> Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
Thanks, good series (need a bit of additional work, though).
What I really miss is the proper split of the driver. See
0f04a81784fe ("pinctrl: mcp23s08: Split to three parts: core, I?C, SPI")
as an example of a such.
--
With Best Regards,
Andy Shevchenko
On Wed, 20 Dec 2023 17:54:07 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:18:57PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Improve code readability and efficiency by using in_range() when checking
> > device tree properties bound.
>
> ...
>
> > count = device_property_count_u32(dev, "irda-mode-ports");
> > - if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> > + if (!in_range(count, 0, SC16IS7XX_MAX_PORTS + 1))
> > return;
>
> Okay, looking at this, it becomes uglier than initial code,
> means my suggestion was not good. Please, drop this patch.
Ok, will drop it for V2.
Hugo Villeneuve
On Wed, 20 Dec 2023 17:58:40 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Move/reorder some functions to remove sc16is7xx_ier_set() and
> > sc16is7xx_stop_tx() prototypes declarations.
> >
> > No functional change.
> >
> > sc16is7xx_ier_set() was introduced in
> > commit cc4c1d05eb10 ("sc16is7xx: Properly resume TX after stop")
>
> Missing period after ). Otherwise LGTM.
Will add it for V2.
By the way, when you say LGTM, would you suggest that I add your
"Acked-by" or "Reviewed-by" tag, after having fixed what you mentioned?
Hugo Villeneuve
On Wed, 20 Dec 2023 18:06:29 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:18:44PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Hello,
> > this patch series brings a few fixes, clean-ups and improvements to the
> > sc16is7xx driver.
> >
> > Some of the patches have been suggested by Andy Shevchenko following this
> > dicussion:
> >
> > Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
>
> Thanks, good series (need a bit of additional work, though).
> What I really miss is the proper split of the driver. See
> 0f04a81784fe ("pinctrl: mcp23s08: Split to three parts: core, I?C, SPI")
> as an example of a such.
Hi Andy,
thank you for the reviews.
I was thinking of doing the split after this current serie, and I
will look at your example as a reference.
Hugo Villeneuve
On Wed, Dec 20, 2023 at 11:30:07AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:58:40 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <[email protected]>
> > Missing period after ). Otherwise LGTM.
>
> Will add it for V2.
>
> By the way, when you say LGTM, would you suggest that I add your
> "Acked-by" or "Reviewed-by" tag, after having fixed what you mentioned?
If you address as suggested, feel free to add
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Wed, 20 Dec 2023 18:04:55 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:19:02PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Add missing space at end of comments.
>
> ...
>
> > - /* Reset FIFOs*/
> > + /* Reset FIFOs */
> > val = SC16IS7XX_FCR_RXRESET_BIT | SC16IS7XX_FCR_TXRESET_BIT;
> > sc16is7xx_port_write(port, SC16IS7XX_FCR_REG, val);
> > udelay(5);
>
> You can combine this with other comment style cleanups and spelling fixes
> (if any). I.o.w. proof-read the code and check if there are any issues
> besides noted ones.
Ok.
Hugo Villeneuve
On Wed, 20 Dec 2023 18:03:18 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Move common code for EFR lock/unlock of mutex into functions for code reuse
> > and clarity.
>
> ...
>
> > @@ -333,6 +333,7 @@ struct sc16is7xx_one {
> > struct sc16is7xx_one_config config;
> > bool irda_mode;
> > unsigned int old_mctrl;
> > + u8 old_lcr; /* Value before EFR access. */
> > };
>
> Have you run `pahole`?
> I believe with
>
> unsigned int old_mctrl;
> u8 old_lcr; /* Value before EFR access. */
> bool irda_mode;
>
> layout it will take less memory.
Hi,
I did not know about this tool, nice.
$ pahole -C sc16is7xx_one drivers/tty/serial/sc16is7xx.o
Before:
/* size: 752, cachelines: 12, members: 10 */
With your proposed change:
/* size: 744, cachelines: 12, members: 10 */
Will add this modification for V2, as well as other issues
noted below.
Thank you,
Hugo
> > +/* In an amazing feat of design, the Enhanced Features Register (EFR)
>
> /*
> * This is NOT the style we use for multi-line
> * comments in the serial subsystem. On contrary
> * this comment can be used as a proper example.
> * (Yes, I noticed it's an old comment, but take
> * a chance to fix it.)
> */
>
> > + * 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
> > + * "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.
> > + */
>
> ...
>
> > + sc16is7xx_port_write(port, SC16IS7XX_LCR_REG,
> > + SC16IS7XX_LCR_CONF_MODE_B);
>
> One line. (Yes, 81 character, but readability is as good as before.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
>
On Wed, 20 Dec 2023 17:41:31 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Commit 834449872105 ("sc16is7xx: Fix for multi-channel stall") changed
> > sc16is7xx_port_irq() from looping multiple times when there was still
> > interrupts to serve. It simply changed the do {} while(1) loop to a
> > do {} while(0) loop, which makes the loop itself now obsolete.
> >
> > Clean the code by removing this obsolete do {} while(0) loop.
>
> I'm just wondering if you used --histogram diff algo when prepared the patches.
> If no, use that by default.
Hi,
I did not, and effectively it makes the patch easier to follow.
I will use it as default from now on.
Hugo Villeneuve