[PATCH 00/18] serial: sc16is7xx: fixes, cleanups and improvements

[PATCH 00/18] serial: sc16is7xx: fixes, cleanups and improvements

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

[PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

If a problem with a device occurs during probing, and we subsequently
try to remove the driver, we get the following error:

$ rmmod sc16is7xx
[   62.783166] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040
[   62.994226] Call trace:
[   62.996672]  serial_core_unregister_port+0x58/0x2b0
[   63.001558]  serial_ctrl_unregister_port+0x18/0x30
[   63.006354]  uart_remove_one_port+0x18/0x30
[   63.010542]  sc16is7xx_spi_remove+0xc0/0x190 [sc16is7xx]
Segmentation fault

Tested on a custom board with two SC16IS742 ICs, and by simulating a fault
when probing channel (port) B of the second device.

The reason is that uart_remove_one_port() has already been called during
probe in the out_ports: error path, and is called again in
sc16is7xx_remove().

Fix the problem by clearing the device drvdata in probe error path to
indicate that all resources have been deallocated. And only deallocate
resources in sc16is7xx_remove() if device drvdata is non-null.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: stable@vger.kernel.org
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e40e4a99277e..b585663c1e6e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1663,6 +1663,8 @@ static int sc16is7xx_probe(struct device *dev,
 out_clk:
 	clk_disable_unprepare(s->clk);
 
+	dev_set_drvdata(dev, NULL);
+
 	return ret;
 }
 
@@ -1671,6 +1673,9 @@ static void sc16is7xx_remove(struct device *dev)
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 	int i;
 
+	if (!s)
+		return;
+
 #ifdef CONFIG_GPIOLIB
 	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
-- 
2.39.2

Re: [PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:45PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If a problem with a device occurs during probing, and we subsequently
> try to remove the driver, we get the following error:
> 
> $ rmmod sc16is7xx
> [   62.783166] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040
> [   62.994226] Call trace:
> [   62.996672]  serial_core_unregister_port+0x58/0x2b0
> [   63.001558]  serial_ctrl_unregister_port+0x18/0x30
> [   63.006354]  uart_remove_one_port+0x18/0x30
> [   63.010542]  sc16is7xx_spi_remove+0xc0/0x190 [sc16is7xx]
> Segmentation fault
> 
> Tested on a custom board with two SC16IS742 ICs, and by simulating a fault
> when probing channel (port) B of the second device.
> 
> The reason is that uart_remove_one_port() has already been called during
> probe in the out_ports: error path, and is called again in
> sc16is7xx_remove().
> 
> Fix the problem by clearing the device drvdata in probe error path to
> indicate that all resources have been deallocated. And only deallocate
> resources in sc16is7xx_remove() if device drvdata is non-null.

...

> +	dev_set_drvdata(dev, NULL);

I believe this is wrong approach to fix the issue as this one is prone
to be cleaned up in the future as we don't do this call explicitly for
the past ~15 years.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/18] serial: sc16is7xx: fix segfault when removing driver

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 05:34:29PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 12:18:45PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

...

> > +	dev_set_drvdata(dev, NULL);
> 
> I believe this is wrong approach to fix the issue as this one is prone
> to be cleaned up in the future as we don't do this call explicitly for
> the past ~15 years.

On top of that the ->remove() is not the only uart_remove_one_port() call.
It has a lot of other stuff to go with.

It seems that ->remove() doesn't check the bit in &sc16is7xx_lines, that
might be the proper fix for the issue you have.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

If an error occurs during probing, the sc16is7xx_lines bitfield may be left
in a state that doesn't represent the correct state of lines allocation.

For example, in a system with two SC16 devices, if an error occurs only
during probing of channel (port) B of the second device, sc16is7xx_lines
final state will be 00001011b instead of the expected 00000011b.

This is caused in part because of the "i--" in the for/loop located in
the out_ports: error path.

Fix this by checking the return value of uart_add_one_port() and set line
allocation bit only if this was successful. This allows the refactor of
the obfuscated for(i--...) loop in the error path, and properly call
uart_remove_one_port() only when needed, and properly unset line allocation
bits.

Also use same mechanism in remove() when calling uart_remove_one_port().

Fixes: c64349722d14 ("sc16is7xx: support multiple devices")
Cc: stable@vger.kernel.org
Cc: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
There is already a patch by Yury Norov <yury.norov@gmail.com> to simplify
sc16is7xx_alloc_line():
https://lore.kernel.org/all/20231212022749.625238-30-yury.norov@gmail.com/

Since my patch gets rid of sc16is7xx_alloc_line() entirely, it would make
Yury's patch unnecessary.
---
 drivers/tty/serial/sc16is7xx.c | 44 ++++++++++++++--------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b585663c1e6e..b92fd01cfeec 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -407,19 +407,6 @@ static void sc16is7xx_port_update(struct uart_port *port, u8 reg,
 	regmap_update_bits(one->regmap, reg, mask, val);
 }
 
-static int sc16is7xx_alloc_line(void)
-{
-	int i;
-
-	BUILD_BUG_ON(SC16IS7XX_MAX_DEVS > BITS_PER_LONG);
-
-	for (i = 0; i < SC16IS7XX_MAX_DEVS; i++)
-		if (!test_and_set_bit(i, &sc16is7xx_lines))
-			break;
-
-	return i;
-}
-
 static void sc16is7xx_power(struct uart_port *port, int on)
 {
 	sc16is7xx_port_update(port, SC16IS7XX_IER_REG,
@@ -1550,6 +1537,13 @@ static int sc16is7xx_probe(struct device *dev,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
+		s->p[i].port.line = find_first_zero_bit(&sc16is7xx_lines,
+							SC16IS7XX_MAX_DEVS);
+		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
+			ret = -ERANGE;
+			goto out_ports;
+		}
+
 		/* Initialize port data */
 		s->p[i].port.dev	= dev;
 		s->p[i].port.irq	= irq;
@@ -1569,14 +1563,8 @@ static int sc16is7xx_probe(struct device *dev,
 		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].port.line	= sc16is7xx_alloc_line();
 		s->p[i].regmap		= regmaps[i];
 
-		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
-			ret = -ENOMEM;
-			goto out_ports;
-		}
-
 		mutex_init(&s->p[i].efr_lock);
 
 		ret = uart_get_rs485_mode(&s->p[i].port);
@@ -1594,8 +1582,13 @@ static int sc16is7xx_probe(struct device *dev,
 		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 */
-		uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
+		ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
+		if (ret)
+			goto out_ports;
+
+		set_bit(s->p[i].port.line, &sc16is7xx_lines);
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
@@ -1653,10 +1646,9 @@ static int sc16is7xx_probe(struct device *dev,
 #endif
 
 out_ports:
-	for (i--; i >= 0; i--) {
-		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
-		clear_bit(s->p[i].port.line, &sc16is7xx_lines);
-	}
+	for (i = 0; i < devtype->nr_uart; i++)
+		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 
 	kthread_stop(s->kworker_task);
 
@@ -1683,8 +1675,8 @@ static void sc16is7xx_remove(struct device *dev)
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
 		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
-		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
-		clear_bit(s->p[i].port.line, &sc16is7xx_lines);
+		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
-- 
2.39.2

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> in a state that doesn't represent the correct state of lines allocation.
> 
> For example, in a system with two SC16 devices, if an error occurs only
> during probing of channel (port) B of the second device, sc16is7xx_lines
> final state will be 00001011b instead of the expected 00000011b.
> 
> This is caused in part because of the "i--" in the for/loop located in
> the out_ports: error path.
> 
> Fix this by checking the return value of uart_add_one_port() and set line
> allocation bit only if this was successful. This allows the refactor of
> the obfuscated for(i--...) loop in the error path, and properly call
> uart_remove_one_port() only when needed, and properly unset line allocation
> bits.
> 
> Also use same mechanism in remove() when calling uart_remove_one_port().

Yes, this seems to be the correct one to fix the problem described in
the patch 1. I dunno why the patch 1 even exists.

As for Yury's patch, you are doing fixes, so your stuff has priority on his.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:40:42 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> > in a state that doesn't represent the correct state of lines allocation.
> > 
> > For example, in a system with two SC16 devices, if an error occurs only
> > during probing of channel (port) B of the second device, sc16is7xx_lines
> > final state will be 00001011b instead of the expected 00000011b.
> > 
> > This is caused in part because of the "i--" in the for/loop located in
> > the out_ports: error path.
> > 
> > Fix this by checking the return value of uart_add_one_port() and set line
> > allocation bit only if this was successful. This allows the refactor of
> > the obfuscated for(i--...) loop in the error path, and properly call
> > uart_remove_one_port() only when needed, and properly unset line allocation
> > bits.
> > 
> > Also use same mechanism in remove() when calling uart_remove_one_port().
> 
> Yes, this seems to be the correct one to fix the problem described in
> the patch 1. I dunno why the patch 1 even exists.

Hi,
this will indeed fix the problem described in patch 1.

However, if I remove patch 1, and I simulate the same probe error as
described in patch 1, now we get stuck forever when trying to 
remove the driver. This is something that I observed before and
that patch 1 also corrected.

The problem is caused in sc16is7xx_remove() when calling this function

    kthread_flush_worker(&s->kworker);

I am not sure how best to handle that without patch 1.

Hugo Villeneuve

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 10:56:39AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:40:42 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:

...

> > Yes, this seems to be the correct one to fix the problem described in
> > the patch 1. I dunno why the patch 1 even exists.
> 
> Hi,
> this will indeed fix the problem described in patch 1.
> 
> However, if I remove patch 1, and I simulate the same probe error as
> described in patch 1, now we get stuck forever when trying to 
> remove the driver. This is something that I observed before and
> that patch 1 also corrected.
> 
> The problem is caused in sc16is7xx_remove() when calling this function
> 
>     kthread_flush_worker(&s->kworker);
> 
> I am not sure how best to handle that without patch 1.

So, it means we need to root cause this issue. Because patch 1 looks
really bogus.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Thu, 21 Dec 2023 10:56:39 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Wed, 20 Dec 2023 17:40:42 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> > On Tue, Dec 19, 2023 at 12:18:46PM -0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > If an error occurs during probing, the sc16is7xx_lines bitfield may be left
> > > in a state that doesn't represent the correct state of lines allocation.
> > > 
> > > For example, in a system with two SC16 devices, if an error occurs only
> > > during probing of channel (port) B of the second device, sc16is7xx_lines
> > > final state will be 00001011b instead of the expected 00000011b.
> > > 
> > > This is caused in part because of the "i--" in the for/loop located in
> > > the out_ports: error path.
> > > 
> > > Fix this by checking the return value of uart_add_one_port() and set line
> > > allocation bit only if this was successful. This allows the refactor of
> > > the obfuscated for(i--...) loop in the error path, and properly call
> > > uart_remove_one_port() only when needed, and properly unset line allocation
> > > bits.
> > > 
> > > Also use same mechanism in remove() when calling uart_remove_one_port().
> > 
> > Yes, this seems to be the correct one to fix the problem described in
> > the patch 1. I dunno why the patch 1 even exists.
> 
> Hi,
> this will indeed fix the problem described in patch 1.
> 
> However, if I remove patch 1, and I simulate the same probe error as
> described in patch 1, now we get stuck forever when trying to 
> remove the driver. This is something that I observed before and
> that patch 1 also corrected.
> 
> The problem is caused in sc16is7xx_remove() when calling this function
> 
>     kthread_flush_worker(&s->kworker);
> 
> I am not sure how best to handle that without patch 1.

Also, if we manage to get past kthread_flush_worker() and 
kthread_stop() (commented out for testing purposes), we get another bug:

# rmmod sc16is7xx
...
crystal-duart-24m already disabled
WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
clk_core_disable+0x1b0/0x1e0
...
Call trace:
clk_core_disable+0x1b0/0x1e0
clk_disable+0x38/0x60
sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]

This one is caused by calling clk_disable_unprepare(). But
clk_disable_unprepare() has already been called in probe error handling
code. Patch 1 also fixed this...

Hugo Villeneuve

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 11:13:37AM -0500, Hugo Villeneuve wrote:
> On Thu, 21 Dec 2023 10:56:39 -0500
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Wed, 20 Dec 2023 17:40:42 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

...

> > this will indeed fix the problem described in patch 1.
> > 
> > However, if I remove patch 1, and I simulate the same probe error as
> > described in patch 1, now we get stuck forever when trying to 
> > remove the driver. This is something that I observed before and
> > that patch 1 also corrected.
> > 
> > The problem is caused in sc16is7xx_remove() when calling this function
> > 
> >     kthread_flush_worker(&s->kworker);
> > 
> > I am not sure how best to handle that without patch 1.
> 
> Also, if we manage to get past kthread_flush_worker() and 
> kthread_stop() (commented out for testing purposes), we get another bug:
> 
> # rmmod sc16is7xx
> ...
> crystal-duart-24m already disabled
> WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
> clk_core_disable+0x1b0/0x1e0
> ...
> Call trace:
> clk_core_disable+0x1b0/0x1e0
> clk_disable+0x38/0x60
> sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]
> 
> This one is caused by calling clk_disable_unprepare(). But
> clk_disable_unprepare() has already been called in probe error handling
> code. Patch 1 also fixed this...

Word "fixed" is incorrect. "Papered over" is what it did.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/18] serial: sc16is7xx: fix invalid sc16is7xx_lines bitfield in case of probe error

[Hugo Villeneuve]

On Thu, 21 Dec 2023 18:16:40 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Dec 21, 2023 at 11:13:37AM -0500, Hugo Villeneuve wrote:
> > On Thu, 21 Dec 2023 10:56:39 -0500
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Wed, 20 Dec 2023 17:40:42 +0200
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> ...
> 
> > > this will indeed fix the problem described in patch 1.
> > > 
> > > However, if I remove patch 1, and I simulate the same probe error as
> > > described in patch 1, now we get stuck forever when trying to 
> > > remove the driver. This is something that I observed before and
> > > that patch 1 also corrected.
> > > 
> > > The problem is caused in sc16is7xx_remove() when calling this function
> > > 
> > >     kthread_flush_worker(&s->kworker);
> > > 
> > > I am not sure how best to handle that without patch 1.
> > 
> > Also, if we manage to get past kthread_flush_worker() and 
> > kthread_stop() (commented out for testing purposes), we get another bug:
> > 
> > # rmmod sc16is7xx
> > ...
> > crystal-duart-24m already disabled
> > WARNING: CPU: 2 PID: 340 at drivers/clk/clk.c:1090
> > clk_core_disable+0x1b0/0x1e0
> > ...
> > Call trace:
> > clk_core_disable+0x1b0/0x1e0
> > clk_disable+0x38/0x60
> > sc16is7xx_remove+0x1e4/0x240 [sc16is7xx]
> > 
> > This one is caused by calling clk_disable_unprepare(). But
> > clk_disable_unprepare() has already been called in probe error handling
> > code. Patch 1 also fixed this...
> 
> Word "fixed" is incorrect. "Papered over" is what it did.

Hi,
I just found the problem, and it was in my bug simulation, not the
driver itself. When I simulated the bug, I forgot to set "ret" to an
error code, and thus I returned 0 at the end of sc16is7xx_probe(). This
is why sc16is7xx_remove() was called when unloading driver, but
shouldn't have.

If I simulate my probe error and return "-EINVAL" at the end of
sc16is7xx_probe(), sc16is7xx_remove() is not called when
unloading the driver.

Sorry for the noise, so I will drop patch 1 and leave patch "fix invalid
sc16is7xx_lines bitfield in case of probe error" as it is, and
simply remove comments about Yury's patch.

Hugo.

[PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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: stable@vger.kernel.org
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 03/18] serial: sc16is7xx: remove obsolete loop in sc16is7xx_port_irq()

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:41:31 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:47PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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

[PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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: stable@vger.kernel.org
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 04/18] serial: sc16is7xx: improve do/while loop in sc16is7xx_irq()

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:42:42 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:48PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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

[PATCH 05/18] serial: sc16is7xx: use DECLARE_BITMAP for sc16is7xx_lines bitfield

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Replace the explicit sc16is7xx_lines bitfield declaration with the generic
macro DECLARE_BITMAP() to reserve just enough memory to contain all
required bits.

This also improves code self-documentation by showing the maximum number
of bits required.

This conversion now makes sc16is7xx_lines an array, so drop the "&" before
sc16is7xx_lines in all bit access functions.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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 8a038a9be09e..597280a5d0f3 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -346,7 +346,7 @@ struct sc16is7xx_port {
 	struct sc16is7xx_one		p[];
 };
 
-static unsigned long sc16is7xx_lines;
+static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
 
 static struct uart_driver sc16is7xx_uart = {
 	.owner		= THIS_MODULE,
@@ -1534,7 +1534,7 @@ static int sc16is7xx_probe(struct device *dev,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
-		s->p[i].port.line = find_first_zero_bit(&sc16is7xx_lines,
+		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
 							SC16IS7XX_MAX_DEVS);
 		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
 			ret = -ERANGE;
@@ -1585,7 +1585,7 @@ static int sc16is7xx_probe(struct device *dev,
 		if (ret)
 			goto out_ports;
 
-		set_bit(s->p[i].port.line, &sc16is7xx_lines);
+		set_bit(s->p[i].port.line, sc16is7xx_lines);
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
@@ -1644,7 +1644,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 out_ports:
 	for (i = 0; i < devtype->nr_uart; i++)
-		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 
 	kthread_stop(s->kworker_task);
@@ -1672,7 +1672,7 @@ static void sc16is7xx_remove(struct device *dev)
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
 		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
-		if (test_and_clear_bit(s->p[i].port.line, &sc16is7xx_lines))
+		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
-- 
2.39.2

Re: [PATCH 05/18] serial: sc16is7xx: use DECLARE_BITMAP for sc16is7xx_lines bitfield

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:49PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Replace the explicit sc16is7xx_lines bitfield declaration with the generic
> macro DECLARE_BITMAP() to reserve just enough memory to contain all
> required bits.
> 
> This also improves code self-documentation by showing the maximum number
> of bits required.
> 
> This conversion now makes sc16is7xx_lines an array, so drop the "&" before
> sc16is7xx_lines in all bit access functions.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 06/18] serial: sc16is7xx: use spi_get_device_match_data()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Use preferred spi_get_device_match_data() instead of
device_get_match_data() and spi_get_device_id() to get the driver match
data.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 597280a5d0f3..8cbf54d0a16c 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1742,14 +1742,10 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	if (spi->dev.of_node) {
-		devtype = device_get_match_data(&spi->dev);
-		if (!devtype)
-			return -ENODEV;
-	} else {
-		const struct spi_device_id *id_entry = spi_get_device_id(spi);
-
-		devtype = (struct sc16is7xx_devtype *)id_entry->driver_data;
+	devtype = (struct sc16is7xx_devtype *)spi_get_device_match_data(spi);
+	if (!devtype) {
+		dev_err(&spi->dev, "Failed to match device\n");
+		return -ENODEV;
 	}
 
 	for (i = 0; i < devtype->nr_uart; i++) {
-- 
2.39.2

Re: [PATCH 06/18] serial: sc16is7xx: use spi_get_device_match_data()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:50PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Use preferred spi_get_device_match_data() instead of
> device_get_match_data() and spi_get_device_id() to get the driver match
> data.

...

> +	devtype = (struct sc16is7xx_devtype *)spi_get_device_match_data(spi);

Nice one, but drop the casting. (Yes, make sure the assignee is const. It might
require an additional change.)

> +	if (!devtype) {
> +		dev_err(&spi->dev, "Failed to match device\n");
> +		return -ENODEV;

		return dev_err_probe(...);

?

>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/18] serial: sc16is7xx: use spi_get_device_match_data()

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:44:52 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:50PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Use preferred spi_get_device_match_data() instead of
> > device_get_match_data() and spi_get_device_id() to get the driver match
> > data.
> 
> ...
> 
> > +	devtype = (struct sc16is7xx_devtype *)spi_get_device_match_data(spi);
> 
> Nice one, but drop the casting. (Yes, make sure the assignee is const. It might
> require an additional change.)

Done, devtype was already const.

> 
> > +	if (!devtype) {
> > +		dev_err(&spi->dev, "Failed to match device\n");
> > +		return -ENODEV;
> 
> 		return dev_err_probe(...);
> 
> ?

Yes, done for V2.

Also done the same two changes for the I2C part.

Hugo.

[PATCH 07/18] serial: sc16is7xx: use i2c_get_match_data()

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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 <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 07/18] serial: sc16is7xx: use i2c_get_match_data()

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:51PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

[PATCH 08/18] serial: sc16is7xx: add driver name to struct uart_driver

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Make sure that the driver name is displayed instead of "unknown" when
displaying the driver infos:

Before:
    cat /proc/tty/drivers | grep ttySC
    unknown              /dev/ttySC    243 0-7 serial

After:
    cat /proc/tty/drivers | grep ttySC
    sc16is7xx            /dev/ttySC    243 0-7 serial

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 97a97a4bd71a..3fb99b6929f3 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -350,6 +350,7 @@ static DECLARE_BITMAP(sc16is7xx_lines, SC16IS7XX_MAX_DEVS);
 
 static struct uart_driver sc16is7xx_uart = {
 	.owner		= THIS_MODULE,
+	.driver_name    = SC16IS7XX_NAME,
 	.dev_name	= "ttySC",
 	.nr		= SC16IS7XX_MAX_DEVS,
 };
-- 
2.39.2

Re: [PATCH 08/18] serial: sc16is7xx: add driver name to struct uart_driver

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:52PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Make sure that the driver name is displayed instead of "unknown" when
> displaying the driver infos:
> 
> Before:
>     cat /proc/tty/drivers | grep ttySC
>     unknown              /dev/ttySC    243 0-7 serial
> 
> After:
>     cat /proc/tty/drivers | grep ttySC
>     sc16is7xx            /dev/ttySC    243 0-7 serial

"Useless use of cat" (you can google for this phrase, it's famous).

	grep ... /proc/...

will work :-)

Otherwise LGTM!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 08/18] serial: sc16is7xx: add driver name to struct uart_driver

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:48:03 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:52PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Make sure that the driver name is displayed instead of "unknown" when
> > displaying the driver infos:
> > 
> > Before:
> >     cat /proc/tty/drivers | grep ttySC
> >     unknown              /dev/ttySC    243 0-7 serial
> > 
> > After:
> >     cat /proc/tty/drivers | grep ttySC
> >     sc16is7xx            /dev/ttySC    243 0-7 serial
> 
> "Useless use of cat" (you can google for this phrase, it's famous).
> 
> 	grep ... /proc/...
> 
> will work :-)
> 
> Otherwise LGTM!

Old habits die hard :)

Interesting read!

Will modify commit message in V2.

Thank you.

Hugo Villeneuve

Re: [PATCH 08/18] serial: sc16is7xx: add driver name to struct uart_driver

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 11:11:36AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:48:03 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:18:52PM -0500, Hugo Villeneuve wrote:

...

> > "Useless use of cat" (you can google for this phrase, it's famous).

> Interesting read!

Yep! Then there are categories: (instead of "cat" you may use) "grep", "gzip"
(e.g., better use zless / zgrep) and so on :-)

-- 
With Best Regards,
Andy Shevchenko

[PATCH 09/18] serial: sc16is7xx: add macro for max number of UART ports

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add macro to hold the maximum number of UART ports per IC/device.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 3fb99b6929f3..29844e2eefc5 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -28,6 +28,7 @@
 
 #define SC16IS7XX_NAME			"sc16is7xx"
 #define SC16IS7XX_MAX_DEVS		8
+#define SC16IS7XX_MAX_PORTS		2 /* Maximum number of UART ports per IC. */
 
 /* SC16IS7XX register definitions */
 #define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
@@ -1396,11 +1397,11 @@ static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s)
 	int i;
 	int ret;
 	int count;
-	u32 irda_port[2];
+	u32 irda_port[SC16IS7XX_MAX_PORTS];
 	struct device *dev = s->p[0].port.dev;
 
 	count = device_property_count_u32(dev, "irda-mode-ports");
-	if (count < 0 || count > ARRAY_SIZE(irda_port))
+	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
 		return;
 
 	ret = device_property_read_u32_array(dev, "irda-mode-ports",
@@ -1423,11 +1424,11 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
 	int i;
 	int ret;
 	int count;
-	u32 mctrl_port[2];
+	u32 mctrl_port[SC16IS7XX_MAX_PORTS];
 	struct device *dev = s->p[0].port.dev;
 
 	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
-	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
 		return 0;
 
 	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
@@ -1471,6 +1472,8 @@ static int sc16is7xx_probe(struct device *dev,
 	int i, ret;
 	struct sc16is7xx_port *s;
 
+	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
+
 	for (i = 0; i < devtype->nr_uart; i++)
 		if (IS_ERR(regmaps[i]))
 			return PTR_ERR(regmaps[i]);
@@ -1730,7 +1733,7 @@ static unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id)
 static int sc16is7xx_spi_probe(struct spi_device *spi)
 {
 	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[2];
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
 	unsigned int i;
 	int ret;
 
@@ -1800,7 +1803,7 @@ MODULE_ALIAS("spi:sc16is7xx");
 static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
 {
 	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[2];
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
 	unsigned int i;
 
 	devtype = (struct sc16is7xx_devtype *)i2c_get_match_data(i2c);
-- 
2.39.2

Re: [PATCH 09/18] serial: sc16is7xx: add macro for max number of UART ports

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Add macro to hold the maximum number of UART ports per IC/device.

...

> -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)

ARRAY_SIZE() is more robust than this. What if you change to support different
devices where this won't be as defined?

>  		return;

...

> -	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
>  		return 0;

Ditto.

...

> +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);

Not sure about this, perhaps it's fine.

Otherwise looks reasonable.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/18] serial: sc16is7xx: add macro for max number of UART ports

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:50:34 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Add macro to hold the maximum number of UART ports per IC/device.
> 
> ...
> 
> > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> 
> ARRAY_SIZE() is more robust than this. What if you change to support different
> devices where this won't be as defined?

Hi,
not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
the maximum for all devices supported by this driver. The irda_port
array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
even if the device that we are probing has only one port for example.

But I can change it back to ARRAY_SIZE(irda_port) if you want.

> 
> >  		return;
> 
> ...
> 
> > -	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> >  		return 0;
> 
> Ditto.
> 
> ...
> 
> > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> 
> Not sure about this, perhaps it's fine.

This check is only there if we add support for a new device and we
incorrectly set nr_uart to an incorrect value, which will cause other
problems anyway, of course :)

This could be removed.

Hugo Villeneuve

Re: [PATCH 09/18] serial: sc16is7xx: add macro for max number of UART ports

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:50:34 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:

...

> > > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> > 
> > ARRAY_SIZE() is more robust than this. What if you change to support different
> > devices where this won't be as defined?
> 
> not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
> the maximum for all devices supported by this driver. The irda_port
> array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
> even if the device that we are probing has only one port for example.

For current models of the device, yes. Who knows the future?
Also, ARRAY_SIZE() make it less points to update if ever needed.

> But I can change it back to ARRAY_SIZE(irda_port) if you want.

Please change it back.

> > >  		return;

...

> > > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> > 
> > Not sure about this, perhaps it's fine.
> 
> This check is only there if we add support for a new device and we
> incorrectly set nr_uart to an incorrect value, which will cause other
> problems anyway, of course :)
> 
> This could be removed.

Let's remove. We can add it back in case something like this (quite unlikely)
happens.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/18] serial: sc16is7xx: add macro for max number of UART ports

[Hugo Villeneuve]

On Thu, 21 Dec 2023 18:55:17 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote:
> > On Wed, 20 Dec 2023 17:50:34 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> 
> ...
> 
> > > > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > > > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> > > 
> > > ARRAY_SIZE() is more robust than this. What if you change to support different
> > > devices where this won't be as defined?
> > 
> > not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
> > the maximum for all devices supported by this driver. The irda_port
> > array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
> > even if the device that we are probing has only one port for example.
> 
> For current models of the device, yes. Who knows the future?
> Also, ARRAY_SIZE() make it less points to update if ever needed.
> 
> > But I can change it back to ARRAY_SIZE(irda_port) if you want.
> 
> Please change it back.
> 
> > > >  		return;
> 
> ...
> 
> > > > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> > > 
> > > Not sure about this, perhaps it's fine.
> > 
> > This check is only there if we add support for a new device and we
> > incorrectly set nr_uart to an incorrect value, which will cause other
> > problems anyway, of course :)
> > 
> > This could be removed.
> 
> Let's remove. We can add it back in case something like this (quite unlikely)
> happens.

Ok, will do both for v2.

Hugo Villeneuve

[PATCH 10/18] serial: sc16is7xx: use HZ_PER_MHZ macro to improve readability

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Improves code readability by making it more obvious that it is a MHz value.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 10/18] serial: sc16is7xx: use HZ_PER_MHZ macro to improve readability

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:54PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Improves code readability by making it more obvious that it is a MHz value.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 11/18] serial: sc16is7xx: add explicit return for some switch default cases

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Allows to simplify code by removing the break statement in the default
switch/case in some functions.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 11/18] serial: sc16is7xx: add explicit return for some switch default cases

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:55PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Allows to simplify code by removing the break statement in the default
> switch/case in some functions.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 12/18] serial: sc16is7xx: replace hardcoded divisor value with BIT() macro

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

To better show why the limit is what it is, since we have only 16 bits for
the divisor.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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 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

Re: [PATCH 12/18] serial: sc16is7xx: replace hardcoded divisor value with BIT() macro

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:56PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> To better show why the limit is what it is, since we have only 16 bits for
> the divisor.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 13/18] serial: sc16is7xx: use in_range() for DT properties bound checks

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Improve code readability and efficiency by using in_range() when checking
device tree properties bound.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 13/18] serial: sc16is7xx: use in_range() for DT properties bound checks

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:57PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 13/18] serial: sc16is7xx: use in_range() for DT properties bound checks

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:54:07 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:57PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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

[PATCH 14/18] serial: sc16is7xx: drop unneeded MODULE_ALIAS

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

The MODULE_DEVICE_TABLE already creates the proper aliases for the
SPI driver.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 29089b11f6f1..a9e44e5ef713 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1793,8 +1793,6 @@ static struct spi_driver sc16is7xx_spi_uart_driver = {
 	.remove		= sc16is7xx_spi_remove,
 	.id_table	= sc16is7xx_spi_id_table,
 };
-
-MODULE_ALIAS("spi:sc16is7xx");
 #endif
 
 #ifdef CONFIG_SERIAL_SC16IS7XX_I2C
-- 
2.39.2

Re: [PATCH 14/18] serial: sc16is7xx: drop unneeded MODULE_ALIAS

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:58PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> The MODULE_DEVICE_TABLE already creates the proper aliases for the

MODULE_DEVICE_TABLE()

> SPI driver.

With the above fixed
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 14/18] serial: sc16is7xx: drop unneeded MODULE_ALIAS

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:54:55 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:58PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > The MODULE_DEVICE_TABLE already creates the proper aliases for the
> 
> MODULE_DEVICE_TABLE()

Done for V2.

Hugo.


> 
> > SPI driver.
> 
> With the above fixed
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 


-- 
Hugo Villeneuve

[PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

To simplify function by avoiding cast.

This is similar to what is done in max310x driver.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
If deemed appropriate for stable kernel backporting:
Fixes: dec273ecc116 ("sc16is7xx: fix FIFO address of secondary UART")
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/sc16is7xx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a9e44e5ef713..3322507ab18e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -380,17 +380,15 @@ static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val)
 	regmap_write(one->regmap, reg, val);
 }
 
-static void sc16is7xx_fifo_read(struct uart_port *port, unsigned int rxlen)
+static void sc16is7xx_fifo_read(struct uart_port *port, u8 *rxbuf, unsigned int rxlen)
 {
-	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
-	regmap_noinc_read(one->regmap, SC16IS7XX_RHR_REG, s->buf, rxlen);
+	regmap_noinc_read(one->regmap, SC16IS7XX_RHR_REG, rxbuf, rxlen);
 }
 
-static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send)
+static void sc16is7xx_fifo_write(struct uart_port *port, u8 *txbuf, u8 to_send)
 {
-	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
 	/*
@@ -400,7 +398,7 @@ static void sc16is7xx_fifo_write(struct uart_port *port, u8 to_send)
 	if (unlikely(!to_send))
 		return;
 
-	regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, s->buf, to_send);
+	regmap_noinc_write(one->regmap, SC16IS7XX_THR_REG, txbuf, to_send);
 }
 
 static void sc16is7xx_port_update(struct uart_port *port, u8 reg,
@@ -576,7 +574,7 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
 			s->buf[0] = sc16is7xx_port_read(port, SC16IS7XX_RHR_REG);
 			bytes_read = 1;
 		} else {
-			sc16is7xx_fifo_read(port, rxlen);
+			sc16is7xx_fifo_read(port, s->buf, rxlen);
 			bytes_read = rxlen;
 		}
 
@@ -665,7 +663,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 			uart_xmit_advance(port, 1);
 		}
 
-		sc16is7xx_fifo_write(port, to_send);
+		sc16is7xx_fifo_write(port, s->buf, to_send);
 	}
 
 	uart_port_lock_irqsave(port, &flags);
-- 
2.39.2

Re: [PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:59PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> To simplify function by avoiding cast.
> 
> This is similar to what is done in max310x driver.

...

> ---
> If deemed appropriate for stable kernel backporting:

I don't think it's eligible.

> ---

I don't see the necessity of the change, OTOH it's harmless.
The problem is that commit message is basically "Yeah, we
do cargo cult." Because I haven't seen what casting you are
talking about.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/18] serial: sc16is7xx: pass R/W buffer in FIFO functions

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:57:59 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:59PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > To simplify function by avoiding cast.
> > 
> > This is similar to what is done in max310x driver.
> 
> ...
> 
> > ---
> > If deemed appropriate for stable kernel backporting:
> 
> I don't think it's eligible.

No problem.


> > ---
> 
> I don't see the necessity of the change, OTOH it's harmless.
> The problem is that commit message is basically "Yeah, we
> do cargo cult." Because I haven't seen what casting you are
> talking about.

I'll try to reword the commit message.

And replace cast with something like "... to remove additional struct
sc16is7xx_port variable..."

Hugo Villeneuve

[PATCH 16/18] serial: sc16is7xx: reorder code to remove prototype declarations

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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 <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 16/18] serial: sc16is7xx: reorder code to remove prototype declarations

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 16/18] serial: sc16is7xx: reorder code to remove prototype declarations

[Hugo Villeneuve]

On Wed, 20 Dec 2023 17:58:40 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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

Re: [PATCH 16/18] serial: sc16is7xx: reorder code to remove prototype declarations

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 11:30:07AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:58:40 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:19:00PM -0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

> > 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 <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 17/18] serial: sc16is7xx: refactor EFR lock

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Move common code for EFR lock/unlock of mutex into functions for code reuse
and clarity.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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

Re: [PATCH 17/18] serial: sc16is7xx: refactor EFR lock

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 17/18] serial: sc16is7xx: refactor EFR lock

[Hugo Villeneuve]

On Wed, 20 Dec 2023 18:03:18 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:19:01PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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
> 
> 
> 
> 

[PATCH 18/18] serial: sc16is7xx: fix whitespace in sc16is7xx_startup() comments

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add missing space at end of comments.

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

Re: [PATCH 18/18] serial: sc16is7xx: fix whitespace in sc16is7xx_startup() comments

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:19:02PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 18/18] serial: sc16is7xx: fix whitespace in sc16is7xx_startup() comments

[Hugo Villeneuve]

On Wed, 20 Dec 2023 18:04:55 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:19:02PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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

Re: [PATCH 00/18] serial: sc16is7xx: fixes, cleanups and improvements

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 12:18:44PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

Re: [PATCH 00/18] serial: sc16is7xx: fixes, cleanups and improvements

[Hugo Villeneuve]

On Wed, 20 Dec 2023 18:06:29 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:44PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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