From: Hugo Villeneuve <[email protected]>
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
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
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 | 168 +++++++++++++-----
2 files changed, 174 insertions(+), 40 deletions(-)
base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847
--
2.30.2
From: Hugo Villeneuve <[email protected]>
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: <[email protected]> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <[email protected]> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
Cc: <[email protected]> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <[email protected]> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
Cc: <[email protected]> # 6.1.x
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 102 ++++++++++++++++++++++++++-------
1 file changed, 81 insertions(+), 21 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d50674d2d0e..ad6b9d613b33 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)
+ 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,43 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
}
#endif
+static int 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 = 0;
+
+ count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
+ if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+ return mctrl_mask;
+
+ ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
+ mctrl_port, count);
+ if (ret)
+ return mctrl_mask;
+
+ 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)
+ return 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 +1446,7 @@ static int sc16is7xx_probe(struct device *dev,
{
unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
unsigned int val;
+ u8 mctrl_mask = 0;
u32 uartclk = 0;
int i, ret;
struct sc16is7xx_port *s;
@@ -1478,12 +1542,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 +1579,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 +1607,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 +1633,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
From: Hugo Villeneuve <[email protected]>
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: <[email protected]> # 6.1.x
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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
From: Hugo Villeneuve <[email protected]>
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: <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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 ad6b9d613b33..2fa09baf28e3 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
From: Hugo Villeneuve <[email protected]>
Add call to uart_get_rs485_mode() to probe for RS485 flags and
properties from device tree.
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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 2fa09baf28e3..388c1d490853 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1544,6 +1544,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
From: Hugo Villeneuve <[email protected]>
Make sure we wait at least 3us before initiating communication with
the device after reset.
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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 388c1d490853..9b2d58023c55 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1521,6 +1521,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
From: Hugo Villeneuve <[email protected]>
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 <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
---
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 9b2d58023c55..59338f5ca180 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
From: Hugo Villeneuve <[email protected]>
In preparation for upcoming patch "fix regression with GPIO
configuration". To facilitate review and make code more modular.
Cc: <[email protected]> # 6.1.x
Signed-off-by: Hugo Villeneuve <[email protected]>
Reviewed-by: Lech Perczak <[email protected]>
Tested-by: Lech Perczak <[email protected]>
---
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
Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> From: Hugo Villeneuve <[email protected]>
>
> 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
Almost good, a few remarks and if addressed as suggested,
Reviewed-by: Andy Shevchenko <[email protected]>
Thank you!
...
> + if (!s->gpio_valid_mask)
I would use == 0, but it's up to you. Both will work equally.
> + return 0;
...
> +static int sc16is7xx_setup_mctrl_ports(struct device *dev)
Not sure why int if you always return an unsigned value.
Otherwise return an error code when it's no defined mask
and check it in the caller.
> +{
> + struct sc16is7xx_port *s = dev_get_drvdata(dev);
> + int i;
> + int ret;
> + int count;
> + u32 mctrl_port[2];
> + u8 mctrl_mask = 0;
I would return 0 directly in the first two cases and split an assignment closer
to the first user.
> + count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> + if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> + return mctrl_mask;
return 0;
> + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> + mctrl_port, count);
> + if (ret)
> + return mctrl_mask;
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)
> + return mctrl_mask;
Maybe positive one?
if (mctrl_mask)
regmap_update_bits(...);
> + 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;
> +}
...
> unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> unsigned int val;
> + u8 mctrl_mask = 0;
This assignment is redundant, so you simply can define it
> u32 uartclk = 0;
u8 mctrl_mask;
> int i, ret;
> struct sc16is7xx_port *s;
--
With Best Regards,
Andy Shevchenko
On Fri, 2 Jun 2023 00:30:14 +0300
[email protected] wrote:
> Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> > From: Hugo Villeneuve <[email protected]>
> >
> > 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
>
> Almost good, a few remarks and if addressed as suggested,
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> Thank you!
>
> ...
>
> > + if (!s->gpio_valid_mask)
>
> I would use == 0, but it's up to you. Both will work equally.
Hi,
done.
> > + return 0;
>
> ...
>
> > +static int sc16is7xx_setup_mctrl_ports(struct device *dev)
>
> Not sure why int if you always return an unsigned value.
> Otherwise return an error code when it's no defined mask
> and check it in the caller.
Changed return type to u8.
> > +{
> > + struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > + int i;
> > + int ret;
> > + int count;
> > + u32 mctrl_port[2];
> > + u8 mctrl_mask = 0;
>
> I would return 0 directly in the first two cases and split an assignment closer
> to the first user.
>
> > + count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> > + if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > + return mctrl_mask;
>
> return 0;
Done.
> > + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> > + mctrl_port, count);
> > + if (ret)
> > + return mctrl_mask;
>
> return 0;
Done.
> mctrl_mask = 0;
Done.
> > + 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)
> > + return mctrl_mask;
>
> Maybe positive one?
> if (mctrl_mask)
> regmap_update_bits(...);
I used negative to save on indentation, but it also fits by converting it to positive, so done.
> > + 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;
> > +}
>
> ...
>
> > unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> > unsigned int val;
> > + u8 mctrl_mask = 0;
>
> This assignment is redundant, so you simply can define it
>
> > u32 uartclk = 0;
>
> u8 mctrl_mask;
Done.
I will send a V7 soon with your Reviewed-by tag.
Thank you,
Hugo.
> > int i, ret;
> > struct sc16is7xx_port *s;
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, 1 Jun 2023 20:41:40 -0400
Hugo Villeneuve <[email protected]> wrote:
> On Fri, 2 Jun 2023 00:30:14 +0300
> [email protected] wrote:
>
> > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> > > From: Hugo Villeneuve <[email protected]>
> > >
> > > 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
> >
> > Almost good, a few remarks and if addressed as suggested,
> > Reviewed-by: Andy Shevchenko <[email protected]>
> >
> > Thank you!
> >
> > ...
> >
> > > + if (!s->gpio_valid_mask)
> >
> > I would use == 0, but it's up to you. Both will work equally.
>
> Hi,
> done.
>
> > > + return 0;
> >
> > ...
> >
> > > +static int sc16is7xx_setup_mctrl_ports(struct device *dev)
> >
> > Not sure why int if you always return an unsigned value.
> > Otherwise return an error code when it's no defined mask
> > and check it in the caller.
>
> Changed return type to u8.
>
>
> > > +{
> > > + struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > > + int i;
> > > + int ret;
> > > + int count;
> > > + u32 mctrl_port[2];
> > > + u8 mctrl_mask = 0;
> >
> > I would return 0 directly in the first two cases and split an assignment closer
> > to the first user.
> >
> > > + count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> > > + if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > > + return mctrl_mask;
> >
> > return 0;
>
> Done.
>
>
> > > + ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> > > + mctrl_port, count);
> > > + if (ret)
> > > + return mctrl_mask;
> >
> > return 0;
>
> Done.
>
>
> > mctrl_mask = 0;
>
> Done.
>
>
> > > + 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)
> > > + return mctrl_mask;
> >
> > Maybe positive one?
> > if (mctrl_mask)
> > regmap_update_bits(...);
>
> I used negative to save on indentation, but it also fits by converting it to positive, so done.
>
>
> > > + 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;
> > > +}
> >
> > ...
> >
> > > unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> > > unsigned int val;
> > > + u8 mctrl_mask = 0;
> >
> > This assignment is redundant, so you simply can define it
> >
> > > u32 uartclk = 0;
> >
> > u8 mctrl_mask;
>
> Done.
>
> I will send a V7 soon with your Reviewed-by tag.
Hi Andy,
Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).
Hugo.
On Fri, 2 Jun 2023 15:58:43 +0300
Andy Shevchenko <[email protected]> wrote:
> On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <[email protected]> wrote:
> > On Thu, 1 Jun 2023 20:41:40 -0400
> > Hugo Villeneuve <[email protected]> wrote:
> > > On Fri, 2 Jun 2023 00:30:14 +0300
> > > [email protected] wrote:
> > > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
>
> ...
>
> > > > Maybe positive one?
> > > > if (mctrl_mask)
> > > > regmap_update_bits(...);
> > >
> > > I used negative to save on indentation, but it also fits by converting it to positive, so done.
>
> I understand, but in this case it is slightly more weird to have
> negative conditional and in either case return the value of the local
> variable.
Yes.
> > Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).
>
> They look fine to me, but Greg is the maintainer, he decides if it's
> really okay or not.
Then I will submit V7 now.
Thank you,
Hugo.
On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <[email protected]> wrote:
> On Thu, 1 Jun 2023 20:41:40 -0400
> Hugo Villeneuve <[email protected]> wrote:
> > On Fri, 2 Jun 2023 00:30:14 +0300
> > [email protected] wrote:
> > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
...
> > > Maybe positive one?
> > > if (mctrl_mask)
> > > regmap_update_bits(...);
> >
> > I used negative to save on indentation, but it also fits by converting it to positive, so done.
I understand, but in this case it is slightly more weird to have
negative conditional and in either case return the value of the local
variable.
...
> Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).
They look fine to me, but Greg is the maintainer, he decides if it's
really okay or not.
--
With Best Regards,
Andy Shevchenko