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

[PATCH v9 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.

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

Thank you.

Link: [v0] https://lkml.org/lkml/2023/5/15/923
      [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
      [v8] https://lkml.org/lkml/2023/6/7/813
           https://lkml.org/lkml/2023/7/21/1055

Changes for V1:
- Abandonned the approach of trying to fix GPIO regression by reverting the
  original patches, because it created more problems than it solved (breaking
  existing users). Instead, add DT property to fix the GPIO behavior.

Changes for V2:
- Integrate multiple patches into this serie.

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"

Changes for V9:
- Change DT property name in commit message and move some comments to cover
  letter for patch "fix regression with GPIO configuration".
- Add proper link to pre-v1 (v0) RFC patch.
- Add changes log for V1 and V2 to this cover letter.

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: 0b5547c51827e053cc754db47d3ec3e6c2c451d2
-- 
2.30.2

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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 v9 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
"nxp,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 DT property.

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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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: [PATCH v9 02/10] serial: sc16is7xx: mark IOCONTROL register as volatile

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote:
> 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

Why 6.1.y?  What commit does this fix?

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

Is this the same as this change:
	https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/

confused,

greg k-h

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> 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")

That commit is in a very old 3.x release.

> Cc: <stable@vger.kernel.org> # 6.1.x

But you say this should only go to 6.1.y?  Why?  What is wrong with the
older kernels?

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

That's a magic value, some comment should be added here to explain why
setting all bits is ok.  Why does this work exactly?  You only say that
the max310x driver does this, but not why it does this at all.

confused,

greg k-h

Re: [PATCH v9 03/10] serial: sc16is7xx: remove obsolete out_thread label

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:35AM -0400, Hugo Villeneuve wrote:
> 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>

Why is this not ok for stable kernels yet it has a Fixes: tag?

Please fix.

thanks,

greg k-h

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

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> In preparation for upcoming patch "fix regression with GPIO
> configuration". To facilitate review and make code more modular.

I would much rather the issue be fixed _before_ the code is refactored,
unless it is impossible to fix it without the refactor?

> 
> Cc: <stable@vger.kernel.org> # 6.1.x

What commit id does this fix?

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

Why is this pointer needed?

Why is it grabbed and yet the reference count is never incremented?  Who
owns the reference count and when will it go away?

And what device is this?  The parent?  Current device?  What type of
device is it?  And why is it needed?

Using "raw" devices is almost never something a driver should do, they
are only passed into functions by the driver core, but then the driver
should instantly turn them into the "real" structure.


thanks,

greg k-h

Re: [PATCH v9 10/10] serial: sc16is7xx: improve comments about variants

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:42AM -0400, Hugo Villeneuve wrote:
> 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>

You have now mixed a patch series full of commits that are to be
backported to stable kernels (i.e. fixes) and general changes that do
not need to be.

Please make these two separate patch series, you can have one depend on
the other, but I can't apply them both to the "for Linus" branch as
obviously they are not all fixes nor need to go to Linus now.

thanks,

greg k-h

Re: [PATCH v9 09/10] serial: sc16is7xx: add post reset delay

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Make sure we wait at least 3us before initiating communication with
> the device after reset.

That says what you do, but not _why_ you do it?

Please read the kernel documentation again for how to write a good
changelog text.  It's usually the hardest part of submitting a patch.

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

5, 10 is NOT 3us.

:(

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

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> changed the function of the GPIOs pins to act as modem control
> lines without any possibility of selecting GPIO function.
> 
> As a consequence, applications that depends on GPIO lines configured
> by default as GPIO pins no longer work as expected.
> 
> Also, the change to select modem control lines function was done only
> for channel A of dual UART variants (752/762). This was not documented
> in the log message.
> 
> Allow to specify GPIO or modem control line function in the device
> tree, and for each of the ports (A or B).
> 
> Do so by using the new device-tree property named
> "nxp,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 DT property.
> 
> 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

Where are these git commit ids from?  I don't see them in Linus's tree,
how are they supposed to be picked up by the stable developers if they
are not valid ones?

confused,

greg k-h

Re: [PATCH v9 08/10] serial: sc16is7xx: add call to get rs485 DT flags and properties

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:40AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Add call to uart_get_rs485_mode() to probe for RS485 flags and
> properties from device tree.

Again, you are saying what you are doing, but not why.  I have no hint
as to if this is a bugfix, or a new features, or something else?

thanks,

greg k-h

Re: [PATCH v9 02/10] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:50:40 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote:
> > 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
> 
> Why 6.1.y?  What commit does this fix?
> 
> > 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;
> 
> Is this the same as this change:
> 	https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/
> 
> confused,

Hi Greg,
yes this is the same.

You simply accepted an exact equivalent of my patch by someone else in
your tree, no confusion there.

I will remove this patch from my series and rebase it on your tree
gregkh_tty/tty-next.

Hugo.

Re: [PATCH v9 09/10] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:57:50 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Make sure we wait at least 3us before initiating communication with
> > the device after reset.
> 
> That says what you do, but not _why_ you do it?

You are right, it is not clear. I will add a note that it is
recommended to do so by the manufacturer in the device datasheet.

> Please read the kernel documentation again for how to write a good
> changelog text.  It's usually the hardest part of submitting a patch.

Yes.


> > 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);
> 
> 5, 10 is NOT 3us.

In v3, Andy Shevchenko suggested "I would put (5, 10) instead to relax
a bit the scheduler."

Should I add a comment to that effect:

/*
 * After reset, the datasheet indicates that the host must wait at least
 * 3us before initializing a communication with the device.
 * Use (5, 10) range to relax the scheduler.
 */

?

Hugo.

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:52:26 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > 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")
> 
> That commit is in a very old 3.x release.
> 
> > Cc: <stable@vger.kernel.org> # 6.1.x
> 
> But you say this should only go to 6.1.y?  Why?  What is wrong with the
> older kernels?

Hi Greg,
I have read (and reread a couple of times)
Documentation/process/stable-kernel-rules.rst to try to understand how
to format the tags, but unfortunately it doesn't contain "Everything
you ever wanted to know about Linux -stable releases" as the title
claims :)

In particular, it doesn't explain or advise which older version we
should target, that is why since I was not sure I specified 6.1.y
because I could test it properly, but not v3.x.

Maybe it would be best to simply drop for now all the "Cc:
<stable@vger.kernel.org>" tags for this series, and following Option 2,
I send an email to stable@vger.kernel.org once the patches have been
merged to Linus' 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 | 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;
> 
> That's a magic value, some comment should be added here to explain why
> setting all bits is ok.  Why does this work exactly?  You only say that
> the max310x driver does this, but not why it does this at all.

I do not understand, because my commit log message is quite long
and, it seems to me, well documenting why this works the way it
does when calling uart_configure_port() in serial_core.c?

I say that the max310x driver also does this, because there is also no
comment in the max310x driver for using the (void __iomem *)~0;
construct. I also located the original commit message for the max310x
driver but no comments were usefull there also.

So, what about adding this comment:

/*
 * Use all ones as membase to make sure uart_configure_port() in
 * serial_core.c does not abort for SPI/I2C devices where the
 * membase address is not applicable.
 */
 s->p[i].port.membase	= (void __iomem *)~0;


Also, keep in mind that in the original discussion about that patch,
there was the following comment from Ilpo Järvinen:

------
This changelog was really well describing the problem! :-)
Yeah, some other solution should be devised. Maybe we should add
another .iotype for thse kind of devices. But I'm fine with this for
this fix. After editing the changelog, feel free to add:
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
------

If wou want, I could also add the same comment to the max310 driver?

Hugo.

Re: [PATCH v9 03/10] serial: sc16is7xx: remove obsolete out_thread label

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:53:10 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:35AM -0400, Hugo Villeneuve wrote:
> > 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>
> 
> Why is this not ok for stable kernels yet it has a Fixes: tag?
> 
> Please fix.
> 
> thanks,
> 
> greg k-h

Hi,
this is a somewhat particular case. It is a change that "fixes" some
previously unseen consequence in original commit, but that does not
result in any binary change in the end. That is why I decided not to
put in a "stable" tag.

If you want, maybe it would be simpler to remove the "Fixes" tag? I
originally put this tag to have a reference to the original commit, but
since it is already mentioned in the commit log message body, it can be
removed.

Hugo.

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Greg KH]

On Tue, Aug 01, 2023 at 01:16:55PM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:52:26 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > 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")
> > 
> > That commit is in a very old 3.x release.
> > 
> > > Cc: <stable@vger.kernel.org> # 6.1.x
> > 
> > But you say this should only go to 6.1.y?  Why?  What is wrong with the
> > older kernels?
> 
> Hi Greg,
> I have read (and reread a couple of times)
> Documentation/process/stable-kernel-rules.rst to try to understand how
> to format the tags, but unfortunately it doesn't contain "Everything
> you ever wanted to know about Linux -stable releases" as the title
> claims :)
> 
> In particular, it doesn't explain or advise which older version we
> should target, that is why since I was not sure I specified 6.1.y
> because I could test it properly, but not v3.x.

If you think this fixes an issue back to 3.x, then just leave it at
that, there's no need to have to test all of these.  If when I apply the
patch to the stable trees, and it does not go back to all of the
active versions specified by Fixes: then you will get an email saying
so and can handle it then if you want to.

> Maybe it would be best to simply drop for now all the "Cc:
> <stable@vger.kernel.org>" tags for this series, and following Option 2,
> I send an email to stable@vger.kernel.org once the patches have been
> merged to Linus' tree?

That will just mean more work for both of us, leave it as is, just drop
the "# 6.1.x" portion please.

> > > 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;
> > 
> > That's a magic value, some comment should be added here to explain why
> > setting all bits is ok.  Why does this work exactly?  You only say that
> > the max310x driver does this, but not why it does this at all.
> 
> I do not understand, because my commit log message is quite long
> and, it seems to me, well documenting why this works the way it
> does when calling uart_configure_port() in serial_core.c?
> 
> I say that the max310x driver also does this, because there is also no
> comment in the max310x driver for using the (void __iomem *)~0;
> construct. I also located the original commit message for the max310x
> driver but no comments were usefull there also.
> 
> So, what about adding this comment:
> 
> /*
>  * Use all ones as membase to make sure uart_configure_port() in
>  * serial_core.c does not abort for SPI/I2C devices where the
>  * membase address is not applicable.
>  */
>  s->p[i].port.membase	= (void __iomem *)~0;

Yes, that would be good, thank you.

> If wou want, I could also add the same comment to the max310 driver?

Yes please.

thanks,

greg k-h

Re: [PATCH v9 03/10] serial: sc16is7xx: remove obsolete out_thread label

[Greg KH]

On Tue, Aug 01, 2023 at 01:29:59PM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:53:10 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:35AM -0400, Hugo Villeneuve wrote:
> > > 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>
> > 
> > Why is this not ok for stable kernels yet it has a Fixes: tag?
> > 
> > Please fix.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi,
> this is a somewhat particular case. It is a change that "fixes" some
> previously unseen consequence in original commit, but that does not
> result in any binary change in the end. That is why I decided not to
> put in a "stable" tag.
> 
> If you want, maybe it would be simpler to remove the "Fixes" tag? I
> originally put this tag to have a reference to the original commit, but
> since it is already mentioned in the commit log message body, it can be
> removed.

Yes, please just remove it, otherwise it will just confuse us.

thanks,

greg k-h

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

On Thu, 3 Aug 2023 09:54:37 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Aug 01, 2023 at 01:16:55PM -0400, Hugo Villeneuve wrote:
> > On Mon, 31 Jul 2023 17:52:26 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > > 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")
> > > 
> > > That commit is in a very old 3.x release.
> > > 
> > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > 
> > > But you say this should only go to 6.1.y?  Why?  What is wrong with the
> > > older kernels?
> > 
> > Hi Greg,
> > I have read (and reread a couple of times)
> > Documentation/process/stable-kernel-rules.rst to try to understand how
> > to format the tags, but unfortunately it doesn't contain "Everything
> > you ever wanted to know about Linux -stable releases" as the title
> > claims :)
> > 
> > In particular, it doesn't explain or advise which older version we
> > should target, that is why since I was not sure I specified 6.1.y
> > because I could test it properly, but not v3.x.
> 
> If you think this fixes an issue back to 3.x, then just leave it at
> that, there's no need to have to test all of these.  If when I apply the
> patch to the stable trees, and it does not go back to all of the
> active versions specified by Fixes: then you will get an email saying
> so and can handle it then if you want to.
> 
> > Maybe it would be best to simply drop for now all the "Cc:
> > <stable@vger.kernel.org>" tags for this series, and following Option 2,
> > I send an email to stable@vger.kernel.org once the patches have been
> > merged to Linus' tree?
> 
> That will just mean more work for both of us, leave it as is, just drop
> the "# 6.1.x" portion please.
> 
> > > > 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;
> > > 
> > > That's a magic value, some comment should be added here to explain why
> > > setting all bits is ok.  Why does this work exactly?  You only say that
> > > the max310x driver does this, but not why it does this at all.
> > 
> > I do not understand, because my commit log message is quite long
> > and, it seems to me, well documenting why this works the way it
> > does when calling uart_configure_port() in serial_core.c?
> > 
> > I say that the max310x driver also does this, because there is also no
> > comment in the max310x driver for using the (void __iomem *)~0;
> > construct. I also located the original commit message for the max310x
> > driver but no comments were usefull there also.
> > 
> > So, what about adding this comment:
> > 
> > /*
> >  * Use all ones as membase to make sure uart_configure_port() in
> >  * serial_core.c does not abort for SPI/I2C devices where the
> >  * membase address is not applicable.
> >  */
> >  s->p[i].port.membase	= (void __iomem *)~0;
> 
> Yes, that would be good, thank you.
> 
> > If wou want, I could also add the same comment to the max310 driver?
> 
> Yes please.

Hi Greg,
I will send a separate patch specifically for this.

Thank you, Hugo.

Re: [PATCH v9 10/10] serial: sc16is7xx: improve comments about variants

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:56:53 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:42AM -0400, Hugo Villeneuve wrote:
> > 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>
> 
> You have now mixed a patch series full of commits that are to be
> backported to stable kernels (i.e. fixes) and general changes that do
> not need to be.
> 
> Please make these two separate patch series, you can have one depend on
> the other, but I can't apply them both to the "for Linus" branch as
> obviously they are not all fixes nor need to go to Linus now.
> 
> thanks,
> 
> greg k-h

Hi,
Ok, will do.

Thank you,
Hugo.

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

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:58:41 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> > 
> > Allow to specify GPIO or modem control line function in the device
> > tree, and for each of the ports (A or B).
> > 
> > Do so by using the new device-tree property named
> > "nxp,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 DT property.
> > 
> > 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
> 
> Where are these git commit ids from?  I don't see them in Linus's tree,
> how are they supposed to be picked up by the stable developers if they
> are not valid ones?
> 
> confused,
> 
> greg k-h

Hi Greg,
once again, I simply misinterpreted stable-kernel-rules.rst.

I wrongly assumed that, for example, this patch had, as a prerequisite,
all the patches before it in this series, and that is why I listed
them.

So I will remove them all, since this patch doesn't have any other
requisites other than the previous patches in this series.

Maybe it would be good to add some notes about that in
stable-kernel-rules.rst?

Thank you, Hugo.

Re: [PATCH v9 08/10] serial: sc16is7xx: add call to get rs485 DT flags and properties

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:59:14 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:40AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Add call to uart_get_rs485_mode() to probe for RS485 flags and
> > properties from device tree.
> 
> Again, you are saying what you are doing, but not why.  I have no hint
> as to if this is a bugfix, or a new features, or something else?
> 
> thanks,
> 
> greg k-h

Hi Greg,
I could change the commit message to:

---------
serial: sc16is7xx: add missing support for rs485 devicetree properties

Retrieve rs485 devicetree properties on registration of sc16is7xx ports
in case they are attached to an rs485 transceiver.
---------

I don't think that it should be considered as a bug fix, but maybe as a
missing feature.

And does it mean that it should also go to older (stable) kernels then?
If yes, then do I need to add the "Fixes" tag?

Thank you,
Hugo.

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

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:55:42 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > In preparation for upcoming patch "fix regression with GPIO
> > configuration". To facilitate review and make code more modular.
> 
> I would much rather the issue be fixed _before_ the code is refactored,
> unless it is impossible to fix it without the refactor?

Hi Greg,
normally I would agree, but the refactor in this case helps a lot to
address some issues raised by you and Andy in V7 of this series.

Maybe I could merge it with the actual patch "fix regression with GPIO
configuration"?


> > Cc: <stable@vger.kernel.org> # 6.1.x
> 
> What commit id does this fix?

It doesn't fix anything, but I tought that I needed this tag since
this patch is a prerequisite for the next patch in the series, which
would be applied to stable kernels. I will remove this tag (assuming
the patch stays as it is, depending on your answer to the above
question).

 
> > 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;
> 
> Why is this pointer needed?
> 
> Why is it grabbed and yet the reference count is never incremented?  Who
> owns the reference count and when will it go away?
> 
> And what device is this?  The parent?  Current device?  What type of
> device is it?  And why is it needed?
> 
> Using "raw" devices is almost never something a driver should do, they
> are only passed into functions by the driver core, but then the driver
> should instantly turn them into the "real" structure.

We already discussed that a lot in previous versions (v7)... I am
trying my best to modify the code to address your concerns, but I am
not fully understanding what you mean about raw devices, and you didn't
answer some of my previous questions/interrogations in v7 about that.

So, in the new function that I
need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
a raw device to read a device tree property and to set
s->gpio.parent:

    count = device_property_count_u32(dev, ...
    ...
    s->gpio.parent = dev;

Do we agree on that?

Then, how do I pass this raw device to the 
device_property_count_u32() function and to the s->gpio.parent
assignment?

Should I modify sc16is7xx_setup_gpio_chip() like so:

    static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
    {
	struct device *dev = &s->p[0].port.dev;

        count = device_property_count_u32(dev, ...
        ...
        s->gpio.parent = dev;

?

If not, can you show me how you would like to do it to avoid me trying
to guess?

Thank you,
Hugo.

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

[Andy Shevchenko]

On Thu, Aug 3, 2023 at 5:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Mon, 31 Jul 2023 17:58:41 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:

...

> > > 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
> >
> > Where are these git commit ids from?  I don't see them in Linus's tree,
> > how are they supposed to be picked up by the stable developers if they
> > are not valid ones?
> >
> > confused,

...

> I wrongly assumed that, for example, this patch had, as a prerequisite,
> all the patches before it in this series, and that is why I listed
> them.

The problem, as I understand it, is not that you listed them (how else
will the backporter know that this patch requires something else?) but
the format (you used wrong SHA-1 sums).

...

> So I will remove them all, since this patch doesn't have any other
> requisites other than the previous patches in this series.
>
> Maybe it would be good to add some notes about that in
> stable-kernel-rules.rst?

This probably is a good idea. Briefly looking at it I see no examples
like yours there.

-- 
With Best Regards,
Andy Shevchenko

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

[Greg KH]

On Fri, Aug 04, 2023 at 12:04:29AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 3, 2023 at 5:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Mon, 31 Jul 2023 17:58:41 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:
> 
> ...
> 
> > > > 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
> > >
> > > Where are these git commit ids from?  I don't see them in Linus's tree,
> > > how are they supposed to be picked up by the stable developers if they
> > > are not valid ones?
> > >
> > > confused,
> 
> ...
> 
> > I wrongly assumed that, for example, this patch had, as a prerequisite,
> > all the patches before it in this series, and that is why I listed
> > them.

That's fine, but if you have already marked those patches for stable
inclusion, no need to list them here too.

> The problem, as I understand it, is not that you listed them (how else
> will the backporter know that this patch requires something else?) but
> the format (you used wrong SHA-1 sums).

Exactly, those are invalid sha1 values.

> > So I will remove them all, since this patch doesn't have any other
> > requisites other than the previous patches in this series.
> >
> > Maybe it would be good to add some notes about that in
> > stable-kernel-rules.rst?
> 
> This probably is a good idea. Briefly looking at it I see no examples
> like yours there.

Because it's not a thing?  Just mark all of these patches in the series
as cc: stable@ and all will happen automatically for you.  Nothing
fancy or complex here, happens daily in other subsystems just fine :)

thanks,

greg k-h

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

[Greg KH]

On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:55:42 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > In preparation for upcoming patch "fix regression with GPIO
> > > configuration". To facilitate review and make code more modular.
> > 
> > I would much rather the issue be fixed _before_ the code is refactored,
> > unless it is impossible to fix it without the refactor?
> 
> Hi Greg,
> normally I would agree, but the refactor in this case helps a lot to
> address some issues raised by you and Andy in V7 of this series.
> 
> Maybe I could merge it with the actual patch "fix regression with GPIO
> configuration"?

Sure.

> > > Cc: <stable@vger.kernel.org> # 6.1.x
> > 
> > What commit id does this fix?
> 
> It doesn't fix anything, but I tought that I needed this tag since
> this patch is a prerequisite for the next patch in the series, which
> would be applied to stable kernels. I will remove this tag (assuming
> the patch stays as it is, depending on your answer to the above
> question).
> 
>  
> > > 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;
> > 
> > Why is this pointer needed?
> > 
> > Why is it grabbed and yet the reference count is never incremented?  Who
> > owns the reference count and when will it go away?
> > 
> > And what device is this?  The parent?  Current device?  What type of
> > device is it?  And why is it needed?
> > 
> > Using "raw" devices is almost never something a driver should do, they
> > are only passed into functions by the driver core, but then the driver
> > should instantly turn them into the "real" structure.
> 
> We already discussed that a lot in previous versions (v7)... I am
> trying my best to modify the code to address your concerns, but I am
> not fully understanding what you mean about raw devices, and you didn't
> answer some of my previous questions/interrogations in v7 about that.

I don't have time to answer all questions, sorry.

Please help review submitted patches to reduce my load and allow me to
answer other stuff :)

> So, in the new function that I
> need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> a raw device to read a device tree property and to set
> s->gpio.parent:
> 
>     count = device_property_count_u32(dev, ...
>     ...
>     s->gpio.parent = dev;
> 
> Do we agree on that?

Yes, but what type of parent is that?

> Then, how do I pass this raw device to the 
> device_property_count_u32() function and to the s->gpio.parent
> assignment?
> 
> Should I modify sc16is7xx_setup_gpio_chip() like so:
> 
>     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
>     {
> 	struct device *dev = &s->p[0].port.dev;
> 
>         count = device_property_count_u32(dev, ...
>         ...
>         s->gpio.parent = dev;

Again, what is the real type of that parent?  It's a port, right, so
pass in the port to this function and then do the "take the struct
device of the port" at that point in time.

thanks,

greg k-h

Re: [PATCH v9 08/10] serial: sc16is7xx: add call to get rs485 DT flags and properties

[Greg KH]

On Thu, Aug 03, 2023 at 10:38:14AM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:59:14 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:40AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > Add call to uart_get_rs485_mode() to probe for RS485 flags and
> > > properties from device tree.
> > 
> > Again, you are saying what you are doing, but not why.  I have no hint
> > as to if this is a bugfix, or a new features, or something else?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> I could change the commit message to:
> 
> ---------
> serial: sc16is7xx: add missing support for rs485 devicetree properties
> 
> Retrieve rs485 devicetree properties on registration of sc16is7xx ports
> in case they are attached to an rs485 transceiver.
> ---------
> 
> I don't think that it should be considered as a bug fix, but maybe as a
> missing feature.
> 
> And does it mean that it should also go to older (stable) kernels then?
> If yes, then do I need to add the "Fixes" tag?

Does it fix a problem?  If so, yes, it should go to older kernels.  If
not, then no.

thanks,

greg k-h

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

[Hugo Villeneuve]

On Fri, 4 Aug 2023 15:14:18 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > On Mon, 31 Jul 2023 17:55:42 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > In preparation for upcoming patch "fix regression with GPIO
> > > > configuration". To facilitate review and make code more modular.
> > > 
> > > I would much rather the issue be fixed _before_ the code is refactored,
> > > unless it is impossible to fix it without the refactor?
> > 
> > Hi Greg,
> > normally I would agree, but the refactor in this case helps a lot to
> > address some issues raised by you and Andy in V7 of this series.
> > 
> > Maybe I could merge it with the actual patch "fix regression with GPIO
> > configuration"?
> 
> Sure.

Hi Greg,
will do.

 
> > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > 
> > > What commit id does this fix?
> > 
> > It doesn't fix anything, but I tought that I needed this tag since
> > this patch is a prerequisite for the next patch in the series, which
> > would be applied to stable kernels. I will remove this tag (assuming
> > the patch stays as it is, depending on your answer to the above
> > question).
> > 
> >  
> > > > 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;
> > > 
> > > Why is this pointer needed?
> > > 
> > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > owns the reference count and when will it go away?
> > > 
> > > And what device is this?  The parent?  Current device?  What type of
> > > device is it?  And why is it needed?
> > > 
> > > Using "raw" devices is almost never something a driver should do, they
> > > are only passed into functions by the driver core, but then the driver
> > > should instantly turn them into the "real" structure.
> > 
> > We already discussed that a lot in previous versions (v7)... I am
> > trying my best to modify the code to address your concerns, but I am
> > not fully understanding what you mean about raw devices, and you didn't
> > answer some of my previous questions/interrogations in v7 about that.
> 
> I don't have time to answer all questions, sorry.
> 
> Please help review submitted patches to reduce my load and allow me to
> answer other stuff :)

Ok.


> > So, in the new function that I
> > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > a raw device to read a device tree property and to set
> > s->gpio.parent:
> > 
> >     count = device_property_count_u32(dev, ...
> >     ...
> >     s->gpio.parent = dev;
> > 
> > Do we agree on that?
> 
> Yes, but what type of parent is that?

I am confused by your question. I do not understand why the type of
parent matters... And what do you call the parent: s, s->gpio or
s->gpio.parent?

For me, the way I understand it, the only question that matters is how I
can extract the raw device structure pointer from maybe "struct
sc16is7xx_port" or some other structure, and then use it in my
new function...

I should not have put "s->gpio.parent = dev" in the example, I think it
just complexifies things. Lets start over with a more simple example and
only:

    count = device_property_count_u32(dev, ...


> > Then, how do I pass this raw device to the 
> > device_property_count_u32() function and to the s->gpio.parent
> > assignment?
> > 
> > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > 
> >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> >     {
> > 	struct device *dev = &s->p[0].port.dev;
> > 
> >         count = device_property_count_u32(dev, ...
> >         ...
> >         s->gpio.parent = dev;
> 
> Again, what is the real type of that parent?  It's a port, right, so
> pass in the port to this function and then do the "take the struct
> device of the port" at that point in time.

With the simplified example, is the following ok:

static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
{
    struct device *dev = &s->p[0].port.dev;

    count = device_property_count_u32(dev, ...
    ...
}

If not, please indicate how you would do it with an actual example...

Thank you,
Hugo.

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

[Greg KH]

On Fri, Aug 04, 2023 at 10:15:54AM -0400, Hugo Villeneuve wrote:
> On Fri, 4 Aug 2023 15:14:18 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > > On Mon, 31 Jul 2023 17:55:42 +0200
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > 
> > > > > In preparation for upcoming patch "fix regression with GPIO
> > > > > configuration". To facilitate review and make code more modular.
> > > > 
> > > > I would much rather the issue be fixed _before_ the code is refactored,
> > > > unless it is impossible to fix it without the refactor?
> > > 
> > > Hi Greg,
> > > normally I would agree, but the refactor in this case helps a lot to
> > > address some issues raised by you and Andy in V7 of this series.
> > > 
> > > Maybe I could merge it with the actual patch "fix regression with GPIO
> > > configuration"?
> > 
> > Sure.
> 
> Hi Greg,
> will do.
> 
>  
> > > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > 
> > > > What commit id does this fix?
> > > 
> > > It doesn't fix anything, but I tought that I needed this tag since
> > > this patch is a prerequisite for the next patch in the series, which
> > > would be applied to stable kernels. I will remove this tag (assuming
> > > the patch stays as it is, depending on your answer to the above
> > > question).
> > > 
> > >  
> > > > > 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;
> > > > 
> > > > Why is this pointer needed?
> > > > 
> > > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > > owns the reference count and when will it go away?
> > > > 
> > > > And what device is this?  The parent?  Current device?  What type of
> > > > device is it?  And why is it needed?
> > > > 
> > > > Using "raw" devices is almost never something a driver should do, they
> > > > are only passed into functions by the driver core, but then the driver
> > > > should instantly turn them into the "real" structure.
> > > 
> > > We already discussed that a lot in previous versions (v7)... I am
> > > trying my best to modify the code to address your concerns, but I am
> > > not fully understanding what you mean about raw devices, and you didn't
> > > answer some of my previous questions/interrogations in v7 about that.
> > 
> > I don't have time to answer all questions, sorry.
> > 
> > Please help review submitted patches to reduce my load and allow me to
> > answer other stuff :)
> 
> Ok.
> 
> 
> > > So, in the new function that I
> > > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > > a raw device to read a device tree property and to set
> > > s->gpio.parent:
> > > 
> > >     count = device_property_count_u32(dev, ...
> > >     ...
> > >     s->gpio.parent = dev;
> > > 
> > > Do we agree on that?
> > 
> > Yes, but what type of parent is that?
> 
> I am confused by your question. I do not understand why the type of
> parent matters... And what do you call the parent: s, s->gpio or
> s->gpio.parent?
> 
> For me, the way I understand it, the only question that matters is how I
> can extract the raw device structure pointer from maybe "struct
> sc16is7xx_port" or some other structure, and then use it in my
> new function...
> 
> I should not have put "s->gpio.parent = dev" in the example, I think it
> just complexifies things. Lets start over with a more simple example and
> only:
> 
>     count = device_property_count_u32(dev, ...
> 
> 
> > > Then, how do I pass this raw device to the 
> > > device_property_count_u32() function and to the s->gpio.parent
> > > assignment?
> > > 
> > > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > > 
> > >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > >     {
> > > 	struct device *dev = &s->p[0].port.dev;
> > > 
> > >         count = device_property_count_u32(dev, ...
> > >         ...
> > >         s->gpio.parent = dev;
> > 
> > Again, what is the real type of that parent?  It's a port, right, so
> > pass in the port to this function and then do the "take the struct
> > device of the port" at that point in time.
> 
> With the simplified example, is the following ok:
> 
> static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> {
>     struct device *dev = &s->p[0].port.dev;
> 
>     count = device_property_count_u32(dev, ...
>     ...
> }
> 
> If not, please indicate how you would do it with an actual example...

At this point, after reviewing 500+ patches today, I really have no
idea, my brain is fried.  Do what you think is right here and submit a
new series and I'll be glad to review it.

thanks,

greg k-h

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

[Hugo Villeneuve]

On Fri, 4 Aug 2023 17:09:13 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Aug 04, 2023 at 10:15:54AM -0400, Hugo Villeneuve wrote:
> > On Fri, 4 Aug 2023 15:14:18 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > > > On Mon, 31 Jul 2023 17:55:42 +0200
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > 
> > > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > > 
> > > > > > In preparation for upcoming patch "fix regression with GPIO
> > > > > > configuration". To facilitate review and make code more modular.
> > > > > 
> > > > > I would much rather the issue be fixed _before_ the code is refactored,
> > > > > unless it is impossible to fix it without the refactor?
> > > > 
> > > > Hi Greg,
> > > > normally I would agree, but the refactor in this case helps a lot to
> > > > address some issues raised by you and Andy in V7 of this series.
> > > > 
> > > > Maybe I could merge it with the actual patch "fix regression with GPIO
> > > > configuration"?
> > > 
> > > Sure.
> > 
> > Hi Greg,
> > will do.
> > 
> >  
> > > > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > > 
> > > > > What commit id does this fix?
> > > > 
> > > > It doesn't fix anything, but I tought that I needed this tag since
> > > > this patch is a prerequisite for the next patch in the series, which
> > > > would be applied to stable kernels. I will remove this tag (assuming
> > > > the patch stays as it is, depending on your answer to the above
> > > > question).
> > > > 
> > > >  
> > > > > > 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;
> > > > > 
> > > > > Why is this pointer needed?
> > > > > 
> > > > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > > > owns the reference count and when will it go away?
> > > > > 
> > > > > And what device is this?  The parent?  Current device?  What type of
> > > > > device is it?  And why is it needed?
> > > > > 
> > > > > Using "raw" devices is almost never something a driver should do, they
> > > > > are only passed into functions by the driver core, but then the driver
> > > > > should instantly turn them into the "real" structure.
> > > > 
> > > > We already discussed that a lot in previous versions (v7)... I am
> > > > trying my best to modify the code to address your concerns, but I am
> > > > not fully understanding what you mean about raw devices, and you didn't
> > > > answer some of my previous questions/interrogations in v7 about that.
> > > 
> > > I don't have time to answer all questions, sorry.
> > > 
> > > Please help review submitted patches to reduce my load and allow me to
> > > answer other stuff :)
> > 
> > Ok.
> > 
> > 
> > > > So, in the new function that I
> > > > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > > > a raw device to read a device tree property and to set
> > > > s->gpio.parent:
> > > > 
> > > >     count = device_property_count_u32(dev, ...
> > > >     ...
> > > >     s->gpio.parent = dev;
> > > > 
> > > > Do we agree on that?
> > > 
> > > Yes, but what type of parent is that?
> > 
> > I am confused by your question. I do not understand why the type of
> > parent matters... And what do you call the parent: s, s->gpio or
> > s->gpio.parent?
> > 
> > For me, the way I understand it, the only question that matters is how I
> > can extract the raw device structure pointer from maybe "struct
> > sc16is7xx_port" or some other structure, and then use it in my
> > new function...
> > 
> > I should not have put "s->gpio.parent = dev" in the example, I think it
> > just complexifies things. Lets start over with a more simple example and
> > only:
> > 
> >     count = device_property_count_u32(dev, ...
> > 
> > 
> > > > Then, how do I pass this raw device to the 
> > > > device_property_count_u32() function and to the s->gpio.parent
> > > > assignment?
> > > > 
> > > > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > > > 
> > > >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > > >     {
> > > > 	struct device *dev = &s->p[0].port.dev;
> > > > 
> > > >         count = device_property_count_u32(dev, ...
> > > >         ...
> > > >         s->gpio.parent = dev;
> > > 
> > > Again, what is the real type of that parent?  It's a port, right, so
> > > pass in the port to this function and then do the "take the struct
> > > device of the port" at that point in time.
> > 
> > With the simplified example, is the following ok:
> > 
> > static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > {
> >     struct device *dev = &s->p[0].port.dev;
> > 
> >     count = device_property_count_u32(dev, ...
> >     ...
> > }
> > 
> > If not, please indicate how you would do it with an actual example...
> 
> At this point, after reviewing 500+ patches today, I really have no
> idea, my brain is fried.  Do what you think is right here and submit a
> new series and I'll be glad to review it.

Ok :)

Will do.

Hugo.