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

[PATCH v8 00/10] 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 with init of first port during probing.

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

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

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

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

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

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

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

Patch 10 improves comments about chip variants.

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

Thank you.

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

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

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

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

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

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

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

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

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


base-commit: a4d7d701121981e3c3fe69ade376fe9f26324161
-- 
2.30.2

[PATCH v8 01/10] 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")
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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..faa51a58671f 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
 		s->p[i].port.iobase	= i;
+		s->p[i].port.membase	= (void __iomem *)~0;
 		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
-- 
2.30.2

[PATCH v8 02/10] 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,
which is incorrect.

Also, if IOCONTROL register is not a volatile register, the upcoming
patch "serial: sc16is7xx: fix regression with GPIO configuration"
doesn't work when setting some shared GPIO lines as modem control
lines.

Therefore mark IOCONTROL register as a volatile register.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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 faa51a58671f..0c903d44429c 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 v8 03/10] serial: sc16is7xx: remove obsolete out_thread label

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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

Fixes: c8f71b49ee4d ("serial: sc16is7xx: setup GPIO controller later in probe")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---

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

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

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

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

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

[PATCH v8 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

In preparation for upcoming patch "fix regression with GPIO
configuration". To facilitate review and make code more modular.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 9e9856846643..f5b4d2d18a71 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -332,6 +332,7 @@ struct sc16is7xx_one {
 
 struct sc16is7xx_port {
 	const struct sc16is7xx_devtype	*devtype;
+	struct device			*dev;
 	struct regmap			*regmap;
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
@@ -1349,6 +1350,25 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 
 	return 0;
 }
+
+static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
+{
+	if (!s->devtype->nr_gpio)
+		return 0;
+
+	s->gpio.owner		 = THIS_MODULE;
+	s->gpio.parent		 = s->dev;
+	s->gpio.label		 = dev_name(s->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 = {
@@ -1412,6 +1432,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 	s->regmap = regmap;
 	s->devtype = devtype;
+	s->dev = dev;
 	dev_set_drvdata(dev, s);
 	mutex_init(&s->efr_lock);
 
@@ -1502,22 +1523,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_ports;
-	}
+	ret = sc16is7xx_setup_gpio_chip(s);
+	if (ret)
+		goto out_ports;
 #endif
 
 	/*
-- 
2.30.2

[PATCH v8 05/10] 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.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
+		compatible = "nxp,sc16is752";
+		reg = <0x53>;
+		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@1 {
+		compatible = "nxp,sc16is752";
+		reg = <1>;
+		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@2 {
+		compatible = "nxp,sc16is752";
+		reg = <2>;
+		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

[PATCH v8 06/10] 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")
Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 104 +++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index f5b4d2d18a71..11edbebe8fa2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@
 
 /* IOControl register bits (Only 750/760) */
 #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
-#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
+#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
+#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
 #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
 
 /* EFCR register bits */
@@ -301,12 +302,12 @@
 /* Misc definitions */
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_REG_SHIFT		2
+#define SC16IS7XX_GPIOS_PER_BANK	4
 
 struct sc16is7xx_devtype {
 	char	name[10];
 	int	nr_gpio;
 	int	nr_uart;
-	int	has_mctrl;
 };
 
 #define SC16IS7XX_RECONF_MD		(1 << 0)
@@ -337,7 +338,9 @@ struct sc16is7xx_port {
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
+	unsigned long			gpio_valid_mask;
 #endif
+	u8				mctrl_mask;
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
 	struct task_struct		*kworker_task;
@@ -448,35 +451,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
 	.name		= "SC16IS74X",
 	.nr_gpio	= 0,
 	.nr_uart	= 1,
-	.has_mctrl	= 0,
 };
 
 static const struct sc16is7xx_devtype sc16is750_devtype = {
 	.name		= "SC16IS750",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is752_devtype = {
 	.name		= "SC16IS752",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is760_devtype = {
 	.name		= "SC16IS760",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.name		= "SC16IS762",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
@@ -1351,14 +1349,43 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
+static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
+					  unsigned long *valid_mask,
+					  unsigned int ngpios)
+{
+	struct sc16is7xx_port *s = gpiochip_get_data(chip);
+
+	*valid_mask = s->gpio_valid_mask;
+
+	return 0;
+}
+
 static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
 {
 	if (!s->devtype->nr_gpio)
 		return 0;
 
+	switch (s->mctrl_mask) {
+	case 0:
+		s->gpio_valid_mask = GENMASK(7, 0);
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
+		s->gpio_valid_mask = GENMASK(3, 0);
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
+		s->gpio_valid_mask = GENMASK(7, 4);
+		break;
+	default:
+		break;
+	}
+
+	if (s->gpio_valid_mask == 0)
+		return 0;
+
 	s->gpio.owner		 = THIS_MODULE;
 	s->gpio.parent		 = s->dev;
 	s->gpio.label		 = dev_name(s->dev);
+	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
 	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
 	s->gpio.get		 = sc16is7xx_gpio_get;
 	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
@@ -1371,6 +1398,47 @@ static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
 }
 #endif
 
+/*
+ * Configure ports designated to operate as modem control lines.
+ */
+static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
+{
+	int i;
+	int ret;
+	int count;
+	u32 mctrl_port[2];
+
+	count = device_property_count_u32(s->dev,
+					  "nxp,modem-control-line-ports");
+	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+		return 0;
+
+	ret = device_property_read_u32_array(s->dev,
+					     "nxp,modem-control-line-ports",
+					     mctrl_port, count);
+	if (ret)
+		return ret;
+
+	s->mctrl_mask = 0;
+
+	for (i = 0; i < count; i++) {
+		/* Use GPIO lines as modem control lines */
+		if (mctrl_port[i] == 0)
+			s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+		else if (mctrl_port[i] == 1)
+			s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+	}
+
+	if (s->mctrl_mask)
+		regmap_update_bits(
+			s->regmap,
+			SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+			SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+			SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
+
+	return 0;
+}
+
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
@@ -1479,12 +1547,6 @@ static int sc16is7xx_probe(struct device *dev,
 				     SC16IS7XX_EFCR_RXDISABLE_BIT |
 				     SC16IS7XX_EFCR_TXDISABLE_BIT);
 
-		/* Use GPIO lines as modem status registers */
-		if (devtype->has_mctrl)
-			sc16is7xx_port_write(&s->p[i].port,
-					     SC16IS7XX_IOCONTROL_REG,
-					     SC16IS7XX_IOCONTROL_MODEM_BIT);
-
 		/* Initialize kthread work structs */
 		kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
 		kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
@@ -1522,6 +1584,10 @@ static int sc16is7xx_probe(struct device *dev,
 				s->p[u].irda_mode = true;
 	}
 
+	ret = sc16is7xx_setup_mctrl_ports(s);
+	if (ret)
+		goto out_ports;
+
 #ifdef CONFIG_GPIOLIB
 	ret = sc16is7xx_setup_gpio_chip(s);
 	if (ret)
@@ -1548,7 +1614,7 @@ static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 #endif
 
@@ -1572,7 +1638,7 @@ static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 #endif
 
-- 
2.30.2

[PATCH v8 07/10] 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")
Cc: <stable@vger.kernel.org>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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 11edbebe8fa2..98c520ef4a02 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1342,9 +1342,18 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 		state |= BIT(offset);
 	else
 		state &= ~BIT(offset);
-	sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);
+
+	/*
+	 * If we write IOSTATE first, and then IODIR, the output value is not
+	 * transferred to the corresponding I/O pin.
+	 * The datasheet states that each register bit will be transferred to
+	 * the corresponding I/O pin programmed as output when writing to
+	 * IOSTATE. Therefore, configure direction first with IODIR, and then
+	 * set value after with IOSTATE.
+	 */
 	sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset),
 			      BIT(offset));
+	sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);
 
 	return 0;
 }
-- 
2.30.2

[PATCH v8 08/10] 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.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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 98c520ef4a02..7246f8997fda 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1549,6 +1549,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 v8 09/10] 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>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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 7246f8997fda..7616703f7438 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev,
 	regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
 			SC16IS7XX_IOCONTROL_SRESET_BIT);
 
+	/*
+	 * After reset, the host must wait at least 3us before initializing a
+	 * communication with the device.
+	 */
+	usleep_range(5, 10);
+
 	for (i = 0; i < devtype->nr_uart; ++i) {
 		s->p[i].line		= i;
 		/* Initialize port data */
-- 
2.30.2

[PATCH v8 10/10] 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>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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 7616703f7438..c9edc51870ca 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 v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Krzysztof Kozlowski]

On 07/06/2023 16:05, 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.
> 
> Cc: <stable@vger.kernel.org> # 6.1.x
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {

Since you keep sending new versions, fix the names. nNode names should
be generic. See also explanation and list of examples in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof

Re: [PATCH v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Wed, 7 Jun 2023 16:30:26 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:05, 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.
> > 
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
> 
> Since you keep sending new versions, fix the names. nNode names should
> be generic. See also explanation and list of examples in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Hi,
by the way, I do not "keep sending new versions" just for the fun of
it...

Even after reading your link, I cannot see what is wrong with that
name.

You are the first one since V1 to think it is inappropriate.

Please suggest what the name should be.

Hugo.

Re: [PATCH v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Krzysztof Kozlowski]

On 07/06/2023 16:41, Hugo Villeneuve wrote:
> On Wed, 7 Jun 2023 16:30:26 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/06/2023 16:05, 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.
>>>
>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
>>
>> Since you keep sending new versions, fix the names. nNode names should
>> be generic. See also explanation and list of examples in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Hi,
> by the way, I do not "keep sending new versions" just for the fun of
> it...

Sure, I know. So when the next version is necessary to send, fix also this.

>
> Even after reading your link, I cannot see what is wrong with that
> name.

They are not generic. They are specific.

> 
> You are the first one since V1 to think it is inappropriate.
> 
> Please suggest what the name should be.

Depends what this is. I know nothing about this device, so I cannot
suggest you. Therefore I gave you link with ideas to choose from.

Best regards,
Krzysztof

Re: [PATCH v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Wed, 7 Jun 2023 16:46:56 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:41, Hugo Villeneuve wrote:
> > On Wed, 7 Jun 2023 16:30:26 +0200
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 07/06/2023 16:05, 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.
> >>>
> >>> Cc: <stable@vger.kernel.org> # 6.1.x
> >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
> >>
> >> Since you keep sending new versions, fix the names. nNode names should
> >> be generic. See also explanation and list of examples in DT specification:
> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> > 
> > Hi,
> > by the way, I do not "keep sending new versions" just for the fun of
> > it...
> 
> Sure, I know. So when the next version is necessary to send, fix also this.
> 
> >
> > Even after reading your link, I cannot see what is wrong with that
> > name.
> 
> They are not generic. They are specific.

What do you mean by "They"? My patch adds only a new property...

This patch will not fix old names, but a future patch could do it.

And what do you mean by "They are not generic. They are specific". Of
course the property is specific, because it relates to something very
specific to this chip?


> > You are the first one since V1 to think it is inappropriate.
> > 
> > Please suggest what the name should be.
> 
> Depends what this is. I know nothing about this device, so I cannot
> suggest you. Therefore I gave you link with ideas to choose from.
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Krzysztof Kozlowski]

On 07/06/2023 16:52, Hugo Villeneuve wrote:
> On Wed, 7 Jun 2023 16:46:56 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/06/2023 16:41, Hugo Villeneuve wrote:
>>> On Wed, 7 Jun 2023 16:30:26 +0200
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 07/06/2023 16:05, 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.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # 6.1.x
>>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
>>>>
>>>> Since you keep sending new versions, fix the names. nNode names should
>>>> be generic. See also explanation and list of examples in DT specification:
>>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>>
>>> Hi,
>>> by the way, I do not "keep sending new versions" just for the fun of
>>> it...
>>
>> Sure, I know. So when the next version is necessary to send, fix also this.
>>
>>>
>>> Even after reading your link, I cannot see what is wrong with that
>>> name.
>>
>> They are not generic. They are specific.
> 
> What do you mean by "They"? My patch adds only a new property...

Your patch adds multiple new examples. My comment was at appropriate
place to fix, although only the first place. I did not refer to
unrelated pieces or other parts of the code. Specifically - I wrote
nothing about property.

> 
> This patch will not fix old names, but a future patch could do it.

No worries, I asked for new code.

> 
> And what do you mean by "They are not generic. They are specific". Of
> course the property is specific, because it relates to something very
> specific to this chip?

Again, I did not comment under a property. I did not refer to any property.


Best regards,
Krzysztof

Re: [PATCH v8 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Wed, 7 Jun 2023 16:57:48 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/06/2023 16:52, Hugo Villeneuve wrote:
> > On Wed, 7 Jun 2023 16:46:56 +0200
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 07/06/2023 16:41, Hugo Villeneuve wrote:
> >>> On Wed, 7 Jun 2023 16:30:26 +0200
> >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>>> On 07/06/2023 16:05, 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.
> >>>>>
> >>>>> Cc: <stable@vger.kernel.org> # 6.1.x
> >>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >>>>> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.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..1a7e4bff0456 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@53 {
> >>>>
> >>>> Since you keep sending new versions, fix the names. nNode names should
> >>>> be generic. See also explanation and list of examples in DT specification:
> >>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >>>
> >>> Hi,
> >>> by the way, I do not "keep sending new versions" just for the fun of
> >>> it...
> >>
> >> Sure, I know. So when the next version is necessary to send, fix also this.
> >>
> >>>
> >>> Even after reading your link, I cannot see what is wrong with that
> >>> name.
> >>
> >> They are not generic. They are specific.
> > 
> > What do you mean by "They"? My patch adds only a new property...
> 
> Your patch adds multiple new examples. My comment was at appropriate
> place to fix, although only the first place. I did not refer to
> unrelated pieces or other parts of the code. Specifically - I wrote
> nothing about property.

Ahhh!!!!!!!!!!!!

Ok, now I see.

But I simply added new examples based on the previous examples, which
already used "inappropriate" names.

If I modify only the node names in my new examples, this will clash
with the existing examples.

I have already prepared a new patch, that I will send one day if we can
ever finish this never ending series, to convert the binding to YAML,
and to fix a few things with old properties missing vendor prefix,
etc.

I will modify all the examples in this new series to follow what you
suggest and incorporate it into this new serie.

Hugo.


> > This patch will not fix old names, but a future patch could do it.
> 
> No worries, I asked for new code.
> 
> > 
> > And what do you mean by "They are not generic. They are specific". Of
> > course the property is specific, because it relates to something very
> > specific to this chip?
> 
> Again, I did not comment under a property. I did not refer to any property.
> 
> 
> Best regards,
> Krzysztof

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

[Lech Perczak]

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

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

-- 
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 v8 00/10] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Mon, 12 Jun 2023 14:10:20 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:

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

Hi Lech,
thank you for testing this again.

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

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

Hugo.


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