[PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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 when debugging IOcontrol register. After testing the GPIO
regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
this patch is also necessary for having the correct IOcontrol register values.

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

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

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

Patch 5 is a refactor of GPIO registration code.

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

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

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

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"

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

 .../bindings/serial/nxp,sc16is7xx.txt         |  46 ++++++
 drivers/tty/serial/sc16is7xx.c                | 150 +++++++++++++-----
 2 files changed, 156 insertions(+), 40 deletions(-)


base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942
-- 
2.30.2

[PATCH v4 1/9] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Bit SRESET (3) is cleared when a reset operation is completed. Having
the IOCONTROL register as non-volatile will always read SRESET as 1.
Therefore mark IOCONTROL register as a volatile register.

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 abad091baeea..fbaadec557d8 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 	case SC16IS7XX_TXLVL_REG:
 	case SC16IS7XX_RXLVL_REG:
 	case SC16IS7XX_IOSTATE_REG:
+	case SC16IS7XX_IOCONTROL_REG:
 		return true;
 	default:
 		break;
-- 
2.30.2

[PATCH v4 2/9] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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 fbaadec557d8..24c9336f6ab2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1428,6 +1428,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

[PATCH v4 3/9] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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")
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
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 24c9336f6ab2..fdf18add1c49 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1443,6 +1443,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

[PATCH v4 4/9] serial: sc16is7xx: fix bug when first setting GPIO direction

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 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 fdf18add1c49..06be1daa733a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1343,9 +1343,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

[PATCH v4 5/9] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

In preparation for patch to fix GPIO regression. To facilitate review
and make code more modular.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 06be1daa733a..7a993add3f04 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1358,6 +1358,26 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 
 	return 0;
 }
+
+static int sc16is7xx_setup_gpio_chip(struct device *dev)
+{
+	struct sc16is7xx_port *s = dev_get_drvdata(dev);
+
+	if (!s->devtype->nr_gpio)
+		return 0;
+
+	s->gpio.owner		 = THIS_MODULE;
+	s->gpio.parent		 = dev;
+	s->gpio.label		 = dev_name(dev);
+	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
+	s->gpio.get		 = sc16is7xx_gpio_get;
+	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
+	s->gpio.set		 = sc16is7xx_gpio_set;
+	s->gpio.base		 = -1;
+	s->gpio.ngpio		 = s->devtype->nr_gpio;
+	s->gpio.can_sleep	 = 1;
+	return gpiochip_add_data(&s->gpio, s);
+}
 #endif
 
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
@@ -1517,22 +1537,9 @@ static int sc16is7xx_probe(struct device *dev,
 	}
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio) {
-		/* Setup GPIO cotroller */
-		s->gpio.owner		 = THIS_MODULE;
-		s->gpio.parent		 = dev;
-		s->gpio.label		 = dev_name(dev);
-		s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
-		s->gpio.get		 = sc16is7xx_gpio_get;
-		s->gpio.direction_output = sc16is7xx_gpio_direction_output;
-		s->gpio.set		 = sc16is7xx_gpio_set;
-		s->gpio.base		 = -1;
-		s->gpio.ngpio		 = devtype->nr_gpio;
-		s->gpio.can_sleep	 = 1;
-		ret = gpiochip_add_data(&s->gpio, s);
-		if (ret)
-			goto out_thread;
-	}
+	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
+	if (ret)
+		goto out_thread;
 #endif
 
 	/*
-- 
2.30.2

[PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Some variants in this series of UART controllers have GPIO pins that
are shared between GPIO and modem control lines.

The pin mux mode (GPIO or modem control lines) can be set for each
ports (channels) supported by the variant.

This adds a property to the device tree to set the GPIO pin mux to
modem control lines on selected ports if needed.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index 0fa8e3e43bf8..74dfbbf7b2cb 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -23,6 +23,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
         sc16is750: sc16is750@51 {
@@ -35,6 +38,26 @@ Example:
                 #gpio-cells = <2>;
         };
 
+	sc16is752: sc16is752@54 {
+		compatible = "nxp,sc16is752";
+		reg = <0x54>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@54 {
+		compatible = "nxp,sc16is752";
+		reg = <0x54>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};
+
 * spi as bus
 
 Required properties:
@@ -59,6 +82,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
 	sc16is750: sc16is750@0 {
@@ -70,3 +96,23 @@ Example:
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	sc16is752: sc16is752@0 {
+		compatible = "nxp,sc16is752";
+		reg = <0>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@0 {
+		compatible = "nxp,sc16is752";
+		reg = <0>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};
-- 
2.30.2

Re: [PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

Hey Hugo,

On Mon, May 29, 2023 at 10:07:08AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Some variants in this series of UART controllers have GPIO pins that
> are shared between GPIO and modem control lines.
> 
> The pin mux mode (GPIO or modem control lines) can be set for each
> ports (channels) supported by the variant.
> 
> This adds a property to the device tree to set the GPIO pin mux to
> modem control lines on selected ports if needed.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Did I not ack this in v2? I didn't notice a reason for dropping it
in the cover etc. Was it intentionally dropped, or missed?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Mon, 29 May 2023 15:16:47 +0100
Conor Dooley <conor.dooley@microchip.com> wrote:

> Hey Hugo,
> 
> On Mon, May 29, 2023 at 10:07:08AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Some variants in this series of UART controllers have GPIO pins that
> > are shared between GPIO and modem control lines.
> > 
> > The pin mux mode (GPIO or modem control lines) can be set for each
> > ports (channels) supported by the variant.
> > 
> > This adds a property to the device tree to set the GPIO pin mux to
> > modem control lines on selected ports if needed.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Did I not ack this in v2? I didn't notice a reason for dropping it
> in the cover etc. Was it intentionally dropped, or missed?
> 
> Cheers,
> Conor.

Hi Conor,
In v3, I slighly modified the example, and that is why I didn't copy your ack.

Hugo.

Re: [PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Mon, May 29, 2023 at 10:26:01AM -0400, Hugo Villeneuve wrote:
> On Mon, 29 May 2023 15:16:47 +0100
> Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> > Hey Hugo,
> > 
> > On Mon, May 29, 2023 at 10:07:08AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > Some variants in this series of UART controllers have GPIO pins that
> > > are shared between GPIO and modem control lines.
> > > 
> > > The pin mux mode (GPIO or modem control lines) can be set for each
> > > ports (channels) supported by the variant.
> > > 
> > > This adds a property to the device tree to set the GPIO pin mux to
> > > modem control lines on selected ports if needed.
> > > 
> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Did I not ack this in v2? I didn't notice a reason for dropping it
> > in the cover etc. Was it intentionally dropped, or missed?

> In v3, I slighly modified the example, and that is why I didn't copy your ack.

Ah, I would say that for "slight modifications" when you have an ack,
you could keep it - but everyone is different and dropping tags is
likely to piss people off less than keeping them, so I understand
sticking on the safe side.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

On Mon, May 29, 2023 at 10:07:08AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Some variants in this series of UART controllers have GPIO pins that
> are shared between GPIO and modem control lines.
> 
> The pin mux mode (GPIO or modem control lines) can be set for each
> ports (channels) supported by the variant.
> 
> This adds a property to the device tree to set the GPIO pin mux to
> modem control lines on selected ports if needed.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index 0fa8e3e43bf8..74dfbbf7b2cb 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -23,6 +23,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>  		   should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +				should have shared GPIO lines configured as
> +				modem control lines.
>  
>  Example:
>          sc16is750: sc16is750@51 {
> @@ -35,6 +38,26 @@ Example:
>                  #gpio-cells = <2>;
>          };
>  
> +	sc16is752: sc16is752@54 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0x54>;
> +		clocks = <&clk20m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> +		gpio-controller; /* Port 0 as GPIOs */
> +		#gpio-cells = <2>;
> +	};
> +
> +	sc16is752: sc16is752@54 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0x54>;

If this were not a txt binding, dt_binding_check would likely complain
that you have two nodes with the same node address & 3 below.
If you end up re-submitting, could you change that please?
Otherwise,
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 6/9] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Mon, 29 May 2023 19:19:27 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Mon, May 29, 2023 at 10:07:08AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Some variants in this series of UART controllers have GPIO pins that
> > are shared between GPIO and modem control lines.
> > 
> > The pin mux mode (GPIO or modem control lines) can be set for each
> > ports (channels) supported by the variant.
> > 
> > This adds a property to the device tree to set the GPIO pin mux to
> > modem control lines on selected ports if needed.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > index 0fa8e3e43bf8..74dfbbf7b2cb 100644
> > --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > @@ -23,6 +23,9 @@ Optional properties:
> >      1 = active low.
> >  - irda-mode-ports: An array that lists the indices of the port that
> >  		   should operate in IrDA mode.
> > +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> > +				should have shared GPIO lines configured as
> > +				modem control lines.
> >  
> >  Example:
> >          sc16is750: sc16is750@51 {
> > @@ -35,6 +38,26 @@ Example:
> >                  #gpio-cells = <2>;
> >          };
> >  
> > +	sc16is752: sc16is752@54 {
> > +		compatible = "nxp,sc16is752";
> > +		reg = <0x54>;
> > +		clocks = <&clk20m>;
> > +		interrupt-parent = <&gpio3>;
> > +		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> > +		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> > +		gpio-controller; /* Port 0 as GPIOs */
> > +		#gpio-cells = <2>;
> > +	};
> > +
> > +	sc16is752: sc16is752@54 {
> > +		compatible = "nxp,sc16is752";
> > +		reg = <0x54>;
> 
> If this were not a txt binding, dt_binding_check would likely complain
> that you have two nodes with the same node address & 3 below.
> If you end up re-submitting, could you change that please?
> Otherwise,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Hi Conor,
for the I2C section, I will use addresse 51, 52 and 54.

For the SPI section, I will use addresse 0, 1 and 2.

I will resubmit if there is a v5.

Thank you, Hugo.

[PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 82 +++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7a993add3f04..34739b31b44b 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)
@@ -336,6 +337,7 @@ struct sc16is7xx_port {
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
+	unsigned long			gpio_valid_mask;
 #endif
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
@@ -447,35 +449,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)
@@ -1359,16 +1356,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
-static int sc16is7xx_setup_gpio_chip(struct device *dev)
+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 device *dev, u8 mctrl_mask)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 
 	if (!s->devtype->nr_gpio)
 		return 0;
 
+	switch (mctrl_mask) {
+	case 0:
+		s->gpio_valid_mask = 0xFF;
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
+		s->gpio_valid_mask = 0x0F;
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
+		s->gpio_valid_mask = 0xF0;
+		break;
+	default:
+		break;
+	}
+
+	if (!s->gpio_valid_mask)
+		return 0;
+
 	s->gpio.owner		 = THIS_MODULE;
 	s->gpio.parent		 = dev;
 	s->gpio.label		 = dev_name(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;
@@ -1392,6 +1418,7 @@ static int sc16is7xx_probe(struct device *dev,
 {
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	unsigned int val;
+	u8 mctrl_mask = 0;
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
@@ -1493,12 +1520,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);
@@ -1534,6 +1555,27 @@ static int sc16is7xx_probe(struct device *dev,
 					 prop, p, u)
 			if (u < devtype->nr_uart)
 				s->p[u].irda_mode = true;
+
+		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
+					 prop, p, u) {
+			if (u >= devtype->nr_uart)
+				continue;
+
+			/* Use GPIO lines as modem control lines */
+			if (u == 0)
+				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+			else if (u == 1)
+				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+		}
+
+		if (mctrl_mask) {
+			regmap_update_bits(
+				s->regmap,
+				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+				SC16IS7XX_IOCONTROL_MODEM_B_BIT,
+				mctrl_mask);
+		}
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -1562,7 +1604,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);
 
 out_thread:
@@ -1588,7 +1630,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

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7390 bytes --]

On Mon, 29 May 2023, Hugo Villeneuve wrote:

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

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.


> ---
>  drivers/tty/serial/sc16is7xx.c | 82 +++++++++++++++++++++++++---------
>  1 file changed, 62 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7a993add3f04..34739b31b44b 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)
> @@ -336,6 +337,7 @@ struct sc16is7xx_port {
>  	struct clk			*clk;
>  #ifdef CONFIG_GPIOLIB
>  	struct gpio_chip		gpio;
> +	unsigned long			gpio_valid_mask;
>  #endif
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
> @@ -447,35 +449,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)
> @@ -1359,16 +1356,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
>  	return 0;
>  }
>  
> -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> +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 device *dev, u8 mctrl_mask)
>  {
>  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
>  
>  	if (!s->devtype->nr_gpio)
>  		return 0;
>  
> +	switch (mctrl_mask) {
> +	case 0:
> +		s->gpio_valid_mask = 0xFF;
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> +		s->gpio_valid_mask = 0x0F;
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> +		s->gpio_valid_mask = 0xF0;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!s->gpio_valid_mask)
> +		return 0;
> +
>  	s->gpio.owner		 = THIS_MODULE;
>  	s->gpio.parent		 = dev;
>  	s->gpio.label		 = dev_name(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;
> @@ -1392,6 +1418,7 @@ static int sc16is7xx_probe(struct device *dev,
>  {
>  	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
>  	unsigned int val;
> +	u8 mctrl_mask = 0;
>  	u32 uartclk = 0;
>  	int i, ret;
>  	struct sc16is7xx_port *s;
> @@ -1493,12 +1520,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);
> @@ -1534,6 +1555,27 @@ static int sc16is7xx_probe(struct device *dev,
>  					 prop, p, u)
>  			if (u < devtype->nr_uart)
>  				s->p[u].irda_mode = true;
> +
> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u) {
> +			if (u >= devtype->nr_uart)
> +				continue;
> +
> +			/* Use GPIO lines as modem control lines */
> +			if (u == 0)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +			else if (u == 1)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +		}
> +
> +		if (mctrl_mask) {
> +			regmap_update_bits(
> +				s->regmap,
> +				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> +				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> +				SC16IS7XX_IOCONTROL_MODEM_B_BIT,
> +				mctrl_mask);
> +		}
>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> @@ -1562,7 +1604,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);
>  
>  out_thread:
> @@ -1588,7 +1630,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
>  
> 

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[andy.shevchenko]

Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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")

Don't forget to refer to the dependency patches form this series.
(I forgot how it should be done, IIRC the documentation about stable kernel
patches can shed a light on this.)


...

> +	switch (mctrl_mask) {
> +	case 0:
> +		s->gpio_valid_mask = 0xFF;

GENMASK()

> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> +		s->gpio_valid_mask = 0x0F;

GENMASK()

> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> +		s->gpio_valid_mask = 0xF0;

GENMASK()

> +		break;
> +	default:
> +		break;
> +	}

...

> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u) {
> +			if (u >= devtype->nr_uart)
> +				continue;
> +
> +			/* Use GPIO lines as modem control lines */
> +			if (u == 0)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +			else if (u == 1)
> +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +		}

Can we use device properties, please?

If you think about backporting to the earlier kernels (w/o properties in use in
this driver), perhaps an additional followup for that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 30 May 2023 01:38:17 +0300
andy.shevchenko@gmail.com wrote:

> Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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")
> 
> Don't forget to refer to the dependency patches form this series.
> (I forgot how it should be done, IIRC the documentation about stable kernel
> patches can shed a light on this.)

Hi,
I will look into it.


> ...
> 
> > +	switch (mctrl_mask) {
> > +	case 0:
> > +		s->gpio_valid_mask = 0xFF;
> 
> GENMASK()
> 
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> > +		s->gpio_valid_mask = 0x0F;
> 
> GENMASK()
> 
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> > +		s->gpio_valid_mask = 0xF0;
> 
> GENMASK()

Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.


> > +		break;
> > +	default:
> > +		break;
> > +	}
> 
> ...
> 
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u) {
> > +			if (u >= devtype->nr_uart)
> > +				continue;
> > +
> > +			/* Use GPIO lines as modem control lines */
> > +			if (u == 0)
> > +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +			else if (u == 1)
> > +				mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +		}
> 
> Can we use device properties, please?

I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?

> If you think about backporting to the earlier kernels (w/o properties in use in
> this driver), perhaps an additional followup for that?

I am not sure what you mean by this?

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 30 May 2023 01:38:17 +0300
> andy.shevchenko@gmail.com wrote:
> > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:

...

> > GENMASK()
>
> Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.

GENMASK() was introduced to increase code robustness:
1) to make sure the bits mentioned are correct
2) to check the bit boundary.

...

> > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > +                                    prop, p, u) {
> > > +                   if (u >= devtype->nr_uart)
> > > +                           continue;
> > > +
> > > +                   /* Use GPIO lines as modem control lines */
> > > +                   if (u == 0)
> > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > +                   else if (u == 1)
> > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > +           }
> >
> > Can we use device properties, please?
>
> I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?

Yes, thank you!

> > If you think about backporting to the earlier kernels (w/o properties in use in
> > this driver), perhaps an additional followup for that?
>
> I am not sure what you mean by this?

If the device property API was not yet available for this fix being
backported to the old enough kernel we have to use old OF stuff. In
that case the device property conversion needs to be done in a
separate change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Wed, 31 May 2023 00:56:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 30 May 2023 01:38:17 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> 
> ...
> 
> > > GENMASK()
> >
> > Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.
> 
> GENMASK() was introduced to increase code robustness:
> 1) to make sure the bits mentioned are correct
> 2) to check the bit boundary.
> 
> ...
> 
> > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > +                                    prop, p, u) {
> > > > +                   if (u >= devtype->nr_uart)
> > > > +                           continue;
> > > > +
> > > > +                   /* Use GPIO lines as modem control lines */
> > > > +                   if (u == 0)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > +                   else if (u == 1)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > +           }
> > >
> > > Can we use device properties, please?
> >
> > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> 
> Yes, thank you!
> 
> > > If you think about backporting to the earlier kernels (w/o properties in use in
> > > this driver), perhaps an additional followup for that?
> >
> > I am not sure what you mean by this?
> 
> If the device property API was not yet available for this fix being
> backported to the old enough kernel we have to use old OF stuff. In
> that case the device property conversion needs to be done in a
> separate change.

Hi,.
ok, now I see.

Thank you,
Hugo.

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Wed, 31 May 2023 00:56:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 30 May 2023 01:38:17 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:
> 
> ...
> 
> > > GENMASK()
> >
> > Ok done, altough even if in general I like the bit manipulation macros because they make the code easier to read/understand, I find it less obvious by using GENMASK in this case IMMO.
> 
> GENMASK() was introduced to increase code robustness:
> 1) to make sure the bits mentioned are correct
> 2) to check the bit boundary.
> 
> ...
> 
> > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > +                                    prop, p, u) {
> > > > +                   if (u >= devtype->nr_uart)
> > > > +                           continue;
> > > > +
> > > > +                   /* Use GPIO lines as modem control lines */
> > > > +                   if (u == 0)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > +                   else if (u == 1)
> > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > +           }
> > >
> > > Can we use device properties, please?
> >
> > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> 
> Yes, thank you!

Hi Andy,
now that I am using the device property API, I think I no longer have the need to test for "if (dev->of_node)" before reading the new property "nxp,modem-control-line-ports"?

If that is the case, I will leave the test "if (dev->of_node)" only for the "irda-mode-ports" property.

The pseudo code woulk look like this:

	if (dev->of_node) {
		struct property *prop;
		const __be32 *p;
		u32 u;

		of_property_for_each_u32(dev->of_node, "irda-mode-ports",
					 prop, p, u)
			if (u < devtype->nr_uart)
				s->p[u].irda_mode = true;
	}

        /* Read "nxp,modem-control-line-ports" using device property API. */
	sc16is7xx_setup_mctrl_ports(dev);

Thank you,
Hugo.


> > > If you think about backporting to the earlier kernels (w/o properties in use in
> > > this driver), perhaps an additional followup for that?
> >
> > I am not sure what you mean by this?
> 
> If the device property API was not yet available for this fix being
> backported to the old enough kernel we have to use old OF stuff. In
> that case the device property conversion needs to be done in a
> separate change.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Thu, Jun 1, 2023 at 2:57 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Wed, 31 May 2023 00:56:57 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, May 30, 2023 at 6:36 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 30 May 2023 01:38:17 +0300
> > > andy.shevchenko@gmail.com wrote:
> > > > Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve kirjoitti:

...

> > > > > +           of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > > > +                                    prop, p, u) {
> > > > > +                   if (u >= devtype->nr_uart)
> > > > > +                           continue;
> > > > > +
> > > > > +                   /* Use GPIO lines as modem control lines */
> > > > > +                   if (u == 0)
> > > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > > > +                   else if (u == 1)
> > > > > +                           mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > > > +           }
> > > >
> > > > Can we use device properties, please?
> > >
> > > I have converted this section to use device_property_count_u32() and device_property_read_u32_array(). Is that Ok?
> >
> > Yes, thank you!
>
> Hi Andy,
> now that I am using the device property API, I think I no longer have the need to test for "if (dev->of_node)" before reading the new property "nxp,modem-control-line-ports"?
>
> If that is the case, I will leave the test "if (dev->of_node)" only for the "irda-mode-ports" property.
>
> The pseudo code woulk look like this:
>
>         if (dev->of_node) {
>                 struct property *prop;
>                 const __be32 *p;
>                 u32 u;
>
>                 of_property_for_each_u32(dev->of_node, "irda-mode-ports",
>                                          prop, p, u)
>                         if (u < devtype->nr_uart)
>                                 s->p[u].irda_mode = true;
>         }
>
>         /* Read "nxp,modem-control-line-ports" using device property API. */
>         sc16is7xx_setup_mctrl_ports(dev);

Looks good to me, thank you for following the advice!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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")

So you are marking this as a "bugfix" and yet, it is at the end of a
much larger series of patches.  Does this fix require all of them?  If
so, it's not really relevant for stable kernels, right?  Or is it?

I'm confused, what should I, as a maintainer, do here?  Take just this
one fix for 6.4-final, and the rest for 6.5-rc1?  And add a proper cc:
stable@ tag?  Or queue them all up for 6.4-final?  Or all for 6.5-rc1?
Or something else?

What would you want to see if you were in my position here to help make
your life easier?

thanks,

greg k-h

Re: [PATCH v4 7/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 30 May 2023 11:25:53 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, May 29, 2023 at 10:07:09AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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")
> 
> So you are marking this as a "bugfix" and yet, it is at the end of a
> much larger series of patches.  Does this fix require all of them?  If
> so, it's not really relevant for stable kernels, right?  Or is it?

Like I said to Andy, I will re-order the patches so that "bugfix" patches are first. See new order below.

> I'm confused, what should I, as a maintainer, do here?  Take just this
> one fix for 6.4-final, and the rest for 6.5-rc1?  And add a proper cc:
> stable@ tag?  Or queue them all up for 6.4-final?  Or all for 6.5-rc1?
> Or something else?
> 
> What would you want to see if you were in my position here to help make
> your life easier?

From what I understand from https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst,
here is the new proposed patches order as well as what I plan to do in the commit message:

2f0f23e598df serial: sc16is7xx: fix broken port 0 uart init
  I will add tag "Cc: <stable@vger.kernel.org>"
  This patch is a prerequiste of "fix regression with GPIO configuration".

f292951c521e serial: sc16is7xx: mark IOCONTROL register as volatile
  I will add tag "Cc: <stable@vger.kernel.org>"
  This patch is a prerequiste of "fix regression with GPIO configuration".
  This patch has no "Fixes:" tag because it doesn't fix a previous bug, but Lech Perczak reported that it was required for 
  patch "fix regression with GPIO configuration" to work.

78930d607121 serial: sc16is7xx: refactor GPIO controller registration
  This patch is a prerequiste of "fix regression with GPIO configuration".
  It was done separately to ease the review process, but from a stable kernel backport, maybe it would be best to integrate it directly into "fix regression with GPIO configuration"?
  If not, should I add tag "Cc: <stable@vger.kernel.org>"?

f7ba105873d7 dt-bindings: sc16is7xx: Add property to change GPIO function
  This patch is a prerequiste of "fix regression with GPIO configuration".
  I will add tag "Cc: <stable@vger.kernel.org>"
  Should I add a tag "Fixes: " like I did in patch "fix regression with GPIO configuration"?

f2238e8f69b0 serial: sc16is7xx: fix regression with GPIO configuration
  I will add tags:
    Cc: <stable@vger.kernel.org> 2f0f23e5 serial: sc16is7xx: fix broken port 0 uart init
    Cc: <stable@vger.kernel.org> f292951c serial: sc16is7xx: mark IOCONTROL register as volatile
    Cc: <stable@vger.kernel.org> 78930d60 serial: sc16is7xx: refactor GPIO controller registration
    Cc: <stable@vger.kernel.org> f7ba1058 dt-bindings: sc16is7xx: Add property to change GPIO function
    Cc: <stable@vger.kernel.org>

2d98ab070b70 serial: sc16is7xx: fix bug when first setting GPIO direction
  This is a standalone bugfix
  I will add tag "Cc: <stable@vger.kernel.org>"

658e39d9073e serial: sc16is7xx: add call to get rs485 DT flags and properties
  Enhancement

588aac544e00 serial: sc16is7xx: add post reset delay
  Enhancement

5bb1b45bca81 serial: sc16is7xx: improve comments about variants
  Comments enhancements

Please tell me if it makes sense and if some tags are wrong/missing.

Hugo.

[PATCH v4 8/9] serial: sc16is7xx: add call to get rs485 DT flags and properties

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add call to uart_get_rs485_mode() to probe for RS485 flags and
properties from device tree.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 34739b31b44b..b305142e9e9c 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1513,6 +1513,10 @@ static int sc16is7xx_probe(struct device *dev,
 			goto out_ports;
 		}
 
+		ret = uart_get_rs485_mode(&s->p[i].port);
+		if (ret)
+			goto out_ports;
+
 		/* Disable all interrupts */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
 		/* Disable TX/RX */
-- 
2.30.2

[PATCH v4 9/9] serial: sc16is7xx: improve comments about variants

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Replace 740/750/760 with generic terms like 74x/75x/76x to account for
variants like 741, 752 and 762.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index b305142e9e9c..5e86d77e1229 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -223,7 +223,7 @@
  * trigger levels. Trigger levels from 4 characters to 60 characters are
  * available with a granularity of four.
  *
- * When the trigger level setting in TLR is zero, the SC16IS740/750/760 uses the
+ * When the trigger level setting in TLR is zero, the SC16IS74x/75x/76x uses the
  * trigger level setting defined in FCR. If TLR has non-zero trigger level value
  * the trigger level defined in FCR is discarded. This applies to both transmit
  * FIFO and receive FIFO trigger level setting.
@@ -234,7 +234,7 @@
 #define SC16IS7XX_TLR_TX_TRIGGER(words)	((((words) / 4) & 0x0f) << 0)
 #define SC16IS7XX_TLR_RX_TRIGGER(words)	((((words) / 4) & 0x0f) << 4)
 
-/* IOControl register bits (Only 750/760) */
+/* IOControl register bits (Only 75x/76x) */
 #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
 #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 */
@@ -249,9 +249,9 @@
 #define SC16IS7XX_EFCR_RTS_INVERT_BIT	(1 << 5) /* RTS output inversion */
 #define SC16IS7XX_EFCR_IRDA_MODE_BIT	(1 << 7) /* IrDA mode
 						  * 0 = rate upto 115.2 kbit/s
-						  *   - Only 750/760
+						  *   - Only 75x/76x
 						  * 1 = rate upto 1.152 Mbit/s
-						  *   - Only 760
+						  *   - Only 76x
 						  */
 
 /* EFR register bits */
-- 
2.30.2

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[andy.shevchenko]

Mon, May 29, 2023 at 10:07:02AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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 when debugging IOcontrol register. After testing the GPIO
> regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> this patch is also necessary for having the correct IOcontrol register values.
> 
> Patch 2 introduces a delay after a reset operation to respect datasheet
> timing recommandations.

These two patches are w/o Fixes tag, they should be moved in the series further
as I explained before.

> Patch 3 fixes an issue with init of first port during probing.
> 
> Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> 
> Patch 5 is a refactor of GPIO registration code.
> 
> Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> for GPIO pins shared with modem status lines.
> 
> Patch 8 allows to read common rs485 device-tree flags and properties.
> 
> Patch 9 improves comments about chip variants.
> 
> I have tested the changes on a custom board with two SC16IS752 DUART using a
> Variscite IMX8MN NANO SOM.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Tue, 30 May 2023 01:31:28 +0300
andy.shevchenko@gmail.com wrote:

> Mon, May 29, 2023 at 10:07:02AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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 when debugging IOcontrol register. After testing the GPIO
> > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > this patch is also necessary for having the correct IOcontrol register values.
> > 
> > Patch 2 introduces a delay after a reset operation to respect datasheet
> > timing recommandations.
> 
> These two patches are w/o Fixes tag, they should be moved in the series further
> as I explained before.

Your explanation was not clear.

Anyway, I moved them in position 7 and 8.

 
> > Patch 3 fixes an issue with init of first port during probing.
> > 
> > Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> > 
> > Patch 5 is a refactor of GPIO registration code.
> > 
> > Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> > for GPIO pins shared with modem status lines.
> > 
> > Patch 8 allows to read common rs485 device-tree flags and properties.
> > 
> > Patch 9 improves comments about chip variants.
> > 
> > I have tested the changes on a custom board with two SC16IS752 DUART using a
> > Variscite IMX8MN NANO SOM.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[andy.shevchenko]

Mon, May 29, 2023 at 10:07:08PM -0400, Hugo Villeneuve kirjoitti:
> On Tue, 30 May 2023 01:31:28 +0300
> andy.shevchenko@gmail.com wrote:
> > Mon, May 29, 2023 at 10:07:02AM -0400, Hugo Villeneuve kirjoitti:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > 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 when debugging IOcontrol register. After testing the GPIO
> > > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > > this patch is also necessary for having the correct IOcontrol register values.
> > > 
> > > Patch 2 introduces a delay after a reset operation to respect datasheet
> > > timing recommandations.
> > 
> > These two patches are w/o Fixes tag, they should be moved in the series further
> > as I explained before.
> 
> Your explanation was not clear.

Sorry if it feels like this. The documentation should have more clarity
on the matter.

> Anyway, I moved them in position 7 and 8.

Thank you, but take also what Greg KH replied to you into consideration.
He is the maintainer and seems other patches needs some additional work
in the scope of Fixes / backport (see stable kernel patches flow in the
kernel documentation, which I also mentioned earlier).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Tue, 30 May 2023 14:15:39 +0300
andy.shevchenko@gmail.com wrote:

> Mon, May 29, 2023 at 10:07:08PM -0400, Hugo Villeneuve kirjoitti:
> > On Tue, 30 May 2023 01:31:28 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Mon, May 29, 2023 at 10:07:02AM -0400, Hugo Villeneuve kirjoitti:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > 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 when debugging IOcontrol register. After testing the GPIO
> > > > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > > > this patch is also necessary for having the correct IOcontrol register values.
> > > > 
> > > > Patch 2 introduces a delay after a reset operation to respect datasheet
> > > > timing recommandations.
> > > 
> > > These two patches are w/o Fixes tag, they should be moved in the series further
> > > as I explained before.
> > 
> > Your explanation was not clear.
> 
> Sorry if it feels like this. The documentation should have more clarity
> on the matter.
> 
> > Anyway, I moved them in position 7 and 8.
> 
> Thank you, but take also what Greg KH replied to you into consideration.
> He is the maintainer and seems other patches needs some additional work
> in the scope of Fixes / backport (see stable kernel patches flow in the
> kernel documentation, which I also mentioned earlier).

Hi,
will do.

Thank you,.
Hugo.

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[andy.shevchenko]

Mon, May 29, 2023 at 10:07:02AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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 when debugging IOcontrol register. After testing the GPIO
> regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> this patch is also necessary for having the correct IOcontrol register values.
> 
> Patch 2 introduces a delay after a reset operation to respect datasheet
> timing recommandations.
> 
> Patch 3 fixes an issue with init of first port during probing.
> 
> Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> 
> Patch 5 is a refactor of GPIO registration code.
> 
> Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> for GPIO pins shared with modem status lines.
> 
> Patch 8 allows to read common rs485 device-tree flags and properties.
> 
> Patch 9 improves comments about chip variants.
> 
> I have tested the changes on a custom board with two SC16IS752 DUART using a
> Variscite IMX8MN NANO SOM.

In general looks good to me, just some minor things and patch ordering issues.
Thank you for doing this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Lech Perczak]

W dniu 29.05.2023 o 16:07, Hugo Villeneuve pisze:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> 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 when debugging IOcontrol register. After testing the GPIO
> regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> this patch is also necessary for having the correct IOcontrol register values.
>
> Patch 2 introduces a delay after a reset operation to respect datasheet
> timing recommandations.
>
> Patch 3 fixes an issue with init of first port during probing.
>
> Patch 4 fixes a bug with the output value when first setting the GPIO direction.
>
> Patch 5 is a refactor of GPIO registration code.
>
> Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> for GPIO pins shared with modem status lines.
>
> Patch 8 allows to read common rs485 device-tree flags and properties.
>
> Patch 9 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>
>
> 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"
>
> Hugo Villeneuve (9):
> serial: sc16is7xx: mark IOCONTROL register as volatile
> serial: sc16is7xx: add post reset delay
> serial: sc16is7xx: fix broken port 0 uart init
> serial: sc16is7xx: fix bug when first setting GPIO direction
> serial: sc16is7xx: refactor GPIO controller registration
> dt-bindings: sc16is7xx: Add property to change GPIO function
> serial: sc16is7xx: fix regression with GPIO configuration
> serial: sc16is7xx: add call to get rs485 DT flags and properties
> serial: sc16is7xx: improve comments about variants
>
> .../bindings/serial/nxp,sc16is7xx.txt | 46 ++++++
> drivers/tty/serial/sc16is7xx.c | 150 +++++++++++++-----
> 2 files changed, 156 insertions(+), 40 deletions(-)
>
>
> base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942

It would be a lot of sending, to do that for every patch separately, so for whole series:
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>

And where applicable - for code patches:
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>

I tested whole series at the same time.
I did my tests on an i.MX6 board with SC16IS760 over SPI, which differs a tiny bit from SC16IS752,
and everything works as it should.
Thank you for fixing this!

> -- 
> 2.30.2
>
-- 
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:   lech.perczak@camlingroup.com
Website: http://www.camlingroup.com

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Tue, 30 May 2023 11:30:07 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:

> W dniu 29.05.2023 o 16:07, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > 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 when debugging IOcontrol register. After testing the GPIO
> > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > this patch is also necessary for having the correct IOcontrol register values.
> >
> > Patch 2 introduces a delay after a reset operation to respect datasheet
> > timing recommandations.
> >
> > Patch 3 fixes an issue with init of first port during probing.
> >
> > Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> >
> > Patch 5 is a refactor of GPIO registration code.
> >
> > Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> > for GPIO pins shared with modem status lines.
> >
> > Patch 8 allows to read common rs485 device-tree flags and properties.
> >
> > Patch 9 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>
> >
> > 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"
> >
> > Hugo Villeneuve (9):
> > serial: sc16is7xx: mark IOCONTROL register as volatile
> > serial: sc16is7xx: add post reset delay
> > serial: sc16is7xx: fix broken port 0 uart init
> > serial: sc16is7xx: fix bug when first setting GPIO direction
> > serial: sc16is7xx: refactor GPIO controller registration
> > dt-bindings: sc16is7xx: Add property to change GPIO function
> > serial: sc16is7xx: fix regression with GPIO configuration
> > serial: sc16is7xx: add call to get rs485 DT flags and properties
> > serial: sc16is7xx: improve comments about variants
> >
> > .../bindings/serial/nxp,sc16is7xx.txt | 46 ++++++
> > drivers/tty/serial/sc16is7xx.c | 150 +++++++++++++-----
> > 2 files changed, 156 insertions(+), 40 deletions(-)
> >
> >
> > base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942
> 
> It would be a lot of sending, to do that for every patch separately, so for whole series:
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> 
> And where applicable - for code patches:
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> 
> I tested whole series at the same time.
> I did my tests on an i.MX6 board with SC16IS760 over SPI, which differs a tiny bit from SC16IS752,
> and everything works as it should.
> Thank you for fixing this!

Hi Lech,
thank for your feedback.

You mentioned before that without the patch "mark IOCONTROL register as volatile", things were not working properly for you. Could you retest by removing this patch and see if things are still working?

Thank you, Hugo.

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Lech Perczak]

W dniu 30.05.2023 o 15:08, Hugo Villeneuve pisze:
> On Tue, 30 May 2023 11:30:07 +0200
> Lech Perczak <lech.perczak@camlingroup.com> wrote:
>
> > W dniu 29.05.2023 o 16:07, Hugo Villeneuve pisze:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > >
> > > 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 when debugging IOcontrol register. After testing the GPIO
> > > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > > this patch is also necessary for having the correct IOcontrol register values.
> > >
> > > Patch 2 introduces a delay after a reset operation to respect datasheet
> > > timing recommandations.
> > >
> > > Patch 3 fixes an issue with init of first port during probing.
> > >
> > > Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> > >
> > > Patch 5 is a refactor of GPIO registration code.
> > >
> > > Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> > > for GPIO pins shared with modem status lines.
> > >
> > > Patch 8 allows to read common rs485 device-tree flags and properties.
> > >
> > > Patch 9 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> <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> <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> <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> <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> <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> <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> <https://lkml.org/lkml/2023/5/25/7 <https://lkml.org/lkml/2023/5/25/7>>
> > >
> > > 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"
> > >
> > > Hugo Villeneuve (9):
> > > serial: sc16is7xx: mark IOCONTROL register as volatile
> > > serial: sc16is7xx: add post reset delay
> > > serial: sc16is7xx: fix broken port 0 uart init
> > > serial: sc16is7xx: fix bug when first setting GPIO direction
> > > serial: sc16is7xx: refactor GPIO controller registration
> > > dt-bindings: sc16is7xx: Add property to change GPIO function
> > > serial: sc16is7xx: fix regression with GPIO configuration
> > > serial: sc16is7xx: add call to get rs485 DT flags and properties
> > > serial: sc16is7xx: improve comments about variants
> > >
> > > .../bindings/serial/nxp,sc16is7xx.txt | 46 ++++++
> > > drivers/tty/serial/sc16is7xx.c | 150 +++++++++++++-----
> > > 2 files changed, 156 insertions(+), 40 deletions(-)
> > >
> > >
> > > base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942
> >
> > It would be a lot of sending, to do that for every patch separately, so for whole series:
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> >
> > And where applicable - for code patches:
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> >
> > I tested whole series at the same time.
> > I did my tests on an i.MX6 board with SC16IS760 over SPI, which differs a tiny bit from SC16IS752,
> > and everything works as it should.
> > Thank you for fixing this!
>
> Hi Lech,
> thank for your feedback.
>
> You mentioned before that without the patch "mark IOCONTROL register as volatile", things were not working properly for you. Could you retest by removing this patch and see if things are still working?
>
> Thank you, Hugo.

Hello Hugo,

Just checked - this patch is required, reverting it causes things to fail, so this patch should be marked as a pre-requisite for the actual fix and included in backports.
Perhaps using direct write to this register made it work, but it was likely by accident.

-- 
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:   lech.perczak@camlingroup.com
Website: http://www.camlingroup.com

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Wed, 31 May 2023 12:43:48 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:

> W dniu 30.05.2023 o 15:08, Hugo Villeneuve pisze:
> > On Tue, 30 May 2023 11:30:07 +0200
> > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> >
> > > W dniu 29.05.2023 o 16:07, Hugo Villeneuve pisze:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > >
> > > > 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 when debugging IOcontrol register. After testing the GPIO
> > > > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > > > this patch is also necessary for having the correct IOcontrol register values.
> > > >
> > > > Patch 2 introduces a delay after a reset operation to respect datasheet
> > > > timing recommandations.
> > > >
> > > > Patch 3 fixes an issue with init of first port during probing.
> > > >
> > > > Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> > > >
> > > > Patch 5 is a refactor of GPIO registration code.
> > > >
> > > > Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> > > > for GPIO pins shared with modem status lines.
> > > >
> > > > Patch 8 allows to read common rs485 device-tree flags and properties.
> > > >
> > > > Patch 9 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> <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> <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> <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> <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> <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> <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> <https://lkml.org/lkml/2023/5/25/7 <https://lkml.org/lkml/2023/5/25/7>>
> > > >
> > > > 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"
> > > >
> > > > Hugo Villeneuve (9):
> > > > serial: sc16is7xx: mark IOCONTROL register as volatile
> > > > serial: sc16is7xx: add post reset delay
> > > > serial: sc16is7xx: fix broken port 0 uart init
> > > > serial: sc16is7xx: fix bug when first setting GPIO direction
> > > > serial: sc16is7xx: refactor GPIO controller registration
> > > > dt-bindings: sc16is7xx: Add property to change GPIO function
> > > > serial: sc16is7xx: fix regression with GPIO configuration
> > > > serial: sc16is7xx: add call to get rs485 DT flags and properties
> > > > serial: sc16is7xx: improve comments about variants
> > > >
> > > > .../bindings/serial/nxp,sc16is7xx.txt | 46 ++++++
> > > > drivers/tty/serial/sc16is7xx.c | 150 +++++++++++++-----
> > > > 2 files changed, 156 insertions(+), 40 deletions(-)
> > > >
> > > >
> > > > base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942
> > >
> > > It would be a lot of sending, to do that for every patch separately, so for whole series:
> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > >
> > > And where applicable - for code patches:
> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > >
> > > I tested whole series at the same time.
> > > I did my tests on an i.MX6 board with SC16IS760 over SPI, which differs a tiny bit from SC16IS752,
> > > and everything works as it should.
> > > Thank you for fixing this!
> >
> > Hi Lech,
> > thank for your feedback.
> >
> > You mentioned before that without the patch "mark IOCONTROL register as volatile", things were not working properly for you. Could you retest by removing this patch and see if things are still working?
> >
> > Thank you, Hugo.
> 
> Hello Hugo,
> 
> Just checked - this patch is required, reverting it causes things to fail, so this patch should be marked as a pre-requisite for the actual fix and included in backports.
> Perhaps using direct write to this register made it work, but it was likely by accident.

Hi Lech,
thank you for the test, I will mark it as such in upcoming series V5.

Hugo.

Re: [PATCH v4 0/9] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Wed, 31 May 2023 09:56:08 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Wed, 31 May 2023 12:43:48 +0200
> Lech Perczak <lech.perczak@camlingroup.com> wrote:
> 
> > W dniu 30.05.2023 o 15:08, Hugo Villeneuve pisze:
> > > On Tue, 30 May 2023 11:30:07 +0200
> > > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> > >
> > > > W dniu 29.05.2023 o 16:07, Hugo Villeneuve pisze:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > >
> > > > > 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 when debugging IOcontrol register. After testing the GPIO
> > > > > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > > > > this patch is also necessary for having the correct IOcontrol register values.
> > > > >
> > > > > Patch 2 introduces a delay after a reset operation to respect datasheet
> > > > > timing recommandations.
> > > > >
> > > > > Patch 3 fixes an issue with init of first port during probing.
> > > > >
> > > > > Patch 4 fixes a bug with the output value when first setting the GPIO direction.
> > > > >
> > > > > Patch 5 is a refactor of GPIO registration code.
> > > > >
> > > > > Patches 6 and 7 fix a GPIO regression by (re)allowing to choose GPIO function
> > > > > for GPIO pins shared with modem status lines.
> > > > >
> > > > > Patch 8 allows to read common rs485 device-tree flags and properties.
> > > > >
> > > > > Patch 9 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> <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> <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> <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> <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> <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> <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> <https://lkml.org/lkml/2023/5/25/7 <https://lkml.org/lkml/2023/5/25/7>>
> > > > >
> > > > > 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"
> > > > >
> > > > > Hugo Villeneuve (9):
> > > > > serial: sc16is7xx: mark IOCONTROL register as volatile
> > > > > serial: sc16is7xx: add post reset delay
> > > > > serial: sc16is7xx: fix broken port 0 uart init
> > > > > serial: sc16is7xx: fix bug when first setting GPIO direction
> > > > > serial: sc16is7xx: refactor GPIO controller registration
> > > > > dt-bindings: sc16is7xx: Add property to change GPIO function
> > > > > serial: sc16is7xx: fix regression with GPIO configuration
> > > > > serial: sc16is7xx: add call to get rs485 DT flags and properties
> > > > > serial: sc16is7xx: improve comments about variants
> > > > >
> > > > > .../bindings/serial/nxp,sc16is7xx.txt | 46 ++++++
> > > > > drivers/tty/serial/sc16is7xx.c | 150 +++++++++++++-----
> > > > > 2 files changed, 156 insertions(+), 40 deletions(-)
> > > > >
> > > > >
> > > > > base-commit: 8b817fded42d8fe3a0eb47b1149d907851a3c942
> > > >
> > > > It would be a lot of sending, to do that for every patch separately, so for whole series:
> > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > >
> > > > And where applicable - for code patches:
> > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > >
> > > > I tested whole series at the same time.
> > > > I did my tests on an i.MX6 board with SC16IS760 over SPI, which differs a tiny bit from SC16IS752,
> > > > and everything works as it should.
> > > > Thank you for fixing this!
> > >
> > > Hi Lech,
> > > thank for your feedback.
> > >
> > > You mentioned before that without the patch "mark IOCONTROL register as volatile", things were not working properly for you. Could you retest by removing this patch and see if things are still working?
> > >
> > > Thank you, Hugo.
> > 
> > Hello Hugo,
> > 
> > Just checked - this patch is required, reverting it causes things to fail, so this patch should be marked as a pre-requisite for the actual fix and included in backports.
> > Perhaps using direct write to this register made it work, but it was likely by accident.
> 
> Hi Lech,
> thank you for the test, I will mark it as such in upcoming series V5.
> 
> Hugo.

Since I reworked a bit patch 5/9 in series 5, I removed your "Tested-by" and "Reviewed-by" tags only for this patch. Please reconfirm these tags when you have tested series 5.

Thank you,
Hugo.

-- 
Hugo Villeneuve <hugo@hugovil.com>