[PATCH v3 00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[PATCH v3 00/11] 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.

Patches 1 and 2 are simple comments fix/improvements.

Patch 3 fixes an issue when debugging IOcontrol register. After testing the GPIO
regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
this patch is also necessary for having the correct IOcontrol register values.

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

Patch 5 fixes an issue with init of first port during probing. This commit
brings some questions and I appreciate if people from the serial subsystem could
comment on my proposed solution.

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

Patch 7, 8 and 9 fix a GPIO regression by (re)allowing to choose GPIO function for
GPIO pins shared with modem status lines.

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

Patch 11 add a custom dump function as relying on regmal debugfs is not really
practical for this driver.

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

Thank you.

Link: [v1] https://lkml.org/lkml/2023/5/17/967
      [v1] https://lkml.org/lkml/2023/5/17/777
      [v1] https://lkml.org/lkml/2023/5/17/780
      [v1] https://lkml.org/lkml/2023/5/17/785
      [v1] https://lkml.org/lkml/2023/5/17/1311
      [v2] https://lkml.org/lkml/2023/5/18/516

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

Hugo Villeneuve (11):
  serial: sc16is7xx: fix syntax error in comments
  serial: sc16is7xx: improve comments about variants
  serial: sc16is7xx: mark IOCONTROL register as volatile
  serial: sc16is7xx: add post reset delay
  serial: sc16is7xx: fix broken port 0 uart init
  serial: sc16is7xx: fix bug when first setting GPIO direction
  dt-bindings: sc16is7xx: Add property to change GPIO function
  serial: sc16is7xx: fix regression with GPIO configuration
  serial: sc16is7xx: add I/O register translation offset
  serial: sc16is7xx: add call to get rs485 DT flags and properties
  serial: sc16is7xx: add dump registers function

 .../bindings/serial/nxp,sc16is7xx.txt         |  46 +++++
 drivers/tty/serial/sc16is7xx.c                | 180 +++++++++++++++---
 2 files changed, 199 insertions(+), 27 deletions(-)


base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096
-- 
2.30.2

[PATCH v3 01/11] serial: sc16is7xx: fix syntax error in comments

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

cotroller -> controller

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index abad091baeea..5bd98e4316f5 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1501,7 +1501,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 #ifdef CONFIG_GPIOLIB
 	if (devtype->nr_gpio) {
-		/* Setup GPIO cotroller */
+		/* Setup GPIO controller */
 		s->gpio.owner		 = THIS_MODULE;
 		s->gpio.parent		 = dev;
 		s->gpio.label		 = dev_name(dev);
-- 
2.30.2

[PATCH v3 02/11] serial: sc16is7xx: improve comments about variants

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5bd98e4316f5..00054bb49780 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_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
 #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
@@ -248,9 +248,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

[PATCH v3 03/11] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

Fixes: dfeae619d781 ("serial: sc16is7xx")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 00054bb49780..a7c4da3cfd2b 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 v3 04/11] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a7c4da3cfd2b..af7e66db54b4 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1428,6 +1428,12 @@ static int sc16is7xx_probe(struct device *dev,
 	regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
 			SC16IS7XX_IOCONTROL_SRESET_BIT);
 
+	/*
+	 * After reset, the host must wait at least 3us before initializing a
+	 * communication with the device.
+	 */
+	usleep_range(3, 5);
+
 	for (i = 0; i < devtype->nr_uart; ++i) {
 		s->p[i].line		= i;
 		/* Initialize port data */
-- 
2.30.2

[PATCH v3 05/11] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

While experimenting with rs485 configuration on a SC16IS752 dual UART,
I found that the sc16is7xx_config_rs485() function was 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. But I found that the max310x driver sets
port->membase to ~0 (all ones). By implementing the same change in our
driver, uart_configure_port() is now correctly executed.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
I am not sure if this change is the best long-term solution to this
problem, and maybe uart_configure_port() itself could be modified to
take into account the fact that some devices have all three *base
values set to zero?

Also, many drivers use port->iobase as an index, is it the correct way
to use it?

For example, for our driver, there was
commit 5da6b1c079e6 ("sc16is7xx: Set iobase to device index") with the
following explanation:
    "Set the .iobase value to the relative index within the device to allow
    infering the order through sysfs."

 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 af7e66db54b4..8a2fc6f89d36 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1443,6 +1443,7 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
 		s->p[i].port.iobase	= i;
+		s->p[i].port.membase	= (void __iomem *)~0;
 		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
-- 
2.30.2

[PATCH v3 06/11] serial: sc16is7xx: fix bug when first setting GPIO direction

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

When we want to configure 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
c16is7xx_gpio_direction_output() for the first time, we first set the
desired value in IOSTATE, and then we configure the pin as an output.
The datasheet states that writing to IOSTATE register will trigger a
transfer of the value to the I/O pin configured as output, so if the
pin is configured as an input, nothing will be transferred.

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

This is what is done in NXP application note AN10587.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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

[PATCH v3 07/11] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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

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

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

[PATCH v3 08/11] 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.

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

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

We also now reduce the number of exported GPIOs according to the
modem-status-line-port DT property.

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

Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a5d8af0f6da0..97ec532a0a19 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@
 
 /* IOControl register bits (Only 75x/76x) */
 #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
-#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
+#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
+#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
 #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
 
 /* EFCR register bits */
@@ -301,12 +302,12 @@
 /* Misc definitions */
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_REG_SHIFT		2
+#define SC16IS7XX_GPIOS_PER_BANK	4
 
 struct sc16is7xx_devtype {
 	char	name[10];
 	int	nr_gpio;
 	int	nr_uart;
-	int	has_mctrl;
 };
 
 #define SC16IS7XX_RECONF_MD		(1 << 0)
@@ -336,6 +337,7 @@ struct sc16is7xx_port {
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
+	int				gpio_configured;
 #endif
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
@@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
 	.name		= "SC16IS74X",
 	.nr_gpio	= 0,
 	.nr_uart	= 1,
-	.has_mctrl	= 0,
 };
 
 static const struct sc16is7xx_devtype sc16is750_devtype = {
 	.name		= "SC16IS750",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is752_devtype = {
 	.name		= "SC16IS752",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is760_devtype = {
 	.name		= "SC16IS760",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.name		= "SC16IS762",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
@@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev,
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_GPIOLIB
+	s->gpio_configured = devtype->nr_gpio;
+#endif /* CONFIG_GPIOLIB */
+
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &uartclk);
 
@@ -1473,12 +1474,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);
@@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev,
 					 prop, p, u)
 			if (u < devtype->nr_uart)
 				s->p[u].irda_mode = true;
+
+		val = 0;
+
+		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
+					 prop, p, u)
+			if (u < devtype->nr_uart) {
+				/* Use GPIO lines as modem control lines */
+				if (u == 0)
+					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+				else if (u == 1)
+					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+
+#ifdef CONFIG_GPIOLIB
+				if (s->gpio_configured >=
+				    SC16IS7XX_GPIOS_PER_BANK)
+					s->gpio_configured -=
+						SC16IS7XX_GPIOS_PER_BANK;
+#endif /* CONFIG_GPIOLIB */
+			}
+
+		if (val)
+			regmap_update_bits(
+				s->regmap,
+				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
 	}
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio) {
+	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
+
+	if (s->gpio_configured) {
 		/* Setup GPIO controller */
 		s->gpio.owner		 = THIS_MODULE;
 		s->gpio.parent		 = dev;
@@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev,
 		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.ngpio		 = s->gpio_configured;
 		s->gpio.can_sleep	 = 1;
 		ret = gpiochip_add_data(&s->gpio, s);
 		if (ret)
@@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_configured)
 		gpiochip_remove(&s->gpio);
 
 out_thread:
@@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_configured)
 		gpiochip_remove(&s->gpio);
 #endif
 
-- 
2.30.2

[PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

If the shared GPIO pins on a dual port/channel variant like the
SC16IS752 are configured as GPIOs for port A, and modem control lines
on port A, we need to translate the Linux GPIO offset to an offset
that is compatible with the I/O registers of the SC16IS7XX (IOState,
IODir and IOIntEna).

Add a new variable to store that offset and set it when we detect that
special case.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 97ec532a0a19..c2cfd057ed9a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -338,6 +338,7 @@ struct sc16is7xx_port {
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
 	int				gpio_configured;
+	int				gpio_offset;
 #endif
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
@@ -1298,12 +1299,50 @@ static const struct uart_ops sc16is7xx_ops = {
 };
 
 #ifdef CONFIG_GPIOLIB
+
+/*
+ * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
+ * This is needed only for the case where a dual port variant is configured to
+ * have only port B as modem status lines.
+ *
+ * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
+ * lower bank (port B) set as modem status lines (special case described above):
+ *
+ * Pin         GPIO pin     Linux GPIO     SC16IS7XX
+ * name        function     offset         offset
+ * --------------------------------------------------
+ * GPIO7/RIA    GPIO7          3              7
+ * GPIO6/CDA    GPIO6          2              6
+ * GPIO5/DTRA   GPIO5          1              5
+ * GPIO4/DSRA   GPIO4          0              4
+ * GPIO3/RIB    RIB           N/A            N/A
+ * GPIO2/CDB    CDB           N/A            N/A
+ * GPIO1/DTRB   DTRB          N/A            N/A
+ * GPIO0/DSRB   DSRB          N/A            N/A
+ *
+ * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,
+ * and lower bank (3..0) as GPIOs:
+ *
+ * Pin         GPIO pin     Linux GPIO     SC16IS7XX
+ * name        function     offset         offset
+ * --------------------------------------------------
+ * GPIO7/RI     RI            N/A            N/A
+ * GPIO6/CD     CD            N/A            N/A
+ * GPIO5/DTR    DTR           N/A            N/A
+ * GPIO4/DSR    DSR           N/A            N/A
+ * GPIO3        GPIO3          3              3
+ * GPIO2        GPIO2          2              2
+ * GPIO1        GPIO1          1              1
+ * GPIO0        GPIO0          0              0
+ */
+
 static int sc16is7xx_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	unsigned int val;
 	struct sc16is7xx_port *s = gpiochip_get_data(chip);
 	struct uart_port *port = &s->p[0].port;
 
+	offset += s->gpio_offset;
 	val = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
 
 	return !!(val & BIT(offset));
@@ -1314,6 +1353,7 @@ static void sc16is7xx_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
 	struct sc16is7xx_port *s = gpiochip_get_data(chip);
 	struct uart_port *port = &s->p[0].port;
 
+	offset += s->gpio_offset;
 	sc16is7xx_port_update(port, SC16IS7XX_IOSTATE_REG, BIT(offset),
 			      val ? BIT(offset) : 0);
 }
@@ -1324,6 +1364,7 @@ static int sc16is7xx_gpio_direction_input(struct gpio_chip *chip,
 	struct sc16is7xx_port *s = gpiochip_get_data(chip);
 	struct uart_port *port = &s->p[0].port;
 
+	offset += s->gpio_offset;
 	sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset), 0);
 
 	return 0;
@@ -1336,6 +1377,8 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	struct uart_port *port = &s->p[0].port;
 	u8 state = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
 
+	offset += s->gpio_offset;
+
 	if (val)
 		state |= BIT(offset);
 	else
@@ -1395,6 +1438,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 #ifdef CONFIG_GPIOLIB
 	s->gpio_configured = devtype->nr_gpio;
+	s->gpio_offset = 0;
 #endif /* CONFIG_GPIOLIB */
 
 	/* Always ask for fixed clock rate from a property. */
@@ -1529,16 +1573,24 @@ static int sc16is7xx_probe(struct device *dev,
 #endif /* CONFIG_GPIOLIB */
 			}
 
-		if (val)
+		if (val) {
+#ifdef CONFIG_GPIOLIB
+			/* Additional I/O regs offset. */
+			if (val == SC16IS7XX_IOCONTROL_MODEM_B_BIT)
+				s->gpio_offset = SC16IS7XX_GPIOS_PER_BANK;
+#endif /* CONFIG_GPIOLIB */
+
 			regmap_update_bits(
 				s->regmap,
 				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
 				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
 				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
+		}
 	}
 
 #ifdef CONFIG_GPIOLIB
 	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
+	dev_dbg(dev, "GPIOs offset: %d\n", s->gpio_offset);
 
 	if (s->gpio_configured) {
 		/* Setup GPIO controller */
-- 
2.30.2

[PATCH v3 10/11] 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>
---
 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 c2cfd057ed9a..03d00b144304 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1511,6 +1511,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 v3 11/11] serial: sc16is7xx: add dump registers function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

With this driver, it is very hard to debug the registers using
the /sys/kernel/debug/regmap interface.

The main reason is that bits 0 and 1 of the register address
correspond to the channels bits, so the register address itself starts
at bit 2, so we must 'mentally' shift each register address by 2 bits
to get its offset.

Also, only channels 0 and 1 are supported, so combinations of bits
0 and 1 being 10b and 11b are invalid, and the display of these
registers is useless.

For example:

cat /sys/kernel/debug/regmap/spi0.0/registers
04: 10 -> Port 0, register offset 1
05: 10 -> Port 1, register offset 1
06: 00 -> Port 2, register offset 1 -> invalid
07: 00 -> port 3, register offset 1 -> invalid
...

Add a debug module parameter to call a custom dump function for each
port registers after the probe phase to help debug.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 03d00b144304..693b6cc371f8 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -347,6 +347,10 @@ struct sc16is7xx_port {
 	struct sc16is7xx_one		p[];
 };
 
+static bool debug;
+module_param(debug, bool, 0644);
+MODULE_PARM_DESC(debug, "enable/disable debug messages");
+
 static unsigned long sc16is7xx_lines;
 
 static struct uart_driver sc16is7xx_uart = {
@@ -387,6 +391,28 @@ static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val)
 	regmap_write(s->regmap, (reg << SC16IS7XX_REG_SHIFT) | line, val);
 }
 
+static int sc16is7xx_port_dump(struct uart_port *port)
+{
+	int i;
+	unsigned char *buf;
+	char name[64];
+	const int regs_count_per_port = 16;
+
+	buf = devm_kzalloc(port->dev, regs_count_per_port, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (i = 0; i < regs_count_per_port; i++)
+		buf[i] = sc16is7xx_port_read(port, i);
+
+	snprintf(name, sizeof(name), "sc16is7xx %s%i: dump ",
+		 sc16is7xx_uart.dev_name, port->line);
+	print_hex_dump(KERN_ERR, name, DUMP_PREFIX_OFFSET, 16, 1,
+		       &((u8 *)buf)[0], regs_count_per_port, 1);
+
+	return 0;
+}
+
 static void sc16is7xx_fifo_read(struct uart_port *port, unsigned int rxlen)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
@@ -1614,6 +1640,10 @@ static int sc16is7xx_probe(struct device *dev,
 	}
 #endif
 
+	if (debug)
+		for (i = 0; i < devtype->nr_uart; ++i)
+			sc16is7xx_port_dump(&s->p[i].port);
+
 	/*
 	 * Setup interrupt. We first try to acquire the IRQ line as level IRQ.
 	 * If that succeeds, we can allow sharing the interrupt as well.
-- 
2.30.2

Re: [PATCH v3 00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:13AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Hello,
> this patch series mainly fixes a GPIO regression and improve RS485 flags and properties
> detection from DT.
> 
> It now also includes various small fixes and improvements that were previously
> sent as separate patches, but that made testing everything difficult.

> Patches 1 and 2 are simple comments fix/improvements.

Usually we put fixes at the beginning of the series, but these patches are
missing Fixed tag. Are they really fixes or can be simply moved to the end of
the series?

> Patch 3 fixes an issue when debugging IOcontrol register. After testing the GPIO
> regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> this patch is also necessary for having the correct IOcontrol register values.
> 
> Patch 4 introduces a delay after a reset operation to respect datasheet
> timing recommandations.
> 
> Patch 5 fixes an issue with init of first port during probing. This commit
> brings some questions and I appreciate if people from the serial subsystem could
> comment on my proposed solution.
> 
> Patch 6 fixes a bug with the output value when first setting the GPIO direction.
> 
> Patch 7, 8 and 9 fix a GPIO regression by (re)allowing to choose GPIO function for
> GPIO pins shared with modem status lines.
> 
> Patch 10 allows to read common rs485 device-tree flags and properties.
> 
> Patch 11 add a custom dump function as relying on regmal debugfs is not really
> practical for this driver.
> 
> I have tested the changes on a custom board with two SC16IS752 DUART using a
> Variscite IMX8MN NANO SOM.

Other comments are per individual emails.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/11] serial: sc16is7xx: add post reset delay

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:17AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Make sure we wait at least 3us before initiating communication with
> the device after reset.

...

> +	usleep_range(3, 5);

I would put (5, 10) instead to relax a bit the scheduler.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/11] serial: sc16is7xx: mark IOCONTROL register as volatile

[Ilpo Järvinen]

On Thu, 25 May 2023, 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.
> Therefore mark IOCONTROL register as a volatile register.
> 
> Fixes: dfeae619d781 ("serial: sc16is7xx")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

What is the impact of this problem? That is, what doesn't work? I only see 
writes to SC16IS7XX_IOCONTROL_REG. If there are no concrete problems 
fixed, don't put Fixes tag.

-- 
 i.

> ---
>  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 00054bb49780..a7c4da3cfd2b 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;
> 

Re: [PATCH v3 04/11] serial: sc16is7xx: add post reset delay

[Ilpo Järvinen]

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Make sure we wait at least 3us before initiating communication with
> the device after reset.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a7c4da3cfd2b..af7e66db54b4 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1428,6 +1428,12 @@ static int sc16is7xx_probe(struct device *dev,
>  	regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
>  			SC16IS7XX_IOCONTROL_SRESET_BIT);
>  
> +	/*
> +	 * After reset, the host must wait at least 3us before initializing a
> +	 * communication with the device.
> +	 */
> +	usleep_range(3, 5);
> +
>  	for (i = 0; i < devtype->nr_uart; ++i) {
>  		s->p[i].line		= i;
>  		/* Initialize port data */

Does this fix a problem? You don't have a Fixes tag nor did you describe
a problem that arises if the is not there in the changelog.

-- 
 i.

Re: [PATCH v3 06/11] serial: sc16is7xx: fix bug when first setting GPIO direction

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:20AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> When we want to configure 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
> c16is7xx_gpio_direction_output() for the first time, we first set the

Missing 's'.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/11] dt-bindings: sc16is7xx: Add property to change GPIO function

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:21AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Some variants in this series of uart controllers have GPIO pins that

UART

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

I'm wondering if we can convert this to YAML first and then add a new property.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> changed the function of the GPIOs pins to act as modem control
> lines without any possibility of selecting GPIO function.
> 
> As a consequence, applications that depends on GPIO lines configured
> by default as GPIO pins no longer work as expected.
> 
> Also, the change to select modem control lines function was done only
> for channel A of dual UART variants (752/762). This was not documented
> in the log message.

> This new patch allows to specify GPIO or modem control line function
> in the device tree, and for each of the ports (A or B).

Imperative mood as stated in documentation, please.
Like "Allow to specify...".

> This is done by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> We also now reduce the number of exported GPIOs according to the
> modem-status-line-port DT property.
> 
> Boards that need to have GPIOS configured as modem control lines
> should add that property to their device tree. Here is a list of
> boards using the sc16is7xx driver in their device tree and that may
> need to be modified:
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts

...

> +#ifdef CONFIG_GPIOLIB

I'm wondering if we can avoid adding new ifdefferies...

> +	s->gpio_configured = devtype->nr_gpio;

The name of the variable is a bit vague WRT its content.
Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?

> +#endif /* CONFIG_GPIOLIB */

...

> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u)

The driver so far is agnostic to property provider. Please keep it that way,
i.e. no of_ APIs.

> +			if (u < devtype->nr_uart) {

Hmm... What other can it be?

> +				/* Use GPIO lines as modem control lines */
> +				if (u == 0)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +				else if (u == 1)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +
> +#ifdef CONFIG_GPIOLIB
> +				if (s->gpio_configured >=
> +				    SC16IS7XX_GPIOS_PER_BANK)

On one line it will be better to read. Esp. taking into account the above remark.

> +					s->gpio_configured -=
> +						SC16IS7XX_GPIOS_PER_BANK;

Ditto.

> +#endif /* CONFIG_GPIOLIB */
> +			}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/11] serial: sc16is7xx: fix broken port 0 uart init

[Ilpo Järvinen]

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

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> While experimenting with rs485 configuration on a SC16IS752 dual UART,

You can remove this intro, it's not necessary.

> I found that the sc16is7xx_config_rs485() function was called only for
> the second port (index 1, channel B), causing initialization problems
> for the first port.

Just start with:

sc16is7xx_config_rs485() function is called only for ...

> 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. But I found that the max310x driver sets
> port->membase to ~0 (all ones).

The max310x driver sets port->membase to ~0 (all ones) to solve the same 
problem.

> By implementing the same change in our
> driver, uart_configure_port() is now correctly executed.

our driver -> this driver

This changelog was really well describing the problem! :-)

> Fixes: dfeae619d781 ("serial: sc16is7xx")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> I am not sure if this change is the best long-term solution to this
> problem, and maybe uart_configure_port() itself could be modified to
> take into account the fact that some devices have all three *base
> values set to zero?

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>

> Also, many drivers use port->iobase as an index, is it the correct way
> to use it?

"Many" for this and max310x? Besides that, uartlite.c has a comment which 
says "mark port in use".

-- 
 i.


> For example, for our driver, there was
> commit 5da6b1c079e6 ("sc16is7xx: Set iobase to device index") with the
> following explanation:
>     "Set the .iobase value to the relative index within the device to allow
>     infering the order through sysfs."
> 
>  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 af7e66db54b4..8a2fc6f89d36 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1443,6 +1443,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
>  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
>  		s->p[i].port.iobase	= i;
> +		s->p[i].port.membase	= (void __iomem *)~0;
>  		s->p[i].port.iotype	= UPIO_PORT;
>  		s->p[i].port.uartclk	= freq;
>  		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
> 

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:23AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If the shared GPIO pins on a dual port/channel variant like the
> SC16IS752 are configured as GPIOs for port A, and modem control lines
> on port A, we need to translate the Linux GPIO offset to an offset
> that is compatible with the I/O registers of the SC16IS7XX (IOState,
> IODir and IOIntEna).
> 
> Add a new variable to store that offset and set it when we detect that
> special case.

...

> +/*
> + * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
> + * This is needed only for the case where a dual port variant is configured to
> + * have only port B as modem status lines.
> + *
> + * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
> + * lower bank (port B) set as modem status lines (special case described above):
> + *
> + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> + * name        function     offset         offset
> + * --------------------------------------------------
> + * GPIO7/RIA    GPIO7          3              7
> + * GPIO6/CDA    GPIO6          2              6
> + * GPIO5/DTRA   GPIO5          1              5
> + * GPIO4/DSRA   GPIO4          0              4
> + * GPIO3/RIB    RIB           N/A            N/A
> + * GPIO2/CDB    CDB           N/A            N/A
> + * GPIO1/DTRB   DTRB          N/A            N/A
> + * GPIO0/DSRB   DSRB          N/A            N/A
> + *
> + * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,

Single space is enough.

> + * and lower bank (3..0) as GPIOs:
> + *
> + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> + * name        function     offset         offset
> + * --------------------------------------------------
> + * GPIO7/RI     RI            N/A            N/A
> + * GPIO6/CD     CD            N/A            N/A
> + * GPIO5/DTR    DTR           N/A            N/A
> + * GPIO4/DSR    DSR           N/A            N/A
> + * GPIO3        GPIO3          3              3
> + * GPIO2        GPIO2          2              2
> + * GPIO1        GPIO1          1              1
> + * GPIO0        GPIO0          0              0
> + */

Wondering if you can always register 8 pins and use valid mask to define which
one are in use?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/11] serial: sc16is7xx: add dump registers function

[andy.shevchenko]

Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> With this driver, it is very hard to debug the registers using
> the /sys/kernel/debug/regmap interface.
> 
> The main reason is that bits 0 and 1 of the register address
> correspond to the channels bits, so the register address itself starts
> at bit 2, so we must 'mentally' shift each register address by 2 bits
> to get its offset.
> 
> Also, only channels 0 and 1 are supported, so combinations of bits
> 0 and 1 being 10b and 11b are invalid, and the display of these
> registers is useless.
> 
> For example:
> 
> cat /sys/kernel/debug/regmap/spi0.0/registers
> 04: 10 -> Port 0, register offset 1
> 05: 10 -> Port 1, register offset 1
> 06: 00 -> Port 2, register offset 1 -> invalid
> 07: 00 -> port 3, register offset 1 -> invalid
> ...
> 
> Add a debug module parameter to call a custom dump function for each
> port registers after the probe phase to help debug.

Not sure about this. Can we rather create an abstract mapping on regmap?
(Something like gpio-pca953x.c has)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration

[Ilpo Järvinen]

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> changed the function of the GPIOs pins to act as modem control
> lines without any possibility of selecting GPIO function.
> 
> As a consequence, applications that depends on GPIO lines configured
> by default as GPIO pins no longer work as expected.
> 
> Also, the change to select modem control lines function was done only
> for channel A of dual UART variants (752/762). This was not documented
> in the log message.
> 
> This new patch allows to specify GPIO or modem control line function
> in the device tree, and for each of the ports (A or B).
> 
> This is done by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> We also now reduce the number of exported GPIOs according to the
> modem-status-line-port DT property.
> 
> Boards that need to have GPIOS configured as modem control lines
> should add that property to their device tree. Here is a list of
> boards using the sc16is7xx driver in their device tree and that may
> need to be modified:
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a5d8af0f6da0..97ec532a0a19 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -236,7 +236,8 @@
>  
>  /* IOControl register bits (Only 75x/76x) */
>  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
>  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
>  
>  /* EFCR register bits */
> @@ -301,12 +302,12 @@
>  /* Misc definitions */
>  #define SC16IS7XX_FIFO_SIZE		(64)
>  #define SC16IS7XX_REG_SHIFT		2
> +#define SC16IS7XX_GPIOS_PER_BANK	4
>  
>  struct sc16is7xx_devtype {
>  	char	name[10];
>  	int	nr_gpio;
>  	int	nr_uart;
> -	int	has_mctrl;
>  };
>  
>  #define SC16IS7XX_RECONF_MD		(1 << 0)
> @@ -336,6 +337,7 @@ struct sc16is7xx_port {
>  	struct clk			*clk;
>  #ifdef CONFIG_GPIOLIB
>  	struct gpio_chip		gpio;
> +	int				gpio_configured;
>  #endif
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
> @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
>  	.name		= "SC16IS74X",
>  	.nr_gpio	= 0,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 0,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is750_devtype = {
>  	.name		= "SC16IS750",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is752_devtype = {
>  	.name		= "SC16IS752",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is760_devtype = {
>  	.name		= "SC16IS760",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is762_devtype = {
>  	.name		= "SC16IS762",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev,
>  		return -ENOMEM;
>  	}
>  
> +#ifdef CONFIG_GPIOLIB
> +	s->gpio_configured = devtype->nr_gpio;
> +#endif /* CONFIG_GPIOLIB */
> +
>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &uartclk);
>  
> @@ -1473,12 +1474,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);
> @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev,
>  					 prop, p, u)
>  			if (u < devtype->nr_uart)
>  				s->p[u].irda_mode = true;
> +
> +		val = 0;
> +
> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u)
> +			if (u < devtype->nr_uart) {

Reverse logic + use continue. It will help with the indentation levels.

> +				/* Use GPIO lines as modem control lines */
> +				if (u == 0)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +				else if (u == 1)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +
> +#ifdef CONFIG_GPIOLIB
> +				if (s->gpio_configured >=
> +				    SC16IS7XX_GPIOS_PER_BANK)
> +					s->gpio_configured -=
> +						SC16IS7XX_GPIOS_PER_BANK;
> +#endif /* CONFIG_GPIOLIB */
> +			}

Please use braces for of_property_for_each_u32 block.

> +
> +		if (val)
> +			regmap_update_bits(
> +				s->regmap,
> +				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> +				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> +				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (devtype->nr_gpio) {
> +	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> +
> +	if (s->gpio_configured) {
>  		/* Setup GPIO controller */
>  		s->gpio.owner		 = THIS_MODULE;
>  		s->gpio.parent		 = dev;
> @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		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.ngpio		 = s->gpio_configured;
>  		s->gpio.can_sleep	 = 1;
>  		ret = gpiochip_add_data(&s->gpio, s);
>  		if (ret)
> @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		return 0;
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (devtype->nr_gpio)
> +	if (s->gpio_configured)
>  		gpiochip_remove(&s->gpio);
>  
>  out_thread:
> @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev)
>  	int i;
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (s->devtype->nr_gpio)
> +	if (s->gpio_configured)
>  		gpiochip_remove(&s->gpio);
>  #endif
>  
> 

-- 
 i.

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[Ilpo Järvinen]

On Thu, 25 May 2023, Hugo Villeneuve wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> If the shared GPIO pins on a dual port/channel variant like the
> SC16IS752 are configured as GPIOs for port A, and modem control lines
> on port A, we need to translate the Linux GPIO offset to an offset
> that is compatible with the I/O registers of the SC16IS7XX (IOState,
> IODir and IOIntEna).
> 
> Add a new variable to store that offset and set it when we detect that
> special case.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 54 +++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 97ec532a0a19..c2cfd057ed9a 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -338,6 +338,7 @@ struct sc16is7xx_port {
>  #ifdef CONFIG_GPIOLIB
>  	struct gpio_chip		gpio;
>  	int				gpio_configured;
> +	int				gpio_offset;
>  #endif
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
> @@ -1298,12 +1299,50 @@ static const struct uart_ops sc16is7xx_ops = {
>  };
>  
>  #ifdef CONFIG_GPIOLIB
> +
> +/*
> + * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
> + * This is needed only for the case where a dual port variant is configured to
> + * have only port B as modem status lines.
> + *
> + * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
> + * lower bank (port B) set as modem status lines (special case described above):
> + *
> + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> + * name        function     offset         offset
> + * --------------------------------------------------
> + * GPIO7/RIA    GPIO7          3              7
> + * GPIO6/CDA    GPIO6          2              6
> + * GPIO5/DTRA   GPIO5          1              5
> + * GPIO4/DSRA   GPIO4          0              4
> + * GPIO3/RIB    RIB           N/A            N/A
> + * GPIO2/CDB    CDB           N/A            N/A
> + * GPIO1/DTRB   DTRB          N/A            N/A
> + * GPIO0/DSRB   DSRB          N/A            N/A
> + *
> + * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,
> + * and lower bank (3..0) as GPIOs:
> + *
> + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> + * name        function     offset         offset
> + * --------------------------------------------------
> + * GPIO7/RI     RI            N/A            N/A
> + * GPIO6/CD     CD            N/A            N/A
> + * GPIO5/DTR    DTR           N/A            N/A
> + * GPIO4/DSR    DSR           N/A            N/A
> + * GPIO3        GPIO3          3              3
> + * GPIO2        GPIO2          2              2
> + * GPIO1        GPIO1          1              1
> + * GPIO0        GPIO0          0              0
> + */
> +
>  static int sc16is7xx_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	unsigned int val;
>  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
>  	struct uart_port *port = &s->p[0].port;
>  
> +	offset += s->gpio_offset;
>  	val = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
>  
>  	return !!(val & BIT(offset));
> @@ -1314,6 +1353,7 @@ static void sc16is7xx_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
>  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
>  	struct uart_port *port = &s->p[0].port;
>  
> +	offset += s->gpio_offset;
>  	sc16is7xx_port_update(port, SC16IS7XX_IOSTATE_REG, BIT(offset),
>  			      val ? BIT(offset) : 0);
>  }
> @@ -1324,6 +1364,7 @@ static int sc16is7xx_gpio_direction_input(struct gpio_chip *chip,
>  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
>  	struct uart_port *port = &s->p[0].port;
>  
> +	offset += s->gpio_offset;
>  	sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset), 0);
>  
>  	return 0;
> @@ -1336,6 +1377,8 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
>  	struct uart_port *port = &s->p[0].port;
>  	u8 state = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
>  
> +	offset += s->gpio_offset;
> +
>  	if (val)
>  		state |= BIT(offset);
>  	else
> @@ -1395,6 +1438,7 @@ static int sc16is7xx_probe(struct device *dev,
>  
>  #ifdef CONFIG_GPIOLIB
>  	s->gpio_configured = devtype->nr_gpio;
> +	s->gpio_offset = 0;
>  #endif /* CONFIG_GPIOLIB */
>  
>  	/* Always ask for fixed clock rate from a property. */
> @@ -1529,16 +1573,24 @@ static int sc16is7xx_probe(struct device *dev,
>  #endif /* CONFIG_GPIOLIB */
>  			}
>  
> -		if (val)
> +		if (val) {
> +#ifdef CONFIG_GPIOLIB
> +			/* Additional I/O regs offset. */
> +			if (val == SC16IS7XX_IOCONTROL_MODEM_B_BIT)
> +				s->gpio_offset = SC16IS7XX_GPIOS_PER_BANK;
> +#endif /* CONFIG_GPIOLIB */
> +
>  			regmap_update_bits(
>  				s->regmap,
>  				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
>  				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
>  				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> +		}
>  	}
>  
>  #ifdef CONFIG_GPIOLIB
>  	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> +	dev_dbg(dev, "GPIOs offset: %d\n", s->gpio_offset);
>  
>  	if (s->gpio_configured) {
>  		/* Setup GPIO controller */
> 

Is the order of this and 8/11 patch correct or should this precede that 
other patch? That is, is 8/11 alone useful enough or would this also be 
wanted? (I'm aware that 8/11 has a Fixes tag).

-- 
 i.

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

[Ilpo Järvinen]

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

On Thu, 25 May 2023, 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.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index c2cfd057ed9a..03d00b144304 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1511,6 +1511,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 */
> 

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

-- 
 i.

Re: [PATCH v3 04/11] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

On Thu, 25 May 2023 13:30:06 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:17AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Make sure we wait at least 3us before initiating communication with
> > the device after reset.
> 
> ...
> 
> > +	usleep_range(3, 5);
> 
> I would put (5, 10) instead to relax a bit the scheduler.

Hi,
Ok, done.

Re: [PATCH v3 00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Thu, 25 May 2023 13:27:52 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:13AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Hello,
> > this patch series mainly fixes a GPIO regression and improve RS485 flags and properties
> > detection from DT.
> > 
> > It now also includes various small fixes and improvements that were previously
> > sent as separate patches, but that made testing everything difficult.
> 
> > Patches 1 and 2 are simple comments fix/improvements.
> 
> Usually we put fixes at the beginning of the series, but these patches are
> missing Fixed tag. Are they really fixes or can be simply moved to the end of
> the series?

Hi,
these are not code fixes, they are comments improvements. I was not aware that you need to put a Fixes tag for correcting syntax errors in comments, or adding comments to improve clarity.

I often submit such comments patches but was never asked to put a Fixes tag before. Seems strange to me...

Hugo.

> > Patch 3 fixes an issue when debugging IOcontrol register. After testing the GPIO
> > regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
> > this patch is also necessary for having the correct IOcontrol register values.
> > 
> > Patch 4 introduces a delay after a reset operation to respect datasheet
> > timing recommandations.
> > 
> > Patch 5 fixes an issue with init of first port during probing. This commit
> > brings some questions and I appreciate if people from the serial subsystem could
> > comment on my proposed solution.
> > 
> > Patch 6 fixes a bug with the output value when first setting the GPIO direction.
> > 
> > Patch 7, 8 and 9 fix a GPIO regression by (re)allowing to choose GPIO function for
> > GPIO pins shared with modem status lines.
> > 
> > Patch 10 allows to read common rs485 device-tree flags and properties.
> > 
> > Patch 11 add a custom dump function as relying on regmal debugfs is not really
> > practical for this driver.
> > 
> > I have tested the changes on a custom board with two SC16IS752 DUART using a
> > Variscite IMX8MN NANO SOM.
> 
> Other comments are per individual emails.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH v3 00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Andy Shevchenko]

On Thu, May 25, 2023 at 4:26 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Thu, 25 May 2023 13:27:52 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:13AM -0400, Hugo Villeneuve kirjoitti:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > >
> > > Hello,
> > > this patch series mainly fixes a GPIO regression and improve RS485 flags and properties
> > > detection from DT.
> > >
> > > It now also includes various small fixes and improvements that were previously
> > > sent as separate patches, but that made testing everything difficult.
> >
> > > Patches 1 and 2 are simple comments fix/improvements.
> >
> > Usually we put fixes at the beginning of the series, but these patches are
> > missing Fixed tag. Are they really fixes or can be simply moved to the end of
> > the series?
>
> these are not code fixes, they are comments improvements. I was not aware that you need to put a Fixes tag for correcting syntax errors in comments, or adding comments to improve clarity.
>
> I often submit such comments patches but was never asked to put a Fixes tag before. Seems strange to me...

In this case there are probably no conflicts, but the usual grouping
of patches is following
1) fixes that may be backported;
2) cleanups / refactoring /etc;
3) new features.
4) additional light-weit cleanups, such as whitespace cleaning (it's a
radical, we probably do not accept pure whitespace cleaning patches,
but you got the idea).

Seems patches 1 and 2 fall into category 4).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

[Hugo Villeneuve]

On Thu, 25 May 2023 16:37:17 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 25, 2023 at 4:26 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Thu, 25 May 2023 13:27:52 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Thu, May 25, 2023 at 12:03:13AM -0400, Hugo Villeneuve kirjoitti:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > >
> > > > Hello,
> > > > this patch series mainly fixes a GPIO regression and improve RS485 flags and properties
> > > > detection from DT.
> > > >
> > > > It now also includes various small fixes and improvements that were previously
> > > > sent as separate patches, but that made testing everything difficult.
> > >
> > > > Patches 1 and 2 are simple comments fix/improvements.
> > >
> > > Usually we put fixes at the beginning of the series, but these patches are
> > > missing Fixed tag. Are they really fixes or can be simply moved to the end of
> > > the series?
> >
> > these are not code fixes, they are comments improvements. I was not aware that you need to put a Fixes tag for correcting syntax errors in comments, or adding comments to improve clarity.
> >
> > I often submit such comments patches but was never asked to put a Fixes tag before. Seems strange to me...
> 
> In this case there are probably no conflicts, but the usual grouping
> of patches is following
> 1) fixes that may be backported;
> 2) cleanups / refactoring /etc;
> 3) new features.
> 4) additional light-weit cleanups, such as whitespace cleaning (it's a
> radical, we probably do not accept pure whitespace cleaning patches,
> but you got the idea).
> 
> Seems patches 1 and 2 fall into category 4).

Ok, I changed the order to put these two patches at the end.

Hugo.

Re: [PATCH v3 03/11] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

On Thu, 25 May 2023 14:02:19 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, 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.
> > Therefore mark IOCONTROL register as a volatile register.
> > 
> > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> What is the impact of this problem? That is, what doesn't work? I only see 
> writes to SC16IS7XX_IOCONTROL_REG. If there are no concrete problems 
> fixed, don't put Fixes tag.

Hi,
there is a concrete problem when dumping the registers as the value read for bit SRESET is incorrect, but it doesn't impact running code.

I can remove the Fixes.

Hugo.


> > ---
> >  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 00054bb49780..a7c4da3cfd2b 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;
> > 
> 

Re: [PATCH v3 04/11] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

On Thu, 25 May 2023 14:05:35 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, Hugo Villeneuve wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Make sure we wait at least 3us before initiating communication with
> > the device after reset.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index a7c4da3cfd2b..af7e66db54b4 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1428,6 +1428,12 @@ static int sc16is7xx_probe(struct device *dev,
> >  	regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> >  			SC16IS7XX_IOCONTROL_SRESET_BIT);
> >  
> > +	/*
> > +	 * After reset, the host must wait at least 3us before initializing a
> > +	 * communication with the device.
> > +	 */
> > +	usleep_range(3, 5);
> > +
> >  	for (i = 0; i < devtype->nr_uart; ++i) {
> >  		s->p[i].line		= i;
> >  		/* Initialize port data */
> 
> Does this fix a problem? You don't have a Fixes tag nor did you describe
> a problem that arises if the is not there in the changelog.

Not for the moment, that is why I didn't put a Fixes tag.

A potential problem that can arise is that on a much faster processor, there is a chance that we could reach the first instruction that request a read/write before the reset post-delay.

Hugo.

Re: [PATCH v3 06/11] serial: sc16is7xx: fix bug when first setting GPIO direction

[Hugo Villeneuve]

On Thu, 25 May 2023 14:10:27 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:20AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > When we want to configure 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
> > c16is7xx_gpio_direction_output() for the first time, we first set the
> 
> Missing 's'.

Fixed.

> > 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.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH v3 07/11] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

On Thu, 25 May 2023 14:11:22 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:21AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Some variants in this series of uart controllers have GPIO pins that
> 
> UART

Hi,
fixed.

> > 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.
> 
> I'm wondering if we can convert this to YAML first and then add a new property.

Hi,
I also thought about it, then decided to focus on simply adding the new property first since I am not an expert in YAML.

I think it would be best to do it after this patch series. Keep in mind that the original intent of this patch series, and this new property, was to fix a regression related to the GPIOs, and I think that converting to YAML would simply delay and add much noise to the discussion at this point.

If someone wants to do it as a separate patch after this, fine. If not, I an willing to give it a go.

Hugo.

Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Thu, 25 May 2023 14:19:52 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> 
> > This new patch allows to specify GPIO or modem control line function
> > in the device tree, and for each of the ports (A or B).
> 
> Imperative mood as stated in documentation, please.
> Like "Allow to specify...".
> 
> > This is done by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > We also now reduce the number of exported GPIOs according to the
> > modem-status-line-port DT property.

Just noticed a mistake:
s/modem-status-line-port/modem-control-line-ports

> > 
> > 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
> 
> ...
> 
> > +#ifdef CONFIG_GPIOLIB
> 
> I'm wondering if we can avoid adding new ifdefferies...

I am simply following waht was already done in the existing driver.

Are you suggesting that we need to remove all these #defines? If not, what exactly do you suggest?


> > +	s->gpio_configured = devtype->nr_gpio;
> 
> The name of the variable is a bit vague WRT its content.
> Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?

Maybe the name could be improved (and/or comments).

devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by the chip.

s->gpio_configured is the number of GPIOs that are configured or requested according to the presence (or not) of the modem-control-line-ports property.

I wanted to avoid using the same name to avoid potential confusion.

Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured?


> > +#endif /* CONFIG_GPIOLIB */
> 
> ...
> 
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u)
> 
> The driver so far is agnostic to property provider. Please keep it that way,
> i.e. no of_ APIs.

The driver, before my patches, was already using the exact same function of_property_for_each_u32() to process the irda-mode-ports property, so I don't understand your comment.

But what do you suggest instead of of_property_for_each_u32()? And do we need to change it also for processing the irda-mode-ports property?


> > +			if (u < devtype->nr_uart) {
> 
> Hmm... What other can it be?

Again, this is similar to the handling of the irda-mode-ports property.

But I am not sure I understand your question/concern?

I think this check is important, because if someone puts the following property in a DT:

    nxp,modem-control-line-ports = <0 1>;

but the variant only supports 1 port, then the check is usefull, no?


> 
> > +				/* Use GPIO lines as modem control lines */
> > +				if (u == 0)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +				else if (u == 1)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +
> > +#ifdef CONFIG_GPIOLIB
> > +				if (s->gpio_configured >=
> > +				    SC16IS7XX_GPIOS_PER_BANK)
> 
> On one line it will be better to read. Esp. taking into account the above remark.

Fixed.

 
> > +					s->gpio_configured -=
> > +						SC16IS7XX_GPIOS_PER_BANK;
> 
> Ditto.

Fixed.

> 
> > +#endif /* CONFIG_GPIOLIB */
> > +			}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH v3 05/11] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

On Thu, 25 May 2023 14:20:59 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, Hugo Villeneuve wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > While experimenting with rs485 configuration on a SC16IS752 dual UART,
> 
> You can remove this intro, it's not necessary.

Fixed.

 
> > I found that the sc16is7xx_config_rs485() function was called only for
> > the second port (index 1, channel B), causing initialization problems
> > for the first port.
> 
> Just start with:
> 
> sc16is7xx_config_rs485() function is called only for ...
> 
> > 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. But I found that the max310x driver sets
> > port->membase to ~0 (all ones).
> 
> The max310x driver sets port->membase to ~0 (all ones) to solve the same 
> problem.

Fixed.


> > By implementing the same change in our
> > driver, uart_configure_port() is now correctly executed.
> 
> our driver -> this driver

Fixed.

 
> This changelog was really well describing the problem! :-)
> 
> > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > I am not sure if this change is the best long-term solution to this
> > problem, and maybe uart_configure_port() itself could be modified to
> > take into account the fact that some devices have all three *base
> > values set to zero?
> 
> 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>

Added.

 
> > Also, many drivers use port->iobase as an index, is it the correct way
> > to use it?
> 
> "Many" for this and max310x? Besides that, uartlite.c has a comment which 
> says "mark port in use".

Ok, 
anyway with your approval I will remove these comments which will not part of the final commit message anyway.

Hugo.


> > For example, for our driver, there was
> > commit 5da6b1c079e6 ("sc16is7xx: Set iobase to device index") with the
> > following explanation:
> >     "Set the .iobase value to the relative index within the device to allow
> >     infering the order through sysfs."
> > 
> >  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 af7e66db54b4..8a2fc6f89d36 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1443,6 +1443,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
> >  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> >  		s->p[i].port.iobase	= i;
> > +		s->p[i].port.membase	= (void __iomem *)~0;
> >  		s->p[i].port.iotype	= UPIO_PORT;
> >  		s->p[i].port.uartclk	= freq;
> >  		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
> > 

Re: [PATCH v3 07/11] dt-bindings: sc16is7xx: Add property to change GPIO function

[Conor Dooley]

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

On Thu, May 25, 2023 at 10:34:43AM -0400, Hugo Villeneuve wrote:
> On Thu, 25 May 2023 14:11:22 +0300
> andy.shevchenko@gmail.com wrote:

> > I'm wondering if we can convert this to YAML first and then add a new property.

> I think it would be best to do it after this patch series.
> Keep in mind that the original intent of this patch series,
> and this new property, was to fix a regression related to the
> GPIOs, and I think that converting to YAML would simply delay
> and add much noise to the discussion at this point.

I think that that is reasonable.

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

Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Thu, 25 May 2023 15:03:41 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, Hugo Villeneuve wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> > 
> > This new patch allows to specify GPIO or modem control line function
> > in the device tree, and for each of the ports (A or B).
> > 
> > This is done by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > We also now reduce the number of exported GPIOs according to the
> > modem-status-line-port DT property.
> > 
> > Boards that need to have GPIOS configured as modem control lines
> > should add that property to their device tree. Here is a list of
> > boards using the sc16is7xx driver in their device tree and that may
> > need to be modified:
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
> >  1 file changed, 44 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index a5d8af0f6da0..97ec532a0a19 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >  
> >  /* IOControl register bits (Only 75x/76x) */
> >  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> > -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
> >  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
> >  
> >  /* EFCR register bits */
> > @@ -301,12 +302,12 @@
> >  /* Misc definitions */
> >  #define SC16IS7XX_FIFO_SIZE		(64)
> >  #define SC16IS7XX_REG_SHIFT		2
> > +#define SC16IS7XX_GPIOS_PER_BANK	4
> >  
> >  struct sc16is7xx_devtype {
> >  	char	name[10];
> >  	int	nr_gpio;
> >  	int	nr_uart;
> > -	int	has_mctrl;
> >  };
> >  
> >  #define SC16IS7XX_RECONF_MD		(1 << 0)
> > @@ -336,6 +337,7 @@ struct sc16is7xx_port {
> >  	struct clk			*clk;
> >  #ifdef CONFIG_GPIOLIB
> >  	struct gpio_chip		gpio;
> > +	int				gpio_configured;
> >  #endif
> >  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
> >  	struct kthread_worker		kworker;
> > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >  	.name		= "SC16IS74X",
> >  	.nr_gpio	= 0,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 0,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is750_devtype = {
> >  	.name		= "SC16IS750",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >  	.name		= "SC16IS752",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is760_devtype = {
> >  	.name		= "SC16IS760",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is762_devtype = {
> >  	.name		= "SC16IS762",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> > @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev,
> >  		return -ENOMEM;
> >  	}
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +	s->gpio_configured = devtype->nr_gpio;
> > +#endif /* CONFIG_GPIOLIB */
> > +
> >  	/* Always ask for fixed clock rate from a property. */
> >  	device_property_read_u32(dev, "clock-frequency", &uartclk);
> >  
> > @@ -1473,12 +1474,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);
> > @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev,
> >  					 prop, p, u)
> >  			if (u < devtype->nr_uart)
> >  				s->p[u].irda_mode = true;
> > +
> > +		val = 0;
> > +
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u)
> > +			if (u < devtype->nr_uart) {
> 
> Reverse logic + use continue. It will help with the indentation levels.

Good suggestion, done.

 
> > +				/* Use GPIO lines as modem control lines */
> > +				if (u == 0)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +				else if (u == 1)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +
> > +#ifdef CONFIG_GPIOLIB
> > +				if (s->gpio_configured >=
> > +				    SC16IS7XX_GPIOS_PER_BANK)
> > +					s->gpio_configured -=
> > +						SC16IS7XX_GPIOS_PER_BANK;
> > +#endif /* CONFIG_GPIOLIB */
> > +			}
> 
> Please use braces for of_property_for_each_u32 block.

Done

 
> > +
> > +		if (val)
> > +			regmap_update_bits(
> > +				s->regmap,
> > +				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > +				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > +				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> >  	}
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (devtype->nr_gpio) {
> > +	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> > +
> > +	if (s->gpio_configured) {
> >  		/* Setup GPIO controller */
> >  		s->gpio.owner		 = THIS_MODULE;
> >  		s->gpio.parent		 = dev;
> > @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		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.ngpio		 = s->gpio_configured;
> >  		s->gpio.can_sleep	 = 1;
> >  		ret = gpiochip_add_data(&s->gpio, s);
> >  		if (ret)
> > @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		return 0;
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (devtype->nr_gpio)
> > +	if (s->gpio_configured)
> >  		gpiochip_remove(&s->gpio);
> >  
> >  out_thread:
> > @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev)
> >  	int i;
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (s->devtype->nr_gpio)
> > +	if (s->gpio_configured)
> >  		gpiochip_remove(&s->gpio);
> >  #endif
> >  
> > 
> 
> -- 
>  i.
> 
> 

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[Hugo Villeneuve]

On Thu, 25 May 2023 15:10:00 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, Hugo Villeneuve wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > If the shared GPIO pins on a dual port/channel variant like the
> > SC16IS752 are configured as GPIOs for port A, and modem control lines
> > on port A, we need to translate the Linux GPIO offset to an offset
> > that is compatible with the I/O registers of the SC16IS7XX (IOState,
> > IODir and IOIntEna).
> > 
> > Add a new variable to store that offset and set it when we detect that
> > special case.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 54 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 97ec532a0a19..c2cfd057ed9a 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -338,6 +338,7 @@ struct sc16is7xx_port {
> >  #ifdef CONFIG_GPIOLIB
> >  	struct gpio_chip		gpio;
> >  	int				gpio_configured;
> > +	int				gpio_offset;
> >  #endif
> >  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
> >  	struct kthread_worker		kworker;
> > @@ -1298,12 +1299,50 @@ static const struct uart_ops sc16is7xx_ops = {
> >  };
> >  
> >  #ifdef CONFIG_GPIOLIB
> > +
> > +/*
> > + * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
> > + * This is needed only for the case where a dual port variant is configured to
> > + * have only port B as modem status lines.
> > + *
> > + * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
> > + * lower bank (port B) set as modem status lines (special case described above):
> > + *
> > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > + * name        function     offset         offset
> > + * --------------------------------------------------
> > + * GPIO7/RIA    GPIO7          3              7
> > + * GPIO6/CDA    GPIO6          2              6
> > + * GPIO5/DTRA   GPIO5          1              5
> > + * GPIO4/DSRA   GPIO4          0              4
> > + * GPIO3/RIB    RIB           N/A            N/A
> > + * GPIO2/CDB    CDB           N/A            N/A
> > + * GPIO1/DTRB   DTRB          N/A            N/A
> > + * GPIO0/DSRB   DSRB          N/A            N/A
> > + *
> > + * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,
> > + * and lower bank (3..0) as GPIOs:
> > + *
> > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > + * name        function     offset         offset
> > + * --------------------------------------------------
> > + * GPIO7/RI     RI            N/A            N/A
> > + * GPIO6/CD     CD            N/A            N/A
> > + * GPIO5/DTR    DTR           N/A            N/A
> > + * GPIO4/DSR    DSR           N/A            N/A
> > + * GPIO3        GPIO3          3              3
> > + * GPIO2        GPIO2          2              2
> > + * GPIO1        GPIO1          1              1
> > + * GPIO0        GPIO0          0              0
> > + */
> > +
> >  static int sc16is7xx_gpio_get(struct gpio_chip *chip, unsigned offset)
> >  {
> >  	unsigned int val;
> >  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> >  	struct uart_port *port = &s->p[0].port;
> >  
> > +	offset += s->gpio_offset;
> >  	val = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
> >  
> >  	return !!(val & BIT(offset));
> > @@ -1314,6 +1353,7 @@ static void sc16is7xx_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> >  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> >  	struct uart_port *port = &s->p[0].port;
> >  
> > +	offset += s->gpio_offset;
> >  	sc16is7xx_port_update(port, SC16IS7XX_IOSTATE_REG, BIT(offset),
> >  			      val ? BIT(offset) : 0);
> >  }
> > @@ -1324,6 +1364,7 @@ static int sc16is7xx_gpio_direction_input(struct gpio_chip *chip,
> >  	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> >  	struct uart_port *port = &s->p[0].port;
> >  
> > +	offset += s->gpio_offset;
> >  	sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset), 0);
> >  
> >  	return 0;
> > @@ -1336,6 +1377,8 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
> >  	struct uart_port *port = &s->p[0].port;
> >  	u8 state = sc16is7xx_port_read(port, SC16IS7XX_IOSTATE_REG);
> >  
> > +	offset += s->gpio_offset;
> > +
> >  	if (val)
> >  		state |= BIT(offset);
> >  	else
> > @@ -1395,6 +1438,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  
> >  #ifdef CONFIG_GPIOLIB
> >  	s->gpio_configured = devtype->nr_gpio;
> > +	s->gpio_offset = 0;
> >  #endif /* CONFIG_GPIOLIB */
> >  
> >  	/* Always ask for fixed clock rate from a property. */
> > @@ -1529,16 +1573,24 @@ static int sc16is7xx_probe(struct device *dev,
> >  #endif /* CONFIG_GPIOLIB */
> >  			}
> >  
> > -		if (val)
> > +		if (val) {
> > +#ifdef CONFIG_GPIOLIB
> > +			/* Additional I/O regs offset. */
> > +			if (val == SC16IS7XX_IOCONTROL_MODEM_B_BIT)
> > +				s->gpio_offset = SC16IS7XX_GPIOS_PER_BANK;
> > +#endif /* CONFIG_GPIOLIB */
> > +
> >  			regmap_update_bits(
> >  				s->regmap,
> >  				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> >  				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> >  				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> > +		}
> >  	}
> >  
> >  #ifdef CONFIG_GPIOLIB
> >  	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> > +	dev_dbg(dev, "GPIOs offset: %d\n", s->gpio_offset);
> >  
> >  	if (s->gpio_configured) {
> >  		/* Setup GPIO controller */
> > 
> 
> Is the order of this and 8/11 patch correct or should this precede that 
> other patch? That is, is 8/11 alone useful enough or would this also be 
> wanted? (I'm aware that 8/11 has a Fixes tag).

In fact, this patch (9/11) could be considered to be part of 8/11. I decided to separate it in order to facilitate the review process.

Maybe I should merge them...

Hugo.

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[Hugo Villeneuve]

On Thu, 25 May 2023 14:22:46 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:23AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > If the shared GPIO pins on a dual port/channel variant like the
> > SC16IS752 are configured as GPIOs for port A, and modem control lines
> > on port A, we need to translate the Linux GPIO offset to an offset
> > that is compatible with the I/O registers of the SC16IS7XX (IOState,
> > IODir and IOIntEna).
> > 
> > Add a new variable to store that offset and set it when we detect that
> > special case.
> 
> ...
> 
> > +/*
> > + * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
> > + * This is needed only for the case where a dual port variant is configured to
> > + * have only port B as modem status lines.
> > + *
> > + * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
> > + * lower bank (port B) set as modem status lines (special case described above):
> > + *
> > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > + * name        function     offset         offset
> > + * --------------------------------------------------
> > + * GPIO7/RIA    GPIO7          3              7
> > + * GPIO6/CDA    GPIO6          2              6
> > + * GPIO5/DTRA   GPIO5          1              5
> > + * GPIO4/DSRA   GPIO4          0              4
> > + * GPIO3/RIB    RIB           N/A            N/A
> > + * GPIO2/CDB    CDB           N/A            N/A
> > + * GPIO1/DTRB   DTRB          N/A            N/A
> > + * GPIO0/DSRB   DSRB          N/A            N/A
> > + *
> > + * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,
> 
> Single space is enough.

Fixed.

 
> > + * and lower bank (3..0) as GPIOs:
> > + *
> > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > + * name        function     offset         offset
> > + * --------------------------------------------------
> > + * GPIO7/RI     RI            N/A            N/A
> > + * GPIO6/CD     CD            N/A            N/A
> > + * GPIO5/DTR    DTR           N/A            N/A
> > + * GPIO4/DSR    DSR           N/A            N/A
> > + * GPIO3        GPIO3          3              3
> > + * GPIO2        GPIO2          2              2
> > + * GPIO1        GPIO1          1              1
> > + * GPIO0        GPIO0          0              0
> > + */
> 
> Wondering if you can always register 8 pins and use valid mask to define which
> one are in use?

I will look into it, I think it may be a good idea and could help to simplify things a bit.

Hugo.

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[Hugo Villeneuve]

On Thu, 25 May 2023 11:31:45 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Thu, 25 May 2023 14:22:46 +0300
> andy.shevchenko@gmail.com wrote:
> 
> > Thu, May 25, 2023 at 12:03:23AM -0400, Hugo Villeneuve kirjoitti:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > If the shared GPIO pins on a dual port/channel variant like the
> > > SC16IS752 are configured as GPIOs for port A, and modem control lines
> > > on port A, we need to translate the Linux GPIO offset to an offset
> > > that is compatible with the I/O registers of the SC16IS7XX (IOState,
> > > IODir and IOIntEna).
> > > 
> > > Add a new variable to store that offset and set it when we detect that
> > > special case.
> > 
> > ...
> > 
> > > +/*
> > > + * We may need to translate the Linux GPIO offset to a SC16IS7XX offset.
> > > + * This is needed only for the case where a dual port variant is configured to
> > > + * have only port B as modem status lines.
> > > + *
> > > + * Example for SC16IS752/762 with upper bank (port A) set as GPIOs, and
> > > + * lower bank (port B) set as modem status lines (special case described above):
> > > + *
> > > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > > + * name        function     offset         offset
> > > + * --------------------------------------------------
> > > + * GPIO7/RIA    GPIO7          3              7
> > > + * GPIO6/CDA    GPIO6          2              6
> > > + * GPIO5/DTRA   GPIO5          1              5
> > > + * GPIO4/DSRA   GPIO4          0              4
> > > + * GPIO3/RIB    RIB           N/A            N/A
> > > + * GPIO2/CDB    CDB           N/A            N/A
> > > + * GPIO1/DTRB   DTRB          N/A            N/A
> > > + * GPIO0/DSRB   DSRB          N/A            N/A
> > > + *
> > > + * Example  for SC16IS750/760 with upper bank (7..4) set as modem status lines,
> > 
> > Single space is enough.
> 
> Fixed.
> 
>  
> > > + * and lower bank (3..0) as GPIOs:
> > > + *
> > > + * Pin         GPIO pin     Linux GPIO     SC16IS7XX
> > > + * name        function     offset         offset
> > > + * --------------------------------------------------
> > > + * GPIO7/RI     RI            N/A            N/A
> > > + * GPIO6/CD     CD            N/A            N/A
> > > + * GPIO5/DTR    DTR           N/A            N/A
> > > + * GPIO4/DSR    DSR           N/A            N/A
> > > + * GPIO3        GPIO3          3              3
> > > + * GPIO2        GPIO2          2              2
> > > + * GPIO1        GPIO1          1              1
> > > + * GPIO0        GPIO0          0              0
> > > + */
> > 
> > Wondering if you can always register 8 pins and use valid mask to define which
> > one are in use?
> 
> I will look into it, I think it may be a good idea and could help to simplify things a bit.

Hi,
finally, this was the way to go. The resulting code/patch is much simpler and elegant this way. Thank you for the suggestion.

I will submit a V4 with all the changes.

Hugo.

Re: [PATCH v3 11/11] serial: sc16is7xx: add dump registers function

[Hugo Villeneuve]

On Thu, 25 May 2023 14:26:43 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > With this driver, it is very hard to debug the registers using
> > the /sys/kernel/debug/regmap interface.
> > 
> > The main reason is that bits 0 and 1 of the register address
> > correspond to the channels bits, so the register address itself starts
> > at bit 2, so we must 'mentally' shift each register address by 2 bits
> > to get its offset.
> > 
> > Also, only channels 0 and 1 are supported, so combinations of bits
> > 0 and 1 being 10b and 11b are invalid, and the display of these
> > registers is useless.
> > 
> > For example:
> > 
> > cat /sys/kernel/debug/regmap/spi0.0/registers
> > 04: 10 -> Port 0, register offset 1
> > 05: 10 -> Port 1, register offset 1
> > 06: 00 -> Port 2, register offset 1 -> invalid
> > 07: 00 -> port 3, register offset 1 -> invalid
> > ...
> > 
> > Add a debug module parameter to call a custom dump function for each
> > port registers after the probe phase to help debug.
> 
> Not sure about this. Can we rather create an abstract mapping on regmap?
> (Something like gpio-pca953x.c has)

Hi,
maybe we can, but more like they do in the driver max310x.c (single, dual and quad UART versions).

I will look into it, but it will probably be a patch that affects a lot of the code, and that I would like to submit separately after this serie, and so I will probably simply drop this current patch (11/11) since it will not be needed anymore.

Hugo.

Re: [PATCH v3 07/11] dt-bindings: sc16is7xx: Add property to change GPIO function

[andy.shevchenko]

Thu, May 25, 2023 at 10:34:43AM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 14:11:22 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:21AM -0400, Hugo Villeneuve kirjoitti:

...

> > I'm wondering if we can convert this to YAML first and then add a new
> > property.
> 
> I also thought about it, then decided to focus on simply adding the new
> property first since I am not an expert in YAML.
> 
> I think it would be best to do it after this patch series. Keep in mind that
> the original intent of this patch series, and this new property, was to fix a
> regression related to the GPIOs, and I think that converting to YAML would
> simply delay and add much noise to the discussion at this point.

The patch doesn't have Fixes tag, so it's definitely not a fix. Also new
property can fix a regression, it's impossible to fix the users' DTBs.

> If someone wants to do it as a separate patch after this, fine. If not, I an
> willing to give it a go.

Not a DT maintainer here, I'm fine with either way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] serial: sc16is7xx: fix regression with GPIO configuration

[andy.shevchenko]

Thu, May 25, 2023 at 11:02:55AM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 14:19:52 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti:

...

> > I'm wondering if we can avoid adding new ifdefferies...
> 
> I am simply following waht was already done in the existing driver.
> 
> Are you suggesting that we need to remove all these #defines? If not, what
> exactly do you suggest?

I was wondering and have nothing to suggest here. It seems a burden we have to
cope with for now.

> > > +	s->gpio_configured = devtype->nr_gpio;
> > 
> > The name of the variable is a bit vague WRT its content.
> > Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?
> 
> Maybe the name could be improved (and/or comments).
> 
> devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by
> the chip.
> 
> s->gpio_configured is the number of GPIOs that are configured or requested
> according to the presence (or not) of the modem-control-line-ports property.
> 
> I wanted to avoid using the same name to avoid potential confusion.
> 
> Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and
> s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured?

Maybe, but first try the approach with valid mask being involved. It may be
that we won't need this variable at all.

...

> > > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > +					 prop, p, u)
> > 
> > The driver so far is agnostic to property provider. Please keep it that way,
> > i.e. no of_ APIs.
> 
> The driver, before my patches, was already using the exact same function
> of_property_for_each_u32() to process the irda-mode-ports property, so I
> don't understand your comment.

This is unfortunate. I missed that one, but i don't care about IrDA so much.

> But what do you suggest instead of of_property_for_each_u32()? And do we need
> to change it also for processing the irda-mode-ports property?

device_property_read_u32_array().

Independently on the IrDA case, this one is more important and would have
consequences if we avoid agnostic APIs.

...

> > > +			if (u < devtype->nr_uart) {
> > 
> > Hmm... What other can it be?
> 
> Again, this is similar to the handling of the irda-mode-ports property.
> 
> But I am not sure I understand your question/concern?
> 
> I think this check is important, because if someone puts the following
> property in a DT:
> 
>     nxp,modem-control-line-ports = <0 1>;
> 
> but the variant only supports 1 port, then the check is usefull, no?

But you have below checks for u value. Wouldn't be enough?

> > > +				/* Use GPIO lines as modem control lines */
> > > +				if (u == 0)
> > > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > +				else if (u == 1)
> > > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > +

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] serial: sc16is7xx: add I/O register translation offset

[andy.shevchenko]

Thu, May 25, 2023 at 01:20:46PM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 11:31:45 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Thu, 25 May 2023 14:22:46 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Thu, May 25, 2023 at 12:03:23AM -0400, Hugo Villeneuve kirjoitti:

...

> > > Wondering if you can always register 8 pins and use valid mask to define which
> > > one are in use?
> > 
> > I will look into it, I think it may be a good idea and could help to
> > simplify things a bit.
> 
> finally, this was the way to go. The resulting code/patch is much simpler and
> elegant this way. Thank you for the suggestion.
> 
> I will submit a V4 with all the changes.

Thank you for trying!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/11] serial: sc16is7xx: add dump registers function

[andy.shevchenko]

Thu, May 25, 2023 at 03:49:46PM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 14:26:43 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti:

...

> > Not sure about this. Can we rather create an abstract mapping on regmap?
> > (Something like gpio-pca953x.c has)
> 
> maybe we can, but more like they do in the driver max310x.c (single, dual and
> quad UART versions).
> 
> I will look into it, but it will probably be a patch that affects a lot of
> the code, and that I would like to submit separately after this serie, and so
> I will probably simply drop this current patch (11/11) since it will not be
> needed anymore.

Whatever, I commented on this solely because Greg KH is usually against new
module parameters. If you sell your point to him, fine to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/11] serial: sc16is7xx: add dump registers function

[Greg KH]

On Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> With this driver, it is very hard to debug the registers using
> the /sys/kernel/debug/regmap interface.
> 
> The main reason is that bits 0 and 1 of the register address
> correspond to the channels bits, so the register address itself starts
> at bit 2, so we must 'mentally' shift each register address by 2 bits
> to get its offset.
> 
> Also, only channels 0 and 1 are supported, so combinations of bits
> 0 and 1 being 10b and 11b are invalid, and the display of these
> registers is useless.
> 
> For example:
> 
> cat /sys/kernel/debug/regmap/spi0.0/registers
> 04: 10 -> Port 0, register offset 1
> 05: 10 -> Port 1, register offset 1
> 06: 00 -> Port 2, register offset 1 -> invalid
> 07: 00 -> port 3, register offset 1 -> invalid
> ...
> 
> Add a debug module parameter to call a custom dump function for each
> port registers after the probe phase to help debug.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 03d00b144304..693b6cc371f8 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -347,6 +347,10 @@ struct sc16is7xx_port {
>  	struct sc16is7xx_one		p[];
>  };
>  
> +static bool debug;
> +module_param(debug, bool, 0644);
> +MODULE_PARM_DESC(debug, "enable/disable debug messages");

Sorry, but no, use the normal dynamic debugging logic that the whole
rest of the kernel uses.  Do not add random per-driver module parameters
like this, that would be a regression from the existing infrastructure
that we have in place already.

thanks,

greg k-h