[PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

After a rework for CONNTECH was done, the driver may need a bit of
love in order to become less verbose (in terms of indentation and
code duplication) and hence easier to read.

This clean up series fixes a couple of (not so critical) issues and
cleans up the recently added code. No functional change indented by
the cleaning up part.

Parker, please test this and give your formal Tested-by tag
(you may do it by replying to this message if all patches are
 successfully tested; more details about tags are available in
 the Submitting Patches documentation).

In v2:
- fixed the EEPROM reading data loop (Ilpo, Parker)

Andy Shevchenko (13):
  serial: 8250_exar: Don't return positive values as error codes
  serial: 8250_exar: Describe all parameters in kernel doc
  serial: 8250_exar: Kill CTI_PCI_DEVICE()
  serial: 8250_exar: Use PCI_SUBVENDOR_ID_IBM for subvendor ID
  serial: 8250_exar: Trivia typo fixes
  serial: 8250_exar: Extract cti_board_init_osc_freq() helper
  serial: 8250_exar: Kill unneeded ->board_init()
  serial: 8250_exar: Decrease indentation level
  serial: 8250_exar: Return directly from switch-cases
  serial: 8250_exar: Switch to use dev_err_probe()
  serial: 8250_exar: Use BIT() in exar_ee_read()
  serial: 8250_exar: Make type of bit the same in exar_ee_*_bit()
  serial: 8250_exar: Keep the includes sorted

 drivers/tty/serial/8250/8250_exar.c | 459 ++++++++++++----------------
 1 file changed, 203 insertions(+), 256 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 01/13] serial: 8250_exar: Don't return positive values as error codes

[Andy Shevchenko]

PCI core may return positive codes for the specific errors.
There is a special API to convert them to negative stanrard ones.
Use that API to avoid returning positive values as error codes.

Fixes: f7ce07062988 ("serial: exar: add CTI specific setup code")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 5e42558cbb01..9930668610ca 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1162,12 +1162,12 @@ static int cti_board_init_fpga(struct exar8250 *priv, struct pci_dev *pcidev)
 	// Enable external interrupts in special cfg space register
 	ret = pci_read_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, &cfg_val);
 	if (ret)
-		return ret;
+		return pcibios_err_to_errno(ret);
 
 	cfg_val |= CTI_FPGA_CFG_INT_EN_EXT_BIT;
 	ret = pci_write_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, cfg_val);
 	if (ret)
-		return ret;
+		return pcibios_err_to_errno(ret);
 
 	// RS485 gate needs to be enabled; otherwise RTS/CTS will not work
 	exar_write_reg(priv, CTI_FPGA_RS485_IO_REG, 0x01);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 02/13] serial: 8250_exar: Describe all parameters in kernel doc

[Andy Shevchenko]

Kernel doc validator is not happy:

 warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17c15x_xr17v25x'
 warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_fpga'
 warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17v35x'

Add missed descriptions.

Fixes: f7ce07062988 ("serial: exar: add CTI specific setup code")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 9930668610ca..150c4abd92fc 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -715,6 +715,7 @@ static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
 /**
  * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
  * @priv: Device's private structure
+ * @pcidev: Pointer to the PCI device for this port
  * @port_num: Port to get type of
  *
  * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
@@ -807,6 +808,7 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 /**
  * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
  * @priv: Device's private structure
+ * @pcidev: Pointer to the PCI device for this port
  * @port_num: Port to get type of
  *
  * FPGA based cards port types are based on PCI IDs.
@@ -836,6 +838,7 @@ static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
 /**
  * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
  * @priv: Device's private structure
+ * @pcidev: Pointer to the PCI device for this port
  * @port_num: port offset
  *
  * CTI XR17V35X based cards have the port types stored in the EEPROM.
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 03/13] serial: 8250_exar: Kill CTI_PCI_DEVICE()

[Andy Shevchenko]

The CTI_PCI_DEVICE() duplicates EXAR_DEVICE(). Kill the former.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 150c4abd92fc..ab0abc14ecf8 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1737,7 +1737,6 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };
 
-// For Connect Tech cards with Exar vendor/device PCI IDs
 #define CTI_EXAR_DEVICE(devid, bd) {                    \
 	PCI_DEVICE_SUB(                                 \
 		PCI_VENDOR_ID_EXAR,                     \
@@ -1747,16 +1746,6 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 		(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) {			\
@@ -1786,6 +1775,7 @@ 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 Tech cards with Exar vendor/device PCI IDs */
 	CTI_EXAR_DEVICE(XR17C152,       pbn_cti_xr17c15x),
 	CTI_EXAR_DEVICE(XR17C154,       pbn_cti_xr17c15x),
 	CTI_EXAR_DEVICE(XR17C158,       pbn_cti_xr17c15x),
@@ -1798,9 +1788,10 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	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),
+	/* Connect Tech cards with Connect Tech vendor/device PCI IDs (FPGA based) */
+	EXAR_DEVICE(CONNECT_TECH, PCI_XR79X_12_XIG00X, pbn_cti_fpga),
+	EXAR_DEVICE(CONNECT_TECH, PCI_XR79X_12_XIG01X, pbn_cti_fpga),
+	EXAR_DEVICE(CONNECT_TECH, PCI_XR79X_16,        pbn_cti_fpga),
 
 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 04/13] serial: 8250_exar: Use PCI_SUBVENDOR_ID_IBM for subvendor ID

[Andy Shevchenko]

Use PCI_SUBVENDOR_ID_IBM for subvendor ID instead of vendor one.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index ab0abc14ecf8..5405a1f463f1 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1752,7 +1752,7 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	PCI_DEVICE_SUB(					\
 		PCI_VENDOR_ID_EXAR,			\
 		PCI_DEVICE_ID_EXAR_##devid,		\
-		PCI_VENDOR_ID_IBM,			\
+		PCI_SUBVENDOR_ID_IBM,			\
 		PCI_SUBDEVICE_ID_IBM_##sdevid), 0, 0,	\
 		(kernel_ulong_t)&bd			\
 	}
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 05/13] serial: 8250_exar: Trivia typo fixes

[Andy Shevchenko]

Trivia typo fixes: interupts --> interrupts and others.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 5405a1f463f1..ac373cde7e39 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -641,7 +641,7 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
  * @port_num: Port number to set tristate off
  *
  * 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
+ * the RS422/RS485 transceiver 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 Full or Half duplex to disable tristate prior to using the port.
  *
@@ -666,7 +666,7 @@ static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
  * @priv: Device's private structure
  *
  * Some older CTI cards require MPIO_0 to be set low to enable the
- * interupts from the UART to the PLX PCI->PCIe bridge.
+ * interrupts from the UART to the PLX PCI->PCIe bridge.
  *
  * Return: 0 on success, negative error code on failure
  */
@@ -927,7 +927,7 @@ static int cti_port_setup_fpga(struct exar8250 *priv,
 
 	port_type = cti_get_port_type_fpga(priv, pcidev, idx);
 
-	// FPGA shares port offests with XR17C15X
+	// FPGA shares port offsets with XR17C15X
 	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
 	port->port.type = PORT_XR17D15X;
 
@@ -1109,7 +1109,7 @@ static int cti_board_init_xr17v25x(struct exar8250 *priv,
 
 	priv->osc_freq = osc_freq;
 
-	/* enable interupts on cards that need the "PLX fix" */
+	/* enable interrupts on cards that need the "PLX fix" */
 	switch (pcidev->subsystem_device) {
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 06/13] serial: 8250_exar: Extract cti_board_init_osc_freq() helper

[Andy Shevchenko]

Extract cti_board_init_osc_freq() helper to avoid code duplication.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 48 ++++++++++++-----------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index ac373cde7e39..956323e2de4a 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -695,7 +695,6 @@ static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
 {
 	u16 lower_word;
 	u16 upper_word;
-	int osc_freq;
 
 	lower_word = exar_ee_read(priv, eeprom_offset);
 	// Check if EEPROM word was blank
@@ -706,10 +705,8 @@ static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
 	if (upper_word == 0xFFFF)
 		return -EIO;
 
-	osc_freq = FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
-		FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
-
-	return osc_freq;
+	return FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
+	       FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
 }
 
 /**
@@ -890,6 +887,19 @@ static int cti_rs485_config_mpio_tristate(struct uart_port *port,
 	return cti_tristate_disable(priv, port->port_id);
 }
 
+static void cti_board_init_osc_freq(struct exar8250 *priv, struct pci_dev *pcidev, u8 eeprom_offset)
+{
+	int osc_freq;
+
+	osc_freq = cti_read_osc_freq(priv, eeprom_offset);
+	if (osc_freq <= 0) {
+		dev_warn(&pcidev->dev, "failed to read OSC freq from EEPROM, using default\n");
+		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
+	}
+
+	priv->osc_freq = osc_freq;
+}
+
 static int cti_port_setup_common(struct exar8250 *priv,
 				struct pci_dev *pcidev,
 				int idx, unsigned int offset,
@@ -1095,19 +1105,9 @@ static int cti_board_init_xr17v35x(struct exar8250 *priv,
 	return 0;
 }
 
-static int cti_board_init_xr17v25x(struct exar8250 *priv,
-				struct pci_dev *pcidev)
+static int cti_board_init_xr17v25x(struct exar8250 *priv, struct pci_dev *pcidev)
 {
-	int osc_freq;
-
-	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
-	if (osc_freq < 0) {
-		dev_warn(&pcidev->dev,
-			"failed to read osc freq from EEPROM, using default\n");
-		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
-	}
-
-	priv->osc_freq = osc_freq;
+	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17V25X_OSC_FREQ);
 
 	/* enable interrupts on cards that need the "PLX fix" */
 	switch (pcidev->subsystem_device) {
@@ -1123,19 +1123,9 @@ static int cti_board_init_xr17v25x(struct exar8250 *priv,
 	return 0;
 }
 
-static int cti_board_init_xr17c15x(struct exar8250 *priv,
-				struct pci_dev *pcidev)
+static int cti_board_init_xr17c15x(struct exar8250 *priv, struct pci_dev *pcidev)
 {
-	int osc_freq;
-
-	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
-	if (osc_freq <= 0) {
-		dev_warn(&pcidev->dev,
-			"failed to read osc freq from EEPROM, using default\n");
-		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
-	}
-
-	priv->osc_freq = osc_freq;
+	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17C15X_OSC_FREQ);
 
 	/* enable interrupts on cards that need the "PLX fix" */
 	switch (pcidev->subsystem_device) {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 07/13] serial: 8250_exar: Kill unneeded ->board_init()

[Andy Shevchenko]

We may reuse ->setup() for the same purpose as it was done before.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 171 +++++++++++++---------------
 1 file changed, 81 insertions(+), 90 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 956323e2de4a..4e683ca76de0 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -237,14 +237,12 @@ struct exar8250_platform {
  * struct exar8250_board - board information
  * @num_ports: number of serial ports
  * @reg_shift: describes UART register mapping in PCI memory
- * @board_init: 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     (*board_init)(struct exar8250 *priv, struct pci_dev *pcidev);
 	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
 			 struct uart_8250_port *port, int idx);
 	void	(*exit)(struct pci_dev *pcidev);
@@ -907,9 +905,6 @@ static int cti_port_setup_common(struct exar8250 *priv,
 {
 	int ret;
 
-	if (priv->osc_freq == 0)
-		return -EINVAL;
-
 	port->port.port_id = idx;
 	port->port.uartclk = priv->osc_freq;
 
@@ -927,6 +922,30 @@ static int cti_port_setup_common(struct exar8250 *priv,
 	return 0;
 }
 
+static int cti_board_init_fpga(struct exar8250 *priv, struct pci_dev *pcidev)
+{
+	int ret;
+	u16 cfg_val;
+
+	// 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(pcidev, CTI_FPGA_CFG_INT_EN_REG, &cfg_val);
+	if (ret)
+		return pcibios_err_to_errno(ret);
+
+	cfg_val |= CTI_FPGA_CFG_INT_EN_EXT_BIT;
+	ret = pci_write_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, cfg_val);
+	if (ret)
+		return pcibios_err_to_errno(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 cti_port_setup_fpga(struct exar8250 *priv,
 				struct pci_dev *pcidev,
 				struct uart_8250_port *port,
@@ -934,6 +953,13 @@ static int cti_port_setup_fpga(struct exar8250 *priv,
 {
 	enum cti_port_type port_type;
 	unsigned int offset;
+	int ret;
+
+	if (idx == 0) {
+		ret = cti_board_init_fpga(priv, pcidev);
+		if (ret)
+			return ret;
+	}
 
 	port_type = cti_get_port_type_fpga(priv, pcidev, idx);
 
@@ -953,6 +979,12 @@ static int cti_port_setup_fpga(struct exar8250 *priv,
 	return cti_port_setup_common(priv, pcidev, idx, offset, port);
 }
 
+static void cti_board_init_xr17v35x(struct exar8250 *priv, struct pci_dev *pcidev)
+{
+	// XR17V35X uses the PCIe clock rather than an oscillator
+	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
+}
+
 static int cti_port_setup_xr17v35x(struct exar8250 *priv,
 				struct pci_dev *pcidev,
 				struct uart_8250_port *port,
@@ -962,6 +994,9 @@ static int cti_port_setup_xr17v35x(struct exar8250 *priv,
 	unsigned int offset;
 	int ret;
 
+	if (idx == 0)
+		cti_board_init_xr17v35x(priv, pcidev);
+
 	port_type = cti_get_port_type_xr17v35x(priv, pcidev, idx);
 
 	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
@@ -999,6 +1034,22 @@ static int cti_port_setup_xr17v35x(struct exar8250 *priv,
 	return 0;
 }
 
+static void cti_board_init_xr17v25x(struct exar8250 *priv, struct pci_dev *pcidev)
+{
+	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17V25X_OSC_FREQ);
+
+	/* enable interrupts on cards that need the "PLX fix" */
+	switch (pcidev->subsystem_device) {
+	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:
+		cti_plx_int_enable(priv);
+		break;
+	default:
+		break;
+	}
+}
+
 static int cti_port_setup_xr17v25x(struct exar8250 *priv,
 				struct pci_dev *pcidev,
 				struct uart_8250_port *port,
@@ -1008,6 +1059,9 @@ static int cti_port_setup_xr17v25x(struct exar8250 *priv,
 	unsigned int offset;
 	int ret;
 
+	if (idx == 0)
+		cti_board_init_xr17v25x(priv, pcidev);
+
 	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
 
 	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
@@ -1055,6 +1109,25 @@ static int cti_port_setup_xr17v25x(struct exar8250 *priv,
 	return 0;
 }
 
+static void cti_board_init_xr17c15x(struct exar8250 *priv, struct pci_dev *pcidev)
+{
+	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17C15X_OSC_FREQ);
+
+	/* enable interrupts on cards that need the "PLX fix" */
+	switch (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_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_plx_int_enable(priv);
+		break;
+	default:
+		break;
+	}
+}
+
 static int cti_port_setup_xr17c15x(struct exar8250 *priv,
 				struct pci_dev *pcidev,
 				struct uart_8250_port *port,
@@ -1063,6 +1136,9 @@ static int cti_port_setup_xr17c15x(struct exar8250 *priv,
 	enum cti_port_type port_type;
 	unsigned int offset;
 
+	if (idx == 0)
+		cti_board_init_xr17c15x(priv, pcidev);
+
 	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
 
 	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
@@ -1096,78 +1172,6 @@ static int cti_port_setup_xr17c15x(struct exar8250 *priv,
 	return cti_port_setup_common(priv, pcidev, idx, offset, port);
 }
 
-static int cti_board_init_xr17v35x(struct exar8250 *priv,
-				struct pci_dev *pcidev)
-{
-	// XR17V35X uses the PCIe clock rather than an oscillator
-	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
-
-	return 0;
-}
-
-static int cti_board_init_xr17v25x(struct exar8250 *priv, struct pci_dev *pcidev)
-{
-	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17V25X_OSC_FREQ);
-
-	/* enable interrupts on cards that need the "PLX fix" */
-	switch (pcidev->subsystem_device) {
-	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:
-		cti_plx_int_enable(priv);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
-static int cti_board_init_xr17c15x(struct exar8250 *priv, struct pci_dev *pcidev)
-{
-	cti_board_init_osc_freq(priv, pcidev, CTI_EE_OFF_XR17C15X_OSC_FREQ);
-
-	/* enable interrupts on cards that need the "PLX fix" */
-	switch (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_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_plx_int_enable(priv);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
-static int cti_board_init_fpga(struct exar8250 *priv, struct pci_dev *pcidev)
-{
-	int ret;
-	u16 cfg_val;
-
-	// 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(pcidev, CTI_FPGA_CFG_INT_EN_REG, &cfg_val);
-	if (ret)
-		return pcibios_err_to_errno(ret);
-
-	cfg_val |= CTI_FPGA_CFG_INT_EN_EXT_BIT;
-	ret = pci_write_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, cfg_val);
-	if (ret)
-		return pcibios_err_to_errno(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
 pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
@@ -1574,15 +1578,6 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	if (board->board_init) {
-		rc = board->board_init(priv, pcidev);
-		if (rc) {
-			dev_err_probe(&pcidev->dev, rc,
-					"failed to init serial board\n");
-			return rc;
-		}
-	}
-
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {
@@ -1664,22 +1659,18 @@ static const struct exar8250_board pbn_fastcom335_8 = {
 };
 
 static const struct exar8250_board pbn_cti_xr17c15x = {
-	.board_init	= cti_board_init_xr17c15x,
 	.setup		= cti_port_setup_xr17c15x,
 };
 
 static const struct exar8250_board pbn_cti_xr17v25x = {
-	.board_init	= cti_board_init_xr17v25x,
 	.setup		= cti_port_setup_xr17v25x,
 };
 
 static const struct exar8250_board pbn_cti_xr17v35x = {
-	.board_init	= cti_board_init_xr17v35x,
 	.setup		= cti_port_setup_xr17v35x,
 };
 
 static const struct exar8250_board pbn_cti_fpga = {
-	.board_init	= cti_board_init_fpga,
 	.setup		= cti_port_setup_fpga,
 };
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 08/13] serial: 8250_exar: Decrease indentation level

[Andy Shevchenko]

Decrease indentation level in some places.
No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 100 ++++++++++++++--------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 4e683ca76de0..1f04d4562878 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -607,28 +607,30 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	writeb(32, p + UART_EXAR_TXTRG);
 	writeb(32, p + UART_EXAR_RXTRG);
 
+	/* Skip the initial (per device) setup */
+	if (idx)
+		return 0;
+
 	/*
 	 * Setup Multipurpose Input/Output pins.
 	 */
-	if (idx == 0) {
-		switch (pcidev->device) {
-		case PCI_DEVICE_ID_COMMTECH_4222PCI335:
-		case PCI_DEVICE_ID_COMMTECH_4224PCI335:
-			writeb(0x78, p + UART_EXAR_MPIOLVL_7_0);
-			writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
-			writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
-			break;
-		case PCI_DEVICE_ID_COMMTECH_2324PCI335:
-		case PCI_DEVICE_ID_COMMTECH_2328PCI335:
-			writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
-			writeb(0xc0, p + UART_EXAR_MPIOINV_7_0);
-			writeb(0xc0, p + UART_EXAR_MPIOSEL_7_0);
-			break;
-		}
-		writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
-		writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
-		writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
+	switch (pcidev->device) {
+	case PCI_DEVICE_ID_COMMTECH_4222PCI335:
+	case PCI_DEVICE_ID_COMMTECH_4224PCI335:
+		writeb(0x78, p + UART_EXAR_MPIOLVL_7_0);
+		writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
+		writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
+		break;
+	case PCI_DEVICE_ID_COMMTECH_2324PCI335:
+	case PCI_DEVICE_ID_COMMTECH_2328PCI335:
+		writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
+		writeb(0xc0, p + UART_EXAR_MPIOINV_7_0);
+		writeb(0xc0, p + UART_EXAR_MPIOSEL_7_0);
+		break;
 	}
+	writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
+	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
+	writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
 
 	return 0;
 }
@@ -853,21 +855,19 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
 	port_flags = exar_ee_read(priv, offset);
 
 	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
-	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.
-		 */
-		dev_warn(&pcidev->dev,
-			"failed to get port %d type from EEPROM\n", port_num);
-		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
-	}
+	if (CTI_PORT_TYPE_VALID(port_type))
+		return port_type;
 
-	return 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.
+	 */
+	dev_warn(&pcidev->dev, "failed to get port %d type from EEPROM\n", port_num);
+
+	return CTI_PORT_TYPE_RS232_422_485_HW;
 }
 
 static int cti_rs485_config_mpio_tristate(struct uart_port *port,
@@ -1190,11 +1190,10 @@ static void setup_gpio(struct pci_dev *pcidev, u8 __iomem *p)
 	 * devices will export them as GPIOs, so we pre-configure them safely
 	 * as inputs.
 	 */
-
 	u8 dir = 0x00;
 
 	if  ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
-		(pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL)) {
+	     (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL)) {
 		// Configure GPIO as inputs for Commtech adapters
 		dir = 0xff;
 	} else {
@@ -1284,27 +1283,28 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
 	if (ret)
 		return ret;
 
-	if (rs485->flags & SER_RS485_ENABLED) {
-		old_lcr = readb(p + UART_LCR);
+	if (!(rs485->flags & SER_RS485_ENABLED))
+		return 0;
 
-		/* Set EFR[4]=1 to enable enhanced feature registers */
-		efr = readb(p + UART_XR_EFR);
-		efr |= UART_EFR_ECB;
-		writeb(efr, p + UART_XR_EFR);
+	old_lcr = readb(p + UART_LCR);
 
-		/* Set MCR to use DTR as Auto-RS485 Enable signal */
-		writeb(UART_MCR_OUT1, p + UART_MCR);
+	/* Set EFR[4]=1 to enable enhanced feature registers */
+	efr = readb(p + UART_XR_EFR);
+	efr |= UART_EFR_ECB;
+	writeb(efr, p + UART_XR_EFR);
 
-		/* Set LCR[7]=1 to enable access to DLD register */
-		writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
+	/* Set MCR to use DTR as Auto-RS485 Enable signal */
+	writeb(UART_MCR_OUT1, p + UART_MCR);
 
-		/* Set DLD[7]=1 for inverted RS485 Enable logic */
-		dld = readb(p + UART_EXAR_DLD);
-		dld |= UART_EXAR_DLD_485_POLARITY;
-		writeb(dld, p + UART_EXAR_DLD);
+	/* Set LCR[7]=1 to enable access to DLD register */
+	writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
 
-		writeb(old_lcr, p + UART_LCR);
-	}
+	/* Set DLD[7]=1 for inverted RS485 Enable logic */
+	dld = readb(p + UART_EXAR_DLD);
+	dld |= UART_EXAR_DLD_485_POLARITY;
+	writeb(dld, p + UART_EXAR_DLD);
+
+	writeb(old_lcr, p + UART_LCR);
 
 	return 0;
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 09/13] serial: 8250_exar: Return directly from switch-cases

[Andy Shevchenko]

There are two similar switch-cases which use different style.
Unify them and one more to return from the cases directly.

While at it, add missing default in another switch-case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 80 +++++++++++------------------
 1 file changed, 29 insertions(+), 51 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 1f04d4562878..51f6af16c557 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -627,6 +627,8 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		writeb(0xc0, p + UART_EXAR_MPIOINV_7_0);
 		writeb(0xc0, p + UART_EXAR_MPIOSEL_7_0);
 		break;
+	default:
+		break;
 	}
 	writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
 	writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
@@ -723,8 +725,6 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 							struct pci_dev *pcidev,
 							unsigned int port_num)
 {
-	enum cti_port_type port_type;
-
 	switch (pcidev->subsystem_device) {
 	// RS232 only cards
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
@@ -734,24 +734,17 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 	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;
+		return CTI_PORT_TYPE_RS232;
 	// 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;
+		return (port_num == 0) ? CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 	// 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;
+		return (port_num < 2) ? CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 	// 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;
+		return (port_num < 4) ? CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 	// RS232/RS422/RS485 HW (jumper) selectable
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
@@ -774,32 +767,24 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 	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;
+		return CTI_PORT_TYPE_RS232_422_485_HW;
 	// 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;
+		return CTI_PORT_TYPE_RS422_485;
 	// 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;
+		return (port_num < 6) ? CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 	// 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;
+		return (port_num < 2) ? CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 	default:
 		dev_err(&pcidev->dev, "unknown/unsupported device\n");
-		port_type = CTI_PORT_TYPE_NONE;
+		return CTI_PORT_TYPE_NONE;
 	}
-
-	return port_type;
 }
 
 /**
@@ -816,20 +801,15 @@ static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
 						struct pci_dev *pcidev,
 						unsigned int port_num)
 {
-	enum cti_port_type port_type;
-
 	switch (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;
+		return CTI_PORT_TYPE_RS232_422_485_HW;
 	default:
 		dev_err(&pcidev->dev, "unknown/unsupported device\n");
 		return CTI_PORT_TYPE_NONE;
 	}
-
-	return port_type;
 }
 
 /**
@@ -1493,35 +1473,33 @@ 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)
+static unsigned int exar_get_nr_ports(struct exar8250_board *board, struct pci_dev *pcidev)
 {
-	unsigned int nr_ports = 0;
+	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
+		return BIT(((pcidev->device & 0x38) >> 3) - 1);
 
-	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
+	// Check if board struct overrides number of ports
+	if (board->num_ports > 0)
+		return board->num_ports;
+
+	// Exar encodes # ports in last nibble of PCI Device ID ex. 0358
+	if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
+		return pcidev->device & 0x0f;
+
+	// Handle CTI FPGA cards
+	if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
 		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;
+			return 12;
 		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
-			nr_ports = 16;
-			break;
+			return 16;
 		default:
-			break;
+			return 0;
 		}
 	}
 
-	return nr_ports;
+	return 0;
 }
 
 static int
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 10/13] serial: 8250_exar: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 51f6af16c557..306bc6d7c141 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -889,11 +889,8 @@ static int cti_port_setup_common(struct exar8250 *priv,
 	port->port.uartclk = priv->osc_freq;
 
 	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0);
-	if (ret) {
-		dev_err(&pcidev->dev,
-			"failed to setup pci for port %d err: %d\n", idx, ret);
+	if (ret)
 		return ret;
-	}
 
 	port->port.private_data = (void *)priv;
 	port->port.pm = exar_pm;
@@ -1522,11 +1519,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
 
 	nr_ports = exar_get_nr_ports(board, pcidev);
-	if (nr_ports == 0) {
-		dev_err_probe(&pcidev->dev, -ENODEV,
-				"failed to get number of ports\n");
-		return -ENODEV;
-	}
+	if (nr_ports == 0)
+		return dev_err_probe(&pcidev->dev, -ENODEV, "failed to get number of ports\n");
 
 	priv = devm_kzalloc(&pcidev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
 	if (!priv)
@@ -1559,7 +1553,7 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {
-			dev_err(&pcidev->dev, "Failed to setup port %u\n", i);
+			dev_err_probe(&pcidev->dev, rc, "Failed to setup port %u\n", i);
 			break;
 		}
 
@@ -1568,10 +1562,9 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 
 		priv->line[i] = serial8250_register_8250_port(&uart);
 		if (priv->line[i] < 0) {
-			dev_err(&pcidev->dev,
-				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
-				uart.port.iobase, uart.port.irq,
-				uart.port.iotype, priv->line[i]);
+			dev_err_probe(&pcidev->dev, priv->line[i],
+				"Couldn't register serial port %lx, type %d, irq %d\n",
+				uart.port.iobase, uart.port.iotype, uart.port.irq);
 			break;
 		}
 	}
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 11/13] serial: 8250_exar: Use BIT() in exar_ee_read()

[Andy Shevchenko]

Use BIT() in exar_ee_read() like other functions do.

While at it, explain the EEPROM type and the dummy bit requirement.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 306bc6d7c141..93d2c8dd3cd8 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -324,6 +324,7 @@ static inline u8 exar_ee_read_bit(struct exar8250 *priv)
  * @ee_addr: Offset of EEPROM to read word from
  *
  * Read a single 16bit word from an Exar UART's EEPROM.
+ * The type of the EEPROM is AT93C46D.
  *
  * Return: EEPROM word
  */
@@ -340,13 +341,13 @@ static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
 	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));
+	for (i = UART_EXAR_REGB_EE_ADDR_SIZE - 1; i >= 0; i--)
+		exar_ee_write_bit(priv, ee_addr & BIT(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);
+	// Read data 1 bit at a time starting with a dummy bit
+	for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
+		if (exar_ee_read_bit(priv))
+			data |= BIT(i);
 	}
 
 	exar_ee_deselect(priv);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 12/13] serial: 8250_exar: Make type of bit the same in exar_ee_*_bit()

[Andy Shevchenko]

Make type of bit parameter and returned one the same in exar_ee_*_bit().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 93d2c8dd3cd8..85085b73706c 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -280,7 +280,7 @@ static inline void exar_ee_deselect(struct exar8250 *priv)
 	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
 }
 
-static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
+static inline void exar_ee_write_bit(struct exar8250 *priv, u8 bit)
 {
 	u8 value = UART_EXAR_REGB_EECS;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 13/13] serial: 8250_exar: Keep the includes sorted

[Andy Shevchenko]

Keep the includes sorted.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_exar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 85085b73706c..616128254bbd 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -6,6 +6,7 @@
  *
  *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
  */
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -20,7 +21,6 @@
 #include <linux/property.h>
 #include <linux/string.h>
 #include <linux/types.h>
-#include <linux/bitfield.h>
 
 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Fri,  3 May 2024 20:15:52 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> After a rework for CONNTECH was done, the driver may need a bit of
> love in order to become less verbose (in terms of indentation and
> code duplication) and hence easier to read.
>
> This clean up series fixes a couple of (not so critical) issues and
> cleans up the recently added code. No functional change indented by
> the cleaning up part.
>
> Parker, please test this and give your formal Tested-by tag
> (you may do it by replying to this message if all patches are
>  successfully tested; more details about tags are available in
>  the Submitting Patches documentation).
>

I was able to test the Connect Tech related code and everything is
work as expected. I can't test the non-CTI related changes but they
are pretty minor.

Tested-by: Parker Newman <pnewman@connecttech.com>

> In v2:
> - fixed the EEPROM reading data loop (Ilpo, Parker)
>
> Andy Shevchenko (13):
>   serial: 8250_exar: Don't return positive values as error codes
>   serial: 8250_exar: Describe all parameters in kernel doc
>   serial: 8250_exar: Kill CTI_PCI_DEVICE()
>   serial: 8250_exar: Use PCI_SUBVENDOR_ID_IBM for subvendor ID
>   serial: 8250_exar: Trivia typo fixes
>   serial: 8250_exar: Extract cti_board_init_osc_freq() helper
>   serial: 8250_exar: Kill unneeded ->board_init()
>   serial: 8250_exar: Decrease indentation level
>   serial: 8250_exar: Return directly from switch-cases
>   serial: 8250_exar: Switch to use dev_err_probe()
>   serial: 8250_exar: Use BIT() in exar_ee_read()
>   serial: 8250_exar: Make type of bit the same in exar_ee_*_bit()
>   serial: 8250_exar: Keep the includes sorted
>
>  drivers/tty/serial/8250/8250_exar.c | 459 ++++++++++++----------------
>  1 file changed, 203 insertions(+), 256 deletions(-)
>

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> On Fri,  3 May 2024 20:15:52 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > After a rework for CONNTECH was done, the driver may need a bit of
> > love in order to become less verbose (in terms of indentation and
> > code duplication) and hence easier to read.
> >
> > This clean up series fixes a couple of (not so critical) issues and
> > cleans up the recently added code. No functional change indented by
> > the cleaning up part.
> >
> > Parker, please test this and give your formal Tested-by tag
> > (you may do it by replying to this message if all patches are
> >  successfully tested; more details about tags are available in
> >  the Submitting Patches documentation).
> 
> I was able to test the Connect Tech related code and everything is
> work as expected. I can't test the non-CTI related changes but they
> are pretty minor.
> 
> Tested-by: Parker Newman <pnewman@connecttech.com>

Sorry for blast from the past, but I have some instersting information
for you. We now have spi-gpio and 93c46 eeprom drivers available to be
used from others via software nodes, can you consider updating your code
to replace custom bitbanging along with r/w ops by the instantiating the
respective drivers?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Fri, 6 Sep 2024 15:46:51 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > On Fri,  3 May 2024 20:15:52 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > After a rework for CONNTECH was done, the driver may need a bit of
> > > love in order to become less verbose (in terms of indentation and
> > > code duplication) and hence easier to read.
> > >
> > > This clean up series fixes a couple of (not so critical) issues and
> > > cleans up the recently added code. No functional change indented by
> > > the cleaning up part.
> > >
> > > Parker, please test this and give your formal Tested-by tag
> > > (you may do it by replying to this message if all patches are
> > >  successfully tested; more details about tags are available in
> > >  the Submitting Patches documentation).
> >
> > I was able to test the Connect Tech related code and everything is
> > work as expected. I can't test the non-CTI related changes but they
> > are pretty minor.
> >
> > Tested-by: Parker Newman <pnewman@connecttech.com>
>
> Sorry for blast from the past, but I have some instersting information
> for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> used from others via software nodes, can you consider updating your code
> to replace custom bitbanging along with r/w ops by the instantiating the
> respective drivers?
>

Hi Andy,
The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
They have a dedicated "EEPROM interface" which is accessed by the
REGB (0x8E) register. It is a very simple bit-bang interface though,
one bit per signal.

I guess in theory I could either add  GPIO wrapper to toggle these bits
and use the spi-gpio driver but I am not sure if that really improves things?
Maybe using the spi-bitbang driver directly is more appropriate?
What do you think?

Thanks,
Parker

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> On Fri, 6 Sep 2024 15:46:51 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > > On Fri,  3 May 2024 20:15:52 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > > After a rework for CONNTECH was done, the driver may need a bit of
> > > > love in order to become less verbose (in terms of indentation and
> > > > code duplication) and hence easier to read.
> > > >
> > > > This clean up series fixes a couple of (not so critical) issues and
> > > > cleans up the recently added code. No functional change indented by
> > > > the cleaning up part.
> > > >
> > > > Parker, please test this and give your formal Tested-by tag
> > > > (you may do it by replying to this message if all patches are
> > > >  successfully tested; more details about tags are available in
> > > >  the Submitting Patches documentation).
> > >
> > > I was able to test the Connect Tech related code and everything is
> > > work as expected. I can't test the non-CTI related changes but they
> > > are pretty minor.
> > >
> > > Tested-by: Parker Newman <pnewman@connecttech.com>
> >
> > Sorry for blast from the past, but I have some instersting information
> > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > used from others via software nodes, can you consider updating your code
> > to replace custom bitbanging along with r/w ops by the instantiating the
> > respective drivers?
> 
> Hi Andy,
> The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> They have a dedicated "EEPROM interface" which is accessed by the
> REGB (0x8E) register. It is a very simple bit-bang interface though,
> one bit per signal.
> 
> I guess in theory I could either add  GPIO wrapper to toggle these bits
> and use the spi-gpio driver but I am not sure if that really improves things?
> Maybe using the spi-bitbang driver directly is more appropriate?
> What do you think?

Yes, spi-bitbang seems better in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Fri, 6 Sep 2024 17:24:44 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > On Fri, 6 Sep 2024 15:46:51 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > > > On Fri,  3 May 2024 20:15:52 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > > After a rework for CONNTECH was done, the driver may need a bit of
> > > > > love in order to become less verbose (in terms of indentation and
> > > > > code duplication) and hence easier to read.
> > > > >
> > > > > This clean up series fixes a couple of (not so critical) issues and
> > > > > cleans up the recently added code. No functional change indented by
> > > > > the cleaning up part.
> > > > >
> > > > > Parker, please test this and give your formal Tested-by tag
> > > > > (you may do it by replying to this message if all patches are
> > > > >  successfully tested; more details about tags are available in
> > > > >  the Submitting Patches documentation).
> > > >
> > > > I was able to test the Connect Tech related code and everything is
> > > > work as expected. I can't test the non-CTI related changes but they
> > > > are pretty minor.
> > > >
> > > > Tested-by: Parker Newman <pnewman@connecttech.com>
> > >
> > > Sorry for blast from the past, but I have some instersting information
> > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > used from others via software nodes, can you consider updating your code
> > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > respective drivers?
> >
> > Hi Andy,
> > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > They have a dedicated "EEPROM interface" which is accessed by the
> > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > one bit per signal.
> >
> > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > and use the spi-gpio driver but I am not sure if that really improves things?
> > Maybe using the spi-bitbang driver directly is more appropriate?
> > What do you think?
>
> Yes, spi-bitbang seems better in this case.
>

I will try to make some time to implement this... Or if someone else from the
community wants to take this on in the mean time I am certainly happy to test
and help out!
Parker

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> On Fri, 6 Sep 2024 17:24:44 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:

...

> > > > Sorry for blast from the past, but I have some instersting information
> > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > used from others via software nodes, can you consider updating your code
> > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > respective drivers?
> > >
> > > Hi Andy,
> > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > They have a dedicated "EEPROM interface" which is accessed by the
> > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > one bit per signal.
> > >
> > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > What do you think?
> >
> > Yes, spi-bitbang seems better in this case.
> 
> I will try to make some time to implement this... Or if someone else from the
> community wants to take this on in the mean time I am certainly happy to test
> and help out!

Sure, I shared this thought due to having lack of time to look myself,
but I prepared the above mentioned drivers to make them work in this case.
(If you are curios, see the Git history for the last few releases with
 --author="Andy Shevchenko")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Fri, 6 Sep 2024 17:42:26 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > On Fri, 6 Sep 2024 17:24:44 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
>
> ...
>
> > > > > Sorry for blast from the past, but I have some instersting information
> > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > used from others via software nodes, can you consider updating your code
> > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > respective drivers?
> > > >
> > > > Hi Andy,
> > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > one bit per signal.
> > > >
> > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > What do you think?
> > >
> > > Yes, spi-bitbang seems better in this case.
> >
> > I will try to make some time to implement this... Or if someone else from the
> > community wants to take this on in the mean time I am certainly happy to test
> > and help out!
>
> Sure, I shared this thought due to having lack of time to look myself,
> but I prepared the above mentioned drivers to make them work in this case.
> (If you are curios, see the Git history for the last few releases with
>  --author="Andy Shevchenko")
>

Looking into it a bit more I think we could just use the eeprom_93cx6
driver without any SPI layer. Just need to add simple register_read()
and register_write() functions to read/write the REB register.

That should be a pretty easy change to make, I can try to make that
change soon unless anyone has any objections to that method?

Thanks,
Parker

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> On Fri, 6 Sep 2024 17:42:26 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:

...

> > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > used from others via software nodes, can you consider updating your code
> > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > respective drivers?
> > > > >
> > > > > Hi Andy,
> > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > one bit per signal.
> > > > >
> > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > What do you think?
> > > >
> > > > Yes, spi-bitbang seems better in this case.
> > >
> > > I will try to make some time to implement this... Or if someone else from the
> > > community wants to take this on in the mean time I am certainly happy to test
> > > and help out!
> >
> > Sure, I shared this thought due to having lack of time to look myself,
> > but I prepared the above mentioned drivers to make them work in this case.
> > (If you are curios, see the Git history for the last few releases with
> >  --author="Andy Shevchenko")
> >
> 
> Looking into it a bit more I think we could just use the eeprom_93cx6
> driver without any SPI layer. Just need to add simple register_read()
> and register_write() functions to read/write the REB register.
> 
> That should be a pretty easy change to make, I can try to make that
> change soon unless anyone has any objections to that method?

Thank you, this is pretty wonderful news!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Mon, 9 Sep 2024 13:06:26 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> > On Fri, 6 Sep 2024 17:42:26 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
>
> ...
>
> > > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > > used from others via software nodes, can you consider updating your code
> > > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > > respective drivers?
> > > > > >
> > > > > > Hi Andy,
> > > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > > one bit per signal.
> > > > > >
> > > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > > What do you think?
> > > > >
> > > > > Yes, spi-bitbang seems better in this case.
> > > >
> > > > I will try to make some time to implement this... Or if someone else from the
> > > > community wants to take this on in the mean time I am certainly happy to test
> > > > and help out!
> > >
> > > Sure, I shared this thought due to having lack of time to look myself,
> > > but I prepared the above mentioned drivers to make them work in this case.
> > > (If you are curios, see the Git history for the last few releases with
> > >  --author="Andy Shevchenko")
> > >
> >
> > Looking into it a bit more I think we could just use the eeprom_93cx6
> > driver without any SPI layer. Just need to add simple register_read()
> > and register_write() functions to read/write the REB register.
> >
> > That should be a pretty easy change to make, I can try to make that
> > change soon unless anyone has any objections to that method?
>
> Thank you, this is pretty wonderful news!
>

I have this mostly working however there is one issue. The eeprom_93cx6
driver doesn't seem to discard the "dummy bit" the 93C46 EEPROM outputs
between the writing of the op-code/address to the EEPROM and the reading
of the data from the EEPROM.

More info can be found on page 6 of the AT93C46 datasheet. I see similar
notes in other 93C46/93C56/93C66 datasheets.
Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

In summary the read operation for the AT93C46 EEPROM is:
Write to EEPROM :	110[A5-A0]	(9 bits)
Read from EEPROM: 	0[D15-D0]	(17 bits)

Where 110 is the READ OpCode, [A5-A0] is the address to read from,
0 is a "dummy bit" and then [D15-D0] is the actual data.

I am seeing the "correct" values being read from the EEPROM when using the
eeprom_93cx6 driver but they are all shifted right by one because the
dummy 0 bit is not being discarded.

The confusing part is the eeprom_93cx6 driver has behaved the same since
at least 2009 and half a dozen or so other drivers use it. I am not sure
if they just work around and/or live with this bug or if they have
different HW that handles the extra dummy bit?

I am hesitant to "fix" the eeprom_93cx6 driver and potentially break the
other users of it. I could add a flag to the eeprom_93cx6 struct to work
around this issue... Unless anyone else has some ideas or input?

Thanks,
Parker

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 01:38:48PM -0400, Parker Newman wrote:
> On Mon, 9 Sep 2024 13:06:26 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> > > On Fri, 6 Sep 2024 17:42:26 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:

...

> > > > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > > > used from others via software nodes, can you consider updating your code
> > > > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > > > respective drivers?
> > > > > > >
> > > > > > > Hi Andy,
> > > > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > > > one bit per signal.
> > > > > > >
> > > > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > > > What do you think?
> > > > > >
> > > > > > Yes, spi-bitbang seems better in this case.
> > > > >
> > > > > I will try to make some time to implement this... Or if someone else from the
> > > > > community wants to take this on in the mean time I am certainly happy to test
> > > > > and help out!
> > > >
> > > > Sure, I shared this thought due to having lack of time to look myself,
> > > > but I prepared the above mentioned drivers to make them work in this case.
> > > > (If you are curios, see the Git history for the last few releases with
> > > >  --author="Andy Shevchenko")
> > > >
> > >
> > > Looking into it a bit more I think we could just use the eeprom_93cx6
> > > driver without any SPI layer. Just need to add simple register_read()
> > > and register_write() functions to read/write the REB register.
> > >
> > > That should be a pretty easy change to make, I can try to make that
> > > change soon unless anyone has any objections to that method?
> >
> > Thank you, this is pretty wonderful news!
> >
> 
> I have this mostly working however there is one issue. The eeprom_93cx6
> driver doesn't seem to discard the "dummy bit" the 93C46 EEPROM outputs
> between the writing of the op-code/address to the EEPROM and the reading
> of the data from the EEPROM.
> 
> More info can be found on page 6 of the AT93C46 datasheet. I see similar
> notes in other 93C46/93C56/93C66 datasheets.
> Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
> 
> In summary the read operation for the AT93C46 EEPROM is:
> Write to EEPROM :	110[A5-A0]	(9 bits)
> Read from EEPROM: 	0[D15-D0]	(17 bits)
> 
> Where 110 is the READ OpCode, [A5-A0] is the address to read from,
> 0 is a "dummy bit" and then [D15-D0] is the actual data.
> 
> I am seeing the "correct" values being read from the EEPROM when using the
> eeprom_93cx6 driver but they are all shifted right by one because the
> dummy 0 bit is not being discarded.
> 
> The confusing part is the eeprom_93cx6 driver has behaved the same since
> at least 2009 and half a dozen or so other drivers use it. I am not sure
> if they just work around and/or live with this bug or if they have
> different HW that handles the extra dummy bit?

I briefly looked at a few users and it seems to me:
1) either the Atmel chip has different HW protocol;
2) or all of them handle that in HW transparently to SW.

> I am hesitant to "fix" the eeprom_93cx6 driver and potentially break the
> other users of it. I could add a flag to the eeprom_93cx6 struct to work
> around this issue... Unless anyone else has some ideas or input?

In my opinion the 93c46 needs an additional configuration setting (in the
respective data structure) and some code to implement what you need here.

But yes, let's wait a bit for other opinions...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Wed, 11 Sep 2024 23:51:09 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Sep 11, 2024 at 01:38:48PM -0400, Parker Newman wrote:
> > On Mon, 9 Sep 2024 13:06:26 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> > > > On Fri, 6 Sep 2024 17:42:26 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > > > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
>
> ...
>
> > > > > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > > > > used from others via software nodes, can you consider updating your code
> > > > > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > > > > respective drivers?
> > > > > > > >
> > > > > > > > Hi Andy,
> > > > > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > > > > one bit per signal.
> > > > > > > >
> > > > > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > > > > What do you think?
> > > > > > >
> > > > > > > Yes, spi-bitbang seems better in this case.
> > > > > >
> > > > > > I will try to make some time to implement this... Or if someone else from the
> > > > > > community wants to take this on in the mean time I am certainly happy to test
> > > > > > and help out!
> > > > >
> > > > > Sure, I shared this thought due to having lack of time to look myself,
> > > > > but I prepared the above mentioned drivers to make them work in this case.
> > > > > (If you are curios, see the Git history for the last few releases with
> > > > >  --author="Andy Shevchenko")
> > > > >
> > > >
> > > > Looking into it a bit more I think we could just use the eeprom_93cx6
> > > > driver without any SPI layer. Just need to add simple register_read()
> > > > and register_write() functions to read/write the REB register.
> > > >
> > > > That should be a pretty easy change to make, I can try to make that
> > > > change soon unless anyone has any objections to that method?
> > >
> > > Thank you, this is pretty wonderful news!
> > >
> >
> > I have this mostly working however there is one issue. The eeprom_93cx6
> > driver doesn't seem to discard the "dummy bit" the 93C46 EEPROM outputs
> > between the writing of the op-code/address to the EEPROM and the reading
> > of the data from the EEPROM.
> >
> > More info can be found on page 6 of the AT93C46 datasheet. I see similar
> > notes in other 93C46/93C56/93C66 datasheets.
> > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
> >
> > In summary the read operation for the AT93C46 EEPROM is:
> > Write to EEPROM :	110[A5-A0]	(9 bits)
> > Read from EEPROM: 	0[D15-D0]	(17 bits)
> >
> > Where 110 is the READ OpCode, [A5-A0] is the address to read from,
> > 0 is a "dummy bit" and then [D15-D0] is the actual data.
> >
> > I am seeing the "correct" values being read from the EEPROM when using the
> > eeprom_93cx6 driver but they are all shifted right by one because the
> > dummy 0 bit is not being discarded.
> >
> > The confusing part is the eeprom_93cx6 driver has behaved the same since
> > at least 2009 and half a dozen or so other drivers use it. I am not sure
> > if they just work around and/or live with this bug or if they have
> > different HW that handles the extra dummy bit?
>
> I briefly looked at a few users and it seems to me:
> 1) either the Atmel chip has different HW protocol;
> 2) or all of them handle that in HW transparently to SW.

The 3 Exar cards I have handy actually use the ST M93C46 version but looking
through our BOMs I see AT/CAT/ST used on various cards over the years.

Looking at the READ timing diagrams in the Atmel and ST datasheets it looks
like the dummy bit should actually be clocked out on the last address bit
clock cycle. If this were so it would be ignored naturally.

This may just be a quirk of the Exar HW. All Exar code I have looked at
manually discards the dummy bit.

> > I am hesitant to "fix" the eeprom_93cx6 driver and potentially break the
> > other users of it. I could add a flag to the eeprom_93cx6 struct to work
> > around this issue... Unless anyone else has some ideas or input?
>
> In my opinion the 93c46 needs an additional configuration setting (in the
> respective data structure) and some code to implement what you need here.

I see the eeprom_93xx46 driver has the QUIRK_EXTRA_READ_CYCLE quirk to solve
this issue. I could add something similar.

> But yes, let's wait a bit for other opinions...
>

Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Andy Shevchenko]

On Thu, Sep 12, 2024 at 08:41:47AM -0400, Parker Newman wrote:
> On Wed, 11 Sep 2024 23:51:09 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Sep 11, 2024 at 01:38:48PM -0400, Parker Newman wrote:
> > > On Mon, 9 Sep 2024 13:06:26 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> > > > > On Fri, 6 Sep 2024 17:42:26 +0300
> > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > > > > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:

...

> > > > > > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > > > > > used from others via software nodes, can you consider updating your code
> > > > > > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > > > > > respective drivers?
> > > > > > > > >
> > > > > > > > > Hi Andy,
> > > > > > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > > > > > one bit per signal.
> > > > > > > > >
> > > > > > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > > > > > What do you think?
> > > > > > > >
> > > > > > > > Yes, spi-bitbang seems better in this case.
> > > > > > >
> > > > > > > I will try to make some time to implement this... Or if someone else from the
> > > > > > > community wants to take this on in the mean time I am certainly happy to test
> > > > > > > and help out!
> > > > > >
> > > > > > Sure, I shared this thought due to having lack of time to look myself,
> > > > > > but I prepared the above mentioned drivers to make them work in this case.
> > > > > > (If you are curios, see the Git history for the last few releases with
> > > > > >  --author="Andy Shevchenko")
> > > > > >
> > > > >
> > > > > Looking into it a bit more I think we could just use the eeprom_93cx6
> > > > > driver without any SPI layer. Just need to add simple register_read()
> > > > > and register_write() functions to read/write the REB register.
> > > > >
> > > > > That should be a pretty easy change to make, I can try to make that
> > > > > change soon unless anyone has any objections to that method?
> > > >
> > > > Thank you, this is pretty wonderful news!
> > > >
> > >
> > > I have this mostly working however there is one issue. The eeprom_93cx6
> > > driver doesn't seem to discard the "dummy bit" the 93C46 EEPROM outputs
> > > between the writing of the op-code/address to the EEPROM and the reading
> > > of the data from the EEPROM.
> > >
> > > More info can be found on page 6 of the AT93C46 datasheet. I see similar
> > > notes in other 93C46/93C56/93C66 datasheets.
> > > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
> > >
> > > In summary the read operation for the AT93C46 EEPROM is:
> > > Write to EEPROM :	110[A5-A0]	(9 bits)
> > > Read from EEPROM: 	0[D15-D0]	(17 bits)
> > >
> > > Where 110 is the READ OpCode, [A5-A0] is the address to read from,
> > > 0 is a "dummy bit" and then [D15-D0] is the actual data.
> > >
> > > I am seeing the "correct" values being read from the EEPROM when using the
> > > eeprom_93cx6 driver but they are all shifted right by one because the
> > > dummy 0 bit is not being discarded.
> > >
> > > The confusing part is the eeprom_93cx6 driver has behaved the same since
> > > at least 2009 and half a dozen or so other drivers use it. I am not sure
> > > if they just work around and/or live with this bug or if they have
> > > different HW that handles the extra dummy bit?
> >
> > I briefly looked at a few users and it seems to me:
> > 1) either the Atmel chip has different HW protocol;
> > 2) or all of them handle that in HW transparently to SW.
> 
> The 3 Exar cards I have handy actually use the ST M93C46 version but looking
> through our BOMs I see AT/CAT/ST used on various cards over the years.
> 
> Looking at the READ timing diagrams in the Atmel and ST datasheets it looks
> like the dummy bit should actually be clocked out on the last address bit
> clock cycle. If this were so it would be ignored naturally.
> 
> This may just be a quirk of the Exar HW. All Exar code I have looked at
> manually discards the dummy bit.
> 
> > > I am hesitant to "fix" the eeprom_93cx6 driver and potentially break the
> > > other users of it. I could add a flag to the eeprom_93cx6 struct to work
> > > around this issue... Unless anyone else has some ideas or input?
> >
> > In my opinion the 93c46 needs an additional configuration setting (in the
> > respective data structure) and some code to implement what you need here.
> 
> I see the eeprom_93xx46 driver has the QUIRK_EXTRA_READ_CYCLE quirk to solve
> this issue. I could add something similar.

Seems like a good plan to me, and thanks again for looking into this!

> > But yes, let's wait a bit for other opinions...

-- 
With Best Regards,
Andy Shevchenko