[PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

From: Matthew Howell <matthew.howell@sealevel.com

Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the 
current implementation of 8250_exar uses RTS for the auto-RS485-Enable 
mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on 
Sealevel cards.

Link: 
https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/

Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
---
Fixed style issues from previous submission.

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 3886f78ecbbf..20d2e7148be5 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -78,6 +78,9 @@
 
 #define UART_EXAR_RS485_DLY(x)	((x) << 4)
 
+#define UART_EXAR_DLD				0x02 /* Divisor Fractional */
+#define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
+
 /*
  * IOT2040 MPIO wiring semantics:
  *
@@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
 	return 0;
 }
 
+static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
+				struct serial_rs485 *rs485)
+{
+	u8 __iomem *p = port->membase;
+	u8 old_lcr;
+
+	generic_rs485_config(port, termios, rs485);
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		/* Set EFR[4]=1 to enable enhanced feature registers */
+		writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
+
+		/* Set MCR to use DTR as Auto-RS485 Enable signal */
+		writeb(UART_MCR_OUT1, p + UART_MCR);
+
+		/* Store original LCR and set LCR[7]=1 to enable access to DLD register */
+		old_lcr = readb(p + UART_LCR);
+		writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
+
+		/* Set DLD[7]=1 for inverted RS485 Enable logic */
+		writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
+
+		writeb(old_lcr, p + UART_LCR);
+    }
+
+	return 0;
+ }
+
 static const struct serial_rs485 generic_rs485_supported = {
 	.flags = SER_RS485_ENABLED,
 };
@@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev)
 	return 0;
 }
 
+static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+		   struct uart_8250_port *port, int idx)
+{
+	int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
+
+	port->port.rs485_config = sealevel_rs485_config;
+
+	return ret;
+}
+
 static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
 
 static const struct exar8250_board pbn_fastcom335_2 = {
@@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };
 
+static const struct exar8250_board pbn_sealevel = {
+	.setup		= pci_sealevel_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_sealevel_16 = {
+	.num_ports	= 16,
+    .setup		= pci_sealevel_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
 #define CONNECT_DEVICE(devid, sdevid, bd) {				\
 	PCI_DEVICE_SUB(							\
 		PCI_VENDOR_ID_EXAR,					\
@@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 		(kernel_ulong_t)&bd			\
 	}
 
+#define SEALEVEL_DEVICE(devid, bd) {			\
+	PCI_DEVICE_SUB(					\
+		PCI_VENDOR_ID_EXAR,			\
+		PCI_DEVICE_ID_EXAR_##devid,		\
+		PCI_VENDOR_ID_SEALEVEL,			\
+		PCI_ANY_ID), 0, 0,	\
+		(kernel_ulong_t)&bd			\
+	}
+
 static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
 	EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
@@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
 	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
 
+	SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
+	SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
+
 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
 
 	/* USRobotics USR298x-OEM PCI Modems */

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Ilpo Järvinen]

On Tue, 5 Sep 2023, Matthew Howell wrote:

> From: Matthew Howell <matthew.howell@sealevel.com
> 
> Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the 
> current implementation of 8250_exar uses RTS for the auto-RS485-Enable 
> mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on 
> Sealevel cards.
> 
> Link: 
> https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/
> 
> Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
> ---
> Fixed style issues from previous submission.
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 3886f78ecbbf..20d2e7148be5 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -78,6 +78,9 @@
>  
>  #define UART_EXAR_RS485_DLY(x)	((x) << 4)
>  
> +#define UART_EXAR_DLD				0x02 /* Divisor Fractional */
> +#define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> +
>  /*
>   * IOT2040 MPIO wiring semantics:
>   *
> @@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
>  	return 0;
>  }
>  
> +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> +				struct serial_rs485 *rs485)
> +{
> +	u8 __iomem *p = port->membase;
> +	u8 old_lcr;
> +
> +	generic_rs485_config(port, termios, rs485);
> +
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		/* Set EFR[4]=1 to enable enhanced feature registers */
> +		writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);

You should split these to 3 lines to make it easier to follow what's the 
actual operation taking place here:
		efr = readb(...);
		efr |= UART_EFR_ECB;
		writeb(...);

...I'm sorry I didn't realize this last time I looked.

> +
> +		/* Set MCR to use DTR as Auto-RS485 Enable signal */
> +		writeb(UART_MCR_OUT1, p + UART_MCR);
> +
> +		/* Store original LCR and set LCR[7]=1 to enable access to DLD register */

I guess "store original LCR" path is pretty obvious and can be dropped.

> +		old_lcr = readb(p + UART_LCR);
> +		writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> +
> +		/* Set DLD[7]=1 for inverted RS485 Enable logic */
> +		writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);

Split this as well.

> +		writeb(old_lcr, p + UART_LCR);
> +    }

This function should also disable RS-485 if SER_RS485_ENABLED is set but 
currently it does nothing?

-- 
 i.


> +
> +	return 0;
> + }
> +
>  static const struct serial_rs485 generic_rs485_supported = {
>  	.flags = SER_RS485_ENABLED,
>  };
> @@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		   struct uart_8250_port *port, int idx)
> +{
> +	int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> +
> +	port->port.rs485_config = sealevel_rs485_config;
> +
> +	return ret;
> +}
> +
>  static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
>  
>  static const struct exar8250_board pbn_fastcom335_2 = {
> @@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
>  	.exit		= pci_xr17v35x_exit,
>  };
>  
> +static const struct exar8250_board pbn_sealevel = {
> +	.setup		= pci_sealevel_setup,
> +	.exit		= pci_xr17v35x_exit,
> +};
> +
> +static const struct exar8250_board pbn_sealevel_16 = {
> +	.num_ports	= 16,
> +    .setup		= pci_sealevel_setup,
> +	.exit		= pci_xr17v35x_exit,
> +};
> +
>  #define CONNECT_DEVICE(devid, sdevid, bd) {				\
>  	PCI_DEVICE_SUB(							\
>  		PCI_VENDOR_ID_EXAR,					\
> @@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
>  		(kernel_ulong_t)&bd			\
>  	}
>  
> +#define SEALEVEL_DEVICE(devid, bd) {			\
> +	PCI_DEVICE_SUB(					\
> +		PCI_VENDOR_ID_EXAR,			\
> +		PCI_DEVICE_ID_EXAR_##devid,		\
> +		PCI_VENDOR_ID_SEALEVEL,			\
> +		PCI_ANY_ID), 0, 0,	\
> +		(kernel_ulong_t)&bd			\
> +	}
> +
>  static const struct pci_device_id exar_pci_tbl[] = {
>  	EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
>  	EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
> @@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
>  	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
>  	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
>  
> +	SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
> +	SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
> +	SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
> +	SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> +	SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> +
>  	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
>  
>  	/* USRobotics USR298x-OEM PCI Modems */
> 

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

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

On Wed, 6 Sep 2023, Ilpo Järvinen wrote:
> On Tue, 5 Sep 2023, Matthew Howell wrote:
> 
> > From: Matthew Howell <matthew.howell@sealevel.com
> >
> > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the
> > current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
> > Sealevel cards.
> >
> > Link: https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/
> >
> > Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
> > ---
> > Fixed style issues from previous submission.
> >
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 3886f78ecbbf..20d2e7148be5 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -78,6 +78,9 @@
> >
> >  #define UART_EXAR_RS485_DLY(x)       ((x) << 4)
> >
> > +#define UART_EXAR_DLD                                0x02 /* Divisor Fractional */
> > +#define UART_EXAR_DLD_485_POLARITY   0x80 /* RS-485 Enable Signal Polarity */
> > +
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
> >       return 0;
> >  }
> >
> > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> > +                             struct serial_rs485 *rs485)
> > +{
> > +     u8 __iomem *p = port->membase;
> > +     u8 old_lcr;
> > +
> > +     generic_rs485_config(port, termios, rs485);
> > +
> > +     if (rs485->flags & SER_RS485_ENABLED) {
> > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> 
> You should split these to 3 lines to make it easier to follow what's the
> actual operation taking place here:
>                 efr = readb(...);
>                 efr |= UART_EFR_ECB;
>                 writeb(...);
> 
> ...I'm sorry I didn't realize this last time I looked.

Ok. Will fix in next submission. 

> > +
> > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > +
> > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> 
> I guess "store original LCR" path is pretty obvious and can be dropped.

Ok. I can drop that part of the comment if it is clear otherwise.
 
> > +             old_lcr = readb(p + UART_LCR);
> > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > +
> > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> 
> Split this as well.
> 
> > +             writeb(old_lcr, p + UART_LCR);
> > +    }
> 
> This function should also disable RS-485 if SER_RS485_ENABLED is set but
> currently it does nothing?
> 
> --
>  i.

That's what I would have thought as well, but I have electrically verified 
disabling SER_RS485_ENABLED disables RS485 on the Sealevel cards I have 
tested. I did this by monitoring the serial signal while a program 
alternates setting and unsetting SER_RS485_ENABLED and verifying the card 
is tri-stating when SER_RS485_ENABLED is set and fails to tri-state when 
SER_RS485_ENABLED is not set.

I do not know where disabling RS485 is happening in the driver since 
generic_rs485_config() does not handle this either, but I left it alone 
since it appears to be working without being handled in the 
..._rs485_config() functions.

> 
> > +
> > +     return 0;
> > + }
> > +
> >  static const struct serial_rs485 generic_rs485_supported = {
> >       .flags = SER_RS485_ENABLED,
> >  };
> > @@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev)
> >       return 0;
> >  }
> >
> > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > +                struct uart_8250_port *port, int idx)
> > +{
> > +     int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > +
> > +     port->port.rs485_config = sealevel_rs485_config;
> > +
> > +     return ret;
> > +}
> > +
> >  static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
> >
> >  static const struct exar8250_board pbn_fastcom335_2 = {
> > @@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> >       .exit           = pci_xr17v35x_exit,
> >  };
> >
> > +static const struct exar8250_board pbn_sealevel = {
> > +     .setup          = pci_sealevel_setup,
> > +     .exit           = pci_xr17v35x_exit,
> > +};
> > +
> > +static const struct exar8250_board pbn_sealevel_16 = {
> > +     .num_ports      = 16,
> > +    .setup           = pci_sealevel_setup,
> > +     .exit           = pci_xr17v35x_exit,
> > +};
> > +
> >  #define CONNECT_DEVICE(devid, sdevid, bd) {                          \
> >       PCI_DEVICE_SUB(                                                 \
> >               PCI_VENDOR_ID_EXAR,                                     \
> > @@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> >               (kernel_ulong_t)&bd                     \
> >       }
> >
> > +#define SEALEVEL_DEVICE(devid, bd) {                 \
> > +     PCI_DEVICE_SUB(                                 \
> > +             PCI_VENDOR_ID_EXAR,                     \
> > +             PCI_DEVICE_ID_EXAR_##devid,             \
> > +             PCI_VENDOR_ID_SEALEVEL,                 \
> > +             PCI_ANY_ID), 0, 0,      \
> > +             (kernel_ulong_t)&bd                     \
> > +     }
> > +
> >  static const struct pci_device_id exar_pci_tbl[] = {
> >       EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
> >       EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
> > @@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
> >       CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
> >       CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
> >
> > +     SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> > +
> >       IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
> >
> >       /* USRobotics USR298x-OEM PCI Modems */
> >
> 

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@sealevel.com

Okay, I started reviewing it and it has all the same issues that I pointed
out previously. Are you sure you sent a _new_ version?

For your convenience I left all comments here.

> 
> Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the 
> current implementation of 8250_exar uses RTS for the auto-RS485-Enable 
> mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on 

All four above and two below (Link: and supposed to be a blank line)
have a trailing space. Which editor do you use? You probably need to configure
it to avoid this.

> Sealevel cards.

> Link: 
> https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/

Should be a single line.

> 

No blank lines in the tag block.

...

> +	generic_rs485_config(port, termios, rs485);

> +	if (rs485->flags & SER_RS485_ENABLED) {

What I meant is to have

	if (!)rs485->flags & SER_RS485_ENABLED))
		return 0;

here, which allows you to reduce indentation level in the below block.

> +		/* Set EFR[4]=1 to enable enhanced feature registers */
> +		writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> +
> +		/* Set MCR to use DTR as Auto-RS485 Enable signal */
> +		writeb(UART_MCR_OUT1, p + UART_MCR);
> +
> +		/* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> +		old_lcr = readb(p + UART_LCR);
> +		writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> +
> +		/* Set DLD[7]=1 for inverted RS485 Enable logic */
> +		writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> +
> +		writeb(old_lcr, p + UART_LCR);
> +    }
> +
> +	return 0;

...

> +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		   struct uart_8250_port *port, int idx)
> +{
> +	int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);

> +	port->port.rs485_config = sealevel_rs485_config;

This is incorrect in a way that we do not assign return values in case it is
an error path. This will be prone to mistakes on the caller side.

	int ret;

	ret = ...
	if (ret)
		return ret;

	...

	return 0;

> +	return ret;
> +}

...

> +static const struct exar8250_board pbn_sealevel_16 = {
> +	.num_ports	= 16,

> +    .setup		= pci_sealevel_setup,

Here is the indentation issue. Use leading TABs.

> +	.exit		= pci_xr17v35x_exit,
> +};

...

> +#define SEALEVEL_DEVICE(devid, bd) {			\
> +	PCI_DEVICE_SUB(					\
> +		PCI_VENDOR_ID_EXAR,			\
> +		PCI_DEVICE_ID_EXAR_##devid,		\
> +		PCI_VENDOR_ID_SEALEVEL,			\

> +		PCI_ANY_ID), 0, 0,	\

Seems trailing \ is indented wrongly, missing TAB?

> +		(kernel_ulong_t)&bd			\
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Ilpo Järvinen]

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

On Wed, 6 Sep 2023, Matthew Howell wrote:

> On Wed, 6 Sep 2023, Ilpo Järvinen wrote:
> > On Tue, 5 Sep 2023, Matthew Howell wrote:
> > 
> > > From: Matthew Howell <matthew.howell@sealevel.com
> > >
> > > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the
> > > current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> > > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
> > > Sealevel cards.
> > >
> > > Link: https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/
> > >
> > > Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
> > > ---
> > > Fixed style issues from previous submission.
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > index 3886f78ecbbf..20d2e7148be5 100644
> > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > @@ -78,6 +78,9 @@
> > >
> > >  #define UART_EXAR_RS485_DLY(x)       ((x) << 4)
> > >
> > > +#define UART_EXAR_DLD                                0x02 /* Divisor Fractional */
> > > +#define UART_EXAR_DLD_485_POLARITY   0x80 /* RS-485 Enable Signal Polarity */
> > > +
> > >  /*
> > >   * IOT2040 MPIO wiring semantics:
> > >   *
> > > @@ -439,6 +442,34 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
> > >       return 0;
> > >  }
> > >
> > > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> > > +                             struct serial_rs485 *rs485)
> > > +{
> > > +     u8 __iomem *p = port->membase;
> > > +     u8 old_lcr;
> > > +
> > > +     generic_rs485_config(port, termios, rs485);
> > > +
> > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > 
> > You should split these to 3 lines to make it easier to follow what's the
> > actual operation taking place here:
> >                 efr = readb(...);
> >                 efr |= UART_EFR_ECB;
> >                 writeb(...);
> > 
> > ...I'm sorry I didn't realize this last time I looked.
> 
> Ok. Will fix in next submission. 
> 
> > > +
> > > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > > +
> > > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> > 
> > I guess "store original LCR" path is pretty obvious and can be dropped.
> 
> Ok. I can drop that part of the comment if it is clear otherwise.
>  
> > > +             old_lcr = readb(p + UART_LCR);
> > > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > > +
> > > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > 
> > Split this as well.
> > 
> > > +             writeb(old_lcr, p + UART_LCR);
> > > +    }
> > 
> > This function should also disable RS-485 if SER_RS485_ENABLED is set but
> > currently it does nothing?
> > 
> > --
> >  i.
> 
> That's what I would have thought as well, but I have electrically verified 
> disabling SER_RS485_ENABLED disables RS485 on the Sealevel cards I have 
> tested. I did this by monitoring the serial signal while a program 
> alternates setting and unsetting SER_RS485_ENABLED and verifying the card 
> is tri-stating when SER_RS485_ENABLED is set and fails to tri-state when 
> SER_RS485_ENABLED is not set.
> 
> I do not know where disabling RS485 is happening in the driver since 
> generic_rs485_config() does not handle this either, but I left it alone 
> since it appears to be working without being handled in the 
> ..._rs485_config() functions.

Okay, I guess it's something in generic_rs485_config() then. I've no 
objection in case it's working as is.

...I was badly mislead by the name of generic_rs485_config() btw. I 
thought it was something in core based on its name (although I also 
couldn't recall when that function would have been added to the core) but 
instead it's static inside exar and it would be better to name it so that 
it's obvious it refers to exar rather than "generic".

-- 
 i.


> 
> > 
> > > +
> > > +     return 0;
> > > + }
> > > +
> > >  static const struct serial_rs485 generic_rs485_supported = {
> > >       .flags = SER_RS485_ENABLED,
> > >  };
> > > @@ -744,6 +775,16 @@ static int __maybe_unused exar_resume(struct device *dev)
> > >       return 0;
> > >  }
> > >
> > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > > +                struct uart_8250_port *port, int idx)
> > > +{
> > > +     int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > > +
> > > +     port->port.rs485_config = sealevel_rs485_config;
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
> > >
> > >  static const struct exar8250_board pbn_fastcom335_2 = {
> > > @@ -809,6 +850,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> > >       .exit           = pci_xr17v35x_exit,
> > >  };
> > >
> > > +static const struct exar8250_board pbn_sealevel = {
> > > +     .setup          = pci_sealevel_setup,
> > > +     .exit           = pci_xr17v35x_exit,
> > > +};
> > > +
> > > +static const struct exar8250_board pbn_sealevel_16 = {
> > > +     .num_ports      = 16,
> > > +    .setup           = pci_sealevel_setup,
> > > +     .exit           = pci_xr17v35x_exit,
> > > +};
> > > +
> > >  #define CONNECT_DEVICE(devid, sdevid, bd) {                          \
> > >       PCI_DEVICE_SUB(                                                 \
> > >               PCI_VENDOR_ID_EXAR,                                     \
> > > @@ -838,6 +890,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> > >               (kernel_ulong_t)&bd                     \
> > >       }
> > >
> > > +#define SEALEVEL_DEVICE(devid, bd) {                 \
> > > +     PCI_DEVICE_SUB(                                 \
> > > +             PCI_VENDOR_ID_EXAR,                     \
> > > +             PCI_DEVICE_ID_EXAR_##devid,             \
> > > +             PCI_VENDOR_ID_SEALEVEL,                 \
> > > +             PCI_ANY_ID), 0, 0,      \
> > > +             (kernel_ulong_t)&bd                     \
> > > +     }
> > > +
> > >  static const struct pci_device_id exar_pci_tbl[] = {
> > >       EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
> > >       EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
> > > @@ -860,6 +921,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
> > >       CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
> > >       CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
> > >
> > > +     SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
> > > +     SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
> > > +     SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
> > > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> > > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> > > +
> > >       IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
> > >
> > >       /* USRobotics USR298x-OEM PCI Modems */
> > >
> > 

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:
> > From: Matthew Howell <matthew.howell@sealevel.com
> 
> Okay, I started reviewing it and it has all the same issues that I pointed
> out previously. Are you sure you sent a _new_ version?

I did send a new version.

> For your convenience I left all comments here.
> 
> >
> > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the
> > current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
> 
> All four above and two below (Link: and supposed to be a blank line)
> have a trailing space. Which editor do you use? You probably need to configure
> it to avoid this.

I use the the default editor in alpine and I had configured alpine per the 
email client configuration reccomendations in the documentation. I 
believe this happened because alpine automatically adds a new line to line 
wrap at the character limit, but since links are exempt from this rule I 
have to manually undo this and I made a mistake in that process.

I will try composing in a different editor and then file-read it into 
alpine instead of using the built in editor to see if that helps avoid the 
issue.
 
> > Sealevel cards.
> 
> > Link:
> > https://lore.kernel.org/all/b0b1863f-40f4-d78e-7bb7-dc4312449d9e@sealevel.com/
> 
> Should be a single line.
> 
> >
> 
> No blank lines in the tag block.
> 
> ...
> 
> > +     generic_rs485_config(port, termios, rs485);
> 
> > +     if (rs485->flags & SER_RS485_ENABLED) {
> 
> What I meant is to have
> 
>         if (!)rs485->flags & SER_RS485_ENABLED))
>                 return 0;
> 
> here, which allows you to reduce indentation level in the below block.
> 
> > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > +
> > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > +
> > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> > +             old_lcr = readb(p + UART_LCR);
> > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > +
> > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > +
> > +             writeb(old_lcr, p + UART_LCR);
> > +    }
> > +
> > +     return 0;

I see where you are coming from now, but I find that slightly less clear 
than having the 'main action' within the conditional statement. And since 
the code is not heavily indented I don't see much benefit of removing the 
indent.

> ...
> 
> > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > +                struct uart_8250_port *port, int idx)
> > +{
> > +     int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> 
> > +     port->port.rs485_config = sealevel_rs485_config;
> 
> This is incorrect in a way that we do not assign return values in case it is
> an error path. This will be prone to mistakes on the caller side.
> 
>         int ret;
> 
>         ret = ...
>         if (ret)
>                 return ret;
> 
>         ...
> 
>         return 0;
> 
> > +     return ret;
> > +}
> ...

Ok, will change for next submission.

> > +static const struct exar8250_board pbn_sealevel_16 = {
> > +     .num_ports      = 16,
> 
> > +    .setup           = pci_sealevel_setup,
> 
> Here is the indentation issue. Use leading TABs.
> 
> > +     .exit           = pci_xr17v35x_exit,
> > +};

Sorry, I had missed that one.

> ...
> 
> > +#define SEALEVEL_DEVICE(devid, bd) {                 \
> > +     PCI_DEVICE_SUB(                                 \
> > +             PCI_VENDOR_ID_EXAR,                     \
> > +             PCI_DEVICE_ID_EXAR_##devid,             \
> > +             PCI_VENDOR_ID_SEALEVEL,                 \
> 
> > +             PCI_ANY_ID), 0, 0,      \
> 
> Seems trailing \ is indented wrongly, missing TAB?
> 
> > +             (kernel_ulong_t)&bd                     \
> > +     }
> 

Ah, I see. In my editor none of the trailing \ lined up so I thought that 
there was some sotr of space/tab rule with trailing slashes I wasn't aware 
of, so I just used the same amount of spaces and tabs as the surrounding 
..._DEVICE() macros. However, I now see that I should have had it set to 
tab size 8 instead of 4.

Will fix in the next submission.

> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 11:05:20AM -0400, Matthew Howell wrote:
> On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:

...

> > > +     generic_rs485_config(port, termios, rs485);
> > 
> > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > 
> > What I meant is to have
> > 
> >         if (!)rs485->flags & SER_RS485_ENABLED))
> >                 return 0;
> > 
> > here, which allows you to reduce indentation level in the below block.
> > 
> > > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > > +
> > > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > > +
> > > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> > > +             old_lcr = readb(p + UART_LCR);
> > > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > > +
> > > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > > +
> > > +             writeb(old_lcr, p + UART_LCR);
> > > +    }
> > > +
> > > +     return 0;
> 
> I see where you are coming from now, but I find that slightly less clear 
> than having the 'main action' within the conditional statement. And since 
> the code is not heavily indented I don't see much benefit of removing the 
> indent.

In that case it might make sense to split to two functions:

func1()
{
	...
}

func2()
{
	if (...)
		return func1()

	return 0;
}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 11:05:20AM -0400, Matthew Howell wrote:
> > On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > > On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:
> 
> ...
> 
> > > > +     generic_rs485_config(port, termios, rs485);
> > >
> > > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > >
> > > What I meant is to have
> > >
> > >         if (!)rs485->flags & SER_RS485_ENABLED))
> > >                 return 0;
> > >
> > > here, which allows you to reduce indentation level in the below block.
> > >
> > > > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > > > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > > > +
> > > > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > > > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > > > +
> > > > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> > > > +             old_lcr = readb(p + UART_LCR);
> > > > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > > > +
> > > > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > > > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > > > +
> > > > +             writeb(old_lcr, p + UART_LCR);
> > > > +    }
> > > > +
> > > > +     return 0;
> >
> > I see where you are coming from now, but I find that slightly less clear
> > than having the 'main action' within the conditional statement. And since
> > the code is not heavily indented I don't see much benefit of removing the
> > indent.
> 
> In that case it might make sense to split to two functions:
> 
> func1()
> {
>         ...
> }
> 
> func2()
> {
>         if (...)
>                 return func1()
> 
>         return 0;
> }

I will have to respectfully disagree on this. Splitting the function into 
two still adds additional redirection, however small, to the function. I 
would also like to point out that the level of indent I am using is not 
uncommon in 8250_exar.c and as such I do not find the styling of the 
function to be out of place. 

I will resubmit and try to address the other concerns raised by you and 
Ilpo but unless Greg KH says otherwise I don't see any reason to change 
the indent at this time.

> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH V4 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Andy Shevchenko]

On Mon, Sep 11, 2023 at 09:22:45AM -0400, Matthew Howell wrote:
> On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 11:05:20AM -0400, Matthew Howell wrote:
> > > On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > > > On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:

...

> > > > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > > >
> > > > What I meant is to have
> > > >
> > > >         if (!)rs485->flags & SER_RS485_ENABLED))
> > > >                 return 0;
> > > >
> > > > here, which allows you to reduce indentation level in the below block.
> > > >
> > > > > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > > > > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > > > > +
> > > > > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > > > > +             writeb(UART_MCR_OUT1, p + UART_MCR);
> > > > > +
> > > > > +             /* Store original LCR and set LCR[7]=1 to enable access to DLD register */
> > > > > +             old_lcr = readb(p + UART_LCR);
> > > > > +             writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
> > > > > +
> > > > > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > > > > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > > > > +
> > > > > +             writeb(old_lcr, p + UART_LCR);
> > > > > +    }
> > > > > +
> > > > > +     return 0;
> > >
> > > I see where you are coming from now, but I find that slightly less clear
> > > than having the 'main action' within the conditional statement. And since
> > > the code is not heavily indented I don't see much benefit of removing the
> > > indent.
> > 
> > In that case it might make sense to split to two functions:
> > 
> > func1()
> > {
> >         ...
> > }
> > 
> > func2()
> > {
> >         if (...)
> >                 return func1()
> > 
> >         return 0;
> > }
> 
> I will have to respectfully disagree on this. Splitting the function into 
> two still adds additional redirection, however small, to the function.

You mean in the C code? Because in assembly it will be the same (as long as
the compiler optimisation is on).

But besides that it's a common practice to split in case the function is long
enough to be on a single screen page (meaning the body of the conditional ~20
LoCs threshold).

> I would also like to point out that the level of indent I am using is not 
> uncommon in 8250_exar.c and as such I do not find the styling of the 
> function to be out of place. 

You are probably referring to actually two functions, i.e.
pci_fastcom335_setup() and xr17v35x_register_gpio(), right?

Each of these three cases (including yours) are different. I do not think
there is a common ground to support your way by pointing out on them.

> I will resubmit and try to address the other concerns raised by you and 
> Ilpo but unless Greg KH says otherwise I don't see any reason to change 
> the indent at this time.


-- 
With Best Regards,
Andy Shevchenko