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

[RESEND 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: f7e3a1bafdea735050dfde00523cf505dc7fd309
-- 
2.30.2

[RESEND 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 2e7e7c409cf2..8ae2afc76a9b 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

[RESEND 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 8ae2afc76a9b..306ae512b38a 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

[RESEND 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>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 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 306ae512b38a..32d43d00a583 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

[RESEND 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 32d43d00a583..5b0aeef9d534 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

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

[RESEND 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>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.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 5b0aeef9d534..bc0a288f258d 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

[RESEND 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 bc0a288f258d..07ae889db296 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

[RESEND 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 07ae889db296..49213be60baf 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

[RESEND 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 49213be60baf..718e982e1efe 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

[RESEND 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 718e982e1efe..d6851360ef7d 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: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Rob Herring]

On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.

Requiring a new DT property is not fixing a kernel regression. You
should be returning the kernel to original behavior and then have a
new DT property for new behavior.

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

That's not the name in the 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

Then again, if no one cares about those boards needing a change then
it can be okay.


Rob

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Fri, 21 Jul 2023 13:24:19 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> 
> Requiring a new DT property is not fixing a kernel regression. You
> should be returning the kernel to original behavior and then have a
> new DT property for new behavior.

Hi Rob,
please read the entire patch history starting from V1
 and you will understand why this course of action was
 not selected.

 
> > 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).
> 
> That's not the name in the patch.

We added a "nxp," prefix at some point.

Do you want me to send a V9 only with this change?

Hugo.


> > 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
> 
> Then again, if no one cares about those boards needing a change then
> it can be okay.
> 
> 
> Rob
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> On Fri, 21 Jul 2023 13:24:19 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > 
> > Requiring a new DT property is not fixing a kernel regression. You
> > should be returning the kernel to original behavior and then have a
> > new DT property for new behavior.
> 
> Hi Rob,
> please read the entire patch history starting from V1
>  and you will understand why this course of action was
>  not selected.

That's not going to happen, sorry, you need to explain it here, in this
patch series, why a specific action is being taken over another one, as
no one has time to go dig through past history, sorry.

thanks,

greg k-h

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sat, 22 Jul 2023 17:15:26 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > On Fri, 21 Jul 2023 13:24:19 -0600
> > Rob Herring <robh+dt@kernel.org> wrote:
> > 
> > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > 
> > > Requiring a new DT property is not fixing a kernel regression. You
> > > should be returning the kernel to original behavior and then have a
> > > new DT property for new behavior.
> > 
> > Hi Rob,
> > please read the entire patch history starting from V1
> >  and you will understand why this course of action was
> >  not selected.
> 
> That's not going to happen, sorry, you need to explain it here, in this
> patch series, why a specific action is being taken over another one, as
> no one has time to go dig through past history, sorry.

Hi Rob,
I initially submitted a patch to revert the kernel to original
behavior, but it created more problems because the patch was
unfortunately split in two separate patches, and mixed with other non
closely-related changes. It was also noted to me that reverting to the
old behavior would break things for some users.

It was suggested to me by a more experienced kernel developer to
"suggest a fix, instead of hurrying a revert":

    https://lkml.org/lkml/2023/5/17/758

That is what we decided to do in the end, and it worked quite well.

Hugo.

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Rob Herring]

On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> On Sat, 22 Jul 2023 17:15:26 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > >
> > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > should be returning the kernel to original behavior and then have a
> > > > new DT property for new behavior.
> > >
> > > Hi Rob,
> > > please read the entire patch history starting from V1
> > >  and you will understand why this course of action was
> > >  not selected.
> >
> > That's not going to happen, sorry, you need to explain it here, in this
> > patch series, why a specific action is being taken over another one, as
> > no one has time to go dig through past history, sorry.
>
> Hi Rob,
> I initially submitted a patch to revert the kernel to original
> behavior, but it created more problems because the patch was
> unfortunately split in two separate patches, and mixed with other non
> closely-related changes. It was also noted to me that reverting to the
> old behavior would break things for some users.
>
> It was suggested to me by a more experienced kernel developer to
> "suggest a fix, instead of hurrying a revert":
>
>     https://lkml.org/lkml/2023/5/17/758

Do I have to go read this to decipher the justification and reasoning?
When Greg says "in this patch series", he means in the commit messages
of the patches. You send v9 already and it doesn't have that. The
patchset needs to stand on its own summarizing any relevant prior
discussions.

I never suggested doing a revert. Obviously, someone still wants the
new feature. The issue is a new feature was added to the kernel, but
you are requiring a DT change to platforms NOT using the feature. Make
the platforms wanting the new feature to need a DT change. That's
still not great, but it's much better to affect new platforms rather
than old, stable platforms. The period of time that regresses is much
smaller (a few kernel releases vs. years potentially). Of course, if
it's just those 3 platforms and their maintainers are fine with
needing this DT change, then that works too. But there's no evidence
here that they are okay with it. You didn't even do the update of the
dts files and just left them broken.

Rob

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Mon, 31 Jul 2023 09:31:53 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Sat, 22 Jul 2023 17:15:26 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > >
> > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > should be returning the kernel to original behavior and then have a
> > > > > new DT property for new behavior.
> > > >
> > > > Hi Rob,
> > > > please read the entire patch history starting from V1
> > > >  and you will understand why this course of action was
> > > >  not selected.
> > >
> > > That's not going to happen, sorry, you need to explain it here, in this
> > > patch series, why a specific action is being taken over another one, as
> > > no one has time to go dig through past history, sorry.
> >
> > Hi Rob,
> > I initially submitted a patch to revert the kernel to original
> > behavior, but it created more problems because the patch was
> > unfortunately split in two separate patches, and mixed with other non
> > closely-related changes. It was also noted to me that reverting to the
> > old behavior would break things for some users.
> >
> > It was suggested to me by a more experienced kernel developer to
> > "suggest a fix, instead of hurrying a revert":
> >
> >     https://lkml.org/lkml/2023/5/17/758
> 
> Do I have to go read this to decipher the justification and reasoning?
> When Greg says "in this patch series", he means in the commit messages
> of the patches. You send v9 already and it doesn't have that. The
> patchset needs to stand on its own summarizing any relevant prior
> discussions.
> 
> I never suggested doing a revert.

Hi Rob,
I am sorry, but this is exactly what I "deciphered" from your
original email.

I am trying very hard to understand exactly what you mean, but it is
not that obvious for me. If something is not clear in my commit message,
I will try to improve it. But before, let's try to focus on making sure
I understand more clearly what you want exactly.

> Obviously, someone still wants the
> new feature.

I assume that you refer to the "new feature" as what was added in
the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
lines")?

Because I did not add a "new feature" myself, I simply restored (or
want to restore) what was working before commit 679875d1d880
(restore GPIO pins as GPIO functions).

I will wait for your clarification on this, and answer your other
comments after.

Hugo.


> The issue is a new feature was added to the kernel, but
> you are requiring a DT change to platforms NOT using the feature.
> Make
> the platforms wanting the new feature to need a DT change. That's
> still not great, but it's much better to affect new platforms rather
> than old, stable platforms. The period of time that regresses is much
> smaller (a few kernel releases vs. years potentially). Of course, if
> it's just those 3 platforms and their maintainers are fine with
> needing this DT change, then that works too. But there's no evidence
> here that they are okay with it. You didn't even do the update of the
> dts files and just left them broken.

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Rob Herring]

On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> On Mon, 31 Jul 2023 09:31:53 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
>
> > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > >
> > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > >
> > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > should be returning the kernel to original behavior and then have a
> > > > > > new DT property for new behavior.
> > > > >
> > > > > Hi Rob,
> > > > > please read the entire patch history starting from V1
> > > > >  and you will understand why this course of action was
> > > > >  not selected.
> > > >
> > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > patch series, why a specific action is being taken over another one, as
> > > > no one has time to go dig through past history, sorry.
> > >
> > > Hi Rob,
> > > I initially submitted a patch to revert the kernel to original
> > > behavior, but it created more problems because the patch was
> > > unfortunately split in two separate patches, and mixed with other non
> > > closely-related changes. It was also noted to me that reverting to the
> > > old behavior would break things for some users.
> > >
> > > It was suggested to me by a more experienced kernel developer to
> > > "suggest a fix, instead of hurrying a revert":
> > >
> > >     https://lkml.org/lkml/2023/5/17/758
> >
> > Do I have to go read this to decipher the justification and reasoning?
> > When Greg says "in this patch series", he means in the commit messages
> > of the patches. You send v9 already and it doesn't have that. The
> > patchset needs to stand on its own summarizing any relevant prior
> > discussions.
> >
> > I never suggested doing a revert.
>
> Hi Rob,
> I am sorry, but this is exactly what I "deciphered" from your
> original email.
>
> I am trying very hard to understand exactly what you mean, but it is
> not that obvious for me. If something is not clear in my commit message,
> I will try to improve it. But before, let's try to focus on making sure
> I understand more clearly what you want exactly.
>
> > Obviously, someone still wants the
> > new feature.
>
> I assume that you refer to the "new feature" as what was added in
> the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> lines")?

Shrug. It's one of the 2 commits mentioned, I don't know which one
exactly. Whichever one changed default behavior from use GPIOs to use
modem ctrl lines.

Reading it again, I *think* this patch is correct. Default behavior is
restored to use GPIOs. The DT property is needed to enable modem ctrl
lines.

What's not okay is just saying, these platforms may or may not need an update:

    arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
    mips/boot/dts/ingenic/cu1830-neo.dts
    mips/boot/dts/ingenic/cu1000-neo.dts

You need to figure that out. Have you checked with maintainers of
these boards? When were they added and by who? At the same time or by
the same person would be a good indication the platform uses modem
ctrl lines. Or were these platforms in use before adding modem ctrl
support? Then they probably use GPIOs or nothing.

If there are platforms which would regress if the modem ctrl feature
was just reverted, which ones are those?

Rob

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Mon, 31 Jul 2023 12:04:45 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Mon, 31 Jul 2023 09:31:53 -0600
> > Rob Herring <robh+dt@kernel.org> wrote:
> >
> > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > >
> > > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > > >
> > > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > > should be returning the kernel to original behavior and then have a
> > > > > > > new DT property for new behavior.
> > > > > >
> > > > > > Hi Rob,
> > > > > > please read the entire patch history starting from V1
> > > > > >  and you will understand why this course of action was
> > > > > >  not selected.
> > > > >
> > > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > > patch series, why a specific action is being taken over another one, as
> > > > > no one has time to go dig through past history, sorry.
> > > >
> > > > Hi Rob,
> > > > I initially submitted a patch to revert the kernel to original
> > > > behavior, but it created more problems because the patch was
> > > > unfortunately split in two separate patches, and mixed with other non
> > > > closely-related changes. It was also noted to me that reverting to the
> > > > old behavior would break things for some users.
> > > >
> > > > It was suggested to me by a more experienced kernel developer to
> > > > "suggest a fix, instead of hurrying a revert":
> > > >
> > > >     https://lkml.org/lkml/2023/5/17/758
> > >
> > > Do I have to go read this to decipher the justification and reasoning?
> > > When Greg says "in this patch series", he means in the commit messages
> > > of the patches. You send v9 already and it doesn't have that. The
> > > patchset needs to stand on its own summarizing any relevant prior
> > > discussions.
> > >
> > > I never suggested doing a revert.
> >
> > Hi Rob,
> > I am sorry, but this is exactly what I "deciphered" from your
> > original email.
> >
> > I am trying very hard to understand exactly what you mean, but it is
> > not that obvious for me. If something is not clear in my commit message,
> > I will try to improve it. But before, let's try to focus on making sure
> > I understand more clearly what you want exactly.
> >
> > > Obviously, someone still wants the
> > > new feature.
> >
> > I assume that you refer to the "new feature" as what was added in
> > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > lines")?
> 
> Shrug. It's one of the 2 commits mentioned, I don't know which one
> exactly. Whichever one changed default behavior from use GPIOs to use
> modem ctrl lines.
> 
> Reading it again, I *think* this patch is correct. Default behavior is
> restored to use GPIOs. The DT property is needed to enable modem ctrl
> lines.

Hi,
this is correct.


> What's not okay is just saying, these platforms may or may not need an update:
> 
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts

Yes, my bad. I initially mentioned them and hoped to get some
feedback, which I never got, and I kind of forgot about it.

> You need to figure that out. Have you checked with maintainers of
> these boards? When were they added and by who? At the same time or by
> the same person would be a good indication the platform uses modem
> ctrl lines. Or were these platforms in use before adding modem ctrl
> support? Then they probably use GPIOs or nothing.
> 
> If there are platforms which would regress if the modem ctrl feature
> was just reverted, which ones are those?

Ok, let me do some checks and get back to you on this.

Thank you,
Hugo.

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Mon, 31 Jul 2023 14:41:15 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Mon, 31 Jul 2023 12:04:45 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > >
> > > On Mon, 31 Jul 2023 09:31:53 -0600
> > > Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > >
> > > > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > > > >
> > > > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > > > should be returning the kernel to original behavior and then have a
> > > > > > > > new DT property for new behavior.
> > > > > > >
> > > > > > > Hi Rob,
> > > > > > > please read the entire patch history starting from V1
> > > > > > >  and you will understand why this course of action was
> > > > > > >  not selected.
> > > > > >
> > > > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > > > patch series, why a specific action is being taken over another one, as
> > > > > > no one has time to go dig through past history, sorry.
> > > > >
> > > > > Hi Rob,
> > > > > I initially submitted a patch to revert the kernel to original
> > > > > behavior, but it created more problems because the patch was
> > > > > unfortunately split in two separate patches, and mixed with other non
> > > > > closely-related changes. It was also noted to me that reverting to the
> > > > > old behavior would break things for some users.
> > > > >
> > > > > It was suggested to me by a more experienced kernel developer to
> > > > > "suggest a fix, instead of hurrying a revert":
> > > > >
> > > > >     https://lkml.org/lkml/2023/5/17/758
> > > >
> > > > Do I have to go read this to decipher the justification and reasoning?
> > > > When Greg says "in this patch series", he means in the commit messages
> > > > of the patches. You send v9 already and it doesn't have that. The
> > > > patchset needs to stand on its own summarizing any relevant prior
> > > > discussions.
> > > >
> > > > I never suggested doing a revert.
> > >
> > > Hi Rob,
> > > I am sorry, but this is exactly what I "deciphered" from your
> > > original email.
> > >
> > > I am trying very hard to understand exactly what you mean, but it is
> > > not that obvious for me. If something is not clear in my commit message,
> > > I will try to improve it. But before, let's try to focus on making sure
> > > I understand more clearly what you want exactly.
> > >
> > > > Obviously, someone still wants the
> > > > new feature.
> > >
> > > I assume that you refer to the "new feature" as what was added in
> > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > lines")?
> > 
> > Shrug. It's one of the 2 commits mentioned, I don't know which one
> > exactly. Whichever one changed default behavior from use GPIOs to use
> > modem ctrl lines.
> > 
> > Reading it again, I *think* this patch is correct. Default behavior is
> > restored to use GPIOs. The DT property is needed to enable modem ctrl
> > lines.
> 
> Hi,
> this is correct.
> 
> 
> > What's not okay is just saying, these platforms may or may not need an update:
> > 
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> 
> Yes, my bad. I initially mentioned them and hoped to get some
> feedback, which I never got, and I kind of forgot about it.
> 
> > You need to figure that out. Have you checked with maintainers of
> > these boards? When were they added and by who? At the same time or by
> > the same person would be a good indication the platform uses modem
> > ctrl lines. Or were these platforms in use before adding modem ctrl
> > support? Then they probably use GPIOs or nothing.
> > 
> > If there are platforms which would regress if the modem ctrl feature
> > was just reverted, which ones are those?
> 
> Ok, let me do some checks and get back to you on this.

Hi Rob,
for this board:
    arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts

it uses a SC16IS740, which doesn't have any GPIOs nor modem
control lines, so no DT changes required.

For these two Ingenic boards:
    mips/boot/dts/ingenic/cu1830-neo.dts
    mips/boot/dts/ingenic/cu1000-neo.dts

They use a SC16IS752, which has shared modem control lines and GPIOs.
Unfortunately, the maintainers have not (yet) responded to my
inquiries. Also, I tried to search for schematics or block diagrams on
the net but couldn't find anything.

These platforms were in use before the patch to add the modem control
lines was added. Then like you said they probably use these shared
lines as GPIOs or nothing, so no DT changes would be required.

Hugo.

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Rob Herring]

On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> On Mon, 31 Jul 2023 14:41:15 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> > On Mon, 31 Jul 2023 12:04:45 -0600
> > Rob Herring <robh+dt@kernel.org> wrote:
> >
> > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > >
> > > > On Mon, 31 Jul 2023 09:31:53 -0600
> > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > >
> > > > > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > >
> > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > > > > >
> > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > > > > should be returning the kernel to original behavior and then have a
> > > > > > > > > new DT property for new behavior.
> > > > > > > >
> > > > > > > > Hi Rob,
> > > > > > > > please read the entire patch history starting from V1
> > > > > > > >  and you will understand why this course of action was
> > > > > > > >  not selected.
> > > > > > >
> > > > > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > > > > patch series, why a specific action is being taken over another one, as
> > > > > > > no one has time to go dig through past history, sorry.
> > > > > >
> > > > > > Hi Rob,
> > > > > > I initially submitted a patch to revert the kernel to original
> > > > > > behavior, but it created more problems because the patch was
> > > > > > unfortunately split in two separate patches, and mixed with other non
> > > > > > closely-related changes. It was also noted to me that reverting to the
> > > > > > old behavior would break things for some users.
> > > > > >
> > > > > > It was suggested to me by a more experienced kernel developer to
> > > > > > "suggest a fix, instead of hurrying a revert":
> > > > > >
> > > > > >     https://lkml.org/lkml/2023/5/17/758
> > > > >
> > > > > Do I have to go read this to decipher the justification and reasoning?
> > > > > When Greg says "in this patch series", he means in the commit messages
> > > > > of the patches. You send v9 already and it doesn't have that. The
> > > > > patchset needs to stand on its own summarizing any relevant prior
> > > > > discussions.
> > > > >
> > > > > I never suggested doing a revert.
> > > >
> > > > Hi Rob,
> > > > I am sorry, but this is exactly what I "deciphered" from your
> > > > original email.
> > > >
> > > > I am trying very hard to understand exactly what you mean, but it is
> > > > not that obvious for me. If something is not clear in my commit message,
> > > > I will try to improve it. But before, let's try to focus on making sure
> > > > I understand more clearly what you want exactly.
> > > >
> > > > > Obviously, someone still wants the
> > > > > new feature.
> > > >
> > > > I assume that you refer to the "new feature" as what was added in
> > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > > lines")?
> > >
> > > Shrug. It's one of the 2 commits mentioned, I don't know which one
> > > exactly. Whichever one changed default behavior from use GPIOs to use
> > > modem ctrl lines.
> > >
> > > Reading it again, I *think* this patch is correct. Default behavior is
> > > restored to use GPIOs. The DT property is needed to enable modem ctrl
> > > lines.
> >
> > Hi,
> > this is correct.
> >
> >
> > > What's not okay is just saying, these platforms may or may not need an update:
> > >
> > >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> > >     mips/boot/dts/ingenic/cu1830-neo.dts
> > >     mips/boot/dts/ingenic/cu1000-neo.dts
> >
> > Yes, my bad. I initially mentioned them and hoped to get some
> > feedback, which I never got, and I kind of forgot about it.
> >
> > > You need to figure that out. Have you checked with maintainers of
> > > these boards? When were they added and by who? At the same time or by
> > > the same person would be a good indication the platform uses modem
> > > ctrl lines. Or were these platforms in use before adding modem ctrl
> > > support? Then they probably use GPIOs or nothing.
> > >
> > > If there are platforms which would regress if the modem ctrl feature
> > > was just reverted, which ones are those?
> >
> > Ok, let me do some checks and get back to you on this.
>
> Hi Rob,
> for this board:
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>
> it uses a SC16IS740, which doesn't have any GPIOs nor modem
> control lines, so no DT changes required.
>
> For these two Ingenic boards:
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts
>
> They use a SC16IS752, which has shared modem control lines and GPIOs.
> Unfortunately, the maintainers have not (yet) responded to my
> inquiries. Also, I tried to search for schematics or block diagrams on
> the net but couldn't find anything.
>
> These platforms were in use before the patch to add the modem control
> lines was added. Then like you said they probably use these shared
> lines as GPIOs or nothing, so no DT changes would be required.

Okay, that's useful (please add to the commit msg).

Still, what platform(s) need the modem control feature? Presumably
that's whatever platform Lech and Tomasz work on. I guess given the
Reviewed-by they are fine with needing a DT change.

Rob

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Thu, 3 Aug 2023 14:10:30 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Mon, 31 Jul 2023 14:41:15 -0400
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > > On Mon, 31 Jul 2023 12:04:45 -0600
> > > Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > >
> > > > > On Mon, 31 Jul 2023 09:31:53 -0600
> > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > >
> > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > > > > > >
> > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > > > > > should be returning the kernel to original behavior and then have a
> > > > > > > > > > new DT property for new behavior.
> > > > > > > > >
> > > > > > > > > Hi Rob,
> > > > > > > > > please read the entire patch history starting from V1
> > > > > > > > >  and you will understand why this course of action was
> > > > > > > > >  not selected.
> > > > > > > >
> > > > > > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > > > > > patch series, why a specific action is being taken over another one, as
> > > > > > > > no one has time to go dig through past history, sorry.
> > > > > > >
> > > > > > > Hi Rob,
> > > > > > > I initially submitted a patch to revert the kernel to original
> > > > > > > behavior, but it created more problems because the patch was
> > > > > > > unfortunately split in two separate patches, and mixed with other non
> > > > > > > closely-related changes. It was also noted to me that reverting to the
> > > > > > > old behavior would break things for some users.
> > > > > > >
> > > > > > > It was suggested to me by a more experienced kernel developer to
> > > > > > > "suggest a fix, instead of hurrying a revert":
> > > > > > >
> > > > > > >     https://lkml.org/lkml/2023/5/17/758
> > > > > >
> > > > > > Do I have to go read this to decipher the justification and reasoning?
> > > > > > When Greg says "in this patch series", he means in the commit messages
> > > > > > of the patches. You send v9 already and it doesn't have that. The
> > > > > > patchset needs to stand on its own summarizing any relevant prior
> > > > > > discussions.
> > > > > >
> > > > > > I never suggested doing a revert.
> > > > >
> > > > > Hi Rob,
> > > > > I am sorry, but this is exactly what I "deciphered" from your
> > > > > original email.
> > > > >
> > > > > I am trying very hard to understand exactly what you mean, but it is
> > > > > not that obvious for me. If something is not clear in my commit message,
> > > > > I will try to improve it. But before, let's try to focus on making sure
> > > > > I understand more clearly what you want exactly.
> > > > >
> > > > > > Obviously, someone still wants the
> > > > > > new feature.
> > > > >
> > > > > I assume that you refer to the "new feature" as what was added in
> > > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > > > lines")?
> > > >
> > > > Shrug. It's one of the 2 commits mentioned, I don't know which one
> > > > exactly. Whichever one changed default behavior from use GPIOs to use
> > > > modem ctrl lines.
> > > >
> > > > Reading it again, I *think* this patch is correct. Default behavior is
> > > > restored to use GPIOs. The DT property is needed to enable modem ctrl
> > > > lines.
> > >
> > > Hi,
> > > this is correct.
> > >
> > >
> > > > What's not okay is just saying, these platforms may or may not need an update:
> > > >
> > > >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> > > >     mips/boot/dts/ingenic/cu1830-neo.dts
> > > >     mips/boot/dts/ingenic/cu1000-neo.dts
> > >
> > > Yes, my bad. I initially mentioned them and hoped to get some
> > > feedback, which I never got, and I kind of forgot about it.
> > >
> > > > You need to figure that out. Have you checked with maintainers of
> > > > these boards? When were they added and by who? At the same time or by
> > > > the same person would be a good indication the platform uses modem
> > > > ctrl lines. Or were these platforms in use before adding modem ctrl
> > > > support? Then they probably use GPIOs or nothing.
> > > >
> > > > If there are platforms which would regress if the modem ctrl feature
> > > > was just reverted, which ones are those?
> > >
> > > Ok, let me do some checks and get back to you on this.
> >
> > Hi Rob,
> > for this board:
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >
> > it uses a SC16IS740, which doesn't have any GPIOs nor modem
> > control lines, so no DT changes required.
> >
> > For these two Ingenic boards:
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> >
> > They use a SC16IS752, which has shared modem control lines and GPIOs.
> > Unfortunately, the maintainers have not (yet) responded to my
> > inquiries. Also, I tried to search for schematics or block diagrams on
> > the net but couldn't find anything.
> >
> > These platforms were in use before the patch to add the modem control
> > lines was added. Then like you said they probably use these shared
> > lines as GPIOs or nothing, so no DT changes would be required.
> 
> Okay, that's useful (please add to the commit msg).

I added the information in the cover letter, but I can add it to the
actual patch commit message if you prefer.

> Still, what platform(s) need the modem control feature? Presumably
> that's whatever platform Lech and Tomasz work on. I guess given the
> Reviewed-by they are fine with needing a DT change.

Ok. I have previously also emailed Lech about that, but he has not
responded yet.

Thank you,
Hugo.

Re: [RESEND PATCH v8 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Thu, 3 Aug 2023 17:38:54 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Thu, 3 Aug 2023 14:10:30 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Thu, Aug 3, 2023 at 11:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > >
> > > On Mon, 31 Jul 2023 14:41:15 -0400
> > > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > >
> > > > On Mon, 31 Jul 2023 12:04:45 -0600
> > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > > On Mon, Jul 31, 2023 at 10:46 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > >
> > > > > > On Mon, 31 Jul 2023 09:31:53 -0600
> > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > > On Mon, Jul 24, 2023 at 9:54 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, 22 Jul 2023 17:15:26 +0200
> > > > > > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > > On Sat, Jul 22, 2023 at 10:47:24AM -0400, Hugo Villeneuve wrote:
> > > > > > > > > > On Fri, 21 Jul 2023 13:24:19 -0600
> > > > > > > > > > Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > > On Fri, Jul 21, 2023 at 10:19 AM Hugo Villeneuve <hugo@hugovil.com> 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.
> > > > > > > > > > >
> > > > > > > > > > > Requiring a new DT property is not fixing a kernel regression. You
> > > > > > > > > > > should be returning the kernel to original behavior and then have a
> > > > > > > > > > > new DT property for new behavior.
> > > > > > > > > >
> > > > > > > > > > Hi Rob,
> > > > > > > > > > please read the entire patch history starting from V1
> > > > > > > > > >  and you will understand why this course of action was
> > > > > > > > > >  not selected.
> > > > > > > > >
> > > > > > > > > That's not going to happen, sorry, you need to explain it here, in this
> > > > > > > > > patch series, why a specific action is being taken over another one, as
> > > > > > > > > no one has time to go dig through past history, sorry.
> > > > > > > >
> > > > > > > > Hi Rob,
> > > > > > > > I initially submitted a patch to revert the kernel to original
> > > > > > > > behavior, but it created more problems because the patch was
> > > > > > > > unfortunately split in two separate patches, and mixed with other non
> > > > > > > > closely-related changes. It was also noted to me that reverting to the
> > > > > > > > old behavior would break things for some users.
> > > > > > > >
> > > > > > > > It was suggested to me by a more experienced kernel developer to
> > > > > > > > "suggest a fix, instead of hurrying a revert":
> > > > > > > >
> > > > > > > >     https://lkml.org/lkml/2023/5/17/758
> > > > > > >
> > > > > > > Do I have to go read this to decipher the justification and reasoning?
> > > > > > > When Greg says "in this patch series", he means in the commit messages
> > > > > > > of the patches. You send v9 already and it doesn't have that. The
> > > > > > > patchset needs to stand on its own summarizing any relevant prior
> > > > > > > discussions.
> > > > > > >
> > > > > > > I never suggested doing a revert.
> > > > > >
> > > > > > Hi Rob,
> > > > > > I am sorry, but this is exactly what I "deciphered" from your
> > > > > > original email.
> > > > > >
> > > > > > I am trying very hard to understand exactly what you mean, but it is
> > > > > > not that obvious for me. If something is not clear in my commit message,
> > > > > > I will try to improve it. But before, let's try to focus on making sure
> > > > > > I understand more clearly what you want exactly.
> > > > > >
> > > > > > > Obviously, someone still wants the
> > > > > > > new feature.
> > > > > >
> > > > > > I assume that you refer to the "new feature" as what was added in
> > > > > > the commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > > > > > lines")?
> > > > >
> > > > > Shrug. It's one of the 2 commits mentioned, I don't know which one
> > > > > exactly. Whichever one changed default behavior from use GPIOs to use
> > > > > modem ctrl lines.
> > > > >
> > > > > Reading it again, I *think* this patch is correct. Default behavior is
> > > > > restored to use GPIOs. The DT property is needed to enable modem ctrl
> > > > > lines.
> > > >
> > > > Hi,
> > > > this is correct.
> > > >
> > > >
> > > > > What's not okay is just saying, these platforms may or may not need an update:
> > > > >
> > > > >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> > > > >     mips/boot/dts/ingenic/cu1830-neo.dts
> > > > >     mips/boot/dts/ingenic/cu1000-neo.dts
> > > >
> > > > Yes, my bad. I initially mentioned them and hoped to get some
> > > > feedback, which I never got, and I kind of forgot about it.
> > > >
> > > > > You need to figure that out. Have you checked with maintainers of
> > > > > these boards? When were they added and by who? At the same time or by
> > > > > the same person would be a good indication the platform uses modem
> > > > > ctrl lines. Or were these platforms in use before adding modem ctrl
> > > > > support? Then they probably use GPIOs or nothing.
> > > > >
> > > > > If there are platforms which would regress if the modem ctrl feature
> > > > > was just reverted, which ones are those?
> > > >
> > > > Ok, let me do some checks and get back to you on this.
> > >
> > > Hi Rob,
> > > for this board:
> > >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> > >
> > > it uses a SC16IS740, which doesn't have any GPIOs nor modem
> > > control lines, so no DT changes required.
> > >
> > > For these two Ingenic boards:
> > >     mips/boot/dts/ingenic/cu1830-neo.dts
> > >     mips/boot/dts/ingenic/cu1000-neo.dts
> > >
> > > They use a SC16IS752, which has shared modem control lines and GPIOs.
> > > Unfortunately, the maintainers have not (yet) responded to my
> > > inquiries. Also, I tried to search for schematics or block diagrams on
> > > the net but couldn't find anything.
> > >
> > > These platforms were in use before the patch to add the modem control
> > > lines was added. Then like you said they probably use these shared
> > > lines as GPIOs or nothing, so no DT changes would be required.
> > 
> > Okay, that's useful (please add to the commit msg).
> 
> I added the information in the cover letter, but I can add it to the
> actual patch commit message if you prefer.
> 
> > Still, what platform(s) need the modem control feature? Presumably
> > that's whatever platform Lech and Tomasz work on. I guess given the
> > Reviewed-by they are fine with needing a DT change.
> 
> Ok. I have previously also emailed Lech about that, but he has not
> responded yet.

Hi Rob,
Lech just confirmed that he made the DT changes
to a board that was used only internally, and that no other DT changes
should be necessary for in-tree DTS.

Hugo.