2023-06-07 14:11:19

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 00/10] serial: sc16is7xx: fix GPIO regression and rs485 improvements

From: Hugo Villeneuve <[email protected]>

Hello,
this patch series mainly fixes a GPIO regression and improve RS485 flags and
properties detection from DT.

It now also includes various small fixes and improvements that were previously
sent as separate patches, but that made testing everything difficult.

Patch 1 fixes an issue with init of first port during probing.

Patch 2 fixes an issue when debugging IOcontrol register, but it is also
necessary for patch "fix regression with GPIO configuration" to work.

Patch 3 fixes an incorrect label in sc16is7xx_probe() cleanup code.

Patch 4 is a refactor of GPIO registration code in preparation for patch 5.

Patches 5 and 6 fix a GPIO regression by (re)allowing to choose GPIO function
for GPIO pins shared with modem status lines.

Patch 7 fixes a bug with the output value when first setting the GPIO direction.

Patch 8 allows to read common rs485 device-tree flags and properties.

Patch 9 introduces a delay after a reset operation to respect datasheet
timing recommandations.

Patch 10 improves comments about chip variants.

I have tested the changes on a custom board with two SC16IS752 DUART using a
Variscite IMX8MN NANO SOM.

Thank you.

Link: [v1] https://lkml.org/lkml/2023/5/17/967
[v1] https://lkml.org/lkml/2023/5/17/777
[v1] https://lkml.org/lkml/2023/5/17/780
[v1] https://lkml.org/lkml/2023/5/17/785
[v1] https://lkml.org/lkml/2023/5/17/1311
[v2] https://lkml.org/lkml/2023/5/18/516
[v3] https://lkml.org/lkml/2023/5/25/7
[v4] https://lkml.org/lkml/2023/5/29/656
[v5] https://lkml.org/lkml/2023/6/1/1046
[v6] https://lkml.org/lkml/2023/6/1/1328
[v7] https://lkml.org/lkml/2023/6/2/861

Changes for V3:
- Integrated all patches into single serie to facilitate debugging and tests.
- Reduce number of exported GPIOs depending on new property
nxp,modem-control-line-ports
- Added additional example in DT bindings

Changes for V4:
- Increase reset post delay to relax scheduler.
- Put comments patches at the end.
- Remove Fixes tag for patch "mark IOCONTROL register as volatile".
- Improve commit messages after reviews.
- Fix coding style issues after reviews.
- Change GPIO registration to always register the maximum number of GPIOs
supported by the chip, but maks-out GPIOs declared as modem control lines.
- Add patch to refactor GPIO registration.
- Remove patch "serial: sc16is7xx: fix syntax error in comments".
- Remove patch "add dump registers function"

Changes for V5:
- Change patch order to facilitate stable backport(s).
- Change duplicate device addresses in DT binding examples.
- Use GENMASK for bit masks.
- Replace of_property_for_each_u32() with device_property_read_u32_array
- Add "Cc: stable..." tags

Changes for V6:
- Fix compilation bug introduced by patch 3

Changes for V7:
- Minor changes and coding style fixes after review for
patch 5 "fix regression with GPIO configuration".

Changes for V8:
- Move mctrl_mask to "struct sc16is7xx_port" to avoid compiler warning when
CONFIG_GPIOLIB is undefined.
- Add "struct device" member to "struct sc16is7xx_port", in order to avoid
passing a raw "struct device" to called functions from sc16is7xx_probe().
- Add new patch "serial: sc16is7xx: remove obsolete out_thread label"

Hugo Villeneuve (10):
serial: sc16is7xx: fix broken port 0 uart init
serial: sc16is7xx: mark IOCONTROL register as volatile
serial: sc16is7xx: remove obsolete out_thread label
serial: sc16is7xx: refactor GPIO controller registration
dt-bindings: sc16is7xx: Add property to change GPIO function
serial: sc16is7xx: fix regression with GPIO configuration
serial: sc16is7xx: fix bug when first setting GPIO direction
serial: sc16is7xx: add call to get rs485 DT flags and properties
serial: sc16is7xx: add post reset delay
serial: sc16is7xx: improve comments about variants

.../bindings/serial/nxp,sc16is7xx.txt | 46 +++++
drivers/tty/serial/sc16is7xx.c | 177 +++++++++++++-----
2 files changed, 181 insertions(+), 42 deletions(-)


base-commit: a4d7d701121981e3c3fe69ade376fe9f26324161
--
2.30.2



2023-06-07 14:13:34

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 07/10] serial: sc16is7xx: fix bug when first setting GPIO direction

From: Hugo Villeneuve <[email protected]>

When configuring a pin as an output pin with a value of logic 0, we
end up as having a value of logic 1 on the output pin. Setting a
logic 0 a second time (or more) after that will correctly output a
logic 0 on the output pin.

By default, all GPIO pins are configured as inputs. When we enter
sc16is7xx_gpio_direction_output() for the first time, we first set the
desired value in IOSTATE, and then we configure the pin as an output.
The datasheet states that writing to IOSTATE register will trigger a
transfer of the value to the I/O pin configured as output, so if the
pin is configured as an input, nothing will be transferred.

Therefore, set the direction first in IODIR, and then set the desired
value in IOSTATE.

This is what is done in NXP application note AN10587.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 11edbebe8fa2..98c520ef4a02 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1342,9 +1342,18 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
state |= BIT(offset);
else
state &= ~BIT(offset);
- sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);
+
+ /*
+ * If we write IOSTATE first, and then IODIR, the output value is not
+ * transferred to the corresponding I/O pin.
+ * The datasheet states that each register bit will be transferred to
+ * the corresponding I/O pin programmed as output when writing to
+ * IOSTATE. Therefore, configure direction first with IODIR, and then
+ * set value after with IOSTATE.
+ */
sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset),
BIT(offset));
+ sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);

return 0;
}
--
2.30.2


2023-06-07 14:16:23

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 09/10] serial: sc16is7xx: add post reset delay

From: Hugo Villeneuve <[email protected]>

Make sure we wait at least 3us before initiating communication with
the device after reset.

Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7246f8997fda..7616703f7438 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev,
regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
SC16IS7XX_IOCONTROL_SRESET_BIT);

+ /*
+ * After reset, the host must wait at least 3us before initializing a
+ * communication with the device.
+ */
+ usleep_range(5, 10);
+
for (i = 0; i < devtype->nr_uart; ++i) {
s->p[i].line = i;
/* Initialize port data */
--
2.30.2


2023-06-07 14:20:33

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 01/10] serial: sc16is7xx: fix broken port 0 uart init

From: Hugo Villeneuve <[email protected]>

The sc16is7xx_config_rs485() function is called only for the second
port (index 1, channel B), causing initialization problems for the
first port.

For the sc16is7xx driver, port->membase and port->mapbase are not set,
and their default values are 0. And we set port->iobase to the device
index. This means that when the first device is registered using the
uart_add_one_port() function, the following values will be in the port
structure:
port->membase = 0
port->mapbase = 0
port->iobase = 0

Therefore, the function uart_configure_port() in serial_core.c will
exit early because of the following check:
/*
* If there isn't a port here, don't do anything further.
*/
if (!port->iobase && !port->mapbase && !port->membase)
return;

Typically, I2C and SPI drivers do not set port->membase and
port->mapbase.

The max310x driver sets port->membase to ~0 (all ones). By
implementing the same change in this driver, uart_configure_port() is
now correctly executed for all ports.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <[email protected]> # 6.1.x
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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 abad091baeea..faa51a58671f 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
s->p[i].port.flags = UPF_FIXED_TYPE | UPF_LOW_LATENCY;
s->p[i].port.iobase = i;
+ s->p[i].port.membase = (void __iomem *)~0;
s->p[i].port.iotype = UPIO_PORT;
s->p[i].port.uartclk = freq;
s->p[i].port.rs485_config = sc16is7xx_config_rs485;
--
2.30.2


2023-06-07 14:22:43

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

From: Hugo Villeneuve <[email protected]>

Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
changed the function of the GPIOs pins to act as modem control
lines without any possibility of selecting GPIO function.

As a consequence, applications that depends on GPIO lines configured
by default as GPIO pins no longer work as expected.

Also, the change to select modem control lines function was done only
for channel A of dual UART variants (752/762). This was not documented
in the log message.

Allow to specify GPIO or modem control line function in the device
tree, and for each of the ports (A or B).

Do so by using the new device-tree property named
"modem-control-line-ports" (property added in separate patch).

When registering GPIO chip controller, mask-out GPIO pins declared as
modem control lines according to this new "modem-control-line-ports"
DT property.

Boards that need to have GPIOS configured as modem control lines
should add that property to their device tree. Here is a list of
boards using the sc16is7xx driver in their device tree and that may
need to be modified:
arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
mips/boot/dts/ingenic/cu1830-neo.dts
mips/boot/dts/ingenic/cu1000-neo.dts

Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
Cc: <[email protected]> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <[email protected]> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
Cc: <[email protected]> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
Cc: <[email protected]> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <[email protected]> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
Cc: <[email protected]> # 6.1.x
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 104 +++++++++++++++++++++++++++------
1 file changed, 85 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index f5b4d2d18a71..11edbebe8fa2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@

/* IOControl register bits (Only 750/760) */
#define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */
-#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */
+#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */
+#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */
#define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */

/* EFCR register bits */
@@ -301,12 +302,12 @@
/* Misc definitions */
#define SC16IS7XX_FIFO_SIZE (64)
#define SC16IS7XX_REG_SHIFT 2
+#define SC16IS7XX_GPIOS_PER_BANK 4

struct sc16is7xx_devtype {
char name[10];
int nr_gpio;
int nr_uart;
- int has_mctrl;
};

#define SC16IS7XX_RECONF_MD (1 << 0)
@@ -337,7 +338,9 @@ struct sc16is7xx_port {
struct clk *clk;
#ifdef CONFIG_GPIOLIB
struct gpio_chip gpio;
+ unsigned long gpio_valid_mask;
#endif
+ u8 mctrl_mask;
unsigned char buf[SC16IS7XX_FIFO_SIZE];
struct kthread_worker kworker;
struct task_struct *kworker_task;
@@ -448,35 +451,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
.name = "SC16IS74X",
.nr_gpio = 0,
.nr_uart = 1,
- .has_mctrl = 0,
};

static const struct sc16is7xx_devtype sc16is750_devtype = {
.name = "SC16IS750",
- .nr_gpio = 4,
+ .nr_gpio = 8,
.nr_uart = 1,
- .has_mctrl = 1,
};

static const struct sc16is7xx_devtype sc16is752_devtype = {
.name = "SC16IS752",
- .nr_gpio = 0,
+ .nr_gpio = 8,
.nr_uart = 2,
- .has_mctrl = 1,
};

static const struct sc16is7xx_devtype sc16is760_devtype = {
.name = "SC16IS760",
- .nr_gpio = 4,
+ .nr_gpio = 8,
.nr_uart = 1,
- .has_mctrl = 1,
};

static const struct sc16is7xx_devtype sc16is762_devtype = {
.name = "SC16IS762",
- .nr_gpio = 0,
+ .nr_gpio = 8,
.nr_uart = 2,
- .has_mctrl = 1,
};

static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
@@ -1351,14 +1349,43 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
return 0;
}

+static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ struct sc16is7xx_port *s = gpiochip_get_data(chip);
+
+ *valid_mask = s->gpio_valid_mask;
+
+ return 0;
+}
+
static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
{
if (!s->devtype->nr_gpio)
return 0;

+ switch (s->mctrl_mask) {
+ case 0:
+ s->gpio_valid_mask = GENMASK(7, 0);
+ break;
+ case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
+ s->gpio_valid_mask = GENMASK(3, 0);
+ break;
+ case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
+ s->gpio_valid_mask = GENMASK(7, 4);
+ break;
+ default:
+ break;
+ }
+
+ if (s->gpio_valid_mask == 0)
+ return 0;
+
s->gpio.owner = THIS_MODULE;
s->gpio.parent = s->dev;
s->gpio.label = dev_name(s->dev);
+ s->gpio.init_valid_mask = sc16is7xx_gpio_init_valid_mask;
s->gpio.direction_input = sc16is7xx_gpio_direction_input;
s->gpio.get = sc16is7xx_gpio_get;
s->gpio.direction_output = sc16is7xx_gpio_direction_output;
@@ -1371,6 +1398,47 @@ static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
}
#endif

+/*
+ * Configure ports designated to operate as modem control lines.
+ */
+static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
+{
+ int i;
+ int ret;
+ int count;
+ u32 mctrl_port[2];
+
+ count = device_property_count_u32(s->dev,
+ "nxp,modem-control-line-ports");
+ if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+ return 0;
+
+ ret = device_property_read_u32_array(s->dev,
+ "nxp,modem-control-line-ports",
+ mctrl_port, count);
+ if (ret)
+ return ret;
+
+ s->mctrl_mask = 0;
+
+ for (i = 0; i < count; i++) {
+ /* Use GPIO lines as modem control lines */
+ if (mctrl_port[i] == 0)
+ s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+ else if (mctrl_port[i] == 1)
+ s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+ }
+
+ if (s->mctrl_mask)
+ regmap_update_bits(
+ s->regmap,
+ SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+ SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+ SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
+
+ return 0;
+}
+
static const struct serial_rs485 sc16is7xx_rs485_supported = {
.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
.delay_rts_before_send = 1,
@@ -1479,12 +1547,6 @@ static int sc16is7xx_probe(struct device *dev,
SC16IS7XX_EFCR_RXDISABLE_BIT |
SC16IS7XX_EFCR_TXDISABLE_BIT);

- /* Use GPIO lines as modem status registers */
- if (devtype->has_mctrl)
- sc16is7xx_port_write(&s->p[i].port,
- SC16IS7XX_IOCONTROL_REG,
- SC16IS7XX_IOCONTROL_MODEM_BIT);
-
/* Initialize kthread work structs */
kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
@@ -1522,6 +1584,10 @@ static int sc16is7xx_probe(struct device *dev,
s->p[u].irda_mode = true;
}

+ ret = sc16is7xx_setup_mctrl_ports(s);
+ if (ret)
+ goto out_ports;
+
#ifdef CONFIG_GPIOLIB
ret = sc16is7xx_setup_gpio_chip(s);
if (ret)
@@ -1548,7 +1614,7 @@ static int sc16is7xx_probe(struct device *dev,
return 0;

#ifdef CONFIG_GPIOLIB
- if (devtype->nr_gpio)
+ if (s->gpio_valid_mask)
gpiochip_remove(&s->gpio);
#endif

@@ -1572,7 +1638,7 @@ static void sc16is7xx_remove(struct device *dev)
int i;

#ifdef CONFIG_GPIOLIB
- if (s->devtype->nr_gpio)
+ if (s->gpio_valid_mask)
gpiochip_remove(&s->gpio);
#endif

--
2.30.2


2023-06-07 14:29:23

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v8 03/10] serial: sc16is7xx: remove obsolete out_thread label

From: Hugo Villeneuve <[email protected]>

Commit c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later
in probe") moved GPIO setup code later in probe function. Doing so
also required to move ports cleanup code (out_ports label) after the
GPIO cleanup code.

After these moves, the out_thread label becomes misplaced and makes
part of the cleanup code illogical.

This patch remove the now obsolete out_thread label and make GPIO
setup code jump to out_ports label if it fails.

Fixes: c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later in probe")
Signed-off-by: Hugo Villeneuve <[email protected]>
---

I put a "Fixes" tag, even if the patch doesn't fix a "real" bug.

It could have been merged with later patch 6/10, but then the explanation
for the change would have been somewhat lost in the noise, and I think it
is important to clearly state in the logs why this change was done, to fix
a "logical" regression introduced by a previous patch.

I can remove the tag if it is deemed non appropriate.

drivers/tty/serial/sc16is7xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 0c903d44429c..9e9856846643 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1516,7 +1516,7 @@ static int sc16is7xx_probe(struct device *dev,
s->gpio.can_sleep = 1;
ret = gpiochip_add_data(&s->gpio, s);
if (ret)
- goto out_thread;
+ goto out_ports;
}
#endif

@@ -1542,8 +1542,6 @@ static int sc16is7xx_probe(struct device *dev,
#ifdef CONFIG_GPIOLIB
if (devtype->nr_gpio)
gpiochip_remove(&s->gpio);
-
-out_thread:
#endif

out_ports:
--
2.30.2


2023-06-12 12:41:37

by Lech Perczak

[permalink] [raw]
Subject: Re: [PATCH v8 00/10] serial: sc16is7xx: fix GPIO regression and rs485 improvements

W dniu 7.06.2023 o 16:05, Hugo Villeneuve pisze:
> From: Hugo Villeneuve <[email protected]>
>
> Hello,
> this patch series mainly fixes a GPIO regression and improve RS485 flags and
> properties detection from DT.
>
> It now also includes various small fixes and improvements that were previously
> sent as separate patches, but that made testing everything difficult.
>
> Patch 1 fixes an issue with init of first port during probing.
>
> Patch 2 fixes an issue when debugging IOcontrol register, but it is also
> necessary for patch "fix regression with GPIO configuration" to work.
>
> Patch 3 fixes an incorrect label in sc16is7xx_probe() cleanup code.
>
> Patch 4 is a refactor of GPIO registration code in preparation for patch 5.
>
> Patches 5 and 6 fix a GPIO regression by (re)allowing to choose GPIO function
> for GPIO pins shared with modem status lines.
>
> Patch 7 fixes a bug with the output value when first setting the GPIO direction.
>
> Patch 8 allows to read common rs485 device-tree flags and properties.
>
> Patch 9 introduces a delay after a reset operation to respect datasheet
> timing recommandations.
>
> Patch 10 improves comments about chip variants.
>
> I have tested the changes on a custom board with two SC16IS752 DUART using a
> Variscite IMX8MN NANO SOM.
>
> Thank you.
>
> Link: [v1] https://lkml.org/lkml/2023/5/17/967 <https://lkml.org/lkml/2023/5/17/967>
> [v1] https://lkml.org/lkml/2023/5/17/777 <https://lkml.org/lkml/2023/5/17/777>
> [v1] https://lkml.org/lkml/2023/5/17/780 <https://lkml.org/lkml/2023/5/17/780>
> [v1] https://lkml.org/lkml/2023/5/17/785 <https://lkml.org/lkml/2023/5/17/785>
> [v1] https://lkml.org/lkml/2023/5/17/1311 <https://lkml.org/lkml/2023/5/17/1311>
> [v2] https://lkml.org/lkml/2023/5/18/516 <https://lkml.org/lkml/2023/5/18/516>
> [v3] https://lkml.org/lkml/2023/5/25/7 <https://lkml.org/lkml/2023/5/25/7>
> [v4] https://lkml.org/lkml/2023/5/29/656 <https://lkml.org/lkml/2023/5/29/656>
> [v5] https://lkml.org/lkml/2023/6/1/1046 <https://lkml.org/lkml/2023/6/1/1046>
> [v6] https://lkml.org/lkml/2023/6/1/1328 <https://lkml.org/lkml/2023/6/1/1328>
> [v7] https://lkml.org/lkml/2023/6/2/861 <https://lkml.org/lkml/2023/6/2/861>
>
> Changes for V3:
> - Integrated all patches into single serie to facilitate debugging and tests.
> - Reduce number of exported GPIOs depending on new property
> nxp,modem-control-line-ports
> - Added additional example in DT bindings
>
> Changes for V4:
> - Increase reset post delay to relax scheduler.
> - Put comments patches at the end.
> - Remove Fixes tag for patch "mark IOCONTROL register as volatile".
> - Improve commit messages after reviews.
> - Fix coding style issues after reviews.
> - Change GPIO registration to always register the maximum number of GPIOs
> supported by the chip, but maks-out GPIOs declared as modem control lines.
> - Add patch to refactor GPIO registration.
> - Remove patch "serial: sc16is7xx: fix syntax error in comments".
> - Remove patch "add dump registers function"
>
> Changes for V5:
> - Change patch order to facilitate stable backport(s).
> - Change duplicate device addresses in DT binding examples.
> - Use GENMASK for bit masks.
> - Replace of_property_for_each_u32() with device_property_read_u32_array
> - Add "Cc: stable..." tags
>
> Changes for V6:
> - Fix compilation bug introduced by patch 3
>
> Changes for V7:
> - Minor changes and coding style fixes after review for
> patch 5 "fix regression with GPIO configuration".
>
> Changes for V8:
> - Move mctrl_mask to "struct sc16is7xx_port" to avoid compiler warning when
> CONFIG_GPIOLIB is undefined.
> - Add "struct device" member to "struct sc16is7xx_port", in order to avoid
> passing a raw "struct device" to called functions from sc16is7xx_probe().
> - Add new patch "serial: sc16is7xx: remove obsolete out_thread label"
>
> Hugo Villeneuve (10):
> serial: sc16is7xx: fix broken port 0 uart init
> serial: sc16is7xx: mark IOCONTROL register as volatile
> serial: sc16is7xx: remove obsolete out_thread label
> serial: sc16is7xx: refactor GPIO controller registration
> dt-bindings: sc16is7xx: Add property to change GPIO function
> serial: sc16is7xx: fix regression with GPIO configuration
> serial: sc16is7xx: fix bug when first setting GPIO direction
> serial: sc16is7xx: add call to get rs485 DT flags and properties
> serial: sc16is7xx: add post reset delay
> serial: sc16is7xx: improve comments about variants
>
> .../bindings/serial/nxp,sc16is7xx.txt | 46 +++++
> drivers/tty/serial/sc16is7xx.c | 177 +++++++++++++-----
> 2 files changed, 181 insertions(+), 42 deletions(-)
>
>
> base-commit: a4d7d701121981e3c3fe69ade376fe9f26324161
> --
> 2.30.2
>
Hello Hugo,

I had a chance to retest and re-review everything, including setting state at direction change.
So if the pending changes to v9 are only the conversion of dt-bindings to YAML and fixups to them, then:
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>

--
Pozdrawiam/With kind regards,
Lech Perczak

Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
Strzegomska 54,
53-611 Wroclaw
Tel: (+48) 71 75 000 16
Email: [email protected]
Website: http://www.camlingroup.com


2023-06-12 14:28:46

by Hugo Villeneuve

[permalink] [raw]
Subject: Re: [PATCH v8 00/10] serial: sc16is7xx: fix GPIO regression and rs485 improvements

On Mon, 12 Jun 2023 14:10:20 +0200
Lech Perczak <[email protected]> wrote:

> W dniu 7.06.2023 o?16:05, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Hello,
> > this patch series mainly fixes a GPIO regression and improve RS485 flags and
> > properties detection from DT.
> >
> > It now also includes various small fixes and improvements that were previously
> > sent as separate patches, but that made testing everything difficult.
> >
> > Patch 1 fixes an issue with init of first port during probing.
> >
> > Patch 2 fixes an issue when debugging IOcontrol register, but it is also
> > necessary for patch "fix regression with GPIO configuration" to work.
> >
> > Patch 3 fixes an incorrect label in sc16is7xx_probe() cleanup code.
> >
> > Patch 4 is a refactor of GPIO registration code in preparation for patch 5.
> >
> > Patches 5 and 6 fix a GPIO regression by (re)allowing to choose GPIO function
> > for GPIO pins shared with modem status lines.
> >
> > Patch 7 fixes a bug with the output value when first setting the GPIO direction.
> >
> > Patch 8 allows to read common rs485 device-tree flags and properties.
> >
> > Patch 9 introduces a delay after a reset operation to respect datasheet
> > timing recommandations.
> >
> > Patch 10 improves comments about chip variants.
> >
> > I have tested the changes on a custom board with two SC16IS752 DUART using a
> > Variscite IMX8MN NANO SOM.
> >
> > Thank you.
> >
> > Link: [v1] https://lkml.org/lkml/2023/5/17/967 <https://lkml.org/lkml/2023/5/17/967>
> > [v1] https://lkml.org/lkml/2023/5/17/777 <https://lkml.org/lkml/2023/5/17/777>
> > [v1] https://lkml.org/lkml/2023/5/17/780 <https://lkml.org/lkml/2023/5/17/780>
> > [v1] https://lkml.org/lkml/2023/5/17/785 <https://lkml.org/lkml/2023/5/17/785>
> > [v1] https://lkml.org/lkml/2023/5/17/1311 <https://lkml.org/lkml/2023/5/17/1311>
> > [v2] https://lkml.org/lkml/2023/5/18/516 <https://lkml.org/lkml/2023/5/18/516>
> > [v3] https://lkml.org/lkml/2023/5/25/7 <https://lkml.org/lkml/2023/5/25/7>
> > [v4] https://lkml.org/lkml/2023/5/29/656 <https://lkml.org/lkml/2023/5/29/656>
> > [v5] https://lkml.org/lkml/2023/6/1/1046 <https://lkml.org/lkml/2023/6/1/1046>
> > [v6] https://lkml.org/lkml/2023/6/1/1328 <https://lkml.org/lkml/2023/6/1/1328>
> > [v7] https://lkml.org/lkml/2023/6/2/861 <https://lkml.org/lkml/2023/6/2/861>
> >
> > Changes for V3:
> > - Integrated all patches into single serie to facilitate debugging and tests.
> > - Reduce number of exported GPIOs depending on new property
> > nxp,modem-control-line-ports
> > - Added additional example in DT bindings
> >
> > Changes for V4:
> > - Increase reset post delay to relax scheduler.
> > - Put comments patches at the end.
> > - Remove Fixes tag for patch "mark IOCONTROL register as volatile".
> > - Improve commit messages after reviews.
> > - Fix coding style issues after reviews.
> > - Change GPIO registration to always register the maximum number of GPIOs
> > supported by the chip, but maks-out GPIOs declared as modem control lines.
> > - Add patch to refactor GPIO registration.
> > - Remove patch "serial: sc16is7xx: fix syntax error in comments".
> > - Remove patch "add dump registers function"
> >
> > Changes for V5:
> > - Change patch order to facilitate stable backport(s).
> > - Change duplicate device addresses in DT binding examples.
> > - Use GENMASK for bit masks.
> > - Replace of_property_for_each_u32() with device_property_read_u32_array
> > - Add "Cc: stable..." tags
> >
> > Changes for V6:
> > - Fix compilation bug introduced by patch 3
> >
> > Changes for V7:
> > - Minor changes and coding style fixes after review for
> > patch 5 "fix regression with GPIO configuration".
> >
> > Changes for V8:
> > - Move mctrl_mask to "struct sc16is7xx_port" to avoid compiler warning when
> > CONFIG_GPIOLIB is undefined.
> > - Add "struct device" member to "struct sc16is7xx_port", in order to avoid
> > passing a raw "struct device" to called functions from sc16is7xx_probe().
> > - Add new patch "serial: sc16is7xx: remove obsolete out_thread label"
> >
> > Hugo Villeneuve (10):
> > serial: sc16is7xx: fix broken port 0 uart init
> > serial: sc16is7xx: mark IOCONTROL register as volatile
> > serial: sc16is7xx: remove obsolete out_thread label
> > serial: sc16is7xx: refactor GPIO controller registration
> > dt-bindings: sc16is7xx: Add property to change GPIO function
> > serial: sc16is7xx: fix regression with GPIO configuration
> > serial: sc16is7xx: fix bug when first setting GPIO direction
> > serial: sc16is7xx: add call to get rs485 DT flags and properties
> > serial: sc16is7xx: add post reset delay
> > serial: sc16is7xx: improve comments about variants
> >
> > .../bindings/serial/nxp,sc16is7xx.txt | 46 +++++
> > drivers/tty/serial/sc16is7xx.c | 177 +++++++++++++-----
> > 2 files changed, 181 insertions(+), 42 deletions(-)
> >
> >
> > base-commit: a4d7d701121981e3c3fe69ade376fe9f26324161
> > --
> > 2.30.2
> >
> Hello Hugo,
>
> I had a chance to retest and re-review everything, including setting state at direction change.
> So if the pending changes to v9 are only the conversion of dt-bindings to YAML and fixups to them, then:
> Reviewed-by: Lech Perczak <[email protected]>
> Tested-by: Lech Perczak <[email protected]>

Hi Lech,
thank you for testing this again.

By the way, conversion of dt-bindings to YAML and fixups to them will
be done in a new serie, after we finish/close this one.

If all goes well, I do not intend to publish a v9, depending on feedback
from Andy and Greg.

Hugo.


> Pozdrawiam/With kind regards,
> Lech Perczak
>
> Sr. Software Engineer
> Camlin Technologies Poland Limited Sp. z o.o.
> Strzegomska 54,
> 53-611 Wroclaw
> Tel: (+48) 71 75 000 16
> Email: [email protected]
> Website: http://www.camlingroup.com
>
>