[PATCH v5 0/3] add support for EXAR XR20M1172 UAR

[PATCH v5 0/3] add support for EXAR XR20M1172 UAR

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

EXAR XR20M1172 UART is mostly SC16IS762-compatible, but
it has additional register which can change UART multiplier
to 4x and 8x, similar to UPF_MAGIC_MULTIPLIER does. So, I used this
flag to guard access to its specific DLD register. It seems than
other EXAR SPI UART modules also have this register, but I tested
only XR20M1172.
Yes, in datasheet this register is called "DLD - Divisor Fractional"
or "DLD - Divisor Fractional Register", calling depends on datasheet
version.

I am sorry about too many submissions and top post reply. About second -
I do not know how to reply properly to this ML from GMail phone app. About first - I just
get very good feedback from Andy Shevchenko, and want to fix his review picks ASAP.

Changes in v2:
  - use full name in git authorship

Changes in v3:
  - change formatting of commit messages to unify width
  - rework commit messages according to code review
  - add XR20M117X namespace for EXAR-specific register
  - do not use UPF_MAGIC_MULTIPLIER for checking EXAR chip,
    use s->devtype directly
  - replace while loop to fls function and expanded check
  - sort compatibles
  - reformat multiline comment.

Changes in v4:
  - rebase onto tty-next branch
  - added Kconfig mention of the chip
  - used rounddown_power_of_two instead of fls and manual shift
  - used FIELD_PREP instead of custom macro
  - removed has_dld bit from common struct, replaced by check function,
    which checks directly by s->devtype
  - fixed tab count
  - properly apply Vladimir Zapolskiy's tag to patch 2 only
  
Changes in v5:
  - fixes for tty-next branch
  - address a new code review picks
  - send properly to all participants
  - added Ack tag

Konstantin Pugin (3):
  serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND
  dt-bindings: sc16is7xx: Add compatible line for XR20M1172 UART
  serial: sc16is7xx: add support for EXAR XR20M1172 UART

 .../bindings/serial/nxp,sc16is7xx.yaml        |  1 +
 drivers/tty/serial/Kconfig                    | 18 +++---
 drivers/tty/serial/sc16is7xx.c                | 62 +++++++++++++++++--
 drivers/tty/serial/sc16is7xx.h                |  1 +
 drivers/tty/serial/sc16is7xx_i2c.c            |  1 +
 drivers/tty/serial/sc16is7xx_spi.c            |  1 +
 6 files changed, 71 insertions(+), 13 deletions(-)


base-commit: f70f95b485d78838ad28dbec804b986d11ad7bb0
-- 
2.44.0

[PATCH v5 1/3] serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

The hardware supports both RTS_ON_SEND and RTS_AFTER_SEND modes, but
after the commit 4afeced55baa ("serial: core: fix sanitizing check for
RTS settings") we always end up with SER_RS485_RTS_AFTER_SEND set and
always write to the register field SC16IS7XX_EFCR_RTS_INVERT_BIT, which
breaks some hardware using these chips.

Fixes: 267913ecf737 ("serial: sc16is7xx: Fill in rs485_supported")
Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 03cf30e20b75..dfcc804f558f 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1449,7 +1449,7 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
 }
 
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
 	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
 };
-- 
2.44.0

[PATCH v5 2/3] dt-bindings: sc16is7xx: Add compatible line for XR20M1172 UART

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

EXAR XR20M1172 UART is mostly register-compatible with NXP SPI UARTs.
It will be handled by same driver, so, it makes sense to add DT
definition for these block into driver's documentation.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
index 5dec15b7e7c3..c4bedf23368b 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     enum:
+      - exar,xr20m1172
       - nxp,sc16is740
       - nxp,sc16is741
       - nxp,sc16is750
-- 
2.44.0

Re: [PATCH v5 2/3] dt-bindings: sc16is7xx: Add compatible line for XR20M1172 UART

[Andy Shevchenko]

On Sat, Apr 20, 2024 at 09:22:05PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> EXAR XR20M1172 UART is mostly register-compatible with NXP SPI UARTs.
> It will be handled by same driver, so, it makes sense to add DT
> definition for these block into driver's documentation.

Still Description is missing the part about Exar involvement here...
OTOH, not a DT guy here and you got the Ack:s, so feel free to ignore
my comment.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

XR20M1172 register set is mostly compatible with SC16IS762, but it has
a support for additional division rates of UART with special DLD register.
So, add handling this register by appropriate devicetree bindings.

Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 drivers/tty/serial/Kconfig         | 18 +++++----
 drivers/tty/serial/sc16is7xx.c     | 60 ++++++++++++++++++++++++++++--
 drivers/tty/serial/sc16is7xx.h     |  1 +
 drivers/tty/serial/sc16is7xx_i2c.c |  1 +
 drivers/tty/serial/sc16is7xx_spi.c |  1 +
 5 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 4fdd7857ef4d..4380bfe7dfff 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1029,15 +1029,17 @@ config SERIAL_SC16IS7XX_CORE
 	select SERIAL_SC16IS7XX_SPI if SPI_MASTER
 	select SERIAL_SC16IS7XX_I2C if I2C
 	help
-	  Core driver for NXP SC16IS7xx UARTs.
+	  Core driver for NXP SC16IS7xx-compatible UARTs.
 	  Supported ICs are:
-
-	    SC16IS740
-	    SC16IS741
-	    SC16IS750
-	    SC16IS752
-	    SC16IS760
-	    SC16IS762
+		NXP:
+	    	SC16IS740
+	    	SC16IS741
+	    	SC16IS750
+	    	SC16IS752
+	    	SC16IS760
+	    	SC16IS762
+		EXAR:
+			XR20M1172
 
 	  The driver supports both I2C and SPI interfaces.
 
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index dfcc804f558f..0f95d5aef71b 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -10,6 +10,7 @@
 #undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -68,6 +69,7 @@
 /* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
 #define SC16IS7XX_DLL_REG		(0x00) /* Divisor Latch Low */
 #define SC16IS7XX_DLH_REG		(0x01) /* Divisor Latch High */
+#define XR20M117X_DLD_REG		(0x02) /* Divisor Fractional Register */
 
 /* Enhanced Register set: Only if (LCR == 0xBF) */
 #define SC16IS7XX_EFR_REG		(0x02) /* Enhanced Features */
@@ -221,6 +223,20 @@
 #define SC16IS7XX_TCR_RX_HALT(words)	((((words) / 4) & 0x0f) << 0)
 #define SC16IS7XX_TCR_RX_RESUME(words)	((((words) / 4) & 0x0f) << 4)
 
+/*
+ * Divisor Fractional Register bits (EXAR extension)
+ * EXAR hardware is mostly compatible with SC16IS7XX, but supports additional feature:
+ * 4x and 8x divisor, instead of default 16x. It has a special register to program it.
+ * Bits 0 to 3 is fractional divisor, it used to set value of last 16 bits of
+ * uartclk * (16 / divisor) / baud, in case of default it will be uartclk / baud.
+ * Bits 4 and 5 used as switches, and should not be set to 1 simultaneously.
+ */
+
+#define XR20M117X_DLD_16X			0
+#define XR20M117X_DLD_DIV_MASK		GENMASK(3, 0)
+#define XR20M117X_DLD_8X			BIT(4)
+#define XR20M117X_DLD_4X			BIT(5)
+
 /*
  * TLR register bits
  * If TLR[3:0] or TLR[7:4] are logical 0, the selectable trigger levels via the
@@ -523,6 +539,13 @@ const struct sc16is7xx_devtype sc16is762_devtype = {
 };
 EXPORT_SYMBOL_GPL(sc16is762_devtype);
 
+const struct sc16is7xx_devtype xr20m1172_devtype = {
+	.name		= "XR20M1172",
+	.nr_gpio	= 8,
+	.nr_uart	= 2,
+};
+EXPORT_SYMBOL_GPL(xr20m1172_devtype);
+
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
 	return reg == SC16IS7XX_RHR_REG;
 }
 
+static bool sc16is7xx_has_dld(struct device *dev)
+{
+		struct sc16is7xx_port *s = dev_get_drvdata(dev);
+
+		if (s->devtype == &xr20m1172_devtype)
+			return true;
+		return false;
+}
+
 static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
-	u8 lcr;
+	unsigned long clk = port->uartclk, div, div16;
+	bool has_dld = sc16is7xx_has_dld(port->dev);
+	u8 dld_mode = XR20M117X_DLD_16X;
 	u8 prescaler = 0;
-	unsigned long clk = port->uartclk, div = clk / 16 / baud;
+	u8 divisor = 16;
+	u8 lcr;
+
+	if (has_dld && DIV_ROUND_CLOSEST(clk, baud) < 16)
+		divisor = rounddown_pow_of_two(DIV_ROUND_CLOSEST(clk, baud));
+
+	div16 = (clk * 16) / divisor / baud;
+	div = div16 / 16;
 
 	if (div >= BIT(16)) {
 		prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
 		div /= 4;
 	}
 
+	/* Count additional divisor for EXAR devices */
+	if (divisor == 8)
+		dld_mode = XR20M117X_DLD_8X;
+	if (divisor == 4)
+		dld_mode = XR20M117X_DLD_4X;
+	dld_mode |= FIELD_PREP(XR20M117X_DLD_DIV_MASK, div16);
+
 	/* Enable enhanced features */
 	sc16is7xx_efr_lock(port);
 	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
@@ -587,12 +635,14 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 	regcache_cache_bypass(one->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_DLH_REG, div / 256);
 	sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
+	if (has_dld)
+		sc16is7xx_port_write(port, XR20M117X_DLD_REG, dld_mode);
 	regcache_cache_bypass(one->regmap, false);
 
 	/* Restore LCR and access to general register set */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
 
-	return DIV_ROUND_CLOSEST(clk / 16, div);
+	return DIV_ROUND_CLOSEST(clk / divisor, div);
 }
 
 static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
@@ -1002,6 +1052,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 				  const struct ktermios *old)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+	bool has_dld = sc16is7xx_has_dld(port->dev);
 	unsigned int lcr, flow = 0;
 	int baud;
 	unsigned long flags;
@@ -1084,7 +1135,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	/* Get baud rate generator configuration */
 	baud = uart_get_baud_rate(port, termios, old,
 				  port->uartclk / 16 / 4 / 0xffff,
-				  port->uartclk / 16);
+				  port->uartclk / (has_dld ? 4 : 16));
 
 	/* Setup baudrate generator */
 	baud = sc16is7xx_set_baud(port, baud);
@@ -1684,6 +1735,7 @@ void sc16is7xx_remove(struct device *dev)
 EXPORT_SYMBOL_GPL(sc16is7xx_remove);
 
 const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
+	{ .compatible = "exar,xr20m1172",	.data = &xr20m1172_devtype, },
 	{ .compatible = "nxp,sc16is740",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is741",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is750",	.data = &sc16is750_devtype, },
diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h
index afb784eaee45..eb2e3bc86f15 100644
--- a/drivers/tty/serial/sc16is7xx.h
+++ b/drivers/tty/serial/sc16is7xx.h
@@ -28,6 +28,7 @@ extern const struct sc16is7xx_devtype sc16is750_devtype;
 extern const struct sc16is7xx_devtype sc16is752_devtype;
 extern const struct sc16is7xx_devtype sc16is760_devtype;
 extern const struct sc16is7xx_devtype sc16is762_devtype;
+extern const struct sc16is7xx_devtype xr20m1172_devtype;
 
 const char *sc16is7xx_regmap_name(u8 port_id);
 
diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
index 3ed47c306d85..839de902821b 100644
--- a/drivers/tty/serial/sc16is7xx_i2c.c
+++ b/drivers/tty/serial/sc16is7xx_i2c.c
@@ -46,6 +46,7 @@ static const struct i2c_device_id sc16is7xx_i2c_id_table[] = {
 	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
 	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
 	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sc16is7xx_i2c_id_table);
diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
index 73df36f8a7fd..2b278282dbd0 100644
--- a/drivers/tty/serial/sc16is7xx_spi.c
+++ b/drivers/tty/serial/sc16is7xx_spi.c
@@ -69,6 +69,7 @@ static const struct spi_device_id sc16is7xx_spi_id_table[] = {
 	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
 	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
 	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table);
-- 
2.44.0

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Jiri Slaby]

On 20. 04. 24, 20:22, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> XR20M1172 register set is mostly compatible with SC16IS762, but it has
> a support for additional division rates of UART with special DLD register.
> So, add handling this register by appropriate devicetree bindings.
...
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
...
> @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
>   	return reg == SC16IS7XX_RHR_REG;
>   }
>   
> +static bool sc16is7xx_has_dld(struct device *dev)
> +{
> +		struct sc16is7xx_port *s = dev_get_drvdata(dev);
> +
> +		if (s->devtype == &xr20m1172_devtype)
> +			return true;
> +		return false;

:) so this should simply be:

return s->devtype == &xr20m1172_devtype;

...
> @@ -1002,6 +1052,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
>   				  const struct ktermios *old)
>   {
>   	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
> +	bool has_dld = sc16is7xx_has_dld(port->dev);
>   	unsigned int lcr, flow = 0;
>   	int baud;
>   	unsigned long flags;
> @@ -1084,7 +1135,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
>   	/* Get baud rate generator configuration */
>   	baud = uart_get_baud_rate(port, termios, old,
>   				  port->uartclk / 16 / 4 / 0xffff,
> -				  port->uartclk / 16);
> +				  port->uartclk / (has_dld ? 4 : 16));

Could you do this instead:
unsigned int divisor = sc16is7xx_has_dld(port->dev) ? 4 : 16;

...

uart_get_baud_rate(..., port->uartclk / divisor);


I am not sure the above warrants for a new version. Just in case you are 
sending one.

thanks,
-- 
js
suse labs

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Konstantin P.]

I do not think this is a requirement for a new version. I need to
wait. Some folks want to test my patches with their hardware)))

On Mon, Apr 22, 2024 at 9:30 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 20. 04. 24, 20:22, Konstantin Pugin wrote:
> > From: Konstantin Pugin <ria.freelander@gmail.com>
> >
> > XR20M1172 register set is mostly compatible with SC16IS762, but it has
> > a support for additional division rates of UART with special DLD register.
> > So, add handling this register by appropriate devicetree bindings.
> ...
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> ...
> > @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
> >       return reg == SC16IS7XX_RHR_REG;
> >   }
> >
> > +static bool sc16is7xx_has_dld(struct device *dev)
> > +{
> > +             struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > +
> > +             if (s->devtype == &xr20m1172_devtype)
> > +                     return true;
> > +             return false;
>
> :) so this should simply be:
>
> return s->devtype == &xr20m1172_devtype;
>
> ...
> > @@ -1002,6 +1052,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
> >                                 const struct ktermios *old)
> >   {
> >       struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
> > +     bool has_dld = sc16is7xx_has_dld(port->dev);
> >       unsigned int lcr, flow = 0;
> >       int baud;
> >       unsigned long flags;
> > @@ -1084,7 +1135,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
> >       /* Get baud rate generator configuration */
> >       baud = uart_get_baud_rate(port, termios, old,
> >                                 port->uartclk / 16 / 4 / 0xffff,
> > -                               port->uartclk / 16);
> > +                               port->uartclk / (has_dld ? 4 : 16));
>
> Could you do this instead:
> unsigned int divisor = sc16is7xx_has_dld(port->dev) ? 4 : 16;
>
> ...
>
> uart_get_baud_rate(..., port->uartclk / divisor);
>
>
> I am not sure the above warrants for a new version. Just in case you are
> sending one.
>
> thanks,
> --
> js
> suse labs
>

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Andy Shevchenko]

On Mon, Apr 22, 2024 at 11:35:32AM +0300, Konstantin P. wrote:
> I do not think this is a requirement for a new version. I need to
> wait. Some folks want to test my patches with their hardware)))

You should not top-post!

...

Yes, it would be good them to provide a formal Tested-by tag, then you may send
a v6 with the tags and Jiri's comments being addressed. Btw, I prefer to have
them addressed now to eliminate unneeded churn in the future.

> On Mon, Apr 22, 2024 at 9:30 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> > On 20. 04. 24, 20:22, Konstantin Pugin wrote:

> > I am not sure the above warrants for a new version. Just in case you are
> > sending one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Konstantin P.]

On Mon, Apr 22, 2024 at 9:30 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 20. 04. 24, 20:22, Konstantin Pugin wrote:
> > From: Konstantin Pugin <ria.freelander@gmail.com>
> >
> > XR20M1172 register set is mostly compatible with SC16IS762, but it has
> > a support for additional division rates of UART with special DLD register.
> > So, add handling this register by appropriate devicetree bindings.
> ...
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> ...
> > @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
> >       return reg == SC16IS7XX_RHR_REG;
> >   }
> >
> > +static bool sc16is7xx_has_dld(struct device *dev)
> > +{
> > +             struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > +
> > +             if (s->devtype == &xr20m1172_devtype)
> > +                     return true;
> > +             return false;
>
> :) so this should simply be:
>
> return s->devtype == &xr20m1172_devtype;
>
I especially want to avoid this construction, because it will lead to
idea than we does not have other
DLD-capable UARTS, which is simply not true, there is, for example,
XR20M1280 UART, which has roughly the same register set
(https://www.alldatasheet.com/datasheet-pdf/pdf/445109/EXAR/XR20M1280.html).
I simply do not have other devices, so I do not
want to risk sending untested patches upstream.

> ...
> > @@ -1002,6 +1052,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
> >                                 const struct ktermios *old)
> >   {
> >       struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
> > +     bool has_dld = sc16is7xx_has_dld(port->dev);
> >       unsigned int lcr, flow = 0;
> >       int baud;
> >       unsigned long flags;
> > @@ -1084,7 +1135,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
> >       /* Get baud rate generator configuration */
> >       baud = uart_get_baud_rate(port, termios, old,
> >                                 port->uartclk / 16 / 4 / 0xffff,
> > -                               port->uartclk / 16);
> > +                               port->uartclk / (has_dld ? 4 : 16));
>
> Could you do this instead:
> unsigned int divisor = sc16is7xx_has_dld(port->dev) ? 4 : 16;
>
> ...
>
> uart_get_baud_rate(..., port->uartclk / divisor);
>
>
> I am not sure the above warrants for a new version. Just in case you are
> sending one.
>
> thanks,
> --
> js
> suse labs
>

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Jiri Slaby]

On 22. 04. 24, 14:00, Konstantin P. wrote:
> On Mon, Apr 22, 2024 at 9:30 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>>
>> On 20. 04. 24, 20:22, Konstantin Pugin wrote:
>>> From: Konstantin Pugin <ria.freelander@gmail.com>
>>>
>>> XR20M1172 register set is mostly compatible with SC16IS762, but it has
>>> a support for additional division rates of UART with special DLD register.
>>> So, add handling this register by appropriate devicetree bindings.
>> ...
>>> --- a/drivers/tty/serial/sc16is7xx.c
>>> +++ b/drivers/tty/serial/sc16is7xx.c
>> ...
>>> @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
>>>        return reg == SC16IS7XX_RHR_REG;
>>>    }
>>>
>>> +static bool sc16is7xx_has_dld(struct device *dev)
>>> +{
>>> +             struct sc16is7xx_port *s = dev_get_drvdata(dev);
>>> +
>>> +             if (s->devtype == &xr20m1172_devtype)
>>> +                     return true;
>>> +             return false;
>>
>> :) so this should simply be:
>>
>> return s->devtype == &xr20m1172_devtype;
>>
> I especially want to avoid this construction, because it will lead to
> idea than we does not have other
> DLD-capable UARTS, which is simply not true, there is, for example,
> XR20M1280 UART, which has roughly the same register set
> (https://www.alldatasheet.com/datasheet-pdf/pdf/445109/EXAR/XR20M1280.html).
> I simply do not have other devices, so I do not
> want to risk sending untested patches upstream.

Sorry, what?

-- 
js
suse labs

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Krzysztof Kozlowski]

On 22/04/2024 22:35, Konstantin P. wrote:
> On Mon, Apr 22, 2024, 23:11 Jiri Slaby <jirislaby@kernel.org> wrote:
> 
>> On 22. 04. 24, 14:00, Konstantin P. wrote:
>>> On Mon, Apr 22, 2024 at 9:30 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>>>>
>>>> On 20. 04. 24, 20:22, Konstantin Pugin wrote:
>>>>> From: Konstantin Pugin <ria.freelander@gmail.com>
>>>>>
>>>>> XR20M1172 register set is mostly compatible with SC16IS762, but it has
>>>>> a support for additional division rates of UART with special DLD
>> register.
>>>>> So, add handling this register by appropriate devicetree bindings.
>>>> ...
>>>>> --- a/drivers/tty/serial/sc16is7xx.c
>>>>> +++ b/drivers/tty/serial/sc16is7xx.c
>>>> ...
>>>>> @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device
>> *dev, unsigned int reg)
>>>>>        return reg == SC16IS7XX_RHR_REG;
>>>>>    }
>>>>>
>>>>> +static bool sc16is7xx_has_dld(struct device *dev)
>>>>> +{
>>>>> +             struct sc16is7xx_port *s = dev_get_drvdata(dev);
>>>>> +
>>>>> +             if (s->devtype == &xr20m1172_devtype)
>>>>> +                     return true;
>>>>> +             return false;
>>>>
>>>> :) so this should simply be:
>>>>
>>>> return s->devtype == &xr20m1172_devtype;
>>>>
>>> I especially want to avoid this construction, because it will lead to
>>> idea than we does not have other
>>> DLD-capable UARTS, which is simply not true, there is, for example,
>>> XR20M1280 UART, which has roughly the same register set
>>> (
>> https://www.alldatasheet.com/datasheet-pdf/pdf/445109/EXAR/XR20M1280.html
>> ).
>>> I simply do not have other devices, so I do not
>>> want to risk sending untested patches upstream.
>>
>> Sorry, what?
>>
>> --
>> js
>> suse labs
>>
> 
> I do not wish those function to be less generic than I did. If you think
> this change is required - I will change. But if it would be okay without a
> change - I prefer to stay as is.

The code does exactly the same, so what do you mean "less generic"? What
does it even mean?

Best regards,
Krzysztof

Re: [PATCH v5 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Andy Shevchenko]

On Sat, Apr 20, 2024 at 09:22:06PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> XR20M1172 register set is mostly compatible with SC16IS762, but it has
> a support for additional division rates of UART with special DLD register.
> So, add handling this register by appropriate devicetree bindings.

...

>  	help
> -	  Core driver for NXP SC16IS7xx UARTs.
> +	  Core driver for NXP SC16IS7xx-compatible UARTs.

'-compatible' --> ' and compatible'

>  	  Supported ICs are:
> -
> -	    SC16IS740
> -	    SC16IS741
> -	    SC16IS750
> -	    SC16IS752
> -	    SC16IS760
> -	    SC16IS762
> +		NXP:
> +	    	SC16IS740
> +	    	SC16IS741
> +	    	SC16IS750
> +	    	SC16IS752
> +	    	SC16IS760
> +	    	SC16IS762

You broke the indentation (as it has mixed TABs and spaces).

> +		EXAR:
> +			XR20M1172

No need to rewrite all of them, just add your line as

	    XR20M1172 (Exar)

>  	  The driver supports both I2C and SPI interfaces.

(Note, this needs to be fixed, hence it justifies a new version of the patch.)

...

> +/*
> + * Divisor Fractional Register bits (EXAR extension)

Missing period at the end of the line.

> + * EXAR hardware is mostly compatible with SC16IS7XX, but supports additional feature:
> + * 4x and 8x divisor, instead of default 16x. It has a special register to program it.
> + * Bits 0 to 3 is fractional divisor, it used to set value of last 16 bits of
> + * uartclk * (16 / divisor) / baud, in case of default it will be uartclk / baud.
> + * Bits 4 and 5 used as switches, and should not be set to 1 simultaneously.
> + */

...

> +static bool sc16is7xx_has_dld(struct device *dev)
> +{
> +		struct sc16is7xx_port *s = dev_get_drvdata(dev);
> +
> +		if (s->devtype == &xr20m1172_devtype)
> +			return true;
> +		return false;

Besides what Jiri noticed, this has been indented one TAB too much.

> +}

-- 
With Best Regards,
Andy Shevchenko