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

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

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hello,
this patch series mainly fixes a GPIO regression and improve RS485 flags and
properties detection from DT.

It now also includes various small fixes and improvements that were previously
sent as separate patches, but that made testing everything difficult.

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

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

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

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

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

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

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

Patch 9 improves comments about chip variants.

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

Thank you.

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

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

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

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

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

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

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

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


base-commit: 9e87b63ed37e202c77aa17d4112da6ae0c7c097c
-- 
2.30.2

[PATCH v7 1/9] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

The sc16is7xx_config_rs485() function is called only for the second
port (index 1, channel B), causing initialization problems for the
first port.

For the sc16is7xx driver, port->membase and port->mapbase are not set,
and their default values are 0. And we set port->iobase to the device
index. This means that when the first device is registered using the
uart_add_one_port() function, the following values will be in the port
structure:
    port->membase = 0
    port->mapbase = 0
    port->iobase  = 0

Therefore, the function uart_configure_port() in serial_core.c will
exit early because of the following check:
	/*
	 * If there isn't a port here, don't do anything further.
	 */
	if (!port->iobase && !port->mapbase && !port->membase)
		return;

Typically, I2C and SPI drivers do not set port->membase and
port->mapbase.

The max310x driver sets port->membase to ~0 (all ones). By
implementing the same change in this driver, uart_configure_port() is
now correctly executed for all ports.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index abad091baeea..faa51a58671f 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
 		s->p[i].port.iobase	= i;
+		s->p[i].port.membase	= (void __iomem *)~0;
 		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
-- 
2.30.2

[PATCH v7 2/9] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

Therefore mark IOCONTROL register as a volatile register.

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index faa51a58671f..0c903d44429c 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 	case SC16IS7XX_TXLVL_REG:
 	case SC16IS7XX_RXLVL_REG:
 	case SC16IS7XX_IOSTATE_REG:
+	case SC16IS7XX_IOCONTROL_REG:
 		return true;
 	default:
 		break;
-- 
2.30.2

[PATCH v7 3/9] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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

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

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

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

[PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
changed the function of the GPIOs pins to act as modem control
lines without any possibility of selecting GPIO function.

As a consequence, applications that depends on GPIO lines configured
by default as GPIO pins no longer work as expected.

Also, the change to select modem control lines function was done only
for channel A of dual UART variants (752/762). This was not documented
in the log message.

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

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

When registering GPIO chip controller, mask-out GPIO pins declared as
modem control lines according to this new "modem-control-line-ports"
DT property.

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

Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
 1 file changed, 82 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d50674d2d0e..edc83f5f6340 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@
 
 /* IOControl register bits (Only 750/760) */
 #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
-#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
+#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
+#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
 #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
 
 /* EFCR register bits */
@@ -301,12 +302,12 @@
 /* Misc definitions */
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_REG_SHIFT		2
+#define SC16IS7XX_GPIOS_PER_BANK	4
 
 struct sc16is7xx_devtype {
 	char	name[10];
 	int	nr_gpio;
 	int	nr_uart;
-	int	has_mctrl;
 };
 
 #define SC16IS7XX_RECONF_MD		(1 << 0)
@@ -336,6 +337,7 @@ struct sc16is7xx_port {
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
+	unsigned long			gpio_valid_mask;
 #endif
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
@@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
 	.name		= "SC16IS74X",
 	.nr_gpio	= 0,
 	.nr_uart	= 1,
-	.has_mctrl	= 0,
 };
 
 static const struct sc16is7xx_devtype sc16is750_devtype = {
 	.name		= "SC16IS750",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is752_devtype = {
 	.name		= "SC16IS752",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is760_devtype = {
 	.name		= "SC16IS760",
-	.nr_gpio	= 4,
+	.nr_gpio	= 8,
 	.nr_uart	= 1,
-	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.name		= "SC16IS762",
-	.nr_gpio	= 0,
+	.nr_gpio	= 8,
 	.nr_uart	= 2,
-	.has_mctrl	= 1,
 };
 
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
@@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
-static int sc16is7xx_setup_gpio_chip(struct device *dev)
+static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
+					  unsigned long *valid_mask,
+					  unsigned int ngpios)
+{
+	struct sc16is7xx_port *s = gpiochip_get_data(chip);
+
+	*valid_mask = s->gpio_valid_mask;
+
+	return 0;
+}
+
+static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(dev);
 
 	if (!s->devtype->nr_gpio)
 		return 0;
 
+	switch (mctrl_mask) {
+	case 0:
+		s->gpio_valid_mask = GENMASK(7, 0);
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
+		s->gpio_valid_mask = GENMASK(3, 0);
+		break;
+	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
+		s->gpio_valid_mask = GENMASK(7, 4);
+		break;
+	default:
+		break;
+	}
+
+	if (s->gpio_valid_mask == 0)
+		return 0;
+
 	s->gpio.owner		 = THIS_MODULE;
 	s->gpio.parent		 = dev;
 	s->gpio.label		 = dev_name(dev);
+	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
 	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
 	s->gpio.get		 = sc16is7xx_gpio_get;
 	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
@@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
 }
 #endif
 
+static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
+{
+	struct sc16is7xx_port *s = dev_get_drvdata(dev);
+	int i;
+	int ret;
+	int count;
+	u32 mctrl_port[2];
+	u8 mctrl_mask;
+
+	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
+	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+		return 0;
+
+	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
+					     mctrl_port, count);
+	if (ret)
+		return 0;
+
+	mctrl_mask = 0;
+
+	for (i = 0; i < count; i++) {
+		/* Use GPIO lines as modem control lines */
+		if (mctrl_port[i] == 0)
+			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+		else if (mctrl_port[i] == 1)
+			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+	}
+
+	if (mctrl_mask)
+		regmap_update_bits(
+			s->regmap,
+			SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+			SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+			SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask);
+
+	return mctrl_mask;
+}
+
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
@@ -1383,6 +1447,7 @@ static int sc16is7xx_probe(struct device *dev,
 {
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	unsigned int val;
+	u8 mctrl_mask;
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
@@ -1478,12 +1543,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);
@@ -1521,8 +1580,10 @@ static int sc16is7xx_probe(struct device *dev,
 				s->p[u].irda_mode = true;
 	}
 
+	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
+
 #ifdef CONFIG_GPIOLIB
-	ret = sc16is7xx_setup_gpio_chip(dev);
+	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
 	if (ret)
 		goto out_thread;
 #endif
@@ -1547,7 +1608,7 @@ static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 
 out_thread:
@@ -1573,7 +1634,7 @@ static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 #endif
 
-- 
2.30.2

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[kernel test robot]

Hi Hugo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9e87b63ed37e202c77aa17d4112da6ae0c7c097c]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
base:   9e87b63ed37e202c77aa17d4112da6ae0c7c097c
patch link:    https://lore.kernel.org/r/20230602152626.284324-6-hugo%40hugovil.com
patch subject: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration
config: microblaze-randconfig-s052-20230531 (https://download.01.org/0day-ci/archive/20230603/202306030535.SjzsDJSD-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.3.0
reproduce:
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/24626643fe711f447b04a2421ef68e8e8cce86d1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
        git checkout 24626643fe711f447b04a2421ef68e8e8cce86d1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/tty/serial/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306030535.SjzsDJSD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1450:12: warning: variable 'mctrl_mask' set but not used [-Wunused-but-set-variable]
    1450 |         u8 mctrl_mask;
         |            ^~~~~~~~~~


vim +/mctrl_mask +1450 drivers/tty/serial/sc16is7xx.c

  1443	
  1444	static int sc16is7xx_probe(struct device *dev,
  1445				   const struct sc16is7xx_devtype *devtype,
  1446				   struct regmap *regmap, int irq)
  1447	{
  1448		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1449		unsigned int val;
> 1450		u8 mctrl_mask;
  1451		u32 uartclk = 0;
  1452		int i, ret;
  1453		struct sc16is7xx_port *s;
  1454	
  1455		if (IS_ERR(regmap))
  1456			return PTR_ERR(regmap);
  1457	
  1458		/*
  1459		 * This device does not have an identification register that would
  1460		 * tell us if we are really connected to the correct device.
  1461		 * The best we can do is to check if communication is at all possible.
  1462		 */
  1463		ret = regmap_read(regmap,
  1464				  SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
  1465		if (ret < 0)
  1466			return -EPROBE_DEFER;
  1467	
  1468		/* Alloc port structure */
  1469		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1470		if (!s) {
  1471			dev_err(dev, "Error allocating port structure\n");
  1472			return -ENOMEM;
  1473		}
  1474	
  1475		/* Always ask for fixed clock rate from a property. */
  1476		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1477	
  1478		s->clk = devm_clk_get_optional(dev, NULL);
  1479		if (IS_ERR(s->clk))
  1480			return PTR_ERR(s->clk);
  1481	
  1482		ret = clk_prepare_enable(s->clk);
  1483		if (ret)
  1484			return ret;
  1485	
  1486		freq = clk_get_rate(s->clk);
  1487		if (freq == 0) {
  1488			if (uartclk)
  1489				freq = uartclk;
  1490			if (pfreq)
  1491				freq = *pfreq;
  1492			if (freq)
  1493				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1494			else
  1495				return -EINVAL;
  1496		}
  1497	
  1498		s->regmap = regmap;
  1499		s->devtype = devtype;
  1500		dev_set_drvdata(dev, s);
  1501		mutex_init(&s->efr_lock);
  1502	
  1503		kthread_init_worker(&s->kworker);
  1504		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1505					      "sc16is7xx");
  1506		if (IS_ERR(s->kworker_task)) {
  1507			ret = PTR_ERR(s->kworker_task);
  1508			goto out_clk;
  1509		}
  1510		sched_set_fifo(s->kworker_task);
  1511	
  1512		/* reset device, purging any pending irq / data */
  1513		regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
  1514				SC16IS7XX_IOCONTROL_SRESET_BIT);
  1515	
  1516		for (i = 0; i < devtype->nr_uart; ++i) {
  1517			s->p[i].line		= i;
  1518			/* Initialize port data */
  1519			s->p[i].port.dev	= dev;
  1520			s->p[i].port.irq	= irq;
  1521			s->p[i].port.type	= PORT_SC16IS7XX;
  1522			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1523			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1524			s->p[i].port.iobase	= i;
  1525			s->p[i].port.membase	= (void __iomem *)~0;
  1526			s->p[i].port.iotype	= UPIO_PORT;
  1527			s->p[i].port.uartclk	= freq;
  1528			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1529			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1530			s->p[i].port.ops	= &sc16is7xx_ops;
  1531			s->p[i].old_mctrl	= 0;
  1532			s->p[i].port.line	= sc16is7xx_alloc_line();
  1533	
  1534			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1535				ret = -ENOMEM;
  1536				goto out_ports;
  1537			}
  1538	
  1539			/* Disable all interrupts */
  1540			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1541			/* Disable TX/RX */
  1542			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1543					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1544					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1545	
  1546			/* Initialize kthread work structs */
  1547			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1548			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1549			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1550			/* Register port */
  1551			uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1552	
  1553			/* Enable EFR */
  1554			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1555					     SC16IS7XX_LCR_CONF_MODE_B);
  1556	
  1557			regcache_cache_bypass(s->regmap, true);
  1558	
  1559			/* Enable write access to enhanced features */
  1560			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1561					     SC16IS7XX_EFR_ENABLE_BIT);
  1562	
  1563			regcache_cache_bypass(s->regmap, false);
  1564	
  1565			/* Restore access to general registers */
  1566			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1567	
  1568			/* Go to suspend mode */
  1569			sc16is7xx_power(&s->p[i].port, 0);
  1570		}
  1571	
  1572		if (dev->of_node) {
  1573			struct property *prop;
  1574			const __be32 *p;
  1575			u32 u;
  1576	
  1577			of_property_for_each_u32(dev->of_node, "irda-mode-ports",
  1578						 prop, p, u)
  1579				if (u < devtype->nr_uart)
  1580					s->p[u].irda_mode = true;
  1581		}
  1582	
  1583		mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
  1584	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> changed the function of the GPIOs pins to act as modem control
> lines without any possibility of selecting GPIO function.
> 
> As a consequence, applications that depends on GPIO lines configured
> by default as GPIO pins no longer work as expected.
> 
> Also, the change to select modem control lines function was done only
> for channel A of dual UART variants (752/762). This was not documented
> in the log message.
> 
> Allow to specify GPIO or modem control line function in the device
> tree, and for each of the ports (A or B).
> 
> Do so by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> When registering GPIO chip controller, mask-out GPIO pins declared as
> modem control lines according to this new "modem-control-line-ports"
> DT property.
> 
> Boards that need to have GPIOS configured as modem control lines
> should add that property to their device tree. Here is a list of
> boards using the sc16is7xx driver in their device tree and that may
> need to be modified:
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
> Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
> Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
> Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
> Cc: <stable@vger.kernel.org> # 6.1.x
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
>  1 file changed, 82 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7d50674d2d0e..edc83f5f6340 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -236,7 +236,8 @@
>  
>  /* IOControl register bits (Only 750/760) */
>  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
>  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
>  
>  /* EFCR register bits */
> @@ -301,12 +302,12 @@
>  /* Misc definitions */
>  #define SC16IS7XX_FIFO_SIZE		(64)
>  #define SC16IS7XX_REG_SHIFT		2
> +#define SC16IS7XX_GPIOS_PER_BANK	4
>  
>  struct sc16is7xx_devtype {
>  	char	name[10];
>  	int	nr_gpio;
>  	int	nr_uart;
> -	int	has_mctrl;
>  };
>  
>  #define SC16IS7XX_RECONF_MD		(1 << 0)
> @@ -336,6 +337,7 @@ struct sc16is7xx_port {
>  	struct clk			*clk;
>  #ifdef CONFIG_GPIOLIB
>  	struct gpio_chip		gpio;
> +	unsigned long			gpio_valid_mask;
>  #endif
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
> @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
>  	.name		= "SC16IS74X",
>  	.nr_gpio	= 0,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 0,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is750_devtype = {
>  	.name		= "SC16IS750",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is752_devtype = {
>  	.name		= "SC16IS752",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is760_devtype = {
>  	.name		= "SC16IS760",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is762_devtype = {
>  	.name		= "SC16IS762",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
>  	return 0;
>  }
>  
> -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
> +					  unsigned long *valid_mask,
> +					  unsigned int ngpios)
> +{
> +	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> +
> +	*valid_mask = s->gpio_valid_mask;
> +
> +	return 0;
> +}
> +
> +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
>  {
>  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
>  
>  	if (!s->devtype->nr_gpio)
>  		return 0;
>  
> +	switch (mctrl_mask) {
> +	case 0:
> +		s->gpio_valid_mask = GENMASK(7, 0);
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> +		s->gpio_valid_mask = GENMASK(3, 0);
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> +		s->gpio_valid_mask = GENMASK(7, 4);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (s->gpio_valid_mask == 0)
> +		return 0;
> +
>  	s->gpio.owner		 = THIS_MODULE;
>  	s->gpio.parent		 = dev;
>  	s->gpio.label		 = dev_name(dev);
> +	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
>  	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
>  	s->gpio.get		 = sc16is7xx_gpio_get;
>  	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
>  }
>  #endif
>  
> +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)

This returns what, mctrl?  If so, please document that, it doesn't look
obvious.  And as the kernel test robot reported, you do nothing with the
return value so why compute it?

And you have a real port here, no need to pass in a "raw" struct device,
right?

thanks,

greg k-h

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:

...

> > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
>
> This returns what, mctrl?  If so, please document that, it doesn't look
> obvious.

Good suggestion. Because I also stumbled over the returned type.

>  And as the kernel test robot reported, you do nothing with the
> return value so why compute it?

It seems that the entire function and respective call has to be moved
under #ifdef CONFIG_GPIOLIB.

> And you have a real port here, no need to pass in a "raw" struct device,
> right?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 14:57:31 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> 
> ...
> 
> > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> >
> > This returns what, mctrl?  If so, please document that, it doesn't look
> > obvious.
> 
> Good suggestion. Because I also stumbled over the returned type.
> 
> >  And as the kernel test robot reported, you do nothing with the
> > return value so why compute it?
> 
> It seems that the entire function and respective call has to be moved
> under #ifdef CONFIG_GPIOLIB.

Hi,
it cannot. See my explanations in response to Greg's comments.

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> On Sun, 4 Jun 2023 14:57:31 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> >
> > ...
> >
> > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> > >
> > > This returns what, mctrl?  If so, please document that, it doesn't look
> > > obvious.
> >
> > Good suggestion. Because I also stumbled over the returned type.
> >
> > >  And as the kernel test robot reported, you do nothing with the
> > > return value so why compute it?
> >
> > It seems that the entire function and respective call has to be moved
> > under #ifdef CONFIG_GPIOLIB.
>
> Hi,
> it cannot. See my explanations in response to Greg's comments.

Then as Greg suggested, store in the structure and make this function
to return an error code (with int), with this amendment you don't need
to add a comment about the returned variable anymore.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 22:31:04 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Sun, 4 Jun 2023 14:57:31 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > >
> > > ...
> > >
> > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> > > >
> > > > This returns what, mctrl?  If so, please document that, it doesn't look
> > > > obvious.
> > >
> > > Good suggestion. Because I also stumbled over the returned type.
> > >
> > > >  And as the kernel test robot reported, you do nothing with the
> > > > return value so why compute it?
> > >
> > > It seems that the entire function and respective call has to be moved
> > > under #ifdef CONFIG_GPIOLIB.
> >
> > Hi,
> > it cannot. See my explanations in response to Greg's comments.
> 
> Then as Greg suggested, store in the structure and make this function
> to return an error code (with int), with this amendment you don't need
> to add a comment about the returned variable anymore.

Hi,
Yes, that is what I have done for V8. Simplifies/clean things a lot.

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 22:31:04 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Sun, 4 Jun 2023 14:57:31 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > >
> > > ...
> > >
> > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> > > >
> > > > This returns what, mctrl?  If so, please document that, it doesn't look
> > > > obvious.
> > >
> > > Good suggestion. Because I also stumbled over the returned type.
> > >
> > > >  And as the kernel test robot reported, you do nothing with the
> > > > return value so why compute it?
> > >
> > > It seems that the entire function and respective call has to be moved
> > > under #ifdef CONFIG_GPIOLIB.
> >
> > Hi,
> > it cannot. See my explanations in response to Greg's comments.
> 
> Then as Greg suggested, store in the structure and make this function
> to return an error code (with int), with this amendment you don't need
> to add a comment about the returned variable anymore.

Hi Andy,
did you have a chance to look at V8 (sent two weks ago) which fixed all
of what we discussed?

Thank you,
Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Sun, 4 Jun 2023 22:31:04 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> did you have a chance to look at V8 (sent two weks ago) which fixed all
> of what we discussed?

The patch 6 already has my tag, anything specific you want me to do?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 20 Jun 2023 18:18:12 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Sun, 4 Jun 2023 22:31:04 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
> > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > of what we discussed?
> 
> The patch 6 already has my tag, anything specific you want me to do?

Hi Andy,
I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
since there were some changes involved in patch 6 and I wanted you to
review them. Can you confirm if the changes are correct?

I also added a new patch "remove obsolete out_thread label". It has no 
real impact on the code generation itself, but maybe you can review and
confirm if tags are ok or not, based on commit message and also
additional commit message.

Thank you, Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 20 Jun 2023 18:18:12 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > of what we discussed?
> >
> > The patch 6 already has my tag, anything specific you want me to do?
>
> Hi Andy,
> I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> since there were some changes involved in patch 6 and I wanted you to
> review them. Can you confirm if the changes are correct?
>
> I also added a new patch "remove obsolete out_thread label". It has no
> real impact on the code generation itself, but maybe you can review and
> confirm if tags are ok or not, based on commit message and also
> additional commit message.

Both are fine to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 20 Jun 2023 18:35:48 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 20 Jun 2023 18:18:12 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
> > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > of what we discussed?
> > >
> > > The patch 6 already has my tag, anything specific you want me to do?
> >
> > Hi Andy,
> > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > since there were some changes involved in patch 6 and I wanted you to
> > review them. Can you confirm if the changes are correct?
> >
> > I also added a new patch "remove obsolete out_thread label". It has no
> > real impact on the code generation itself, but maybe you can review and
> > confirm if tags are ok or not, based on commit message and also
> > additional commit message.
> 
> Both are fine to me.

Hi,
Ok, thank you for reviewing this.

I guess now we are good to go with this series if the stable tags and
patches order are good after Greg's review?

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Tue, 20 Jun 2023 18:35:48 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > of what we discussed?
> > > >
> > > > The patch 6 already has my tag, anything specific you want me to do?
> > >
> > > Hi Andy,
> > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > since there were some changes involved in patch 6 and I wanted you to
> > > review them. Can you confirm if the changes are correct?
> > >
> > > I also added a new patch "remove obsolete out_thread label". It has no
> > > real impact on the code generation itself, but maybe you can review and
> > > confirm if tags are ok or not, based on commit message and also
> > > additional commit message.
> >
> > Both are fine to me.
>
> Hi,
> Ok, thank you for reviewing this.
>
> I guess now we are good to go with this series if the stable tags and
> patches order are good after Greg's review?

Taking into account that we are at rc7, and even with Fixes tags in
your series I think Greg might take this after v6.5-0rc1 is out. It's
up to him how to proceed with that. Note, he usually has thousands of
patches in backlog, you might need to respin it after the above
mentioned rc1.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 20 Jun 2023 18:45:51 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Tue, 20 Jun 2023 18:35:48 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
> > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > of what we discussed?
> > > > >
> > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > >
> > > > Hi Andy,
> > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > since there were some changes involved in patch 6 and I wanted you to
> > > > review them. Can you confirm if the changes are correct?
> > > >
> > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > real impact on the code generation itself, but maybe you can review and
> > > > confirm if tags are ok or not, based on commit message and also
> > > > additional commit message.
> > >
> > > Both are fine to me.
> >
> > Hi,
> > Ok, thank you for reviewing this.
> >
> > I guess now we are good to go with this series if the stable tags and
> > patches order are good after Greg's review?
> 
> Taking into account that we are at rc7, and even with Fixes tags in
> your series I think Greg might take this after v6.5-0rc1 is out. It's
> up to him how to proceed with that. Note, he usually has thousands of
> patches in backlog, you might need to respin it after the above
> mentioned rc1.

Ok, understood.

Let's wait then.

Thank you.
Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Tue, 20 Jun 2023 12:16:45 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Tue, 20 Jun 2023 18:45:51 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > ...
> > 
> > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > of what we discussed?
> > > > > >
> > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > >
> > > > > Hi Andy,
> > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > review them. Can you confirm if the changes are correct?
> > > > >
> > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > real impact on the code generation itself, but maybe you can review and
> > > > > confirm if tags are ok or not, based on commit message and also
> > > > > additional commit message.
> > > >
> > > > Both are fine to me.
> > >
> > > Hi,
> > > Ok, thank you for reviewing this.
> > >
> > > I guess now we are good to go with this series if the stable tags and
> > > patches order are good after Greg's review?
> > 
> > Taking into account that we are at rc7, and even with Fixes tags in
> > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > up to him how to proceed with that. Note, he usually has thousands of
> > patches in backlog, you might need to respin it after the above
> > mentioned rc1.
> 
> Ok, understood.
> 
> Let's wait then.

Hi Andy/Greg,
we are now at v6.5-rc2 and I still do not see any of our patches in
linus or gregkh_tty repos.

Is there something missing from my part (or someone else) to go forward
with integrating these patches (v8) for v6.5?

Thank you,
Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> On Tue, 20 Jun 2023 12:16:45 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> > On Tue, 20 Jun 2023 18:45:51 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > 
> > > ...
> > > 
> > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > of what we discussed?
> > > > > > >
> > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > >
> > > > > > Hi Andy,
> > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > review them. Can you confirm if the changes are correct?
> > > > > >
> > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > additional commit message.
> > > > >
> > > > > Both are fine to me.
> > > >
> > > > Hi,
> > > > Ok, thank you for reviewing this.
> > > >
> > > > I guess now we are good to go with this series if the stable tags and
> > > > patches order are good after Greg's review?
> > > 
> > > Taking into account that we are at rc7, and even with Fixes tags in
> > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > up to him how to proceed with that. Note, he usually has thousands of
> > > patches in backlog, you might need to respin it after the above
> > > mentioned rc1.
> > 
> > Ok, understood.
> > 
> > Let's wait then.
> 
> Hi Andy/Greg,
> we are now at v6.5-rc2 and I still do not see any of our patches in
> linus or gregkh_tty repos.
> 
> Is there something missing from my part (or someone else) to go forward
> with integrating these patches (v8) for v6.5?

My queue is huge right now, please be patient, I want to have them all
handled by the end of next week...

You can always help out by reviewing other patches on the mailing list
to reduce my review load.

thanks,

greg k-h

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote:
> On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> > On Tue, 20 Jun 2023 12:16:45 -0400
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > 
> > > On Tue, 20 Jun 2023 18:45:51 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > 
> > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > 
> > > > ...
> > > > 
> > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > > of what we discussed?
> > > > > > > >
> > > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > > >
> > > > > > > Hi Andy,
> > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > > review them. Can you confirm if the changes are correct?
> > > > > > >
> > > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > > additional commit message.
> > > > > >
> > > > > > Both are fine to me.
> > > > >
> > > > > Hi,
> > > > > Ok, thank you for reviewing this.
> > > > >
> > > > > I guess now we are good to go with this series if the stable tags and
> > > > > patches order are good after Greg's review?
> > > > 
> > > > Taking into account that we are at rc7, and even with Fixes tags in
> > > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > > up to him how to proceed with that. Note, he usually has thousands of
> > > > patches in backlog, you might need to respin it after the above
> > > > mentioned rc1.
> > > 
> > > Ok, understood.
> > > 
> > > Let's wait then.
> > 
> > Hi Andy/Greg,
> > we are now at v6.5-rc2 and I still do not see any of our patches in
> > linus or gregkh_tty repos.
> > 
> > Is there something missing from my part (or someone else) to go forward
> > with integrating these patches (v8) for v6.5?
> 
> My queue is huge right now, please be patient, I want to have them all
> handled by the end of next week...
> 
> You can always help out by reviewing other patches on the mailing list
> to reduce my review load.

Wait, no, this series was superseeded by v8, and in there you said you
were going to send a new series.  So please, fix it up and send the
updated version of the series, this one isn't going to be applied for
obvious reasons.

thanks,

greg k-h

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Thu, 20 Jul 2023 21:38:21 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote:
> > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> > > On Tue, 20 Jun 2023 12:16:45 -0400
> > > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > 
> > > > On Tue, 20 Jun 2023 18:45:51 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > 
> > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > 
> > > > > ...
> > > > > 
> > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > > > of what we discussed?
> > > > > > > > >
> > > > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > > > >
> > > > > > > > Hi Andy,
> > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > > > review them. Can you confirm if the changes are correct?
> > > > > > > >
> > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > > > additional commit message.
> > > > > > >
> > > > > > > Both are fine to me.
> > > > > >
> > > > > > Hi,
> > > > > > Ok, thank you for reviewing this.
> > > > > >
> > > > > > I guess now we are good to go with this series if the stable tags and
> > > > > > patches order are good after Greg's review?
> > > > > 
> > > > > Taking into account that we are at rc7, and even with Fixes tags in
> > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > > > up to him how to proceed with that. Note, he usually has thousands of
> > > > > patches in backlog, you might need to respin it after the above
> > > > > mentioned rc1.
> > > > 
> > > > Ok, understood.
> > > > 
> > > > Let's wait then.
> > > 
> > > Hi Andy/Greg,
> > > we are now at v6.5-rc2 and I still do not see any of our patches in
> > > linus or gregkh_tty repos.
> > > 
> > > Is there something missing from my part (or someone else) to go forward
> > > with integrating these patches (v8) for v6.5?
> > 
> > My queue is huge right now, please be patient, I want to have them all
> > handled by the end of next week...
> > 
> > You can always help out by reviewing other patches on the mailing list
> > to reduce my review load.
> 
> Wait, no, this series was superseeded by v8, and in there you said you
> were going to send a new series.  So please, fix it up and send the
> updated version of the series, this one isn't going to be applied for
> obvious reasons.

Hi Greg,
I never said that I would resend another update for this current
serie (unless of course if it was to address a new comment). Re-reading
that email made me realise that it was maybe not perfectly clear the
way I wrote it.

What I said was that, once V8 was finally applied and
incorporated in the kernel, then I would send a completely new and
different serie to address issues/concerns/improvements/suggestions
noted during the review of this serie (example: conversion of bindings
to YAML and improve DTS node names, etc). We already agreed with some
maintainers (ex: Conor Dooley) that it was reasonnable to do so.

That is why I asked Andy if we were good to go with V8 and he
confirmed that, and that it was now up to you to integrate it if your
review was satisfactory.

Hope this clears things and we can integrate it soon.

Thank you, Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Fri, Jul 21, 2023 at 11:25:17AM -0400, Hugo Villeneuve wrote:
> On Thu, 20 Jul 2023 21:38:21 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote:
> > > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> > > > On Tue, 20 Jun 2023 12:16:45 -0400
> > > > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > 
> > > > > On Tue, 20 Jun 2023 18:45:51 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > 
> > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > > > > of what we discussed?
> > > > > > > > > >
> > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > > > > >
> > > > > > > > > Hi Andy,
> > > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > > > > review them. Can you confirm if the changes are correct?
> > > > > > > > >
> > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > > > > additional commit message.
> > > > > > > >
> > > > > > > > Both are fine to me.
> > > > > > >
> > > > > > > Hi,
> > > > > > > Ok, thank you for reviewing this.
> > > > > > >
> > > > > > > I guess now we are good to go with this series if the stable tags and
> > > > > > > patches order are good after Greg's review?
> > > > > > 
> > > > > > Taking into account that we are at rc7, and even with Fixes tags in
> > > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > > > > up to him how to proceed with that. Note, he usually has thousands of
> > > > > > patches in backlog, you might need to respin it after the above
> > > > > > mentioned rc1.
> > > > > 
> > > > > Ok, understood.
> > > > > 
> > > > > Let's wait then.
> > > > 
> > > > Hi Andy/Greg,
> > > > we are now at v6.5-rc2 and I still do not see any of our patches in
> > > > linus or gregkh_tty repos.
> > > > 
> > > > Is there something missing from my part (or someone else) to go forward
> > > > with integrating these patches (v8) for v6.5?
> > > 
> > > My queue is huge right now, please be patient, I want to have them all
> > > handled by the end of next week...
> > > 
> > > You can always help out by reviewing other patches on the mailing list
> > > to reduce my review load.
> > 
> > Wait, no, this series was superseeded by v8, and in there you said you
> > were going to send a new series.  So please, fix it up and send the
> > updated version of the series, this one isn't going to be applied for
> > obvious reasons.
> 
> Hi Greg,
> I never said that I would resend another update for this current
> serie (unless of course if it was to address a new comment). Re-reading
> that email made me realise that it was maybe not perfectly clear the
> way I wrote it.
> 
> What I said was that, once V8 was finally applied and
> incorporated in the kernel, then I would send a completely new and
> different serie to address issues/concerns/improvements/suggestions
> noted during the review of this serie (example: conversion of bindings
> to YAML and improve DTS node names, etc). We already agreed with some
> maintainers (ex: Conor Dooley) that it was reasonnable to do so.
> 
> That is why I asked Andy if we were good to go with V8 and he
> confirmed that, and that it was now up to you to integrate it if your
> review was satisfactory.
> 
> Hope this clears things and we can integrate it soon.

I don't have any of your series in my review queue at all, so please
resend them.

thanks,

greg k-h

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Fri, 21 Jul 2023 17:40:18 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Jul 21, 2023 at 11:25:17AM -0400, Hugo Villeneuve wrote:
> > On Thu, 20 Jul 2023 21:38:21 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Wed, Jul 19, 2023 at 09:14:23PM +0200, Greg KH wrote:
> > > > On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> > > > > On Tue, 20 Jun 2023 12:16:45 -0400
> > > > > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > 
> > > > > > On Tue, 20 Jun 2023 18:45:51 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > 
> > > > > > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > 
> > > > > > > ...
> > > > > > > 
> > > > > > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > > > > > of what we discussed?
> > > > > > > > > > >
> > > > > > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > > > > > >
> > > > > > > > > > Hi Andy,
> > > > > > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > > > > > review them. Can you confirm if the changes are correct?
> > > > > > > > > >
> > > > > > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > > > > > additional commit message.
> > > > > > > > >
> > > > > > > > > Both are fine to me.
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > > Ok, thank you for reviewing this.
> > > > > > > >
> > > > > > > > I guess now we are good to go with this series if the stable tags and
> > > > > > > > patches order are good after Greg's review?
> > > > > > > 
> > > > > > > Taking into account that we are at rc7, and even with Fixes tags in
> > > > > > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > > > > > up to him how to proceed with that. Note, he usually has thousands of
> > > > > > > patches in backlog, you might need to respin it after the above
> > > > > > > mentioned rc1.
> > > > > > 
> > > > > > Ok, understood.
> > > > > > 
> > > > > > Let's wait then.
> > > > > 
> > > > > Hi Andy/Greg,
> > > > > we are now at v6.5-rc2 and I still do not see any of our patches in
> > > > > linus or gregkh_tty repos.
> > > > > 
> > > > > Is there something missing from my part (or someone else) to go forward
> > > > > with integrating these patches (v8) for v6.5?
> > > > 
> > > > My queue is huge right now, please be patient, I want to have them all
> > > > handled by the end of next week...
> > > > 
> > > > You can always help out by reviewing other patches on the mailing list
> > > > to reduce my review load.
> > > 
> > > Wait, no, this series was superseeded by v8, and in there you said you
> > > were going to send a new series.  So please, fix it up and send the
> > > updated version of the series, this one isn't going to be applied for
> > > obvious reasons.
> > 
> > Hi Greg,
> > I never said that I would resend another update for this current
> > serie (unless of course if it was to address a new comment). Re-reading
> > that email made me realise that it was maybe not perfectly clear the
> > way I wrote it.
> > 
> > What I said was that, once V8 was finally applied and
> > incorporated in the kernel, then I would send a completely new and
> > different serie to address issues/concerns/improvements/suggestions
> > noted during the review of this serie (example: conversion of bindings
> > to YAML and improve DTS node names, etc). We already agreed with some
> > maintainers (ex: Conor Dooley) that it was reasonnable to do so.
> > 
> > That is why I asked Andy if we were good to go with V8 and he
> > confirmed that, and that it was now up to you to integrate it if your
> > review was satisfactory.
> > 
> > Hope this clears things and we can integrate it soon.
> 
> I don't have any of your series in my review queue at all, so please
> resend them.

OK, I will resend V8 then.

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 09:47:44 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> > 
> > Allow to specify GPIO or modem control line function in the device
> > tree, and for each of the ports (A or B).
> > 
> > Do so by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > When registering GPIO chip controller, mask-out GPIO pins declared as
> > modem control lines according to this new "modem-control-line-ports"
> > DT property.
> > 
> > Boards that need to have GPIOS configured as modem control lines
> > should add that property to their device tree. Here is a list of
> > boards using the sc16is7xx driver in their device tree and that may
> > need to be modified:
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
> > Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
> > Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
> > Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
> >  1 file changed, 82 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 7d50674d2d0e..edc83f5f6340 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >  
> >  /* IOControl register bits (Only 750/760) */
> >  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> > -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
> >  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
> >  
> >  /* EFCR register bits */
> > @@ -301,12 +302,12 @@
> >  /* Misc definitions */
> >  #define SC16IS7XX_FIFO_SIZE		(64)
> >  #define SC16IS7XX_REG_SHIFT		2
> > +#define SC16IS7XX_GPIOS_PER_BANK	4
> >  
> >  struct sc16is7xx_devtype {
> >  	char	name[10];
> >  	int	nr_gpio;
> >  	int	nr_uart;
> > -	int	has_mctrl;
> >  };
> >  
> >  #define SC16IS7XX_RECONF_MD		(1 << 0)
> > @@ -336,6 +337,7 @@ struct sc16is7xx_port {
> >  	struct clk			*clk;
> >  #ifdef CONFIG_GPIOLIB
> >  	struct gpio_chip		gpio;
> > +	unsigned long			gpio_valid_mask;
> >  #endif
> >  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
> >  	struct kthread_worker		kworker;
> > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >  	.name		= "SC16IS74X",
> >  	.nr_gpio	= 0,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 0,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is750_devtype = {
> >  	.name		= "SC16IS750",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >  	.name		= "SC16IS752",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is760_devtype = {
> >  	.name		= "SC16IS760",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is762_devtype = {
> >  	.name		= "SC16IS762",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> > @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
> >  	return 0;
> >  }
> >  
> > -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> > +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
> > +					  unsigned long *valid_mask,
> > +					  unsigned int ngpios)
> > +{
> > +	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> > +
> > +	*valid_mask = s->gpio_valid_mask;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
> >  {
> >  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> >  
> >  	if (!s->devtype->nr_gpio)
> >  		return 0;
> >  
> > +	switch (mctrl_mask) {
> > +	case 0:
> > +		s->gpio_valid_mask = GENMASK(7, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> > +		s->gpio_valid_mask = GENMASK(3, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> > +		s->gpio_valid_mask = GENMASK(7, 4);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (s->gpio_valid_mask == 0)
> > +		return 0;
> > +
> >  	s->gpio.owner		 = THIS_MODULE;
> >  	s->gpio.parent		 = dev;
> >  	s->gpio.label		 = dev_name(dev);
> > +	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
> >  	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
> >  	s->gpio.get		 = sc16is7xx_gpio_get;
> >  	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> > @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
> >  }
> >  #endif
> >  
> > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> 
> This returns what, mctrl?  If so, please document that, it doesn't look
> obvious.  And as the kernel test robot reported, you do nothing with the
> return value so why compute it?

Hi Greg,
I will  the following comment to document return value:

/*
 * Configure ports designated to operate as modem control lines.
 * Return mask of ports configured as modem control lines.
 */
static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)


The kernel test robot identified a case, when CONFIG_GPIOLIB is not defined, where the value returned by sc16is7xx_setup_mctrl_ports() is not used. But the function sc16is7xx_setup_mctrl_ports() still need to be called to configure the modem status line ports correctly in that case.

And if CONFIG_GPIOLIB is defined, the value is definitely used (and needed).

Here is what I suggest to silence the warning:

	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);

#ifdef CONFIG_GPIOLIB
	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
	if (ret)
		goto out_thread;
#else
	(void) mctrl_mask;
#endif

I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...


> And you have a real port here, no need to pass in a "raw" struct device,
> right?

The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
struct sc16is7xx_port from it:

    struct sc16is7xx_port *s = dev_get_drvdata(dev);

Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:

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

But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> Here is what I suggest to silence the warning:
> 
> 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> 
> #ifdef CONFIG_GPIOLIB
> 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> 	if (ret)
> 		goto out_thread;
> #else
> 	(void) mctrl_mask;
> #endif

Eeek,  no, please no...

First off, please don't put #ifdef in .c files if at all possible.
Secondly, that (void) craziness is just that.  Rework this to not be an
issue some other way please.

> I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...

Sure, that sounds best.

> > And you have a real port here, no need to pass in a "raw" struct device,
> > right?
> 
> The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> struct sc16is7xx_port from it:
> 
>     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> 
> Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> 
> static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> {
> 	struct device *dev = &s->p[0].port.dev;
> 
> But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

You should never need a "raw" struct device for stuff (if so, something
is really odd).  Except for error messages, but that's not really a big
deal, right?

Don't pass around struct device in a driver, use the real types as you
know you have it and it saves odd casting around and it just doesn't
look safe at all to do so.

And if you have that crazy s->p.... stuff in multiple places, then
perhaps you might want to rethink the structure somehow?  Or at the very
least, write an inline function to get it when needed.

Also, meta comment, you might want to use some \n characters in your
emails, your lines are really long :)

thanks,

greg k-h

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 20:29:58 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> > Here is what I suggest to silence the warning:
> > 
> > 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> > 
> > #ifdef CONFIG_GPIOLIB
> > 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> > 	if (ret)
> > 		goto out_thread;
> > #else
> > 	(void) mctrl_mask;
> > #endif
> 
> Eeek,  no, please no...
> 
> First off, please don't put #ifdef in .c files if at all possible.

Hi Greg,
Andy also made a similar comment, but couldn't suggest a valid
alternative when I asked him what to do about that.

Just as a sidenote, I didn't add those #ifdef, they were already
present in the driver in multiple places.

What would be your suggestion to get rid of those #ifdef, simply delete
them all?

If you suggest me what to do, I will be happy to submit a
future patch after this series is finalized to clean that aspect.


> Secondly, that (void) craziness is just that.  Rework this to not be an
> issue some other way please.
> 
> > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...
> 
> Sure, that sounds best.

Ok, I will do that.


> > > And you have a real port here, no need to pass in a "raw" struct device,
> > > right?
> > 
> > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> > struct sc16is7xx_port from it:
> > 
> >     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > 
> > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> > 
> > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> > {
> > 	struct device *dev = &s->p[0].port.dev;
> > 
> > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...
> 
> You should never need a "raw" struct device for stuff (if so, something
> is really odd).  Except for error messages, but that's not really a big
> deal, right?

> Don't pass around struct device in a driver, use the real types as you
> know you have it and it saves odd casting around and it just doesn't
> look safe at all to do so.

If you look at the patch, you will see that I need "struct device *dev"
at two places in the sc16is7xx_setup_mctrl_ports() function to read the
device properties:

...
+static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
...
+	count = device_property_count_u32(dev,...
...
+	ret = device_property_read_u32_array(dev,
...

I do not understand why this is odd?


> And if you have that crazy s->p.... stuff in multiple places, the
> perhaps you might want to rethink the structure somehow?  Or at the very
> least, write an inline function to get it when needed.

I am not sure what you mean by that, since again that "crazy" stuff is
already used everywhere in this driver?


> Also, meta comment, you might want to use some \n characters in your
> emails, your lines are really long :)

Strange, I use sylpheed as a mail client, and the option "Wrap lines at
72 characters" is enabled by default, but somehow you must also check
the box "Wrap on input" for it to work, not very intuitive :) Thanks for
pointing that to me.

Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 19:16:13 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Sun, 4 Jun 2023 20:29:58 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> > > Here is what I suggest to silence the warning:
> > > 
> > > 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> > > 
> > > #ifdef CONFIG_GPIOLIB
> > > 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> > > 	if (ret)
> > > 		goto out_thread;
> > > #else
> > > 	(void) mctrl_mask;
> > > #endif
> > 
> > Eeek,  no, please no...
> > 
> > First off, please don't put #ifdef in .c files if at all possible.
> 
> Hi Greg,
> Andy also made a similar comment, but couldn't suggest a valid
> alternative when I asked him what to do about that.
> 
> Just as a sidenote, I didn't add those #ifdef, they were already
> present in the driver in multiple places.
> 
> What would be your suggestion to get rid of those #ifdef, simply delete
> them all?
> 
> If you suggest me what to do, I will be happy to submit a
> future patch after this series is finalized to clean that aspect.
> 
> 
> > Secondly, that (void) craziness is just that.  Rework this to not be an
> > issue some other way please.
> > 
> > > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...
> > 
> > Sure, that sounds best.
> 
> Ok, I will do that.
> 
> 
> > > > And you have a real port here, no need to pass in a "raw" struct device,
> > > > right?
> > > 
> > > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> > > struct sc16is7xx_port from it:
> > > 
> > >     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > > 
> > > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> > > 
> > > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> > > {
> > > 	struct device *dev = &s->p[0].port.dev;
> > > 
> > > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...
> > 
> > You should never need a "raw" struct device for stuff (if so, something
> > is really odd).  Except for error messages, but that's not really a big
> > deal, right?
> 
> > Don't pass around struct device in a driver, use the real types as you
> > know you have it and it saves odd casting around and it just doesn't
> > look safe at all to do so.
> 
> If you look at the patch, you will see that I need "struct device *dev"
> at two places in the sc16is7xx_setup_mctrl_ports() function to read the
> device properties:
> 
> ...
> +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> ...
> +	count = device_property_count_u32(dev,...
> ...
> +	ret = device_property_read_u32_array(dev,
> ...
> 
> I do not understand why this is odd?

Hi Greg,
I finally added a "struct device" member inside "struct sc16is7xx_port"
and now I simply pass "struct sc16is7xx_port *s as the sole argument to
sc16is7xx_setup_mctrl_ports().

That should take care of your concern I hope, and I will submit a V8
soon with all these changes.

Hugo.


> > And if you have that crazy s->p.... stuff in multiple places, the
> > perhaps you might want to rethink the structure somehow?  Or at the very
> > least, write an inline function to get it when needed.
> 
> I am not sure what you mean by that, since again that "crazy" stuff is
> already used everywhere in this driver?
> 
> 
> > Also, meta comment, you might want to use some \n characters in your
> > emails, your lines are really long :)
> 
> Strange, I use sylpheed as a mail client, and the option "Wrap lines at
> 72 characters" is enabled by default, but somehow you must also check
> the box "Wrap on input" for it to work, not very intuitive :) Thanks for
> pointing that to me.
> 
> Hugo.

Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Sun, 4 Jun 2023 19:16:13 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Sun, 4 Jun 2023 20:29:58 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> > > Here is what I suggest to silence the warning:
> > > 
> > > 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> > > 
> > > #ifdef CONFIG_GPIOLIB
> > > 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> > > 	if (ret)
> > > 		goto out_thread;
> > > #else
> > > 	(void) mctrl_mask;
> > > #endif
> > 
> > Eeek,  no, please no...
> > 
> > First off, please don't put #ifdef in .c files if at all possible.
> 
> Hi Greg,
> Andy also made a similar comment, but couldn't suggest a valid
> alternative when I asked him what to do about that.
> 
> Just as a sidenote, I didn't add those #ifdef, they were already
> present in the driver in multiple places.
> 
> What would be your suggestion to get rid of those #ifdef, simply delete
> them all?
> 
> If you suggest me what to do, I will be happy to submit a
> future patch after this series is finalized to clean that aspect.

Hi Greg,
altough I just send a new V8, I am still curious to hear your point of
view about those #ifdef...

Hugo.


> > Secondly, that (void) craziness is just that.  Rework this to not be an
> > issue some other way please.
> > 
> > > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...
> > 
> > Sure, that sounds best.
> 
> Ok, I will do that.
> 
> 
> > > > And you have a real port here, no need to pass in a "raw" struct device,
> > > > right?
> > > 
> > > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> > > struct sc16is7xx_port from it:
> > > 
> > >     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > > 
> > > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> > > 
> > > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> > > {
> > > 	struct device *dev = &s->p[0].port.dev;
> > > 
> > > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...
> > 
> > You should never need a "raw" struct device for stuff (if so, something
> > is really odd).  Except for error messages, but that's not really a big
> > deal, right?
> 
> > Don't pass around struct device in a driver, use the real types as you
> > know you have it and it saves odd casting around and it just doesn't
> > look safe at all to do so.
> 
> If you look at the patch, you will see that I need "struct device *dev"
> at two places in the sc16is7xx_setup_mctrl_ports() function to read the
> device properties:
> 
> ...
> +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> ...
> +	count = device_property_count_u32(dev,...
> ...
> +	ret = device_property_read_u32_array(dev,
> ...
> 
> I do not understand why this is odd?
> 
> 
> > And if you have that crazy s->p.... stuff in multiple places, the
> > perhaps you might want to rethink the structure somehow?  Or at the very
> > least, write an inline function to get it when needed.
> 
> I am not sure what you mean by that, since again that "crazy" stuff is
> already used everywhere in this driver?
> 
> 
> > Also, meta comment, you might want to use some \n characters in your
> > emails, your lines are really long :)
> 
> Strange, I use sylpheed as a mail client, and the option "Wrap lines at
> 72 characters" is enabled by default, but somehow you must also check
> the box "Wrap on input" for it to work, not very intuitive :) Thanks for
> pointing that to me.
> 
> Hugo.

[PATCH v7 6/9] serial: sc16is7xx: fix bug when first setting GPIO direction

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

When configuring a pin as an output pin with a value of logic 0, we
end up as having a value of logic 1 on the output pin. Setting a
logic 0 a second time (or more) after that will correctly output a
logic 0 on the output pin.

By default, all GPIO pins are configured as inputs. When we enter
sc16is7xx_gpio_direction_output() for the first time, we first set the
desired value in IOSTATE, and then we configure the pin as an output.
The datasheet states that writing to IOSTATE register will trigger a
transfer of the value to the I/O pin configured as output, so if the
pin is configured as an input, nothing will be transferred.

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

This is what is done in NXP application note AN10587.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <stable@vger.kernel.org>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index edc83f5f6340..7a9b91f0c710 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1340,9 +1340,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 v7 7/9] serial: sc16is7xx: add call to get rs485 DT flags and properties

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7a9b91f0c710..9eef7fcd8863 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1545,6 +1545,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 v7 8/9] serial: sc16is7xx: add post reset delay

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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

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

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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

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

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