From: Parker Newman <[email protected]>
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
From: Parker Newman <[email protected]>
- Added Connect Tech and Exar IDs not already in pci_ids.h
Signed-off-by: Parker Newman <[email protected]>
---
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
From: Parker Newman <[email protected]>
- 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 <[email protected]>
---
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
From: Parker Newman <[email protected]>
Adds support for configuring and setting a single MPIO
Signed-off-by: Parker Newman <[email protected]>
---
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
On Thu, Apr 11, 2024 at 04:25:41PM -0400, [email protected] 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
On Thu, 11 Apr 2024, [email protected] wrote:
> From: Parker Newman <[email protected]>
>
> Adds support for configuring and setting a single MPIO
>
> Signed-off-by: Parker Newman <[email protected]>
> ---
> 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.
On Fri, 12 Apr 2024 07:29:16 +0200
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Apr 11, 2024 at 04:25:41PM -0400, [email protected] 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
On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
Ilpo Järvinen <[email protected]> wrote:
> On Thu, 11 Apr 2024, [email protected] wrote:
>
> > From: Parker Newman <[email protected]>
> >
> > Adds support for configuring and setting a single MPIO
> >
> > Signed-off-by: Parker Newman <[email protected]>
> > ---
> > 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
On Fri, 12 Apr 2024, Parker Newman wrote:
> On Fri, 12 Apr 2024 13:20:41 +0300 (EEST)
> Ilpo Järvinen <[email protected]> wrote:
>
> > On Thu, 11 Apr 2024, [email protected] wrote:
> >
> > > From: Parker Newman <[email protected]>
> > >
> > > Adds support for configuring and setting a single MPIO
> > >
> > > Signed-off-by: Parker Newman <[email protected]>
> > > ---
> > > 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.
On Thu, 11 Apr 2024, [email protected] wrote:
> From: Parker Newman <[email protected]>
>
> - 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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]> wrote:
>
> > On Thu, 11 Apr 2024, [email protected] wrote:
> >
> > > From: Parker Newman <[email protected]>
> > >
> > > - 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
On Fri, 12 Apr 2024 13:57:01 +0300 (EEST)
Ilpo Järvinen <[email protected]> wrote:
> On Thu, 11 Apr 2024, [email protected] wrote:
>
> > From: Parker Newman <[email protected]>
> >
> > - 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 <[email protected]>
> > ---
> > 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
> >
> >
On Fri, 12 Apr 2024 17:28:20 +0200
Greg Kroah-Hartman <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Thu, 11 Apr 2024, [email protected] wrote:
> > >
> > > > From: Parker Newman <[email protected]>
> > > >
> > > > - 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