[PATCH v2 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[PATCH v2 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[parker]

From: Parker Newman <pnewman@connecttech.com>

Hello,
These patches add proper support for most of Connect Tech's (CTI) Exar
based serial cards. Previously, only a subset of CTI's cards would work
with the Exar driver while the rest required the CTI out-of-tree driver.
These patches are intended to phase out the out-of-tree driver.

I am new to the mailing lists and contributing to the kernel so please
let me know if I have made any mistakes or if you have any feedback.

Changes in v2:
- Put missing PCI IDs in 8250_exar.c instead of pci_ids.h
- Split large patch into smaller ones

Thank you,

Parker Newman (7):
  serial: exar: adding missing CTI and Exar PCI ids
  serial: exar: add support for reading from Exar EEPROM
  serial: exar: add support for config/set single MPIO
  serial: exar: add optional board_setup function
  serial: exar: add some CTI helper functions
  serial: exar: add CTI board and port setup functions
  serial: exar: fix: fix crash during shutdown if setup fails

 drivers/tty/serial/8250/8250_exar.c | 1083 +++++++++++++++++++++++++--
 1 file changed, 1017 insertions(+), 66 deletions(-)


base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
--
2.43.2

[PATCH v2 1/7] serial: exar: adding missing CTI and Exar PCI ids

[parker]

From: Parker Newman <pnewman@connecttech.com>

- Added Connect Tech and Exar IDs not already in pci_ids.h

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 42 +++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 0440df7de1ed..4d1e07343d0b 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -46,8 +46,50 @@
 #define PCI_DEVICE_ID_COMMTECH_4228PCIE		0x0021
 #define PCI_DEVICE_ID_COMMTECH_4222PCIE		0x0022

+#define PCI_VENDOR_ID_CONNECT_TECH				0x12c4
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO        0x0340
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A      0x0341
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B      0x0342
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS           0x0350
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A         0x0351
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B         0x0352
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS           0x0353
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A        0x0354
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B        0x0355
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO      0x0360
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A    0x0361
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B    0x0362
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP             0x0370
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232         0x0371
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485         0x0372
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP           0x0373
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP           0x0374
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP           0x0375
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS      0x0376
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT   0x0380
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT  0x0381
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO        0x0382
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO    0x0392
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP        0x03A0
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232    0x03A1
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485    0x03A2
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS 0x03A3
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XEG001               0x0602
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_BASE           0x1000
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_2              0x1002
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_4              0x1004
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_8              0x1008
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_12             0x100C
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_16             0x1010
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X          0x110c
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X          0x110d
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16                 0x1110
+
 #define PCI_DEVICE_ID_EXAR_XR17V4358		0x4358
 #define PCI_DEVICE_ID_EXAR_XR17V8358		0x8358
+#define PCI_DEVICE_ID_EXAR_XR17V252		0x0252
+#define PCI_DEVICE_ID_EXAR_XR17V254		0x0254
+#define PCI_DEVICE_ID_EXAR_XR17V258		0x0258

 #define PCI_SUBDEVICE_ID_USR_2980		0x0128
 #define PCI_SUBDEVICE_ID_USR_2981		0x0129
--
2.43.2

[PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[parker]

From: Parker Newman <pnewman@connecttech.com>

- Adds support for reading a word from the Exar EEPROM.
- Adds exar_write_reg/exar_read_reg for reading and writing to the UART's
config registers.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 4d1e07343d0b..49d690344e65 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -128,6 +128,16 @@
 #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
 #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */

+/* EEPROM registers */
+#define UART_EXAR_REGB                  0x8e
+#define UART_EXAR_REGB_EECK             BIT(4)
+#define UART_EXAR_REGB_EECS             BIT(5)
+#define UART_EXAR_REGB_EEDI             BIT(6)
+#define UART_EXAR_REGB_EEDO             BIT(7)
+#define UART_EXAR_REGB_EE_ADDR_SIZE     6
+#define UART_EXAR_REGB_EE_DATA_SIZE     16
+
+
 /*
  * IOT2040 MPIO wiring semantics:
  *
@@ -195,6 +205,106 @@ struct exar8250 {
 	int			line[];
 };

+static inline void exar_write_reg(struct exar8250 *priv,
+				unsigned int reg, uint8_t value)
+{
+	if (!priv || !priv->virt)
+		return;
+
+	writeb(value, priv->virt + reg);
+}
+
+static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned int reg)
+{
+	if (!priv || !priv->virt)
+		return 0;
+
+	return readb(priv->virt + reg);
+}
+
+static inline void exar_ee_select(struct exar8250 *priv, bool enable)
+{
+	uint8_t value = 0x00;
+
+	if (enable)
+		value |= UART_EXAR_REGB_EECS;
+
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+}
+
+static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
+{
+	uint8_t value = UART_EXAR_REGB_EECS;
+
+	if (bit)
+		value |= UART_EXAR_REGB_EEDI;
+
+	//Clock out the bit on the i2c interface
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+
+	value |= UART_EXAR_REGB_EECK;
+
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+}
+
+static inline uint8_t exar_ee_read_bit(struct exar8250 *priv)
+{
+	uint8_t regb;
+	uint8_t value = UART_EXAR_REGB_EECS;
+
+	//Clock in the bit on the i2c interface
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+
+	value |= UART_EXAR_REGB_EECK;
+
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+
+	regb = exar_read_reg(priv, UART_EXAR_REGB);
+
+	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+}
+
+/**
+ * exar_ee_read() - Read a word from the EEPROM
+ * @priv: Device's private structure
+ * @ee_addr: Offset of EEPROM to read word from
+ *
+ * Read a single 16bit word from an Exar UART's EEPROM
+ *
+ * Return: EEPROM word on success, negative error code on failure
+ */
+static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
+{
+	int i;
+	int data = 0;
+
+	exar_ee_select(priv, true);
+
+	//Send read command (opcode 110)
+	exar_ee_write_bit(priv, 1);
+	exar_ee_write_bit(priv, 1);
+	exar_ee_write_bit(priv, 0);
+
+	//Send address to read from
+	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
+		exar_ee_write_bit(priv, (ee_addr & i));
+
+	//Read data 1 bit at a time
+	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
+		data <<= 1;
+		data |= exar_ee_read_bit(priv);
+	}
+
+	exar_ee_select(priv, false);
+
+	return data;
+}
+
 static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
 	/*
--
2.43.2

Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[Greg Kroah-Hartman]

On Thu, Apr 11, 2024 at 04:25:40PM -0400, parker@finest.io wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> - Adds support for reading a word from the Exar EEPROM.
> - Adds exar_write_reg/exar_read_reg for reading and writing to the UART's
> config registers.

First off, thanks for splitting this up, looks much better.

Some minor nits here:

> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 4d1e07343d0b..49d690344e65 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -128,6 +128,16 @@
>  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
>  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> 
> +/* EEPROM registers */
> +#define UART_EXAR_REGB                  0x8e
> +#define UART_EXAR_REGB_EECK             BIT(4)
> +#define UART_EXAR_REGB_EECS             BIT(5)
> +#define UART_EXAR_REGB_EEDI             BIT(6)
> +#define UART_EXAR_REGB_EEDO             BIT(7)
> +#define UART_EXAR_REGB_EE_ADDR_SIZE     6
> +#define UART_EXAR_REGB_EE_DATA_SIZE     16

Use tabs after the define name and before the value?

> +
> +
>  /*
>   * IOT2040 MPIO wiring semantics:
>   *
> @@ -195,6 +205,106 @@ struct exar8250 {
>  	int			line[];
>  };
> 
> +static inline void exar_write_reg(struct exar8250 *priv,
> +				unsigned int reg, uint8_t value)
> +{
> +	if (!priv || !priv->virt)
> +		return;
> +
> +	writeb(value, priv->virt + reg);
> +}
> +
> +static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned int reg)
> +{
> +	if (!priv || !priv->virt)
> +		return 0;

How can either of these ever happen?  You control when this is called,
right?  So just make sure that isn't an issue.

> +
> +	return readb(priv->virt + reg);
> +}
> +
> +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> +{
> +	uint8_t value = 0x00;

This is the kernel, please use kernel types, not userspace types (i.e.
u8 not uint8_t).  Yes, there are lots of places in the kernel that have
userspace types, but let's not add to the mess please.

> +
> +	if (enable)
> +		value |= UART_EXAR_REGB_EECS;
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);

Why wait this amount of time?  A comment would be nice.  Why not just
do a read to ensure the write happened instead?

> +}
> +
> +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> +{
> +	uint8_t value = UART_EXAR_REGB_EECS;

Same comment about the type, here and everywhere else.

> +
> +	if (bit)
> +		value |= UART_EXAR_REGB_EEDI;
> +
> +	//Clock out the bit on the i2c interface

Comments using // are fine, but please put a space after the "//" to
make them readable

> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);

Same commment about the time value, here and everywhere else.  Why slow
things down if you don't have to?

thanks,

greg k-h

Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[Parker Newman]

On Fri, 12 Apr 2024 07:26:49 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Apr 11, 2024 at 04:25:40PM -0400, parker@finest.io wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > - Adds support for reading a word from the Exar EEPROM.
> > - Adds exar_write_reg/exar_read_reg for reading and writing to the UART's
> > config registers.
>
> First off, thanks for splitting this up, looks much better.
>
> Some minor nits here:
>
> >
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 4d1e07343d0b..49d690344e65 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -128,6 +128,16 @@
> >  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
> >  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> >
> > +/* EEPROM registers */
> > +#define UART_EXAR_REGB                  0x8e
> > +#define UART_EXAR_REGB_EECK             BIT(4)
> > +#define UART_EXAR_REGB_EECS             BIT(5)
> > +#define UART_EXAR_REGB_EEDI             BIT(6)
> > +#define UART_EXAR_REGB_EEDO             BIT(7)
> > +#define UART_EXAR_REGB_EE_ADDR_SIZE     6
> > +#define UART_EXAR_REGB_EE_DATA_SIZE     16
>
> Use tabs after the define name and before the value?

There should be tabs there... I will re-tab them in v3 to make sure.

>
> > +
> > +
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -195,6 +205,106 @@ struct exar8250 {
> >  	int			line[];
> >  };
> >
> > +static inline void exar_write_reg(struct exar8250 *priv,
> > +				unsigned int reg, uint8_t value)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return;
> > +
> > +	writeb(value, priv->virt + reg);
> > +}
> > +
> > +static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return 0;
>
> How can either of these ever happen?  You control when this is called,
> right?  So just make sure that isn't an issue.

I think I was a bit over paranoid here. I was considering there are other
3rd party cards supported in this driver that use uart_port->private_data
differently or don't set it. I agree they aren't really needed in the end.

> > +
> > +	return readb(priv->virt + reg);
> > +}
> > +
> > +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> > +{
> > +	uint8_t value = 0x00;
>
> This is the kernel, please use kernel types, not userspace types (i.e.
> u8 not uint8_t).  Yes, there are lots of places in the kernel that have
> userspace types, but let's not add to the mess please.

Yes there is some confusion on which should be used.
Thanks for the clarification. I will convert in v3.

>
> > +
> > +	if (enable)
> > +		value |= UART_EXAR_REGB_EECS;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
>
> Why wait this amount of time?  A comment would be nice.  Why not just
> do a read to ensure the write happened instead?

The Exar UART uses a bit-bang I2C interface for the EEPROM so a delay is
needed for the I2C bit time (2us = 500khz). This is legacy code so I will
double check this is actually needed and add comments if it is.

> > +}
> > +
> > +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> > +{
> > +	uint8_t value = UART_EXAR_REGB_EECS;
>
> Same comment about the type, here and everywhere else.
>
> > +
> > +	if (bit)
> > +		value |= UART_EXAR_REGB_EEDI;
> > +
> > +	//Clock out the bit on the i2c interface
>
> Comments using // are fine, but please put a space after the "//" to
> make them readable

I will fix.

> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
>
> Same commment about the time value, here and everywhere else.  Why slow
> things down if you don't have to?
>
> thanks,
>
> greg k-h

Thanks for the feedback!
- Parker

Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[Ilpo Järvinen]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> - Adds support for reading a word from the Exar EEPROM.
> - Adds exar_write_reg/exar_read_reg for reading and writing to the UART's
> config registers.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 4d1e07343d0b..49d690344e65 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -128,6 +128,16 @@
>  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
>  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> 
> +/* EEPROM registers */
> +#define UART_EXAR_REGB                  0x8e
> +#define UART_EXAR_REGB_EECK             BIT(4)
> +#define UART_EXAR_REGB_EECS             BIT(5)
> +#define UART_EXAR_REGB_EEDI             BIT(6)
> +#define UART_EXAR_REGB_EEDO             BIT(7)
> +#define UART_EXAR_REGB_EE_ADDR_SIZE     6
> +#define UART_EXAR_REGB_EE_DATA_SIZE     16
> +
> +

Extra new line.

>  /*
>   * IOT2040 MPIO wiring semantics:
>   *
> @@ -195,6 +205,106 @@ struct exar8250 {
>  	int			line[];
>  };
> 
> +static inline void exar_write_reg(struct exar8250 *priv,
> +				unsigned int reg, uint8_t value)
> +{
> +	if (!priv || !priv->virt)
> +		return;
> +
> +	writeb(value, priv->virt + reg);
> +}
> +
> +static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned int reg)
> +{
> +	if (!priv || !priv->virt)
> +		return 0;
> +
> +	return readb(priv->virt + reg);
> +}
> +
> +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> +{
> +	uint8_t value = 0x00;
> +
> +	if (enable)
> +		value |= UART_EXAR_REGB_EECS;

You could just do:
	u8 value;

	value = enable ? UART_EXAR_REGB_EECS : 0;

Or even:

	exar_write_reg(priv, UART_EXAR_REGB, enable ? UART_EXAR_REGB_EECS : 0);
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +}
> +
> +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> +{
> +	uint8_t value = UART_EXAR_REGB_EECS;
> +
> +	if (bit)
> +		value |= UART_EXAR_REGB_EEDI;
> +
> +	//Clock out the bit on the i2c interface
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +
> +	value |= UART_EXAR_REGB_EECK;
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +}
> +
> +static inline uint8_t exar_ee_read_bit(struct exar8250 *priv)
> +{
> +	uint8_t regb;
> +	uint8_t value = UART_EXAR_REGB_EECS;
> +
> +	//Clock in the bit on the i2c interface
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +
> +	value |= UART_EXAR_REGB_EECK;
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +
> +	regb = exar_read_reg(priv, UART_EXAR_REGB);
> +
> +	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
> +}
> +
> +/**
> + * exar_ee_read() - Read a word from the EEPROM
> + * @priv: Device's private structure
> + * @ee_addr: Offset of EEPROM to read word from
> + *
> + * Read a single 16bit word from an Exar UART's EEPROM

Add missing .

> + *
> + * Return: EEPROM word on success, negative error code on failure

This function does not return any -Exx code as far as I can see??

> + */
> +static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> +{
> +	int i;
> +	int data = 0;
> +
> +	exar_ee_select(priv, true);
> +
> +	//Send read command (opcode 110)
> +	exar_ee_write_bit(priv, 1);
> +	exar_ee_write_bit(priv, 1);
> +	exar_ee_write_bit(priv, 0);
> +
> +	//Send address to read from
> +	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> +		exar_ee_write_bit(priv, (ee_addr & i));
> +
> +	//Read data 1 bit at a time
> +	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> +		data <<= 1;
> +		data |= exar_ee_read_bit(priv);
> +	}
> +
> +	exar_ee_select(priv, false);
> +
> +	return data;
> +}

-- 
 i.

Re: [PATCH v2 2/7] serial: exar: add support for reading from Exar EEPROM

[Parker Newman]

On Fri, 12 Apr 2024 13:36:42 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > - Adds support for reading a word from the Exar EEPROM.
> > - Adds exar_write_reg/exar_read_reg for reading and writing to the UART's
> > config registers.
> > 
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 110 ++++++++++++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 4d1e07343d0b..49d690344e65 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -128,6 +128,16 @@
> >  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
> >  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> > 
> > +/* EEPROM registers */
> > +#define UART_EXAR_REGB                  0x8e
> > +#define UART_EXAR_REGB_EECK             BIT(4)
> > +#define UART_EXAR_REGB_EECS             BIT(5)
> > +#define UART_EXAR_REGB_EEDI             BIT(6)
> > +#define UART_EXAR_REGB_EEDO             BIT(7)
> > +#define UART_EXAR_REGB_EE_ADDR_SIZE     6
> > +#define UART_EXAR_REGB_EE_DATA_SIZE     16
> > +
> > +  
> 
> Extra new line.
> 
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -195,6 +205,106 @@ struct exar8250 {
> >  	int			line[];
> >  };
> > 
> > +static inline void exar_write_reg(struct exar8250 *priv,
> > +				unsigned int reg, uint8_t value)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return;
> > +
> > +	writeb(value, priv->virt + reg);
> > +}
> > +
> > +static inline uint8_t exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > +{
> > +	if (!priv || !priv->virt)
> > +		return 0;
> > +
> > +	return readb(priv->virt + reg);
> > +}
> > +
> > +static inline void exar_ee_select(struct exar8250 *priv, bool enable)
> > +{
> > +	uint8_t value = 0x00;
> > +
> > +	if (enable)
> > +		value |= UART_EXAR_REGB_EECS;  
> 
> You could just do:
> 	u8 value;
> 
> 	value = enable ? UART_EXAR_REGB_EECS : 0;
> 
> Or even:
> 
> 	exar_write_reg(priv, UART_EXAR_REGB, enable ? UART_EXAR_REGB_EECS : 0);
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +}
> > +
> > +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> > +{
> > +	uint8_t value = UART_EXAR_REGB_EECS;
> > +
> > +	if (bit)
> > +		value |= UART_EXAR_REGB_EEDI;
> > +
> > +	//Clock out the bit on the i2c interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	value |= UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +}
> > +
> > +static inline uint8_t exar_ee_read_bit(struct exar8250 *priv)
> > +{
> > +	uint8_t regb;
> > +	uint8_t value = UART_EXAR_REGB_EECS;
> > +
> > +	//Clock in the bit on the i2c interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	value |= UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	regb = exar_read_reg(priv, UART_EXAR_REGB);
> > +
> > +	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
> > +}
> > +
> > +/**
> > + * exar_ee_read() - Read a word from the EEPROM
> > + * @priv: Device's private structure
> > + * @ee_addr: Offset of EEPROM to read word from
> > + *
> > + * Read a single 16bit word from an Exar UART's EEPROM  
> 
> Add missing .
> 
> > + *
> > + * Return: EEPROM word on success, negative error code on failure  
> 
> This function does not return any -Exx code as far as I can see??
> 
> > + */
> > +static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> > +{
> > +	int i;
> > +	int data = 0;
> > +
> > +	exar_ee_select(priv, true);
> > +
> > +	//Send read command (opcode 110)
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 0);
> > +
> > +	//Send address to read from
> > +	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> > +		exar_ee_write_bit(priv, (ee_addr & i));
> > +
> > +	//Read data 1 bit at a time
> > +	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > +		data <<= 1;
> > +		data |= exar_ee_read_bit(priv);
> > +	}
> > +
> > +	exar_ee_select(priv, false);
> > +
> > +	return data;
> > +}  
> 

I will fix all of these. 
Thanks,
-Parker

[PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[parker]

From: Parker Newman <pnewman@connecttech.com>

Adds support for configuring and setting a single MPIO

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 49d690344e65..9915a99cb7c6 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
 	return data;
 }

+/**
+ * exar_mpio_config() - Configure an EXar MPIO as input or output
+ * @priv: Device's private structure
+ * @mpio_num: MPIO number/offset to configure
+ * @output: Configure as output if true, inout if false
+ *
+ * Configure a single MPIO as an input or output and disable trisate.
+ * If configuring as output it is reccomended to set value with
+ * exar_mpio_set prior to calling this function to ensure default state.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int exar_mpio_config(struct exar8250 *priv,
+			unsigned int mpio_num, bool output)
+{
+	uint8_t sel_reg; //MPIO Select register (input/output)
+	uint8_t tri_reg; //MPIO Tristate register
+	uint8_t value;
+	unsigned int bit;
+
+	if (mpio_num < 8) {
+		sel_reg = UART_EXAR_MPIOSEL_7_0;
+		tri_reg = UART_EXAR_MPIO3T_7_0;
+		bit = mpio_num;
+	} else if (mpio_num >= 8 && mpio_num < 16) {
+		sel_reg = UART_EXAR_MPIOSEL_15_8;
+		tri_reg = UART_EXAR_MPIO3T_15_8;
+		bit = mpio_num - 8;
+	} else {
+		return -EINVAL;
+	}
+
+	//Disable MPIO pin tri-state
+	value = exar_read_reg(priv, tri_reg);
+	value &= ~(BIT(bit));
+	exar_write_reg(priv, tri_reg, value);
+
+	value = exar_read_reg(priv, sel_reg);
+	//Set MPIO as input (1) or output (0)
+	if (output)
+		value &= ~(BIT(bit));
+	else
+		value |= BIT(bit);
+
+	exar_write_reg(priv, sel_reg, value);
+
+	return 0;
+}
+/**
+ * exar_mpio_set() - Set an Exar MPIO output high or low
+ * @priv: Device's private structure
+ * @mpio_num: MPIO number/offset to set
+ * @high: Set MPIO high if true, low if false
+ *
+ * Set a single MPIO high or low. exar_mpio_config must also be called
+ * to configure the pin as an output.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int exar_mpio_set(struct exar8250 *priv,
+		unsigned int mpio_num, bool high)
+{
+	uint8_t reg;
+	uint8_t value;
+	unsigned int bit;
+
+	if (mpio_num < 8) {
+		reg = UART_EXAR_MPIOSEL_7_0;
+		bit = mpio_num;
+	} else if (mpio_num >= 8 && mpio_num < 16) {
+		reg = UART_EXAR_MPIOSEL_15_8;
+		bit = mpio_num - 8;
+	} else {
+		return -EINVAL;
+	}
+
+	value = exar_read_reg(priv, reg);
+
+	if (high)
+		value |= BIT(bit);
+	else
+		value &= ~(BIT(bit));
+
+	exar_write_reg(priv, reg, value);
+
+	return 0;
+}
+
 static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
 	/*
--
2.43.2

Re: [PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[Greg Kroah-Hartman]

On Thu, Apr 11, 2024 at 04:25:41PM -0400, parker@finest.io wrote:
> +/**
> + * exar_mpio_config() - Configure an EXar MPIO as input or output
> + * @priv: Device's private structure
> + * @mpio_num: MPIO number/offset to configure
> + * @output: Configure as output if true, inout if false
> + *
> + * Configure a single MPIO as an input or output and disable trisate.
> + * If configuring as output it is reccomended to set value with
> + * exar_mpio_set prior to calling this function to ensure default state.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int exar_mpio_config(struct exar8250 *priv,
> +			unsigned int mpio_num, bool output)

When you have a bool in a function, every time you read the code you
have to go and figure out what that boolean means.

Have 2 functions:
	exar_mpio_config_input()
	exar_mpio_config_output()

and then have THEM call this function with the bool set or not.  That
way when reading the code you know exactly what is happening.

Same with other functions in this patch.  Naming is hard, make it easy
please.

thanks,

greg k-h

Re: [PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[Parker Newman]

On Fri, 12 Apr 2024 07:29:16 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Apr 11, 2024 at 04:25:41PM -0400, parker@finest.io wrote:
> > +/**
> > + * exar_mpio_config() - Configure an EXar MPIO as input or output
> > + * @priv: Device's private structure
> > + * @mpio_num: MPIO number/offset to configure
> > + * @output: Configure as output if true, inout if false
> > + *
> > + * Configure a single MPIO as an input or output and disable trisate.
> > + * If configuring as output it is reccomended to set value with
> > + * exar_mpio_set prior to calling this function to ensure default state.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int exar_mpio_config(struct exar8250 *priv,
> > +			unsigned int mpio_num, bool output)
>
> When you have a bool in a function, every time you read the code you
> have to go and figure out what that boolean means.
>
> Have 2 functions:
> 	exar_mpio_config_input()
> 	exar_mpio_config_output()
>
> and then have THEM call this function with the bool set or not.  That
> way when reading the code you know exactly what is happening.
>
> Same with other functions in this patch.  Naming is hard, make it easy
> please.

Good feedback thanks.

> thanks,
>
> greg k-h

Re: [PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[Ilpo Järvinen]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Adds support for configuring and setting a single MPIO
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 49d690344e65..9915a99cb7c6 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
>  	return data;
>  }
> 
> +/**
> + * exar_mpio_config() - Configure an EXar MPIO as input or output
> + * @priv: Device's private structure
> + * @mpio_num: MPIO number/offset to configure
> + * @output: Configure as output if true, inout if false
> + *
> + * Configure a single MPIO as an input or output and disable trisate.

tristate

> + * If configuring as output it is reccomended to set value with
> + * exar_mpio_set prior to calling this function to ensure default state.

Use () if talking about function.

> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int exar_mpio_config(struct exar8250 *priv,
> +			unsigned int mpio_num, bool output)
> +{
> +	uint8_t sel_reg; //MPIO Select register (input/output)
> +	uint8_t tri_reg; //MPIO Tristate register
> +	uint8_t value;
> +	unsigned int bit;
> +
> +	if (mpio_num < 8) {
> +		sel_reg = UART_EXAR_MPIOSEL_7_0;
> +		tri_reg = UART_EXAR_MPIO3T_7_0;
> +		bit = mpio_num;
> +	} else if (mpio_num >= 8 && mpio_num < 16) {
> +		sel_reg = UART_EXAR_MPIOSEL_15_8;
> +		tri_reg = UART_EXAR_MPIO3T_15_8;
> +		bit = mpio_num - 8;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	//Disable MPIO pin tri-state
> +	value = exar_read_reg(priv, tri_reg);
> +	value &= ~(BIT(bit));

Use more meaningful variable name than "bit", it could perhaps even avoid 
the need to use the comment if the code is self-explanary with better 
variable name.

> +	exar_write_reg(priv, tri_reg, value);
> +
> +	value = exar_read_reg(priv, sel_reg);
> +	//Set MPIO as input (1) or output (0)

Unnecessary comment.

> +	if (output)
> +		value &= ~(BIT(bit));

Unnecessary parenthesis.

> +	else
> +		value |= BIT(bit);
> +
> +	exar_write_reg(priv, sel_reg, value);

Don't leave empty line into RMW sequence.

> +
> +	return 0;
> +}
> +/**
> + * exar_mpio_set() - Set an Exar MPIO output high or low
> + * @priv: Device's private structure
> + * @mpio_num: MPIO number/offset to set
> + * @high: Set MPIO high if true, low if false
> + *
> + * Set a single MPIO high or low. exar_mpio_config must also be called
> + * to configure the pin as an output.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int exar_mpio_set(struct exar8250 *priv,
> +		unsigned int mpio_num, bool high)
> +{
> +	uint8_t reg;
> +	uint8_t value;
> +	unsigned int bit;
> +
> +	if (mpio_num < 8) {
> +		reg = UART_EXAR_MPIOSEL_7_0;
> +		bit = mpio_num;
> +	} else if (mpio_num >= 8 && mpio_num < 16) {
> +		reg = UART_EXAR_MPIOSEL_15_8;
> +		bit = mpio_num - 8;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	value = exar_read_reg(priv, reg);
> +
> +	if (high)
> +		value |= BIT(bit);
> +	else
> +		value &= ~(BIT(bit));

Extra parenthesis.

> +
> +	exar_write_reg(priv, reg, value);

Again, I'd put this kind of simple RMW sequence without newlines.

> +
> +	return 0;
> +}

There are zero users of these functions so I couldn't review if two 
functions are really needed, or if the difference could be simply handled 
using a boolean parameter.

-- 
 i.

Re: [PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[Parker Newman]

On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > Adds support for configuring and setting a single MPIO
> > 
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 49d690344e65..9915a99cb7c6 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> >  	return data;
> >  }
> > 
> > +/**
> > + * exar_mpio_config() - Configure an EXar MPIO as input or output
> > + * @priv: Device's private structure
> > + * @mpio_num: MPIO number/offset to configure
> > + * @output: Configure as output if true, inout if false
> > + *
> > + * Configure a single MPIO as an input or output and disable trisate.  
> 
> tristate
> 
> > + * If configuring as output it is reccomended to set value with
> > + * exar_mpio_set prior to calling this function to ensure default state.  
> 
> Use () if talking about function.
> 
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int exar_mpio_config(struct exar8250 *priv,
> > +			unsigned int mpio_num, bool output)
> > +{
> > +	uint8_t sel_reg; //MPIO Select register (input/output)
> > +	uint8_t tri_reg; //MPIO Tristate register
> > +	uint8_t value;
> > +	unsigned int bit;
> > +
> > +	if (mpio_num < 8) {
> > +		sel_reg = UART_EXAR_MPIOSEL_7_0;
> > +		tri_reg = UART_EXAR_MPIO3T_7_0;
> > +		bit = mpio_num;
> > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > +		sel_reg = UART_EXAR_MPIOSEL_15_8;
> > +		tri_reg = UART_EXAR_MPIO3T_15_8;
> > +		bit = mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	//Disable MPIO pin tri-state
> > +	value = exar_read_reg(priv, tri_reg);
> > +	value &= ~(BIT(bit));  
> 
> Use more meaningful variable name than "bit", it could perhaps even avoid 
> the need to use the comment if the code is self-explanary with better 
> variable name.
> 
> > +	exar_write_reg(priv, tri_reg, value);
> > +
> > +	value = exar_read_reg(priv, sel_reg);
> > +	//Set MPIO as input (1) or output (0)  
> 
> Unnecessary comment.
> 
> > +	if (output)
> > +		value &= ~(BIT(bit));  
> 
> Unnecessary parenthesis.
> 
> > +	else
> > +		value |= BIT(bit);
> > +
> > +	exar_write_reg(priv, sel_reg, value);  
> 
> Don't leave empty line into RMW sequence.
> 
> > +
> > +	return 0;
> > +}
> > +/**
> > + * exar_mpio_set() - Set an Exar MPIO output high or low
> > + * @priv: Device's private structure
> > + * @mpio_num: MPIO number/offset to set
> > + * @high: Set MPIO high if true, low if false
> > + *
> > + * Set a single MPIO high or low. exar_mpio_config must also be called
> > + * to configure the pin as an output.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int exar_mpio_set(struct exar8250 *priv,
> > +		unsigned int mpio_num, bool high)
> > +{
> > +	uint8_t reg;
> > +	uint8_t value;
> > +	unsigned int bit;
> > +
> > +	if (mpio_num < 8) {
> > +		reg = UART_EXAR_MPIOSEL_7_0;
> > +		bit = mpio_num;
> > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > +		reg = UART_EXAR_MPIOSEL_15_8;
> > +		bit = mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	value = exar_read_reg(priv, reg);
> > +
> > +	if (high)
> > +		value |= BIT(bit);
> > +	else
> > +		value &= ~(BIT(bit));  
> 
> Extra parenthesis.
> 
> > +
> > +	exar_write_reg(priv, reg, value);  
> 
> Again, I'd put this kind of simple RMW sequence without newlines.
> 
> > +
> > +	return 0;
> > +}  

I will fix above. 

> There are zero users of these functions so I couldn't review if two 
> functions are really needed, or if the difference could be simply handled 
> using a boolean parameter.
> 

The functions are used by code in other patches in this series. 

I kept exar_mpio_set() and exar_mpio_config() separate because we plan on
adding support for other features in the future that require reading and 
writing MPIO. 

Thanks,
-Parker


 

Re: [PATCH v2 3/7] serial: exar: add support for config/set single MPIO

[Ilpo Järvinen]

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

On Fri, 12 Apr 2024, Parker Newman wrote:

> On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 11 Apr 2024, parker@finest.io wrote:
> > 
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > Adds support for configuring and setting a single MPIO
> > > 
> > > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_exar.c | 88 +++++++++++++++++++++++++++++
> > >  1 file changed, 88 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > index 49d690344e65..9915a99cb7c6 100644
> > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > @@ -305,6 +305,94 @@ static int exar_ee_read(struct exar8250 *priv, uint8_t ee_addr)
> > >  	return data;
> > >  }
> > > 
> > > +/**
> > > + * exar_mpio_config() - Configure an EXar MPIO as input or output
> > > + * @priv: Device's private structure
> > > + * @mpio_num: MPIO number/offset to configure
> > > + * @output: Configure as output if true, inout if false
> > > + *
> > > + * Configure a single MPIO as an input or output and disable trisate.  
> > 
> > tristate
> > 
> > > + * If configuring as output it is reccomended to set value with
> > > + * exar_mpio_set prior to calling this function to ensure default state.  
> > 
> > Use () if talking about function.
> > 
> > > + *
> > > + * Return: 0 on success, negative error code on failure
> > > + */
> > > +static int exar_mpio_config(struct exar8250 *priv,
> > > +			unsigned int mpio_num, bool output)
> > > +{
> > > +	uint8_t sel_reg; //MPIO Select register (input/output)
> > > +	uint8_t tri_reg; //MPIO Tristate register
> > > +	uint8_t value;
> > > +	unsigned int bit;
> > > +
> > > +	if (mpio_num < 8) {
> > > +		sel_reg = UART_EXAR_MPIOSEL_7_0;
> > > +		tri_reg = UART_EXAR_MPIO3T_7_0;
> > > +		bit = mpio_num;
> > > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > > +		sel_reg = UART_EXAR_MPIOSEL_15_8;
> > > +		tri_reg = UART_EXAR_MPIO3T_15_8;
> > > +		bit = mpio_num - 8;
> > > +	} else {
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	//Disable MPIO pin tri-state
> > > +	value = exar_read_reg(priv, tri_reg);
> > > +	value &= ~(BIT(bit));  
> > 
> > Use more meaningful variable name than "bit", it could perhaps even avoid 
> > the need to use the comment if the code is self-explanary with better 
> > variable name.
> > 
> > > +	exar_write_reg(priv, tri_reg, value);
> > > +
> > > +	value = exar_read_reg(priv, sel_reg);
> > > +	//Set MPIO as input (1) or output (0)  
> > 
> > Unnecessary comment.
> > 
> > > +	if (output)
> > > +		value &= ~(BIT(bit));  
> > 
> > Unnecessary parenthesis.
> > 
> > > +	else
> > > +		value |= BIT(bit);
> > > +
> > > +	exar_write_reg(priv, sel_reg, value);  
> > 
> > Don't leave empty line into RMW sequence.
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +/**
> > > + * exar_mpio_set() - Set an Exar MPIO output high or low
> > > + * @priv: Device's private structure
> > > + * @mpio_num: MPIO number/offset to set
> > > + * @high: Set MPIO high if true, low if false
> > > + *
> > > + * Set a single MPIO high or low. exar_mpio_config must also be called
> > > + * to configure the pin as an output.
> > > + *
> > > + * Return: 0 on success, negative error code on failure
> > > + */
> > > +static int exar_mpio_set(struct exar8250 *priv,
> > > +		unsigned int mpio_num, bool high)
> > > +{
> > > +	uint8_t reg;
> > > +	uint8_t value;
> > > +	unsigned int bit;
> > > +
> > > +	if (mpio_num < 8) {
> > > +		reg = UART_EXAR_MPIOSEL_7_0;
> > > +		bit = mpio_num;
> > > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > > +		reg = UART_EXAR_MPIOSEL_15_8;
> > > +		bit = mpio_num - 8;
> > > +	} else {
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	value = exar_read_reg(priv, reg);
> > > +
> > > +	if (high)
> > > +		value |= BIT(bit);
> > > +	else
> > > +		value &= ~(BIT(bit));  
> > 
> > Extra parenthesis.
> > 
> > > +
> > > +	exar_write_reg(priv, reg, value);  
> > 
> > Again, I'd put this kind of simple RMW sequence without newlines.
> > 
> > > +
> > > +	return 0;
> > > +}  
> 
> I will fix above. 
> 
> > There are zero users of these functions so I couldn't review if two 
> > functions are really needed, or if the difference could be simply handled 
> > using a boolean parameter.
> > 
> 
> The functions are used by code in other patches in this series. 
> 
> I kept exar_mpio_set() and exar_mpio_config() separate because we plan on
> adding support for other features in the future that require reading and 
> writing MPIO. 

Ok. After getting up to the point where the callers were, I started to 
understand things somewhat better so keeping them separate seems fine 
with how I ended up understanding things.

But please put these functions into the patch which is using them when you 
reorganize the series.

-- 
 i.

[PATCH v2 4/7] serial: exar: add optional board_setup function

[parker]

From: Parker Newman <pnewman@connecttech.com>

Adds an optional "board_setup" function pointer to struct
exar8250_board. This gets called once during probe prior to setting up
the ports.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 9915a99cb7c6..b30f3855652a 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -143,7 +143,7 @@
  *
  * MPIO		Port	Function
  * ----		----	--------
- * 0		2 	Mode bit 0
+ * 0		2	Mode bit 0
  * 1		2	Mode bit 1
  * 2		2	Terminate bus
  * 3		-	<reserved>
@@ -179,22 +179,24 @@ struct exar8250_platform {
 	int (*rs485_config)(struct uart_port *port, struct ktermios *termios,
 			    struct serial_rs485 *rs485);
 	const struct serial_rs485 *rs485_supported;
-	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
-	void (*unregister_gpio)(struct uart_8250_port *);
+	int (*register_gpio)(struct pci_dev *pcidev, struct uart_8250_port *port);
+	void (*unregister_gpio)(struct uart_8250_port *port);
 };

 /**
  * struct exar8250_board - board information
  * @num_ports: number of serial ports
  * @reg_shift: describes UART register mapping in PCI memory
- * @setup: quirk run at ->probe() stage
+ * @board_setup: quirk run once at ->probe() stage before setting up ports
+ * @setup: quirk run at ->probe() stage for each port
  * @exit: quirk run at ->remove() stage
  */
 struct exar8250_board {
 	unsigned int num_ports;
 	unsigned int reg_shift;
-	int	(*setup)(struct exar8250 *, struct pci_dev *,
-			 struct uart_8250_port *, int);
+	int     (*board_setup)(struct exar8250 *priv);
+	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
+			 struct uart_8250_port *port, int idx);
 	void	(*exit)(struct pci_dev *pcidev);
 };

@@ -966,6 +968,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;

+	if (board->board_setup) {
+		rc = board->board_setup(priv);
+		if (rc) {
+			pci_err(pcidev,
+				"failed to setup serial board: %d\n", rc);
+			return rc;
+		}
+	}
+
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {
--
2.43.2

Re: [PATCH v2 4/7] serial: exar: add optional board_setup function

[Ilpo Järvinen]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Adds an optional "board_setup" function pointer to struct
> exar8250_board. This gets called once during probe prior to setting up
> the ports.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 9915a99cb7c6..b30f3855652a 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -143,7 +143,7 @@
>   *
>   * MPIO		Port	Function
>   * ----		----	--------
> - * 0		2 	Mode bit 0
> + * 0		2	Mode bit 0

Unrelated change.

>   * 1		2	Mode bit 1
>   * 2		2	Terminate bus
>   * 3		-	<reserved>
> @@ -179,22 +179,24 @@ struct exar8250_platform {
>  	int (*rs485_config)(struct uart_port *port, struct ktermios *termios,
>  			    struct serial_rs485 *rs485);
>  	const struct serial_rs485 *rs485_supported;
> -	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
> -	void (*unregister_gpio)(struct uart_8250_port *);
> +	int (*register_gpio)(struct pci_dev *pcidev, struct uart_8250_port *port);
> +	void (*unregister_gpio)(struct uart_8250_port *port);
>  };
> 
>  /**
>   * struct exar8250_board - board information
>   * @num_ports: number of serial ports
>   * @reg_shift: describes UART register mapping in PCI memory
> - * @setup: quirk run at ->probe() stage
> + * @board_setup: quirk run once at ->probe() stage before setting up ports
> + * @setup: quirk run at ->probe() stage for each port
>   * @exit: quirk run at ->remove() stage
>   */
>  struct exar8250_board {
>  	unsigned int num_ports;
>  	unsigned int reg_shift;
> -	int	(*setup)(struct exar8250 *, struct pci_dev *,
> -			 struct uart_8250_port *, int);
> +	int     (*board_setup)(struct exar8250 *priv);
> +	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
> +			 struct uart_8250_port *port, int idx);
>  	void	(*exit)(struct pci_dev *pcidev);
>  };
> 
> @@ -966,6 +968,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
>  	if (rc)
>  		return rc;
> 
> +	if (board->board_setup) {
> +		rc = board->board_setup(priv);

Could this be called board_init() as having both ->board_setup() and 
->setup() is bit confusing.

> +		if (rc) {
> +			pci_err(pcidev,
> +				"failed to setup serial board: %d\n", rc);

pci_err() belongs to pci subsystem. Please use dev_err() or return 
dev_err_probe().

> +			return rc;
> +		}
> +	}
> +
>  	for (i = 0; i < nr_ports && i < maxnr; i++) {
>  		rc = board->setup(priv, pcidev, &uart, i);
>  		if (rc) {


-- 
 i.

Re: [PATCH v2 4/7] serial: exar: add optional board_setup function

[Parker Newman]

On Fri, 12 Apr 2024 13:08:58 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > Adds an optional "board_setup" function pointer to struct
> > exar8250_board. This gets called once during probe prior to setting up
> > the ports.
> > 
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 9915a99cb7c6..b30f3855652a 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -143,7 +143,7 @@
> >   *
> >   * MPIO		Port	Function
> >   * ----		----	--------
> > - * 0		2 	Mode bit 0
> > + * 0		2	Mode bit 0  
> 
> Unrelated change.
>

Sorry, should have added a note about fixing this and the missing argument names
below due to checkpatch.pl. I will add a note in v3. 
 
> >   * 1		2	Mode bit 1
> >   * 2		2	Terminate bus
> >   * 3		-	<reserved>
> > @@ -179,22 +179,24 @@ struct exar8250_platform {
> >  	int (*rs485_config)(struct uart_port *port, struct ktermios *termios,
> >  			    struct serial_rs485 *rs485);
> >  	const struct serial_rs485 *rs485_supported;
> > -	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
> > -	void (*unregister_gpio)(struct uart_8250_port *);
> > +	int (*register_gpio)(struct pci_dev *pcidev, struct uart_8250_port *port);
> > +	void (*unregister_gpio)(struct uart_8250_port *port);
> >  };
> > 
> >  /**
> >   * struct exar8250_board - board information
> >   * @num_ports: number of serial ports
> >   * @reg_shift: describes UART register mapping in PCI memory
> > - * @setup: quirk run at ->probe() stage
> > + * @board_setup: quirk run once at ->probe() stage before setting up ports
> > + * @setup: quirk run at ->probe() stage for each port
> >   * @exit: quirk run at ->remove() stage
> >   */
> >  struct exar8250_board {
> >  	unsigned int num_ports;
> >  	unsigned int reg_shift;
> > -	int	(*setup)(struct exar8250 *, struct pci_dev *,
> > -			 struct uart_8250_port *, int);
> > +	int     (*board_setup)(struct exar8250 *priv);
> > +	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
> > +			 struct uart_8250_port *port, int idx);
> >  	void	(*exit)(struct pci_dev *pcidev);
> >  };
> > 
> > @@ -966,6 +968,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> >  	if (rc)
> >  		return rc;
> > 
> > +	if (board->board_setup) {
> > +		rc = board->board_setup(priv);  
> 
> Could this be called board_init() as having both ->board_setup() and 
> ->setup() is bit confusing.  

sure, good idea.

> > +		if (rc) {
> > +			pci_err(pcidev,
> > +				"failed to setup serial board: %d\n", rc);  
> 
> pci_err() belongs to pci subsystem. Please use dev_err() or return 
> dev_err_probe().

Did not know it was not allowed outside the PCI subsystem. I will switch 
to dev_err() etc. 

> > +			return rc;
> > +		}
> > +	}
> > +
> >  	for (i = 0; i < nr_ports && i < maxnr; i++) {
> >  		rc = board->setup(priv, pcidev, &uart, i);
> >  		if (rc) {  
> 
> 

Thanks,
-Parker

[PATCH v2 5/7] serial: exar: add some CTI helper functions

[parker]

From: Parker Newman <pnewman@connecttech.com>

- Adds various helper functions for CTI boards.
- Add osc_freq and pcidev to struct exar8250
- Added a exar_get_nr_ports function

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 363 +++++++++++++++++++++++++++-
 1 file changed, 357 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b30f3855652a..6f3697e34722 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -137,6 +137,9 @@
 #define UART_EXAR_REGB_EE_ADDR_SIZE     6
 #define UART_EXAR_REGB_EE_DATA_SIZE     16

+#define UART_EXAR_XR17C15X_PORT_OFFSET  0x200
+#define UART_EXAR_XR17V25X_PORT_OFFSET  0x200
+#define UART_EXAR_XR17V35X_PORT_OFFSET  0x400

 /*
  * IOT2040 MPIO wiring semantics:
@@ -173,6 +176,46 @@
 #define IOT2040_UARTS_ENABLE		0x03
 #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */

+/* CTI EEPROM offsets */
+#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words (4 bytes) */
+#define CTI_EE_OFF_XR17V25X_OSC_FREQ    0x08  /* 2 words (4 bytes) */
+#define CTI_EE_OFF_XR17C15X_PART_NUM    0x0A  /* 4 words (8 bytes) */
+#define CTI_EE_OFF_XR17V25X_PART_NUM    0x0E  /* 4 words (8 bytes) */
+#define CTI_EE_OFF_XR17C15X_SERIAL_NUM  0x0E  /* 1 word  (2 bytes) */
+#define CTI_EE_OFF_XR17V25X_SERIAL_NUM  0x12  /* 1 word  (2 bytes) */
+#define CTI_EE_OFF_XR17V35X_SERIAL_NUM  0x11  /* 2 word  (4 bytes) */
+#define CTI_EE_OFF_XR17V35X_BOARD_FLAGS 0x13  /* 1 word  (2 bytes) */
+#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word (per port) */
+
+#define CTI_FPGA_RS485_IO_REG		0x2008
+
+#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
+#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
+#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
+
+/*
+ * CTI Serial port line types. These match the values stored in the first
+ * nibble of the CTI EEPROM port_flags word.
+ */
+enum cti_port_type {
+	CTI_PORT_TYPE_NONE = 0,
+	CTI_PORT_TYPE_RS232,            //RS232 ONLY
+	CTI_PORT_TYPE_RS422_485,        //RS422/RS485 ONLY
+	CTI_PORT_TYPE_RS232_422_485_HW, //RS232/422/485 HW ONLY Switchable
+	CTI_PORT_TYPE_RS232_422_485_SW, //RS232/422/485 SW ONLY Switchable
+	CTI_PORT_TYPE_RS232_422_485_4B, //RS232/422/485 HW/SW (4bit ex. BCG004)
+	CTI_PORT_TYPE_RS232_422_485_2B, //RS232/422/485 HW/SW (2bit ex. BBG008)
+	CTI_PORT_TYPE_MAX,
+};
+
+#define CTI_PORT_TYPE_VALID(_port_type) \
+	(((_port_type) > CTI_PORT_TYPE_NONE) && \
+	((_port_type) < CTI_PORT_TYPE_MAX))
+
+#define CTI_PORT_TYPE_RS485(_port_type) \
+	(((_port_type) > CTI_PORT_TYPE_RS232) && \
+	((_port_type) < CTI_PORT_TYPE_MAX))
+
 struct exar8250;

 struct exar8250_platform {
@@ -202,6 +245,8 @@ struct exar8250_board {

 struct exar8250 {
 	unsigned int		nr;
+	unsigned int            osc_freq;
+	struct pci_dev		*pcidev;
 	struct exar8250_board	*board;
 	void __iomem		*virt;
 	int			line[];
@@ -557,6 +602,279 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	return 0;
 }

+/**
+ * cti_set_tristate() - Enable/Disable RS485 transciever tristate
+ * @priv: Device's private structure
+ * @port_num: Port number to set tristate on/off
+ * @enable: Enable tristate if true, disable if false
+ *
+ * Most RS485 capable cards have a power on tristate jumper/switch that ensures
+ * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
+ * not the master. When this jumper is installed the user must set the RS485
+ * mode to disable tristate prior to using the port.
+ *
+ * Some Exar UARTs have an auto-tristate feature while others require setting
+ * an MPIO to disable the tristate.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int cti_set_tristate(struct exar8250 *priv,
+			unsigned int port_num, bool enable)
+{
+	int ret = 0;
+
+	if (!priv || (port_num >= priv->nr))
+		return -EINVAL;
+
+	//Only Exar based cards use MPIO, return 0 otherwise
+	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
+		return 0;
+
+	pci_dbg(priv->pcidev, "%s tristate for port %u\n",
+		(enable ? "enabling" : "disabling"), port_num);
+
+	ret = exar_mpio_set(priv, port_num, !enable);
+	if (ret)
+		return ret;
+
+	//ensure MPIO is an output
+	ret = exar_mpio_config(priv, port_num, true);
+
+	return ret;
+}
+
+/**
+ * cti_set_plx_int_enable() - Enable/Disable PCI interrupts
+ * @priv: Device's private structure
+ * @enable: Enable interrupts if true, disable if false
+ *
+ * Some older CTI cards require MPIO_0 to be set low to enable the PCI
+ * interupts from the UART to the PLX PCI->PCIe bridge.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
+{
+	int ret = 0;
+
+	if (!priv)
+		return -EINVAL;
+
+	//Only Exar based cards use MPIO, return 0 otherwise
+	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
+		return 0;
+
+	pci_dbg(priv->pcidev, "%s plx fix\n",
+		(enable ? "enabling" : "disabling"));
+
+	//INT enabled when MPIO0 is LOW
+	ret = exar_mpio_set(priv, 0, !enable);
+	if (ret)
+		return ret;
+
+	//ensure MPIO is an output
+	ret = exar_mpio_config(priv, 0, true);
+
+	return ret;
+}
+
+/**
+ * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
+ * @priv: Device's private structure
+ *
+ * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
+ * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
+ *
+ * Return: frequency on success, negative error code on failure
+ */
+static int cti_read_osc_freq(struct exar8250 *priv, uint8_t eeprom_offset)
+{
+	int osc_freq;
+
+	if (!priv)
+		return -EINVAL;
+
+	osc_freq = (exar_ee_read(priv, eeprom_offset));
+	osc_freq |= (exar_ee_read(priv, (eeprom_offset + 1)) << 16);
+
+	//check if EEPROM word was blank
+	if ((osc_freq == 0xFFFF) || (osc_freq == 0x0000))
+		return -EIO;
+
+	pci_dbg(priv->pcidev, "osc_freq from EEPROM %d\n", osc_freq);
+
+	return osc_freq;
+}
+
+/**
+ * cti_get_port_type_xr17c15x_xr17v25x() - Get the port type of a xr17c15x
+ * or xr17v25x card
+ *
+ * @priv: Device's private structure
+ * @port_num: Port to get type of
+ *
+ * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
+							unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	if (!priv)
+		return CTI_PORT_TYPE_NONE;
+
+	switch (priv->pcidev->subsystem_device) {
+	//RS232 only cards
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
+		port_type = CTI_PORT_TYPE_RS232;
+		break;
+	//1x RS232, 1x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
+		port_type = (port_num == 0) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	//2x RS232, 2x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
+		port_type = (port_num < 2) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	//4x RS232, 4x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		port_type = (port_num < 4) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	//RS232/RS422/RS485 HW (jumper) selectable
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+		break;
+	//RS422/RS485 HW (jumper) selectable
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+		port_type = CTI_PORT_TYPE_RS422_485;
+		break;
+	//6x RS232, 2x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		port_type = (port_num < 6) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	//2x RS232, 6x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		port_type = (port_num < 2) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	default:
+		pci_err(priv->pcidev, "unknown/unsupported device\n");
+		port_type = CTI_PORT_TYPE_NONE;
+	}
+
+	return port_type;
+}
+
+/**
+ * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
+ * @priv: Device's private structure
+ * @port_num: Port to get type of
+ *
+ * FPGA based cards port types are based on PCI IDs
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
+						unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	if (!priv)
+		return CTI_PORT_TYPE_NONE;
+
+	switch (priv->pcidev->device) {
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+		break;
+	default:
+		pci_err(priv->pcidev, "unknown/unsupported device\n");
+		return CTI_PORT_TYPE_NONE;
+	}
+
+	return port_type;
+}
+
+/**
+ * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
+ * @priv: Device's private structure
+ * @port_num: port offset
+ *
+ * CTI XR17V35X based cards have the port types stored in the EEPROM.
+ * This function reads the port type for a single port.
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
+						unsigned int port_num)
+{
+	uint16_t port_flags;
+	uint8_t offset;
+	enum cti_port_type port_type;
+
+	if (!priv)
+		return CTI_PORT_TYPE_NONE;
+
+	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
+	port_flags = exar_ee_read(priv, offset);
+
+	port_type = (port_flags & 0x00FF);
+
+	if (!CTI_PORT_TYPE_VALID(port_type)) {
+		/*
+		 * If the port type is missing the card assume it is a
+		 * RS232/RS422/RS485 card to be safe.
+		 *
+		 * There is one known board (BEG013) that only has
+		 * 3 of 4 port types written to the EEPROM so this
+		 * acts as a work around.
+		 */
+		pci_warn(priv->pcidev,
+			"failed to get port %d type from EEPROM\n", port_num);
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+	}
+
+	return port_type;
+}
+
 static int
 pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		       struct uart_8250_port *port, int idx)
@@ -914,6 +1232,39 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }

+static unsigned int exar_get_nr_ports(struct exar8250_board *board,
+					struct pci_dev *pcidev)
+{
+	unsigned int nr_ports = 0;
+
+	if (!board || !pcidev)
+		return 0;
+
+	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {
+		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
+	} else if (board->num_ports > 0) {
+		//Check if board struct overrides number of ports
+		nr_ports = board->num_ports;
+	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
+		//Exar encodes # ports in last nibble of PCI Device ID ex. 0358
+		nr_ports = pcidev->device & 0x0f;
+	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
+		//Handle CTI FPGA cards
+		switch (pcidev->device) {
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
+			nr_ports = 12;
+			break;
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
+			nr_ports = 16;
+		default:
+			break;
+		}
+	}
+
+	return nr_ports;
+}
+
 static int
 exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 {
@@ -933,18 +1284,18 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)

 	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);

-	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
-		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
-	else if (board->num_ports)
-		nr_ports = board->num_ports;
-	else
-		nr_ports = pcidev->device & 0x0f;
+	nr_ports = exar_get_nr_ports(board, pcidev);
+	if (nr_ports == 0) {
+		pci_err(pcidev, "failed to get number of ports\n");
+		return -ENODEV;
+	}

 	priv = devm_kzalloc(&pcidev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;

 	priv->board = board;
+	priv->pcidev = pcidev;
 	priv->virt = pcim_iomap(pcidev, bar, 0);
 	if (!priv->virt)
 		return -ENOMEM;
--
2.43.2

Re: [PATCH v2 5/7] serial: exar: add some CTI helper functions

[Ilpo Järvinen]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> - Adds various helper functions for CTI boards.
> - Add osc_freq and pcidev to struct exar8250
> - Added a exar_get_nr_ports function
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 363 +++++++++++++++++++++++++++-
>  1 file changed, 357 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index b30f3855652a..6f3697e34722 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -137,6 +137,9 @@
>  #define UART_EXAR_REGB_EE_ADDR_SIZE     6
>  #define UART_EXAR_REGB_EE_DATA_SIZE     16
> 
> +#define UART_EXAR_XR17C15X_PORT_OFFSET  0x200
> +#define UART_EXAR_XR17V25X_PORT_OFFSET  0x200
> +#define UART_EXAR_XR17V35X_PORT_OFFSET  0x400
> 
>  /*
>   * IOT2040 MPIO wiring semantics:
> @@ -173,6 +176,46 @@
>  #define IOT2040_UARTS_ENABLE		0x03
>  #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
> 
> +/* CTI EEPROM offsets */
> +#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words (4 bytes) */
> +#define CTI_EE_OFF_XR17V25X_OSC_FREQ    0x08  /* 2 words (4 bytes) */
> +#define CTI_EE_OFF_XR17C15X_PART_NUM    0x0A  /* 4 words (8 bytes) */
> +#define CTI_EE_OFF_XR17V25X_PART_NUM    0x0E  /* 4 words (8 bytes) */
> +#define CTI_EE_OFF_XR17C15X_SERIAL_NUM  0x0E  /* 1 word  (2 bytes) */
> +#define CTI_EE_OFF_XR17V25X_SERIAL_NUM  0x12  /* 1 word  (2 bytes) */
> +#define CTI_EE_OFF_XR17V35X_SERIAL_NUM  0x11  /* 2 word  (4 bytes) */
> +#define CTI_EE_OFF_XR17V35X_BOARD_FLAGS 0x13  /* 1 word  (2 bytes) */

I'm not convinced but words and bytes is really needed.

> +#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word (per port) */

There's something wrong with alignment of more than one define above.

> +
> +#define CTI_FPGA_RS485_IO_REG		0x2008
> +
> +#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
> +#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
> +#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
> +
> +/*
> + * CTI Serial port line types. These match the values stored in the first
> + * nibble of the CTI EEPROM port_flags word.
> + */
> +enum cti_port_type {
> +	CTI_PORT_TYPE_NONE = 0,
> +	CTI_PORT_TYPE_RS232,            //RS232 ONLY
> +	CTI_PORT_TYPE_RS422_485,        //RS422/RS485 ONLY
> +	CTI_PORT_TYPE_RS232_422_485_HW, //RS232/422/485 HW ONLY Switchable
> +	CTI_PORT_TYPE_RS232_422_485_SW, //RS232/422/485 SW ONLY Switchable
> +	CTI_PORT_TYPE_RS232_422_485_4B, //RS232/422/485 HW/SW (4bit ex. BCG004)
> +	CTI_PORT_TYPE_RS232_422_485_2B, //RS232/422/485 HW/SW (2bit ex. BBG008)
> +	CTI_PORT_TYPE_MAX,
> +};
> +
> +#define CTI_PORT_TYPE_VALID(_port_type) \
> +	(((_port_type) > CTI_PORT_TYPE_NONE) && \
> +	((_port_type) < CTI_PORT_TYPE_MAX))
> +
> +#define CTI_PORT_TYPE_RS485(_port_type) \
> +	(((_port_type) > CTI_PORT_TYPE_RS232) && \
> +	((_port_type) < CTI_PORT_TYPE_MAX))
> +
>  struct exar8250;
> 
>  struct exar8250_platform {
> @@ -202,6 +245,8 @@ struct exar8250_board {
> 
>  struct exar8250 {
>  	unsigned int		nr;
> +	unsigned int            osc_freq;
> +	struct pci_dev		*pcidev;
>  	struct exar8250_board	*board;
>  	void __iomem		*virt;
>  	int			line[];
> @@ -557,6 +602,279 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>  	return 0;
>  }
> 
> +/**
> + * cti_set_tristate() - Enable/Disable RS485 transciever tristate
> + * @priv: Device's private structure
> + * @port_num: Port number to set tristate on/off
> + * @enable: Enable tristate if true, disable if false
> + *
> + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> + * not the master. When this jumper is installed the user must set the RS485
> + * mode to disable tristate prior to using the port.
> + *
> + * Some Exar UARTs have an auto-tristate feature while others require setting
> + * an MPIO to disable the tristate.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cti_set_tristate(struct exar8250 *priv,
> +			unsigned int port_num, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (!priv || (port_num >= priv->nr))
> +		return -EINVAL;
> +
> +	//Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;
> +
> +	pci_dbg(priv->pcidev, "%s tristate for port %u\n",
> +		(enable ? "enabling" : "disabling"), port_num);

dev_dbg()

Rephrasing the string slightly, you could consider using 
str_enable_disable() from linux/string_choices.h

> +
> +	ret = exar_mpio_set(priv, port_num, !enable);
> +	if (ret)
> +		return ret;
> +
> +	//ensure MPIO is an output
> +	ret = exar_mpio_config(priv, port_num, true);
> +
> +	return ret;
> +}
> +
> +/**
> + * cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> + * @priv: Device's private structure
> + * @enable: Enable interrupts if true, disable if false
> + *
> + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> + * interupts from the UART to the PLX PCI->PCIe bridge.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	//Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;
> +
> +	pci_dbg(priv->pcidev, "%s plx fix\n",
> +		(enable ? "enabling" : "disabling"));
> +
> +	//INT enabled when MPIO0 is LOW
> +	ret = exar_mpio_set(priv, 0, !enable);
> +	if (ret)
> +		return ret;
> +
> +	//ensure MPIO is an output
> +	ret = exar_mpio_config(priv, 0, true);
> +
> +	return ret;
> +}
> +
> +/**
> + * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
> + * @priv: Device's private structure

Missing second parameter.

> + *
> + * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
> + * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
> + *
> + * Return: frequency on success, negative error code on failure
> + */
> +static int cti_read_osc_freq(struct exar8250 *priv, uint8_t eeprom_offset)
> +{
> +	int osc_freq;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	osc_freq = (exar_ee_read(priv, eeprom_offset));

Unnecessary parenthesis.

> +	osc_freq |= (exar_ee_read(priv, (eeprom_offset + 1)) << 16);

Add the field #define with GENMASK() and use FIELD_PREP() here? Perhaps 
both lines should use FIELD_PREP() even if one of them has 0 shift.

> +
> +	//check if EEPROM word was blank
> +	if ((osc_freq == 0xFFFF) || (osc_freq == 0x0000))
> +		return -EIO;
> +
> +	pci_dbg(priv->pcidev, "osc_freq from EEPROM %d\n", osc_freq);
> +
> +	return osc_freq;
> +}
> +
> +/**
> + * cti_get_port_type_xr17c15x_xr17v25x() - Get the port type of a xr17c15x
> + * or xr17v25x card

I suppose this shorter version would be enough to provide the same amount  
information:

Get the port type of xr17c15x/xr17v25x

> + *

No empty line.

> + * @priv: Device's private structure
> + * @port_num: Port to get type of
> + *
> + * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
> +							unsigned int port_num)
> +{
> +	enum cti_port_type port_type;
> +
> +	if (!priv)
> +		return CTI_PORT_TYPE_NONE;

Can this happen?

> +	switch (priv->pcidev->subsystem_device) {
> +	//RS232 only cards
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
> +		port_type = CTI_PORT_TYPE_RS232;
> +		break;
> +	//1x RS232, 1x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
> +		port_type = (port_num == 0) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	//2x RS232, 2x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
> +		port_type = (port_num < 2) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	//4x RS232, 4x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> +		port_type = (port_num < 4) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	//RS232/RS422/RS485 HW (jumper) selectable
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +		break;
> +	//RS422/RS485 HW (jumper) selectable
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> +		port_type = CTI_PORT_TYPE_RS422_485;
> +		break;
> +	//6x RS232, 2x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> +		port_type = (port_num < 6) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	//2x RS232, 6x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> +		port_type = (port_num < 2) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	default:
> +		pci_err(priv->pcidev, "unknown/unsupported device\n");
> +		port_type = CTI_PORT_TYPE_NONE;
> +	}
> +
> +	return port_type;
> +}
> +
> +/**
> + * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
> + * @priv: Device's private structure
> + * @port_num: Port to get type of
> + *
> + * FPGA based cards port types are based on PCI IDs
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
> +						unsigned int port_num)
> +{
> +	enum cti_port_type port_type;
> +
> +	if (!priv)
> +		return CTI_PORT_TYPE_NONE;
> +
> +	switch (priv->pcidev->device) {
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +		break;
> +	default:
> +		pci_err(priv->pcidev, "unknown/unsupported device\n");

dev_err()

> +		return CTI_PORT_TYPE_NONE;
> +	}
> +
> +	return port_type;
> +}
> +
> +/**
> + * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
> + * @priv: Device's private structure
> + * @port_num: port offset
> + *
> + * CTI XR17V35X based cards have the port types stored in the EEPROM.
> + * This function reads the port type for a single port.
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> +						unsigned int port_num)
> +{
> +	uint16_t port_flags;
> +	uint8_t offset;
> +	enum cti_port_type port_type;
> +
> +	if (!priv)
> +		return CTI_PORT_TYPE_NONE;
> +
> +	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
> +	port_flags = exar_ee_read(priv, offset);
> +
> +	port_type = (port_flags & 0x00FF);

Add named define with GENMASK() and use FIELD_GET()

> +
> +	if (!CTI_PORT_TYPE_VALID(port_type)) {
> +		/*
> +		 * If the port type is missing the card assume it is a
> +		 * RS232/RS422/RS485 card to be safe.
> +		 *
> +		 * There is one known board (BEG013) that only has
> +		 * 3 of 4 port types written to the EEPROM so this
> +		 * acts as a work around.
> +		 */
> +		pci_warn(priv->pcidev,

dev_warn(). Please fix all pci_xx() logging, I won't flag them from this 
point onwards.

> +			"failed to get port %d type from EEPROM\n", port_num);
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +	}
> +
> +	return port_type;
> +}
> +
>  static int
>  pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>  		       struct uart_8250_port *port, int idx)
> @@ -914,6 +1232,39 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
> 
> +static unsigned int exar_get_nr_ports(struct exar8250_board *board,
> +					struct pci_dev *pcidev)
> +{
> +	unsigned int nr_ports = 0;
> +
> +	if (!board || !pcidev)
> +		return 0;
> +
> +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {
> +		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> +	} else if (board->num_ports > 0) {
> +		//Check if board struct overrides number of ports
> +		nr_ports = board->num_ports;
> +	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
> +		//Exar encodes # ports in last nibble of PCI Device ID ex. 0358
> +		nr_ports = pcidev->device & 0x0f;
> +	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
> +		//Handle CTI FPGA cards
> +		switch (pcidev->device) {
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> +			nr_ports = 12;
> +			break;
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> +			nr_ports = 16;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return nr_ports;
> +}
> +
>  static int
>  exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
>  {
> @@ -933,18 +1284,18 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> 
>  	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
> 
> -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> -		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> -	else if (board->num_ports)
> -		nr_ports = board->num_ports;
> -	else
> -		nr_ports = pcidev->device & 0x0f;
> +	nr_ports = exar_get_nr_ports(board, pcidev);
> +	if (nr_ports == 0) {

Can you please do this refactoring in a preparatory patch, and only add 
the new stuff in this patch into exar_get_nr_ports() patch.

-- 
 i.

> +		pci_err(pcidev, "failed to get number of ports\n");
> +		return -ENODEV;
> +	}
> 
>  	priv = devm_kzalloc(&pcidev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> 
>  	priv->board = board;
> +	priv->pcidev = pcidev;
>  	priv->virt = pcim_iomap(pcidev, bar, 0);
>  	if (!priv->virt)
>  		return -ENOMEM;
> --
> 2.43.2
> 
> 

Re: [PATCH v2 5/7] serial: exar: add some CTI helper functions

[Parker Newman]

On Fri, 12 Apr 2024 13:48:56 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > - Adds various helper functions for CTI boards.
> > - Add osc_freq and pcidev to struct exar8250
> > - Added a exar_get_nr_ports function
> > 
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 363 +++++++++++++++++++++++++++-
> >  1 file changed, 357 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index b30f3855652a..6f3697e34722 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -137,6 +137,9 @@
> >  #define UART_EXAR_REGB_EE_ADDR_SIZE     6
> >  #define UART_EXAR_REGB_EE_DATA_SIZE     16
> > 
> > +#define UART_EXAR_XR17C15X_PORT_OFFSET  0x200
> > +#define UART_EXAR_XR17V25X_PORT_OFFSET  0x200
> > +#define UART_EXAR_XR17V35X_PORT_OFFSET  0x400
> > 
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> > @@ -173,6 +176,46 @@
> >  #define IOT2040_UARTS_ENABLE		0x03
> >  #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
> > 
> > +/* CTI EEPROM offsets */
> > +#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words (4 bytes) */
> > +#define CTI_EE_OFF_XR17V25X_OSC_FREQ    0x08  /* 2 words (4 bytes) */
> > +#define CTI_EE_OFF_XR17C15X_PART_NUM    0x0A  /* 4 words (8 bytes) */
> > +#define CTI_EE_OFF_XR17V25X_PART_NUM    0x0E  /* 4 words (8 bytes) */
> > +#define CTI_EE_OFF_XR17C15X_SERIAL_NUM  0x0E  /* 1 word  (2 bytes) */
> > +#define CTI_EE_OFF_XR17V25X_SERIAL_NUM  0x12  /* 1 word  (2 bytes) */
> > +#define CTI_EE_OFF_XR17V35X_SERIAL_NUM  0x11  /* 2 word  (4 bytes) */
> > +#define CTI_EE_OFF_XR17V35X_BOARD_FLAGS 0x13  /* 1 word  (2 bytes) */  
> 
> I'm not convinced but words and bytes is really needed.
> 
> > +#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word (per port) */  
> 
> There's something wrong with alignment of more than one define above.
> 
> > +
> > +#define CTI_FPGA_RS485_IO_REG		0x2008
> > +
> > +#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
> > +#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
> > +#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
> > +
> > +/*
> > + * CTI Serial port line types. These match the values stored in the first
> > + * nibble of the CTI EEPROM port_flags word.
> > + */
> > +enum cti_port_type {
> > +	CTI_PORT_TYPE_NONE = 0,
> > +	CTI_PORT_TYPE_RS232,            //RS232 ONLY
> > +	CTI_PORT_TYPE_RS422_485,        //RS422/RS485 ONLY
> > +	CTI_PORT_TYPE_RS232_422_485_HW, //RS232/422/485 HW ONLY Switchable
> > +	CTI_PORT_TYPE_RS232_422_485_SW, //RS232/422/485 SW ONLY Switchable
> > +	CTI_PORT_TYPE_RS232_422_485_4B, //RS232/422/485 HW/SW (4bit ex. BCG004)
> > +	CTI_PORT_TYPE_RS232_422_485_2B, //RS232/422/485 HW/SW (2bit ex. BBG008)
> > +	CTI_PORT_TYPE_MAX,
> > +};
> > +
> > +#define CTI_PORT_TYPE_VALID(_port_type) \
> > +	(((_port_type) > CTI_PORT_TYPE_NONE) && \
> > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > +
> > +#define CTI_PORT_TYPE_RS485(_port_type) \
> > +	(((_port_type) > CTI_PORT_TYPE_RS232) && \
> > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > +
> >  struct exar8250;
> > 
> >  struct exar8250_platform {
> > @@ -202,6 +245,8 @@ struct exar8250_board {
> > 
> >  struct exar8250 {
> >  	unsigned int		nr;
> > +	unsigned int            osc_freq;
> > +	struct pci_dev		*pcidev;
> >  	struct exar8250_board	*board;
> >  	void __iomem		*virt;
> >  	int			line[];
> > @@ -557,6 +602,279 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> >  	return 0;
> >  }
> > 
> > +/**
> > + * cti_set_tristate() - Enable/Disable RS485 transciever tristate
> > + * @priv: Device's private structure
> > + * @port_num: Port number to set tristate on/off
> > + * @enable: Enable tristate if true, disable if false
> > + *
> > + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> > + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> > + * not the master. When this jumper is installed the user must set the RS485
> > + * mode to disable tristate prior to using the port.
> > + *
> > + * Some Exar UARTs have an auto-tristate feature while others require setting
> > + * an MPIO to disable the tristate.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int cti_set_tristate(struct exar8250 *priv,
> > +			unsigned int port_num, bool enable)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!priv || (port_num >= priv->nr))
> > +		return -EINVAL;
> > +
> > +	//Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > +		return 0;
> > +
> > +	pci_dbg(priv->pcidev, "%s tristate for port %u\n",
> > +		(enable ? "enabling" : "disabling"), port_num);  
> 
> dev_dbg()
> 
> Rephrasing the string slightly, you could consider using 
> str_enable_disable() from linux/string_choices.h
> 
> > +
> > +	ret = exar_mpio_set(priv, port_num, !enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	//ensure MPIO is an output
> > +	ret = exar_mpio_config(priv, port_num, true);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> > + * @priv: Device's private structure
> > + * @enable: Enable interrupts if true, disable if false
> > + *
> > + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> > + * interupts from the UART to the PLX PCI->PCIe bridge.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	//Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > +		return 0;
> > +
> > +	pci_dbg(priv->pcidev, "%s plx fix\n",
> > +		(enable ? "enabling" : "disabling"));
> > +
> > +	//INT enabled when MPIO0 is LOW
> > +	ret = exar_mpio_set(priv, 0, !enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	//ensure MPIO is an output
> > +	ret = exar_mpio_config(priv, 0, true);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
> > + * @priv: Device's private structure  
> 
> Missing second parameter.
> 
> > + *
> > + * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
> > + * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
> > + *
> > + * Return: frequency on success, negative error code on failure
> > + */
> > +static int cti_read_osc_freq(struct exar8250 *priv, uint8_t eeprom_offset)
> > +{
> > +	int osc_freq;
> > +
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	osc_freq = (exar_ee_read(priv, eeprom_offset));  
> 
> Unnecessary parenthesis.
> 
> > +	osc_freq |= (exar_ee_read(priv, (eeprom_offset + 1)) << 16);  
> 
> Add the field #define with GENMASK() and use FIELD_PREP() here? Perhaps 
> both lines should use FIELD_PREP() even if one of them has 0 shift.
> 
> > +
> > +	//check if EEPROM word was blank
> > +	if ((osc_freq == 0xFFFF) || (osc_freq == 0x0000))
> > +		return -EIO;
> > +
> > +	pci_dbg(priv->pcidev, "osc_freq from EEPROM %d\n", osc_freq);
> > +
> > +	return osc_freq;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_xr17c15x_xr17v25x() - Get the port type of a xr17c15x
> > + * or xr17v25x card  
> 
> I suppose this shorter version would be enough to provide the same amount  
> information:
> 
> Get the port type of xr17c15x/xr17v25x
> 
> > + *  
> 
> No empty line.
> 
> > + * @priv: Device's private structure
> > + * @port_num: Port to get type of
> > + *
> > + * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
> > +							unsigned int port_num)
> > +{
> > +	enum cti_port_type port_type;
> > +
> > +	if (!priv)
> > +		return CTI_PORT_TYPE_NONE;  
> 
> Can this happen?
> 
> > +	switch (priv->pcidev->subsystem_device) {
> > +	//RS232 only cards
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
> > +		port_type = CTI_PORT_TYPE_RS232;
> > +		break;
> > +	//1x RS232, 1x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
> > +		port_type = (port_num == 0) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	//2x RS232, 2x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
> > +		port_type = (port_num < 2) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	//4x RS232, 4x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > +		port_type = (port_num < 4) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	//RS232/RS422/RS485 HW (jumper) selectable
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +		break;
> > +	//RS422/RS485 HW (jumper) selectable
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > +		port_type = CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	//6x RS232, 2x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > +		port_type = (port_num < 6) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	//2x RS232, 6x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > +		port_type = (port_num < 2) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	default:
> > +		pci_err(priv->pcidev, "unknown/unsupported device\n");
> > +		port_type = CTI_PORT_TYPE_NONE;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
> > + * @priv: Device's private structure
> > + * @port_num: Port to get type of
> > + *
> > + * FPGA based cards port types are based on PCI IDs
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
> > +						unsigned int port_num)
> > +{
> > +	enum cti_port_type port_type;
> > +
> > +	if (!priv)
> > +		return CTI_PORT_TYPE_NONE;
> > +
> > +	switch (priv->pcidev->device) {
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +		break;
> > +	default:
> > +		pci_err(priv->pcidev, "unknown/unsupported device\n");  
> 
> dev_err()
> 
> > +		return CTI_PORT_TYPE_NONE;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
> > + * @priv: Device's private structure
> > + * @port_num: port offset
> > + *
> > + * CTI XR17V35X based cards have the port types stored in the EEPROM.
> > + * This function reads the port type for a single port.
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> > +						unsigned int port_num)
> > +{
> > +	uint16_t port_flags;
> > +	uint8_t offset;
> > +	enum cti_port_type port_type;
> > +
> > +	if (!priv)
> > +		return CTI_PORT_TYPE_NONE;
> > +
> > +	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
> > +	port_flags = exar_ee_read(priv, offset);
> > +
> > +	port_type = (port_flags & 0x00FF);  
> 
> Add named define with GENMASK() and use FIELD_GET()
> 
> > +
> > +	if (!CTI_PORT_TYPE_VALID(port_type)) {
> > +		/*
> > +		 * If the port type is missing the card assume it is a
> > +		 * RS232/RS422/RS485 card to be safe.
> > +		 *
> > +		 * There is one known board (BEG013) that only has
> > +		 * 3 of 4 port types written to the EEPROM so this
> > +		 * acts as a work around.
> > +		 */
> > +		pci_warn(priv->pcidev,  
> 
> dev_warn(). Please fix all pci_xx() logging, I won't flag them from this 
> point onwards.
> 
> > +			"failed to get port %d type from EEPROM\n", port_num);
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> >  static int
> >  pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> >  		       struct uart_8250_port *port, int idx)
> > @@ -914,6 +1232,39 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static unsigned int exar_get_nr_ports(struct exar8250_board *board,
> > +					struct pci_dev *pcidev)
> > +{
> > +	unsigned int nr_ports = 0;
> > +
> > +	if (!board || !pcidev)
> > +		return 0;
> > +
> > +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {
> > +		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> > +	} else if (board->num_ports > 0) {
> > +		//Check if board struct overrides number of ports
> > +		nr_ports = board->num_ports;
> > +	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
> > +		//Exar encodes # ports in last nibble of PCI Device ID ex. 0358
> > +		nr_ports = pcidev->device & 0x0f;
> > +	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
> > +		//Handle CTI FPGA cards
> > +		switch (pcidev->device) {
> > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> > +			nr_ports = 12;
> > +			break;
> > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> > +			nr_ports = 16;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return nr_ports;
> > +}
> > +
> >  static int
> >  exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> >  {
> > @@ -933,18 +1284,18 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> > 
> >  	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
> > 
> > -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> > -		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> > -	else if (board->num_ports)
> > -		nr_ports = board->num_ports;
> > -	else
> > -		nr_ports = pcidev->device & 0x0f;
> > +	nr_ports = exar_get_nr_ports(board, pcidev);
> > +	if (nr_ports == 0) {  
> 
> Can you please do this refactoring in a preparatory patch, and only add 
> the new stuff in this patch into exar_get_nr_ports() patch.
> 

I will fix above. 

Just to clarify By "do this refactoring in a preparatory patch" do you mean: 
1. Move existing code to new function in "preparatory patch"
2. Add new code to new function in another patch
 
Thanks,
-Parker

Re: [PATCH v2 5/7] serial: exar: add some CTI helper functions

[Ilpo Järvinen]

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

On Fri, 12 Apr 2024, Parker Newman wrote:

> On Fri, 12 Apr 2024 13:48:56 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 11 Apr 2024, parker@finest.io wrote:
> > 
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > - Adds various helper functions for CTI boards.
> > > - Add osc_freq and pcidev to struct exar8250
> > > - Added a exar_get_nr_ports function
> > > 
> > > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_exar.c | 363 +++++++++++++++++++++++++++-
> > >  1 file changed, 357 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > index b30f3855652a..6f3697e34722 100644
> > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > @@ -137,6 +137,9 @@
> > >  #define UART_EXAR_REGB_EE_ADDR_SIZE     6
> > >  #define UART_EXAR_REGB_EE_DATA_SIZE     16
> > > 
> > > +#define UART_EXAR_XR17C15X_PORT_OFFSET  0x200
> > > +#define UART_EXAR_XR17V25X_PORT_OFFSET  0x200
> > > +#define UART_EXAR_XR17V35X_PORT_OFFSET  0x400
> > > 
> > >  /*
> > >   * IOT2040 MPIO wiring semantics:
> > > @@ -173,6 +176,46 @@
> > >  #define IOT2040_UARTS_ENABLE		0x03
> > >  #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
> > > 
> > > +/* CTI EEPROM offsets */
> > > +#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words (4 bytes) */
> > > +#define CTI_EE_OFF_XR17V25X_OSC_FREQ    0x08  /* 2 words (4 bytes) */
> > > +#define CTI_EE_OFF_XR17C15X_PART_NUM    0x0A  /* 4 words (8 bytes) */
> > > +#define CTI_EE_OFF_XR17V25X_PART_NUM    0x0E  /* 4 words (8 bytes) */
> > > +#define CTI_EE_OFF_XR17C15X_SERIAL_NUM  0x0E  /* 1 word  (2 bytes) */
> > > +#define CTI_EE_OFF_XR17V25X_SERIAL_NUM  0x12  /* 1 word  (2 bytes) */
> > > +#define CTI_EE_OFF_XR17V35X_SERIAL_NUM  0x11  /* 2 word  (4 bytes) */
> > > +#define CTI_EE_OFF_XR17V35X_BOARD_FLAGS 0x13  /* 1 word  (2 bytes) */  
> > 
> > I'm not convinced but words and bytes is really needed.
> > 
> > > +#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word (per port) */  
> > 
> > There's something wrong with alignment of more than one define above.
> > 
> > > +
> > > +#define CTI_FPGA_RS485_IO_REG		0x2008
> > > +
> > > +#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
> > > +#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
> > > +#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
> > > +
> > > +/*
> > > + * CTI Serial port line types. These match the values stored in the first
> > > + * nibble of the CTI EEPROM port_flags word.
> > > + */
> > > +enum cti_port_type {
> > > +	CTI_PORT_TYPE_NONE = 0,
> > > +	CTI_PORT_TYPE_RS232,            //RS232 ONLY
> > > +	CTI_PORT_TYPE_RS422_485,        //RS422/RS485 ONLY
> > > +	CTI_PORT_TYPE_RS232_422_485_HW, //RS232/422/485 HW ONLY Switchable
> > > +	CTI_PORT_TYPE_RS232_422_485_SW, //RS232/422/485 SW ONLY Switchable
> > > +	CTI_PORT_TYPE_RS232_422_485_4B, //RS232/422/485 HW/SW (4bit ex. BCG004)
> > > +	CTI_PORT_TYPE_RS232_422_485_2B, //RS232/422/485 HW/SW (2bit ex. BBG008)
> > > +	CTI_PORT_TYPE_MAX,
> > > +};
> > > +
> > > +#define CTI_PORT_TYPE_VALID(_port_type) \
> > > +	(((_port_type) > CTI_PORT_TYPE_NONE) && \
> > > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > > +
> > > +#define CTI_PORT_TYPE_RS485(_port_type) \
> > > +	(((_port_type) > CTI_PORT_TYPE_RS232) && \
> > > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > > +
> > >  struct exar8250;
> > > 
> > >  struct exar8250_platform {
> > > @@ -202,6 +245,8 @@ struct exar8250_board {
> > > 
> > >  struct exar8250 {
> > >  	unsigned int		nr;
> > > +	unsigned int            osc_freq;
> > > +	struct pci_dev		*pcidev;
> > >  	struct exar8250_board	*board;
> > >  	void __iomem		*virt;
> > >  	int			line[];
> > > @@ -557,6 +602,279 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > >  	return 0;
> > >  }
> > > 
> > > +/**
> > > + * cti_set_tristate() - Enable/Disable RS485 transciever tristate
> > > + * @priv: Device's private structure
> > > + * @port_num: Port number to set tristate on/off
> > > + * @enable: Enable tristate if true, disable if false
> > > + *
> > > + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> > > + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> > > + * not the master. When this jumper is installed the user must set the RS485
> > > + * mode to disable tristate prior to using the port.
> > > + *
> > > + * Some Exar UARTs have an auto-tristate feature while others require setting
> > > + * an MPIO to disable the tristate.
> > > + *
> > > + * Return: 0 on success, negative error code on failure
> > > + */
> > > +static int cti_set_tristate(struct exar8250 *priv,
> > > +			unsigned int port_num, bool enable)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!priv || (port_num >= priv->nr))
> > > +		return -EINVAL;
> > > +
> > > +	//Only Exar based cards use MPIO, return 0 otherwise
> > > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > > +		return 0;
> > > +
> > > +	pci_dbg(priv->pcidev, "%s tristate for port %u\n",
> > > +		(enable ? "enabling" : "disabling"), port_num);  
> > 
> > dev_dbg()
> > 
> > Rephrasing the string slightly, you could consider using 
> > str_enable_disable() from linux/string_choices.h
> > 
> > > +
> > > +	ret = exar_mpio_set(priv, port_num, !enable);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	//ensure MPIO is an output
> > > +	ret = exar_mpio_config(priv, port_num, true);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> > > + * @priv: Device's private structure
> > > + * @enable: Enable interrupts if true, disable if false
> > > + *
> > > + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> > > + * interupts from the UART to the PLX PCI->PCIe bridge.
> > > + *
> > > + * Return: 0 on success, negative error code on failure
> > > + */
> > > +static int cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!priv)
> > > +		return -EINVAL;
> > > +
> > > +	//Only Exar based cards use MPIO, return 0 otherwise
> > > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > > +		return 0;
> > > +
> > > +	pci_dbg(priv->pcidev, "%s plx fix\n",
> > > +		(enable ? "enabling" : "disabling"));
> > > +
> > > +	//INT enabled when MPIO0 is LOW
> > > +	ret = exar_mpio_set(priv, 0, !enable);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	//ensure MPIO is an output
> > > +	ret = exar_mpio_config(priv, 0, true);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
> > > + * @priv: Device's private structure  
> > 
> > Missing second parameter.
> > 
> > > + *
> > > + * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
> > > + * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
> > > + *
> > > + * Return: frequency on success, negative error code on failure
> > > + */
> > > +static int cti_read_osc_freq(struct exar8250 *priv, uint8_t eeprom_offset)
> > > +{
> > > +	int osc_freq;
> > > +
> > > +	if (!priv)
> > > +		return -EINVAL;
> > > +
> > > +	osc_freq = (exar_ee_read(priv, eeprom_offset));  
> > 
> > Unnecessary parenthesis.
> > 
> > > +	osc_freq |= (exar_ee_read(priv, (eeprom_offset + 1)) << 16);  
> > 
> > Add the field #define with GENMASK() and use FIELD_PREP() here? Perhaps 
> > both lines should use FIELD_PREP() even if one of them has 0 shift.
> > 
> > > +
> > > +	//check if EEPROM word was blank
> > > +	if ((osc_freq == 0xFFFF) || (osc_freq == 0x0000))
> > > +		return -EIO;
> > > +
> > > +	pci_dbg(priv->pcidev, "osc_freq from EEPROM %d\n", osc_freq);
> > > +
> > > +	return osc_freq;
> > > +}
> > > +
> > > +/**
> > > + * cti_get_port_type_xr17c15x_xr17v25x() - Get the port type of a xr17c15x
> > > + * or xr17v25x card  
> > 
> > I suppose this shorter version would be enough to provide the same amount  
> > information:
> > 
> > Get the port type of xr17c15x/xr17v25x
> > 
> > > + *  
> > 
> > No empty line.
> > 
> > > + * @priv: Device's private structure
> > > + * @port_num: Port to get type of
> > > + *
> > > + * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs
> > > + *
> > > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > > + */
> > > +static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
> > > +							unsigned int port_num)
> > > +{
> > > +	enum cti_port_type port_type;
> > > +
> > > +	if (!priv)
> > > +		return CTI_PORT_TYPE_NONE;  
> > 
> > Can this happen?
> > 
> > > +	switch (priv->pcidev->subsystem_device) {
> > > +	//RS232 only cards
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
> > > +		port_type = CTI_PORT_TYPE_RS232;
> > > +		break;
> > > +	//1x RS232, 1x RS422/RS485
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
> > > +		port_type = (port_num == 0) ?
> > > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	//2x RS232, 2x RS422/RS485
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
> > > +		port_type = (port_num < 2) ?
> > > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	//4x RS232, 4x RS422/RS485
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > > +		port_type = (port_num < 4) ?
> > > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	//RS232/RS422/RS485 HW (jumper) selectable
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > > +		break;
> > > +	//RS422/RS485 HW (jumper) selectable
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > > +		port_type = CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	//6x RS232, 2x RS422/RS485
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > > +		port_type = (port_num < 6) ?
> > > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	//2x RS232, 6x RS422/RS485
> > > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > > +		port_type = (port_num < 2) ?
> > > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > > +		break;
> > > +	default:
> > > +		pci_err(priv->pcidev, "unknown/unsupported device\n");
> > > +		port_type = CTI_PORT_TYPE_NONE;
> > > +	}
> > > +
> > > +	return port_type;
> > > +}
> > > +
> > > +/**
> > > + * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
> > > + * @priv: Device's private structure
> > > + * @port_num: Port to get type of
> > > + *
> > > + * FPGA based cards port types are based on PCI IDs
> > > + *
> > > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > > + */
> > > +static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
> > > +						unsigned int port_num)
> > > +{
> > > +	enum cti_port_type port_type;
> > > +
> > > +	if (!priv)
> > > +		return CTI_PORT_TYPE_NONE;
> > > +
> > > +	switch (priv->pcidev->device) {
> > > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> > > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> > > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> > > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > > +		break;
> > > +	default:
> > > +		pci_err(priv->pcidev, "unknown/unsupported device\n");  
> > 
> > dev_err()
> > 
> > > +		return CTI_PORT_TYPE_NONE;
> > > +	}
> > > +
> > > +	return port_type;
> > > +}
> > > +
> > > +/**
> > > + * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
> > > + * @priv: Device's private structure
> > > + * @port_num: port offset
> > > + *
> > > + * CTI XR17V35X based cards have the port types stored in the EEPROM.
> > > + * This function reads the port type for a single port.
> > > + *
> > > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > > + */
> > > +static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> > > +						unsigned int port_num)
> > > +{
> > > +	uint16_t port_flags;
> > > +	uint8_t offset;
> > > +	enum cti_port_type port_type;
> > > +
> > > +	if (!priv)
> > > +		return CTI_PORT_TYPE_NONE;
> > > +
> > > +	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
> > > +	port_flags = exar_ee_read(priv, offset);
> > > +
> > > +	port_type = (port_flags & 0x00FF);  
> > 
> > Add named define with GENMASK() and use FIELD_GET()
> > 
> > > +
> > > +	if (!CTI_PORT_TYPE_VALID(port_type)) {
> > > +		/*
> > > +		 * If the port type is missing the card assume it is a
> > > +		 * RS232/RS422/RS485 card to be safe.
> > > +		 *
> > > +		 * There is one known board (BEG013) that only has
> > > +		 * 3 of 4 port types written to the EEPROM so this
> > > +		 * acts as a work around.
> > > +		 */
> > > +		pci_warn(priv->pcidev,  
> > 
> > dev_warn(). Please fix all pci_xx() logging, I won't flag them from this 
> > point onwards.
> > 
> > > +			"failed to get port %d type from EEPROM\n", port_num);
> > > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > > +	}
> > > +
> > > +	return port_type;
> > > +}
> > > +
> > >  static int
> > >  pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > >  		       struct uart_8250_port *port, int idx)
> > > @@ -914,6 +1232,39 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
> > >  	return IRQ_HANDLED;
> > >  }
> > > 
> > > +static unsigned int exar_get_nr_ports(struct exar8250_board *board,
> > > +					struct pci_dev *pcidev)
> > > +{
> > > +	unsigned int nr_ports = 0;
> > > +
> > > +	if (!board || !pcidev)
> > > +		return 0;
> > > +
> > > +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {
> > > +		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> > > +	} else if (board->num_ports > 0) {
> > > +		//Check if board struct overrides number of ports
> > > +		nr_ports = board->num_ports;
> > > +	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
> > > +		//Exar encodes # ports in last nibble of PCI Device ID ex. 0358
> > > +		nr_ports = pcidev->device & 0x0f;
> > > +	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
> > > +		//Handle CTI FPGA cards
> > > +		switch (pcidev->device) {
> > > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> > > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> > > +			nr_ports = 12;
> > > +			break;
> > > +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> > > +			nr_ports = 16;
> > > +		default:
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return nr_ports;
> > > +}
> > > +
> > >  static int
> > >  exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> > >  {
> > > @@ -933,18 +1284,18 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> > > 
> > >  	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
> > > 
> > > -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> > > -		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> > > -	else if (board->num_ports)
> > > -		nr_ports = board->num_ports;
> > > -	else
> > > -		nr_ports = pcidev->device & 0x0f;
> > > +	nr_ports = exar_get_nr_ports(board, pcidev);
> > > +	if (nr_ports == 0) {  
> > 
> > Can you please do this refactoring in a preparatory patch, and only add 
> > the new stuff in this patch into exar_get_nr_ports() patch.
> > 
> 
> I will fix above. 
> 
> Just to clarify By "do this refactoring in a preparatory patch" do you mean: 
> 1. Move existing code to new function in "preparatory patch"
> 2. Add new code to new function in another patch

Yes.

In general, when a patch gets large, moving such trivial changes out 
of it will help review tremendously (and also if somebody has to look into 
the commit history 5-15 years from now).

-- 
 i.

[PATCH v2 6/7] serial: exar: add CTI board and port setup functions

[parker]

From: Parker Newman <pnewman@connecttech.com>

- Removed old port setup function and replaced with UART specific ones
- Added board setup functions for CTI boards
- Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE
- Moved "generic rs485" support up in the file

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 454 ++++++++++++++++++++++++----
 1 file changed, 401 insertions(+), 53 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 6f3697e34722..d8425113a9f1 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -440,6 +440,31 @@ static int exar_mpio_set(struct exar8250 *priv,
 	return 0;
 }

+static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
+				struct serial_rs485 *rs485)
+{
+	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
+	u8 __iomem *p = port->membase;
+	u8 value;
+
+	value = readb(p + UART_EXAR_FCTR);
+	if (is_rs485)
+		value |= UART_FCTR_EXAR_485;
+	else
+		value &= ~UART_FCTR_EXAR_485;
+
+	writeb(value, p + UART_EXAR_FCTR);
+
+	if (is_rs485)
+		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
+
+	return 0;
+}
+
+static const struct serial_rs485 generic_rs485_supported = {
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
+};
+
 static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
 	/*
@@ -875,15 +900,332 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
 	return port_type;
 }

-static int
-pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
-		       struct uart_8250_port *port, int idx)
+static int cti_rs485_config_mpio_tristate(struct uart_port *port,
+					struct ktermios *termios,
+					struct serial_rs485 *rs485)
 {
-	unsigned int offset = idx * 0x200;
-	unsigned int baud = 1843200;
+	struct exar8250 *priv;
+	int ret;

-	port->port.uartclk = baud * 16;
-	return default_setup(priv, pcidev, idx, offset, port);
+	priv = (struct exar8250 *)port->private_data;
+	if (!priv)
+		return -EINVAL;
+
+	ret = generic_rs485_config(port, termios, rs485);
+	if (ret)
+		return ret;
+
+	//disable power-on tri-state via MPIO
+	return cti_set_tristate(priv, port->port_id, false);
+}
+
+static int cti_port_setup_common(struct exar8250 *priv,
+				int idx, unsigned int offset,
+				struct uart_8250_port *port)
+{
+	int ret;
+
+	if (!priv || !port)
+		return -EINVAL;
+
+	if (priv->osc_freq == 0)
+		return -EINVAL;
+
+	port->port.port_id = idx;
+	port->port.uartclk = priv->osc_freq;
+
+	ret = serial8250_pci_setup_port(priv->pcidev, port, 0, offset, 0);
+	if (ret) {
+		pci_err(priv->pcidev,
+			"failed to setup pci for port %d err: %d\n", idx, ret);
+		return ret;
+	}
+
+	port->port.private_data = (void *)priv;
+	port->port.pm = exar_pm;
+	port->port.shutdown = exar_shutdown;
+
+	return 0;
+}
+
+static int cti_port_setup_fpga(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+
+	port_type = cti_get_port_type_fpga(priv, idx);
+
+	//FPGA shares port offests with XR17C15X
+	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		port->port.rs485_config = generic_rs485_config;
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	return cti_port_setup_common(priv, idx, offset, port);
+}
+
+static int cti_port_setup_xr17v35x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+	int ret;
+
+	port_type = cti_get_port_type_xr17v35x(priv, idx);
+
+	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
+	port->port.type = PORT_XR17V35X;
+
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	switch (port_type) {
+	case CTI_PORT_TYPE_RS422_485:
+	case CTI_PORT_TYPE_RS232_422_485_HW:
+		port->port.rs485_config = cti_rs485_config_mpio_tristate;
+		port->port.rs485_supported = generic_rs485_supported;
+		break;
+	case CTI_PORT_TYPE_RS232_422_485_SW:
+	case CTI_PORT_TYPE_RS232_422_485_4B:
+	case CTI_PORT_TYPE_RS232_422_485_2B:
+		port->port.rs485_config = generic_rs485_config;
+		port->port.rs485_supported = generic_rs485_supported;
+		break;
+	default:
+		break;
+	}
+
+	ret = cti_port_setup_common(priv, idx, offset, port);
+	if (ret)
+		return ret;
+
+	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
+	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
+	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);
+
+	return 0;
+}
+
+static int cti_port_setup_xr17v25x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+	int ret;
+
+	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
+
+	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	//xr17v25x supports fractional baudrates
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		switch (priv->pcidev->subsystem_device) {
+		//These cards support power on 485 tri-state via MPIO
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+			port->port.rs485_config = cti_rs485_config_mpio_tristate;
+			break;
+		//Otherwise auto or no power on 485 tri-state support
+		default:
+			port->port.rs485_config = generic_rs485_config;
+			break;
+		}
+
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	ret = cti_port_setup_common(priv, idx, offset, port);
+	if (ret)
+		return ret;
+
+	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
+	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
+	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);
+
+	return 0;
+}
+
+static int cti_port_setup_xr17c15x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+
+	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
+
+	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		switch (priv->pcidev->subsystem_device) {
+		//These cards support power on 485 tri-state via MPIO
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+			port->port.rs485_config = cti_rs485_config_mpio_tristate;
+			break;
+		//Otherwise auto or no power on 485 tri-state support
+		default:
+			port->port.rs485_config = generic_rs485_config;
+			break;
+		}
+
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	return cti_port_setup_common(priv, idx, offset, port);
+}
+
+static int cti_board_setup_xr17v35x(struct exar8250 *priv)
+{
+	if (!priv)
+		return -EINVAL;
+
+	//XR17V35X use the PCIe clock rather than crystal
+	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
+
+	return 0;
+}
+
+static int cti_board_setup_xr17v25x(struct exar8250 *priv)
+{
+	int osc_freq;
+
+	if (!priv)
+		return -EINVAL;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
+	if (osc_freq < 0) {
+		pci_warn(priv->pcidev,
+			"failed to read osc freq from EEPROM, using default\n");
+		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
+	}
+
+	priv->osc_freq = osc_freq;
+
+	/* enable interupts on cards that need the "PLX fix" */
+	switch (priv->pcidev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
+		cti_set_plx_int_enable(priv, true);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cti_board_setup_xr17c15x(struct exar8250 *priv)
+{
+	int osc_freq;
+
+	if (!priv)
+		return -EINVAL;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
+	if (osc_freq <= 0) {
+		pci_warn(priv->pcidev,
+			"failed to read osc freq from EEPROM, using default\n");
+		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
+	}
+
+	priv->osc_freq = osc_freq;
+
+	/* enable interrupts on cards that need the "PLX fix" */
+	switch (priv->pcidev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
+		cti_set_plx_int_enable(priv, true);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cti_board_setup_fpga(struct exar8250 *priv)
+{
+	int ret;
+	uint16_t cfg_val;
+
+	if (!priv)
+		return -EINVAL;
+
+	//FPGA OSC is fixed to the 33MHz PCI clock
+	priv->osc_freq = CTI_DEFAULT_FPGA_OSC_FREQ;
+
+	//Enable external interrupts in special cfg space register
+	ret = pci_read_config_word(priv->pcidev, 0x48, &cfg_val);
+	if (ret)
+		return ret;
+
+	cfg_val |= BIT(15);
+
+	ret = pci_write_config_word(priv->pcidev, 0x48, cfg_val);
+	if (ret)
+		return ret;
+
+	//RS485 gate needs to be enabled; otherwise RTS/CTS will not work
+	exar_write_reg(priv, CTI_FPGA_RS485_IO_REG, 0x01);
+
+	return 0;
 }

 static int
@@ -985,27 +1327,6 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
 	port->port.private_data = NULL;
 }

-static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
-				struct serial_rs485 *rs485)
-{
-	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
-	u8 __iomem *p = port->membase;
-	u8 value;
-
-	value = readb(p + UART_EXAR_FCTR);
-	if (is_rs485)
-		value |= UART_FCTR_EXAR_485;
-	else
-		value &= ~UART_FCTR_EXAR_485;
-
-	writeb(value, p + UART_EXAR_FCTR);
-
-	if (is_rs485)
-		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
-
-	return 0;
-}
-
 static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
 				  struct serial_rs485 *rs485)
 {
@@ -1044,10 +1365,6 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
 	return 0;
 }

-static const struct serial_rs485 generic_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
-};
-
 static const struct exar8250_platform exar8250_default_platform = {
 	.register_gpio = xr17v35x_register_gpio,
 	.unregister_gpio = xr17v35x_unregister_gpio,
@@ -1408,8 +1725,24 @@ static const struct exar8250_board pbn_fastcom335_8 = {
 	.setup		= pci_fastcom335_setup,
 };

-static const struct exar8250_board pbn_connect = {
-	.setup		= pci_connect_tech_setup,
+static const struct exar8250_board pbn_cti_xr17c15x = {
+	.board_setup	= cti_board_setup_xr17c15x,
+	.setup		= cti_port_setup_xr17c15x,
+};
+
+static const struct exar8250_board pbn_cti_xr17v25x = {
+	.board_setup	= cti_board_setup_xr17v25x,
+	.setup		= cti_port_setup_xr17v25x,
+};
+
+static const struct exar8250_board pbn_cti_xr17v35x = {
+	.board_setup	= cti_board_setup_xr17v35x,
+	.setup		= cti_port_setup_xr17v35x,
+};
+
+static const struct exar8250_board pbn_cti_fpga = {
+	.board_setup	= cti_board_setup_fpga,
+	.setup		= cti_port_setup_fpga,
 };

 static const struct exar8250_board pbn_exar_ibm_saturn = {
@@ -1456,15 +1789,27 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };

-#define CONNECT_DEVICE(devid, sdevid, bd) {				\
-	PCI_DEVICE_SUB(							\
-		PCI_VENDOR_ID_EXAR,					\
-		PCI_DEVICE_ID_EXAR_##devid,				\
-		PCI_SUBVENDOR_ID_CONNECT_TECH,				\
-		PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_##sdevid), 0, 0,	\
-		(kernel_ulong_t)&bd					\
+//For Connect Tech cards with Exar vendor/device PCI IDs
+#define CTI_EXAR_DEVICE(devid, bd) {                    \
+	PCI_DEVICE_SUB(                                 \
+		PCI_VENDOR_ID_EXAR,                     \
+		PCI_DEVICE_ID_EXAR_##devid,             \
+		PCI_SUBVENDOR_ID_CONNECT_TECH,          \
+		PCI_ANY_ID), 0, 0,                      \
+		(kernel_ulong_t)&bd                     \
+	}
+
+//For Connect Tech cards with Connect Tech vendor/device PCI IDs (FPGA based)
+#define CTI_PCI_DEVICE(devid, bd) {                     \
+	PCI_DEVICE_SUB(                                 \
+		PCI_VENDOR_ID_CONNECT_TECH,             \
+		PCI_DEVICE_ID_CONNECT_TECH_PCI_##devid, \
+		PCI_ANY_ID,                             \
+		PCI_ANY_ID), 0, 0,                      \
+		(kernel_ulong_t)&bd                     \
 	}

+
 #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }

 #define IBM_DEVICE(devid, sdevid, bd) {			\
@@ -1494,18 +1839,21 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(ACCESSIO, COM_4SM, pbn_exar_XR17C15x),
 	EXAR_DEVICE(ACCESSIO, COM_8SM, pbn_exar_XR17C15x),

-	CONNECT_DEVICE(XR17C152, UART_2_232, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4_232, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8_232, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_1_1, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_2_2, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_4_4, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_2, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_2_485, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
+	CTI_EXAR_DEVICE(XR17C152,       pbn_cti_xr17c15x),
+	CTI_EXAR_DEVICE(XR17C154,       pbn_cti_xr17c15x),
+	CTI_EXAR_DEVICE(XR17C158,       pbn_cti_xr17c15x),
+
+	CTI_EXAR_DEVICE(XR17V252,       pbn_cti_xr17v25x),
+	CTI_EXAR_DEVICE(XR17V254,       pbn_cti_xr17v25x),
+	CTI_EXAR_DEVICE(XR17V258,       pbn_cti_xr17v25x),
+
+	CTI_EXAR_DEVICE(XR17V352,       pbn_cti_xr17v35x),
+	CTI_EXAR_DEVICE(XR17V354,       pbn_cti_xr17v35x),
+	CTI_EXAR_DEVICE(XR17V358,       pbn_cti_xr17v35x),
+
+	CTI_PCI_DEVICE(XR79X_12_XIG00X, pbn_cti_fpga),
+	CTI_PCI_DEVICE(XR79X_12_XIG01X, pbn_cti_fpga),
+	CTI_PCI_DEVICE(XR79X_16,        pbn_cti_fpga),

 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),

--
2.43.2

Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup functions

[Ilpo Järvinen]

On Thu, 11 Apr 2024, parker@finest.io wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> - Removed old port setup function and replaced with UART specific ones
> - Added board setup functions for CTI boards
> - Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE

In general, you should try to do refactoring in a preparatory patch (one 
refactoring thing at a time) and add new stuff in another patch in 
the series. I didn't go to figure out how much it applies to those three 
items because you likely know the answer immediately.

> - Moved "generic rs485" support up in the file

Please do this in a separate patch.


Another general level problem with your series is that it adds functions 
x, y, etc. without users, whereas the expected way of doing things would 
be to add the functions in the change they are getting used so it's easier 
to follow what's going on.

I believe if you separate the refactoring & moving code around into own 
changes (no functional change type patches), the new stuff is much 
smaller so there is no need to split that illogically into incomplete 
fragments in some patches.

--
 i.

> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 454 ++++++++++++++++++++++++----
>  1 file changed, 401 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 6f3697e34722..d8425113a9f1 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -440,6 +440,31 @@ static int exar_mpio_set(struct exar8250 *priv,
>  	return 0;
>  }
> 
> +static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> +				struct serial_rs485 *rs485)
> +{
> +	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> +	u8 __iomem *p = port->membase;
> +	u8 value;
> +
> +	value = readb(p + UART_EXAR_FCTR);
> +	if (is_rs485)
> +		value |= UART_FCTR_EXAR_485;
> +	else
> +		value &= ~UART_FCTR_EXAR_485;
> +
> +	writeb(value, p + UART_EXAR_FCTR);
> +
> +	if (is_rs485)
> +		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> +
> +	return 0;
> +}
> +
> +static const struct serial_rs485 generic_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> +};
> +
>  static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
>  {
>  	/*
> @@ -875,15 +900,332 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
>  	return port_type;
>  }
> 
> -static int
> -pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> -		       struct uart_8250_port *port, int idx)
> +static int cti_rs485_config_mpio_tristate(struct uart_port *port,
> +					struct ktermios *termios,
> +					struct serial_rs485 *rs485)
>  {
> -	unsigned int offset = idx * 0x200;
> -	unsigned int baud = 1843200;
> +	struct exar8250 *priv;
> +	int ret;
> 
> -	port->port.uartclk = baud * 16;
> -	return default_setup(priv, pcidev, idx, offset, port);
> +	priv = (struct exar8250 *)port->private_data;
> +	if (!priv)
> +		return -EINVAL;
> +
> +	ret = generic_rs485_config(port, termios, rs485);
> +	if (ret)
> +		return ret;
> +
> +	//disable power-on tri-state via MPIO
> +	return cti_set_tristate(priv, port->port_id, false);
> +}
> +
> +static int cti_port_setup_common(struct exar8250 *priv,
> +				int idx, unsigned int offset,
> +				struct uart_8250_port *port)
> +{
> +	int ret;
> +
> +	if (!priv || !port)
> +		return -EINVAL;
> +
> +	if (priv->osc_freq == 0)
> +		return -EINVAL;
> +
> +	port->port.port_id = idx;
> +	port->port.uartclk = priv->osc_freq;
> +
> +	ret = serial8250_pci_setup_port(priv->pcidev, port, 0, offset, 0);
> +	if (ret) {
> +		pci_err(priv->pcidev,
> +			"failed to setup pci for port %d err: %d\n", idx, ret);
> +		return ret;
> +	}
> +
> +	port->port.private_data = (void *)priv;
> +	port->port.pm = exar_pm;
> +	port->port.shutdown = exar_shutdown;
> +
> +	return 0;
> +}
> +
> +static int cti_port_setup_fpga(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +
> +	port_type = cti_get_port_type_fpga(priv, idx);
> +
> +	//FPGA shares port offests with XR17C15X
> +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> +	port->port.type = PORT_XR17D15X;
> +
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	if (CTI_PORT_TYPE_RS485(port_type)) {
> +		port->port.rs485_config = generic_rs485_config;
> +		port->port.rs485_supported = generic_rs485_supported;
> +	}
> +
> +	return cti_port_setup_common(priv, idx, offset, port);
> +}
> +
> +static int cti_port_setup_xr17v35x(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +	int ret;
> +
> +	port_type = cti_get_port_type_xr17v35x(priv, idx);
> +
> +	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
> +	port->port.type = PORT_XR17V35X;
> +
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	switch (port_type) {
> +	case CTI_PORT_TYPE_RS422_485:
> +	case CTI_PORT_TYPE_RS232_422_485_HW:
> +		port->port.rs485_config = cti_rs485_config_mpio_tristate;
> +		port->port.rs485_supported = generic_rs485_supported;
> +		break;
> +	case CTI_PORT_TYPE_RS232_422_485_SW:
> +	case CTI_PORT_TYPE_RS232_422_485_4B:
> +	case CTI_PORT_TYPE_RS232_422_485_2B:
> +		port->port.rs485_config = generic_rs485_config;
> +		port->port.rs485_supported = generic_rs485_supported;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	ret = cti_port_setup_common(priv, idx, offset, port);
> +	if (ret)
> +		return ret;
> +
> +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);
> +
> +	return 0;
> +}
> +
> +static int cti_port_setup_xr17v25x(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +	int ret;
> +
> +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
> +
> +	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
> +	port->port.type = PORT_XR17D15X;
> +
> +	//xr17v25x supports fractional baudrates
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	if (CTI_PORT_TYPE_RS485(port_type)) {
> +		switch (priv->pcidev->subsystem_device) {
> +		//These cards support power on 485 tri-state via MPIO
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> +			break;
> +		//Otherwise auto or no power on 485 tri-state support
> +		default:
> +			port->port.rs485_config = generic_rs485_config;
> +			break;
> +		}
> +
> +		port->port.rs485_supported = generic_rs485_supported;
> +	}
> +
> +	ret = cti_port_setup_common(priv, idx, offset, port);
> +	if (ret)
> +		return ret;
> +
> +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);
> +
> +	return 0;
> +}
> +
> +static int cti_port_setup_xr17c15x(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +
> +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
> +
> +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> +	port->port.type = PORT_XR17D15X;
> +
> +	if (CTI_PORT_TYPE_RS485(port_type)) {
> +		switch (priv->pcidev->subsystem_device) {
> +		//These cards support power on 485 tri-state via MPIO
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> +			break;
> +		//Otherwise auto or no power on 485 tri-state support
> +		default:
> +			port->port.rs485_config = generic_rs485_config;
> +			break;
> +		}
> +
> +		port->port.rs485_supported = generic_rs485_supported;
> +	}
> +
> +	return cti_port_setup_common(priv, idx, offset, port);
> +}
> +
> +static int cti_board_setup_xr17v35x(struct exar8250 *priv)
> +{
> +	if (!priv)
> +		return -EINVAL;
> +
> +	//XR17V35X use the PCIe clock rather than crystal
> +	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
> +
> +	return 0;
> +}
> +
> +static int cti_board_setup_xr17v25x(struct exar8250 *priv)
> +{
> +	int osc_freq;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
> +	if (osc_freq < 0) {
> +		pci_warn(priv->pcidev,
> +			"failed to read osc freq from EEPROM, using default\n");
> +		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
> +	}
> +
> +	priv->osc_freq = osc_freq;
> +
> +	/* enable interupts on cards that need the "PLX fix" */
> +	switch (priv->pcidev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> +		cti_set_plx_int_enable(priv, true);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cti_board_setup_xr17c15x(struct exar8250 *priv)
> +{
> +	int osc_freq;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
> +	if (osc_freq <= 0) {
> +		pci_warn(priv->pcidev,
> +			"failed to read osc freq from EEPROM, using default\n");
> +		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
> +	}
> +
> +	priv->osc_freq = osc_freq;
> +
> +	/* enable interrupts on cards that need the "PLX fix" */
> +	switch (priv->pcidev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> +		cti_set_plx_int_enable(priv, true);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cti_board_setup_fpga(struct exar8250 *priv)
> +{
> +	int ret;
> +	uint16_t cfg_val;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	//FPGA OSC is fixed to the 33MHz PCI clock
> +	priv->osc_freq = CTI_DEFAULT_FPGA_OSC_FREQ;
> +
> +	//Enable external interrupts in special cfg space register
> +	ret = pci_read_config_word(priv->pcidev, 0x48, &cfg_val);
> +	if (ret)
> +		return ret;
> +
> +	cfg_val |= BIT(15);
> +
> +	ret = pci_write_config_word(priv->pcidev, 0x48, cfg_val);
> +	if (ret)
> +		return ret;
> +
> +	//RS485 gate needs to be enabled; otherwise RTS/CTS will not work
> +	exar_write_reg(priv, CTI_FPGA_RS485_IO_REG, 0x01);
> +
> +	return 0;
>  }
> 
>  static int
> @@ -985,27 +1327,6 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
>  	port->port.private_data = NULL;
>  }
> 
> -static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> -				struct serial_rs485 *rs485)
> -{
> -	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> -	u8 __iomem *p = port->membase;
> -	u8 value;
> -
> -	value = readb(p + UART_EXAR_FCTR);
> -	if (is_rs485)
> -		value |= UART_FCTR_EXAR_485;
> -	else
> -		value &= ~UART_FCTR_EXAR_485;
> -
> -	writeb(value, p + UART_EXAR_FCTR);
> -
> -	if (is_rs485)
> -		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> -
> -	return 0;
> -}
> -
>  static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
>  				  struct serial_rs485 *rs485)
>  {
> @@ -1044,10 +1365,6 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
>  	return 0;
>  }
> 
> -static const struct serial_rs485 generic_rs485_supported = {
> -	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> -};
> -
>  static const struct exar8250_platform exar8250_default_platform = {
>  	.register_gpio = xr17v35x_register_gpio,
>  	.unregister_gpio = xr17v35x_unregister_gpio,
> @@ -1408,8 +1725,24 @@ static const struct exar8250_board pbn_fastcom335_8 = {
>  	.setup		= pci_fastcom335_setup,
>  };
> 
> -static const struct exar8250_board pbn_connect = {
> -	.setup		= pci_connect_tech_setup,
> +static const struct exar8250_board pbn_cti_xr17c15x = {
> +	.board_setup	= cti_board_setup_xr17c15x,
> +	.setup		= cti_port_setup_xr17c15x,
> +};
> +
> +static const struct exar8250_board pbn_cti_xr17v25x = {
> +	.board_setup	= cti_board_setup_xr17v25x,
> +	.setup		= cti_port_setup_xr17v25x,
> +};
> +
> +static const struct exar8250_board pbn_cti_xr17v35x = {
> +	.board_setup	= cti_board_setup_xr17v35x,
> +	.setup		= cti_port_setup_xr17v35x,
> +};
> +
> +static const struct exar8250_board pbn_cti_fpga = {
> +	.board_setup	= cti_board_setup_fpga,
> +	.setup		= cti_port_setup_fpga,
>  };
> 
>  static const struct exar8250_board pbn_exar_ibm_saturn = {
> @@ -1456,15 +1789,27 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
>  	.exit		= pci_xr17v35x_exit,
>  };
> 
> -#define CONNECT_DEVICE(devid, sdevid, bd) {				\
> -	PCI_DEVICE_SUB(							\
> -		PCI_VENDOR_ID_EXAR,					\
> -		PCI_DEVICE_ID_EXAR_##devid,				\
> -		PCI_SUBVENDOR_ID_CONNECT_TECH,				\
> -		PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_##sdevid), 0, 0,	\
> -		(kernel_ulong_t)&bd					\
> +//For Connect Tech cards with Exar vendor/device PCI IDs
> +#define CTI_EXAR_DEVICE(devid, bd) {                    \
> +	PCI_DEVICE_SUB(                                 \
> +		PCI_VENDOR_ID_EXAR,                     \
> +		PCI_DEVICE_ID_EXAR_##devid,             \
> +		PCI_SUBVENDOR_ID_CONNECT_TECH,          \
> +		PCI_ANY_ID), 0, 0,                      \
> +		(kernel_ulong_t)&bd                     \
> +	}
> +
> +//For Connect Tech cards with Connect Tech vendor/device PCI IDs (FPGA based)
> +#define CTI_PCI_DEVICE(devid, bd) {                     \
> +	PCI_DEVICE_SUB(                                 \
> +		PCI_VENDOR_ID_CONNECT_TECH,             \
> +		PCI_DEVICE_ID_CONNECT_TECH_PCI_##devid, \
> +		PCI_ANY_ID,                             \
> +		PCI_ANY_ID), 0, 0,                      \
> +		(kernel_ulong_t)&bd                     \
>  	}
> 
> +
>  #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }
> 
>  #define IBM_DEVICE(devid, sdevid, bd) {			\
> @@ -1494,18 +1839,21 @@ static const struct pci_device_id exar_pci_tbl[] = {
>  	EXAR_DEVICE(ACCESSIO, COM_4SM, pbn_exar_XR17C15x),
>  	EXAR_DEVICE(ACCESSIO, COM_8SM, pbn_exar_XR17C15x),
> 
> -	CONNECT_DEVICE(XR17C152, UART_2_232, pbn_connect),
> -	CONNECT_DEVICE(XR17C154, UART_4_232, pbn_connect),
> -	CONNECT_DEVICE(XR17C158, UART_8_232, pbn_connect),
> -	CONNECT_DEVICE(XR17C152, UART_1_1, pbn_connect),
> -	CONNECT_DEVICE(XR17C154, UART_2_2, pbn_connect),
> -	CONNECT_DEVICE(XR17C158, UART_4_4, pbn_connect),
> -	CONNECT_DEVICE(XR17C152, UART_2, pbn_connect),
> -	CONNECT_DEVICE(XR17C154, UART_4, pbn_connect),
> -	CONNECT_DEVICE(XR17C158, UART_8, pbn_connect),
> -	CONNECT_DEVICE(XR17C152, UART_2_485, pbn_connect),
> -	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
> -	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
> +	CTI_EXAR_DEVICE(XR17C152,       pbn_cti_xr17c15x),
> +	CTI_EXAR_DEVICE(XR17C154,       pbn_cti_xr17c15x),
> +	CTI_EXAR_DEVICE(XR17C158,       pbn_cti_xr17c15x),
> +
> +	CTI_EXAR_DEVICE(XR17V252,       pbn_cti_xr17v25x),
> +	CTI_EXAR_DEVICE(XR17V254,       pbn_cti_xr17v25x),
> +	CTI_EXAR_DEVICE(XR17V258,       pbn_cti_xr17v25x),
> +
> +	CTI_EXAR_DEVICE(XR17V352,       pbn_cti_xr17v35x),
> +	CTI_EXAR_DEVICE(XR17V354,       pbn_cti_xr17v35x),
> +	CTI_EXAR_DEVICE(XR17V358,       pbn_cti_xr17v35x),
> +
> +	CTI_PCI_DEVICE(XR79X_12_XIG00X, pbn_cti_fpga),
> +	CTI_PCI_DEVICE(XR79X_12_XIG01X, pbn_cti_fpga),
> +	CTI_PCI_DEVICE(XR79X_16,        pbn_cti_fpga),
> 
>  	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
> 
> --
> 2.43.2
> 
> 

Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup functions

[Parker Newman]

On Fri, 12 Apr 2024 13:57:01 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 11 Apr 2024, parker@finest.io wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > - Removed old port setup function and replaced with UART specific ones
> > - Added board setup functions for CTI boards
> > - Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE  
> 
> In general, you should try to do refactoring in a preparatory patch (one 
> refactoring thing at a time) and add new stuff in another patch in 
> the series. I didn't go to figure out how much it applies to those three 
> items because you likely know the answer immediately.
> 
> > - Moved "generic rs485" support up in the file  
> 
> Please do this in a separate patch.
> 

Will do.

> 
> Another general level problem with your series is that it adds functions 
> x, y, etc. without users, whereas the expected way of doing things would 
> be to add the functions in the change they are getting used so it's easier 
> to follow what's going on.
> 
> I believe if you separate the refactoring & moving code around into own 
> changes (no functional change type patches), the new stuff is much 
> smaller so there is no need to split that illogically into incomplete 
> fragments in some patches.
> 
> --
>  i.
> 

Thanks for the feedback, I am new to the mailing lists and am trying to balance
what you mention above with not having giant patches. 

> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 454 ++++++++++++++++++++++++----
> >  1 file changed, 401 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 6f3697e34722..d8425113a9f1 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -440,6 +440,31 @@ static int exar_mpio_set(struct exar8250 *priv,
> >  	return 0;
> >  }
> > 
> > +static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> > +				struct serial_rs485 *rs485)
> > +{
> > +	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> > +	u8 __iomem *p = port->membase;
> > +	u8 value;
> > +
> > +	value = readb(p + UART_EXAR_FCTR);
> > +	if (is_rs485)
> > +		value |= UART_FCTR_EXAR_485;
> > +	else
> > +		value &= ~UART_FCTR_EXAR_485;
> > +
> > +	writeb(value, p + UART_EXAR_FCTR);
> > +
> > +	if (is_rs485)
> > +		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct serial_rs485 generic_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> > +};
> > +
> >  static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
> >  {
> >  	/*
> > @@ -875,15 +900,332 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> >  	return port_type;
> >  }
> > 
> > -static int
> > -pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > -		       struct uart_8250_port *port, int idx)
> > +static int cti_rs485_config_mpio_tristate(struct uart_port *port,
> > +					struct ktermios *termios,
> > +					struct serial_rs485 *rs485)
> >  {
> > -	unsigned int offset = idx * 0x200;
> > -	unsigned int baud = 1843200;
> > +	struct exar8250 *priv;
> > +	int ret;
> > 
> > -	port->port.uartclk = baud * 16;
> > -	return default_setup(priv, pcidev, idx, offset, port);
> > +	priv = (struct exar8250 *)port->private_data;
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	ret = generic_rs485_config(port, termios, rs485);
> > +	if (ret)
> > +		return ret;
> > +
> > +	//disable power-on tri-state via MPIO
> > +	return cti_set_tristate(priv, port->port_id, false);
> > +}
> > +
> > +static int cti_port_setup_common(struct exar8250 *priv,
> > +				int idx, unsigned int offset,
> > +				struct uart_8250_port *port)
> > +{
> > +	int ret;
> > +
> > +	if (!priv || !port)
> > +		return -EINVAL;
> > +
> > +	if (priv->osc_freq == 0)
> > +		return -EINVAL;
> > +
> > +	port->port.port_id = idx;
> > +	port->port.uartclk = priv->osc_freq;
> > +
> > +	ret = serial8250_pci_setup_port(priv->pcidev, port, 0, offset, 0);
> > +	if (ret) {
> > +		pci_err(priv->pcidev,
> > +			"failed to setup pci for port %d err: %d\n", idx, ret);
> > +		return ret;
> > +	}
> > +
> > +	port->port.private_data = (void *)priv;
> > +	port->port.pm = exar_pm;
> > +	port->port.shutdown = exar_shutdown;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_port_setup_fpga(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +
> > +	port_type = cti_get_port_type_fpga(priv, idx);
> > +
> > +	//FPGA shares port offests with XR17C15X
> > +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17D15X;
> > +
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	if (CTI_PORT_TYPE_RS485(port_type)) {
> > +		port->port.rs485_config = generic_rs485_config;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +	}
> > +
> > +	return cti_port_setup_common(priv, idx, offset, port);
> > +}
> > +
> > +static int cti_port_setup_xr17v35x(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +	int ret;
> > +
> > +	port_type = cti_get_port_type_xr17v35x(priv, idx);
> > +
> > +	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17V35X;
> > +
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	switch (port_type) {
> > +	case CTI_PORT_TYPE_RS422_485:
> > +	case CTI_PORT_TYPE_RS232_422_485_HW:
> > +		port->port.rs485_config = cti_rs485_config_mpio_tristate;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +		break;
> > +	case CTI_PORT_TYPE_RS232_422_485_SW:
> > +	case CTI_PORT_TYPE_RS232_422_485_4B:
> > +	case CTI_PORT_TYPE_RS232_422_485_2B:
> > +		port->port.rs485_config = generic_rs485_config;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	ret = cti_port_setup_common(priv, idx, offset, port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_port_setup_xr17v25x(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +	int ret;
> > +
> > +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
> > +
> > +	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17D15X;
> > +
> > +	//xr17v25x supports fractional baudrates
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	if (CTI_PORT_TYPE_RS485(port_type)) {
> > +		switch (priv->pcidev->subsystem_device) {
> > +		//These cards support power on 485 tri-state via MPIO
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> > +			break;
> > +		//Otherwise auto or no power on 485 tri-state support
> > +		default:
> > +			port->port.rs485_config = generic_rs485_config;
> > +			break;
> > +		}
> > +
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +	}
> > +
> > +	ret = cti_port_setup_common(priv, idx, offset, port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_port_setup_xr17c15x(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +
> > +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, idx);
> > +
> > +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17D15X;
> > +
> > +	if (CTI_PORT_TYPE_RS485(port_type)) {
> > +		switch (priv->pcidev->subsystem_device) {
> > +		//These cards support power on 485 tri-state via MPIO
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> > +			break;
> > +		//Otherwise auto or no power on 485 tri-state support
> > +		default:
> > +			port->port.rs485_config = generic_rs485_config;
> > +			break;
> > +		}
> > +
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +	}
> > +
> > +	return cti_port_setup_common(priv, idx, offset, port);
> > +}
> > +
> > +static int cti_board_setup_xr17v35x(struct exar8250 *priv)
> > +{
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	//XR17V35X use the PCIe clock rather than crystal
> > +	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_board_setup_xr17v25x(struct exar8250 *priv)
> > +{
> > +	int osc_freq;
> > +
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
> > +	if (osc_freq < 0) {
> > +		pci_warn(priv->pcidev,
> > +			"failed to read osc freq from EEPROM, using default\n");
> > +		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
> > +	}
> > +
> > +	priv->osc_freq = osc_freq;
> > +
> > +	/* enable interupts on cards that need the "PLX fix" */
> > +	switch (priv->pcidev->subsystem_device) {
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> > +		cti_set_plx_int_enable(priv, true);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_board_setup_xr17c15x(struct exar8250 *priv)
> > +{
> > +	int osc_freq;
> > +
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
> > +	if (osc_freq <= 0) {
> > +		pci_warn(priv->pcidev,
> > +			"failed to read osc freq from EEPROM, using default\n");
> > +		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
> > +	}
> > +
> > +	priv->osc_freq = osc_freq;
> > +
> > +	/* enable interrupts on cards that need the "PLX fix" */
> > +	switch (priv->pcidev->subsystem_device) {
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> > +		cti_set_plx_int_enable(priv, true);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_board_setup_fpga(struct exar8250 *priv)
> > +{
> > +	int ret;
> > +	uint16_t cfg_val;
> > +
> > +	if (!priv)
> > +		return -EINVAL;
> > +
> > +	//FPGA OSC is fixed to the 33MHz PCI clock
> > +	priv->osc_freq = CTI_DEFAULT_FPGA_OSC_FREQ;
> > +
> > +	//Enable external interrupts in special cfg space register
> > +	ret = pci_read_config_word(priv->pcidev, 0x48, &cfg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cfg_val |= BIT(15);
> > +
> > +	ret = pci_write_config_word(priv->pcidev, 0x48, cfg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	//RS485 gate needs to be enabled; otherwise RTS/CTS will not work
> > +	exar_write_reg(priv, CTI_FPGA_RS485_IO_REG, 0x01);
> > +
> > +	return 0;
> >  }
> > 
> >  static int
> > @@ -985,27 +1327,6 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
> >  	port->port.private_data = NULL;
> >  }
> > 
> > -static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> > -				struct serial_rs485 *rs485)
> > -{
> > -	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> > -	u8 __iomem *p = port->membase;
> > -	u8 value;
> > -
> > -	value = readb(p + UART_EXAR_FCTR);
> > -	if (is_rs485)
> > -		value |= UART_FCTR_EXAR_485;
> > -	else
> > -		value &= ~UART_FCTR_EXAR_485;
> > -
> > -	writeb(value, p + UART_EXAR_FCTR);
> > -
> > -	if (is_rs485)
> > -		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> > -
> > -	return 0;
> > -}
> > -
> >  static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> >  				  struct serial_rs485 *rs485)
> >  {
> > @@ -1044,10 +1365,6 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
> >  	return 0;
> >  }
> > 
> > -static const struct serial_rs485 generic_rs485_supported = {
> > -	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> > -};
> > -
> >  static const struct exar8250_platform exar8250_default_platform = {
> >  	.register_gpio = xr17v35x_register_gpio,
> >  	.unregister_gpio = xr17v35x_unregister_gpio,
> > @@ -1408,8 +1725,24 @@ static const struct exar8250_board pbn_fastcom335_8 = {
> >  	.setup		= pci_fastcom335_setup,
> >  };
> > 
> > -static const struct exar8250_board pbn_connect = {
> > -	.setup		= pci_connect_tech_setup,
> > +static const struct exar8250_board pbn_cti_xr17c15x = {
> > +	.board_setup	= cti_board_setup_xr17c15x,
> > +	.setup		= cti_port_setup_xr17c15x,
> > +};
> > +
> > +static const struct exar8250_board pbn_cti_xr17v25x = {
> > +	.board_setup	= cti_board_setup_xr17v25x,
> > +	.setup		= cti_port_setup_xr17v25x,
> > +};
> > +
> > +static const struct exar8250_board pbn_cti_xr17v35x = {
> > +	.board_setup	= cti_board_setup_xr17v35x,
> > +	.setup		= cti_port_setup_xr17v35x,
> > +};
> > +
> > +static const struct exar8250_board pbn_cti_fpga = {
> > +	.board_setup	= cti_board_setup_fpga,
> > +	.setup		= cti_port_setup_fpga,
> >  };
> > 
> >  static const struct exar8250_board pbn_exar_ibm_saturn = {
> > @@ -1456,15 +1789,27 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> >  	.exit		= pci_xr17v35x_exit,
> >  };
> > 
> > -#define CONNECT_DEVICE(devid, sdevid, bd) {				\
> > -	PCI_DEVICE_SUB(							\
> > -		PCI_VENDOR_ID_EXAR,					\
> > -		PCI_DEVICE_ID_EXAR_##devid,				\
> > -		PCI_SUBVENDOR_ID_CONNECT_TECH,				\
> > -		PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_##sdevid), 0, 0,	\
> > -		(kernel_ulong_t)&bd					\
> > +//For Connect Tech cards with Exar vendor/device PCI IDs
> > +#define CTI_EXAR_DEVICE(devid, bd) {                    \
> > +	PCI_DEVICE_SUB(                                 \
> > +		PCI_VENDOR_ID_EXAR,                     \
> > +		PCI_DEVICE_ID_EXAR_##devid,             \
> > +		PCI_SUBVENDOR_ID_CONNECT_TECH,          \
> > +		PCI_ANY_ID), 0, 0,                      \
> > +		(kernel_ulong_t)&bd                     \
> > +	}
> > +
> > +//For Connect Tech cards with Connect Tech vendor/device PCI IDs (FPGA based)
> > +#define CTI_PCI_DEVICE(devid, bd) {                     \
> > +	PCI_DEVICE_SUB(                                 \
> > +		PCI_VENDOR_ID_CONNECT_TECH,             \
> > +		PCI_DEVICE_ID_CONNECT_TECH_PCI_##devid, \
> > +		PCI_ANY_ID,                             \
> > +		PCI_ANY_ID), 0, 0,                      \
> > +		(kernel_ulong_t)&bd                     \
> >  	}
> > 
> > +
> >  #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }
> > 
> >  #define IBM_DEVICE(devid, sdevid, bd) {			\
> > @@ -1494,18 +1839,21 @@ static const struct pci_device_id exar_pci_tbl[] = {
> >  	EXAR_DEVICE(ACCESSIO, COM_4SM, pbn_exar_XR17C15x),
> >  	EXAR_DEVICE(ACCESSIO, COM_8SM, pbn_exar_XR17C15x),
> > 
> > -	CONNECT_DEVICE(XR17C152, UART_2_232, pbn_connect),
> > -	CONNECT_DEVICE(XR17C154, UART_4_232, pbn_connect),
> > -	CONNECT_DEVICE(XR17C158, UART_8_232, pbn_connect),
> > -	CONNECT_DEVICE(XR17C152, UART_1_1, pbn_connect),
> > -	CONNECT_DEVICE(XR17C154, UART_2_2, pbn_connect),
> > -	CONNECT_DEVICE(XR17C158, UART_4_4, pbn_connect),
> > -	CONNECT_DEVICE(XR17C152, UART_2, pbn_connect),
> > -	CONNECT_DEVICE(XR17C154, UART_4, pbn_connect),
> > -	CONNECT_DEVICE(XR17C158, UART_8, pbn_connect),
> > -	CONNECT_DEVICE(XR17C152, UART_2_485, pbn_connect),
> > -	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
> > -	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
> > +	CTI_EXAR_DEVICE(XR17C152,       pbn_cti_xr17c15x),
> > +	CTI_EXAR_DEVICE(XR17C154,       pbn_cti_xr17c15x),
> > +	CTI_EXAR_DEVICE(XR17C158,       pbn_cti_xr17c15x),
> > +
> > +	CTI_EXAR_DEVICE(XR17V252,       pbn_cti_xr17v25x),
> > +	CTI_EXAR_DEVICE(XR17V254,       pbn_cti_xr17v25x),
> > +	CTI_EXAR_DEVICE(XR17V258,       pbn_cti_xr17v25x),
> > +
> > +	CTI_EXAR_DEVICE(XR17V352,       pbn_cti_xr17v35x),
> > +	CTI_EXAR_DEVICE(XR17V354,       pbn_cti_xr17v35x),
> > +	CTI_EXAR_DEVICE(XR17V358,       pbn_cti_xr17v35x),
> > +
> > +	CTI_PCI_DEVICE(XR79X_12_XIG00X, pbn_cti_fpga),
> > +	CTI_PCI_DEVICE(XR79X_12_XIG01X, pbn_cti_fpga),
> > +	CTI_PCI_DEVICE(XR79X_16,        pbn_cti_fpga),
> > 
> >  	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
> > 
> > --
> > 2.43.2
> > 
> >   

Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup functions

[Greg Kroah-Hartman]

On Fri, Apr 12, 2024 at 11:19:26AM -0400, Parker Newman wrote:
> On Fri, 12 Apr 2024 13:57:01 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 11 Apr 2024, parker@finest.io wrote:
> > 
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > - Removed old port setup function and replaced with UART specific ones
> > > - Added board setup functions for CTI boards
> > > - Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE  
> > 
> > In general, you should try to do refactoring in a preparatory patch (one 
> > refactoring thing at a time) and add new stuff in another patch in 
> > the series. I didn't go to figure out how much it applies to those three 
> > items because you likely know the answer immediately.
> > 
> > > - Moved "generic rs485" support up in the file  
> > 
> > Please do this in a separate patch.
> > 
> 
> Will do.
> 
> > 
> > Another general level problem with your series is that it adds functions 
> > x, y, etc. without users, whereas the expected way of doing things would 
> > be to add the functions in the change they are getting used so it's easier 
> > to follow what's going on.
> > 
> > I believe if you separate the refactoring & moving code around into own 
> > changes (no functional change type patches), the new stuff is much 
> > smaller so there is no need to split that illogically into incomplete 
> > fragments in some patches.
> > 
> > --
> >  i.
> > 
> 
> Thanks for the feedback, I am new to the mailing lists and am trying to balance
> what you mention above with not having giant patches. 

It's a fine line, and takes a while to learn, but as a first cut, this
was pretty good, I didn't have any major problems with the structure of
it, so nice work.

thanks,

greg k-h

Re: [PATCH v2 6/7] serial: exar: add CTI board and port setup functions

[Parker Newman]

On Fri, 12 Apr 2024 17:28:20 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Apr 12, 2024 at 11:19:26AM -0400, Parker Newman wrote:
> > On Fri, 12 Apr 2024 13:57:01 +0300 (EEST)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > On Thu, 11 Apr 2024, parker@finest.io wrote:
> > >   
> > > > From: Parker Newman <pnewman@connecttech.com>
> > > > 
> > > > - Removed old port setup function and replaced with UART specific ones
> > > > - Added board setup functions for CTI boards
> > > > - Replaced CONNECT_DEVICE macro with CTI_EXAR_DEVICE and CTI_PCI_DEVICE    
> > > 
> > > In general, you should try to do refactoring in a preparatory patch (one 
> > > refactoring thing at a time) and add new stuff in another patch in 
> > > the series. I didn't go to figure out how much it applies to those three 
> > > items because you likely know the answer immediately.
> > >   
> > > > - Moved "generic rs485" support up in the file    
> > > 
> > > Please do this in a separate patch.
> > >   
> > 
> > Will do.
> >   
> > > 
> > > Another general level problem with your series is that it adds functions 
> > > x, y, etc. without users, whereas the expected way of doing things would 
> > > be to add the functions in the change they are getting used so it's easier 
> > > to follow what's going on.
> > > 
> > > I believe if you separate the refactoring & moving code around into own 
> > > changes (no functional change type patches), the new stuff is much 
> > > smaller so there is no need to split that illogically into incomplete 
> > > fragments in some patches.
> > > 
> > > --
> > >  i.
> > >   
> > 
> > Thanks for the feedback, I am new to the mailing lists and am trying to balance
> > what you mention above with not having giant patches.   
> 
> It's a fine line, and takes a while to learn, but as a first cut, this
> was pretty good, I didn't have any major problems with the structure of
> it, so nice work.
> 
> thanks,
> 
> greg k-h

Thanks, I appreciate both your feedback I think I have a better handle on it.
-Parker

[PATCH v2 7/7] serial: exar: fix: fix crash during shutdown if setup fails

[parker]

From: Parker Newman <pnewman@connecttech.com>

If a port fails to register with serial8250_register_8250_port the
kernel can crash when shutting down or module removal.

This is because "priv->line[i]" will be set to a negative error code
andin the exar_pci_remove function serial8250_unregister_port is called
without checking if the "priv->line[i]" value is valid.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index d8425113a9f1..1c6343ed76e9 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1675,7 +1675,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
 	unsigned int i;

 	for (i = 0; i < priv->nr; i++)
-		serial8250_unregister_port(priv->line[i]);
+		if (priv->line[i] >= 0)
+			serial8250_unregister_port(priv->line[i]);

 	/* Ensure that every init quirk is properly torn down */
 	if (priv->board->exit)
--
2.43.2