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.
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 | 454 ++++++++++++----------------
1 file changed, 200 insertions(+), 254 deletions(-)
--
2.43.0.rc1.1336.g36b5255a03ac
Use BIT() in exar_ee_read() like other functions do.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 306bc6d7c141..bf3730f4231d 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -340,13 +340,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);
+ if (exar_ee_read_bit(priv))
+ data |= BIT(i);
}
exar_ee_deselect(priv);
--
2.43.0.rc1.1336.g36b5255a03ac
The CTI_PCI_DEVICE() duplicates EXAR_DEVICE(). Kill the former.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Use PCI_SUBVENDOR_ID_IBM for subvendor ID instead of vendor one.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Make type of bit parameter and returned one the same in exar_ee_*_bit().
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 bf3730f4231d..6764aaa20df2 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
Trivia typo fixes: interupts --> interrupts and others.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
---
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
Decrease indentation level in some places.
No functional change intended.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Switch to use dev_err_probe() to simplify the error path and
unify a message template.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
On Thu, 2 May 2024 17:43:57 +0300
Andy Shevchenko <[email protected]> wrote:
> The CTI_PCI_DEVICE() duplicates EXAR_DEVICE(). Kill the former.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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) }
>
This is not correct. The CTI_PCI_DEVICE() macro is for cards that have the
Connect Tech PCI Vendor ID (not Sub-Vendor ID). EXAR_DEVICE() is for cards with
Exar PCI Vendor ID.
Thanks,
Parker
> #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),
>
On Thu, May 02, 2024 at 11:13:14AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 17:43:57 +0300
> Andy Shevchenko <[email protected]> wrote:
> > The CTI_PCI_DEVICE() duplicates EXAR_DEVICE(). Kill the former.
..
> > -// 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, \
#define PCI_DEVICE_SUB(vend, dev, subvend, subdev) \
.vendor = (vend), .device = (dev), \
.subvendor = (subvend), .subdevice = (subdev)
#define PCI_DEVICE_DATA(vend, dev, data) \
.vendor = PCI_VENDOR_ID_##vend, .device = PCI_DEVICE_ID_##vend##_##dev, \
.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0, \
.driver_data = (kernel_ulong_t)(data)
> > - PCI_ANY_ID, \
> > - PCI_ANY_ID), 0, 0, \
> > - (kernel_ulong_t)&bd \
> > - }
> > -
> > #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }
> This is not correct. The CTI_PCI_DEVICE() macro is for cards that have the
> Connect Tech PCI Vendor ID (not Sub-Vendor ID). EXAR_DEVICE() is for cards with
> Exar PCI Vendor ID.
Above I added current code of these macros, can you elaborate how it's incorrect?
--
With Best Regards,
Andy Shevchenko
On Thu, 2 May 2024 18:29:05 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 11:13:14AM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 17:43:57 +0300
> > Andy Shevchenko <[email protected]> wrote:
>
> > > The CTI_PCI_DEVICE() duplicates EXAR_DEVICE(). Kill the former.
>
> ...
>
> > > -// 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, \
>
>
> #define PCI_DEVICE_SUB(vend, dev, subvend, subdev) \
> .vendor = (vend), .device = (dev), \
> .subvendor = (subvend), .subdevice = (subdev)
>
> #define PCI_DEVICE_DATA(vend, dev, data) \
> .vendor = PCI_VENDOR_ID_##vend, .device = PCI_DEVICE_ID_##vend##_##dev, \
> .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0, \
> .driver_data = (kernel_ulong_t)(data)
>
>
> > > - PCI_ANY_ID, \
> > > - PCI_ANY_ID), 0, 0, \
> > > - (kernel_ulong_t)&bd \
> > > - }
> > > -
> > > #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }
>
> > This is not correct. The CTI_PCI_DEVICE() macro is for cards that have the
> > Connect Tech PCI Vendor ID (not Sub-Vendor ID). EXAR_DEVICE() is for cards with
> > Exar PCI Vendor ID.
>
> Above I added current code of these macros, can you elaborate how it's incorrect?
>
Sorry, you are correct. I was mixed up.
Thanks,
Parker
On Thu, May 02, 2024 at 11:36:10AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 18:29:05 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, May 02, 2024 at 11:13:14AM -0400, Parker Newman wrote:
..
> > Above I added current code of these macros, can you elaborate how it's incorrect?
>
> Sorry, you are correct. I was mixed up.
No problem. And since you are here, can you tell me what type of EEPROM is
connected to the chip in your case?
--
With Best Regards,
Andy Shevchenko
On Thu, 2 May 2024 17:43:54 +0300
Andy Shevchenko <[email protected]> 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.
>
Just an FYI I submitted a patch series that fixed several of these issues but I
think it fell through the cracks (I know everyone is very busy!).
Link: https://lore.kernel.org/linux-serial/[email protected]/
I believe my previous patch series is no longer required. This one fixes
everything.
Thanks,
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 | 454 ++++++++++++----------------
> 1 file changed, 200 insertions(+), 254 deletions(-)
>
We may reuse ->setup() for the same purpose as it was done before.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
---
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
Keep the includes sorted.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 6764aaa20df2..dd179a291594 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
On Thu, 2 May 2024 18:43:59 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 11:36:10AM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 18:29:05 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Thu, May 02, 2024 at 11:13:14AM -0400, Parker Newman wrote:
>
> ...
>
> > > Above I added current code of these macros, can you elaborate how it's incorrect?
> >
> > Sorry, you are correct. I was mixed up.
>
> No problem. And since you are here, can you tell me what type of EEPROM is
> connected to the chip in your case?
>
Microchip AT93C46D and various other vendors equivalent parts.
Let me know if you have any other questions.
Parker
On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 17:43:54 +0300
> Andy Shevchenko <[email protected]> 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.
> >
>
> Just an FYI I submitted a patch series that fixed several of these issues but I
> think it fell through the cracks (I know everyone is very busy!).
>
> Link: https://lore.kernel.org/linux-serial/[email protected]/
>
> I believe my previous patch series is no longer required. This one fixes
> everything.
I haven't noticed that, if it contains duplicated patches, I may replace mine
with yours if you insist.
In any case it's better to reply there that you prefer this series to be
applied, so Greg will not pick it up.
--
With Best Regards,
Andy Shevchenko
On Thu, May 02, 2024 at 11:54:39AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 18:43:59 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, May 02, 2024 at 11:36:10AM -0400, Parker Newman wrote:
> > > On Thu, 2 May 2024 18:29:05 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, May 02, 2024 at 11:13:14AM -0400, Parker Newman wrote:
..
> > > > Above I added current code of these macros, can you elaborate how it's incorrect?
> > >
> > > Sorry, you are correct. I was mixed up.
> >
> > No problem. And since you are here, can you tell me what type of EEPROM is
> > connected to the chip in your case?
>
> Microchip AT93C46D and various other vendors equivalent parts.
Thank you!
> Let me know if you have any other questions.
Nope, just asking you to test and review the series when you have time.
--
With Best Regards,
Andy Shevchenko
On Thu, 2 May 2024 19:01:01 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 17:43:54 +0300
> > Andy Shevchenko <[email protected]> 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.
> > >
> >
> > Just an FYI I submitted a patch series that fixed several of these issues but I
> > think it fell through the cracks (I know everyone is very busy!).
> >
> > Link: https://lore.kernel.org/linux-serial/[email protected]/
> >
> > I believe my previous patch series is no longer required. This one fixes
> > everything.
>
> I haven't noticed that, if it contains duplicated patches, I may replace mine
> with yours if you insist.
>
> In any case it's better to reply there that you prefer this series to be
> applied, so Greg will not pick it up.
>
I do not have a preference. I am fine with using yours if it is easier on
the maintainers.
I will send a reply on my previous series that it is not needed and link
to this. I am new to the mailing lists so I didn't know what the procedure
is for this situation.
Thanks for the fixes :),
Parker
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 <[email protected]>
---
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
On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo J?rvinen wrote:
> On Thu, 2 May 2024, Andy Shevchenko wrote:
..
> > // 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);
> > + if (exar_ee_read_bit(priv))
> > + data |= BIT(i);
>
> Does this end up reversing the order of bits? In the original, data was left
> shifted which moved the existing bits and added the lsb but the replacement
> adds highest bit on each iteration?
Oh, seems a good catch!
I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
so two loops has to be aligned.
--
With Best Regards,
Andy Shevchenko
On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 19:01:01 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > On Thu, 2 May 2024 17:43:54 +0300
> > > Andy Shevchenko <[email protected]> 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.
> > >
> > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > think it fell through the cracks (I know everyone is very busy!).
> > >
> > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > >
> > > I believe my previous patch series is no longer required. This one fixes
> > > everything.
> >
> > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > with yours if you insist.
> >
> > In any case it's better to reply there that you prefer this series to be
> > applied, so Greg will not pick it up.
> >
>
> I do not have a preference. I am fine with using yours if it is easier on
> the maintainers.
Up to you, there is no issue to take your patches in case they are the same
(or quite similar) as mine. I can pick them up, just tell me if you want this
to happen with a list of the patches (as mail Message-Id).
> I will send a reply on my previous series that it is not needed and link
> to this. I am new to the mailing lists so I didn't know what the procedure
> is for this situation.
It's not about mailing lists, it's just a common sense.
> Thanks for the fixes :),
You are welcome!
--
With Best Regards,
Andy Shevchenko
On Thu, 2 May 2024 20:22:47 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 19:01:01 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > > On Thu, 2 May 2024 17:43:54 +0300
> > > > Andy Shevchenko <[email protected]> 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.
> > > >
> > > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > > think it fell through the cracks (I know everyone is very busy!).
> > > >
> > > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > > >
> > > > I believe my previous patch series is no longer required. This one fixes
> > > > everything.
> > >
> > > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > > with yours if you insist.
> > >
> > > In any case it's better to reply there that you prefer this series to be
> > > applied, so Greg will not pick it up.
> > >
> >
> > I do not have a preference. I am fine with using yours if it is easier on
> > the maintainers.
>
> Up to you, there is no issue to take your patches in case they are the same
> (or quite similar) as mine. I can pick them up, just tell me if you want this
> to happen with a list of the patches (as mail Message-Id).
>
Just use yours.
Thanks,
Parker
> > I will send a reply on my previous series that it is not needed and link
> > to this. I am new to the mailing lists so I didn't know what the procedure
> > is for this situation.
>
> It's not about mailing lists, it's just a common sense.
>
> > Thanks for the fixes :),
>
> You are welcome!
>
Extract cti_board_init_osc_freq() helper to avoid code duplication.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
On Thu, May 02, 2024 at 01:49:49PM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 20:22:47 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> > > On Thu, 2 May 2024 19:01:01 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > > > On Thu, 2 May 2024 17:43:54 +0300
> > > > > Andy Shevchenko <[email protected]> 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.
> > > > >
> > > > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > > > think it fell through the cracks (I know everyone is very busy!).
> > > > >
> > > > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > > > >
> > > > > I believe my previous patch series is no longer required. This one fixes
> > > > > everything.
> > > >
> > > > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > > > with yours if you insist.
> > > >
> > > > In any case it's better to reply there that you prefer this series to be
> > > > applied, so Greg will not pick it up.
> > > >
> > >
> > > I do not have a preference. I am fine with using yours if it is easier on
> > > the maintainers.
> >
> > Up to you, there is no issue to take your patches in case they are the same
> > (or quite similar) as mine. I can pick them up, just tell me if you want this
> > to happen with a list of the patches (as mail Message-Id).
>
> Just use yours.
Okay, thanks!
If you are going to test, better to pay attention to the BIT() conversion patch
as Ilpo noted an issue. I believe it's easy to drop (via local git-rebase run)
or move and test separately.
--
With Best Regards,
Andy Shevchenko
On Thu, 2 May 2024, Andy Shevchenko wrote:
> Use BIT() in exar_ee_read() like other functions do.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 306bc6d7c141..bf3730f4231d 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -340,13 +340,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);
> + if (exar_ee_read_bit(priv))
> + data |= BIT(i);
Does this end up reversing the order of bits? In the original, data was
left shifted which moved the existing bits and added the lsb but the
replacement adds highest bit on each iteration?
--
i.
On Thu, 2 May 2024 21:01:54 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 01:49:49PM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 20:22:47 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> > > > On Thu, 2 May 2024 19:01:01 +0300
> > > > Andy Shevchenko <[email protected]> wrote:
> > > > > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > > > > On Thu, 2 May 2024 17:43:54 +0300
> > > > > > Andy Shevchenko <[email protected]> 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.
> > > > > >
> > > > > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > > > > think it fell through the cracks (I know everyone is very busy!).
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > > > > >
> > > > > > I believe my previous patch series is no longer required. This one fixes
> > > > > > everything.
> > > > >
> > > > > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > > > > with yours if you insist.
> > > > >
> > > > > In any case it's better to reply there that you prefer this series to be
> > > > > applied, so Greg will not pick it up.
> > > > >
> > > >
> > > > I do not have a preference. I am fine with using yours if it is easier on
> > > > the maintainers.
> > >
> > > Up to you, there is no issue to take your patches in case they are the same
> > > (or quite similar) as mine. I can pick them up, just tell me if you want this
> > > to happen with a list of the patches (as mail Message-Id).
> >
> > Just use yours.
>
> Okay, thanks!
>
> If you are going to test, better to pay attention to the BIT() conversion patch
> as Ilpo noted an issue. I believe it's easy to drop (via local git-rebase run)
> or move and test separately.
>
I am working on testing now but patches 7 and 12 did not apply with git am.
Both failed around lines 1095/1096.
I can apply them manually but I may be using the wrong branch (tty-next).
Which branch/commit did you create your patches from? I don't see it in the
patch submission.
On Thu, 2 May 2024 20:20:01 +0300
Andy Shevchenko <[email protected]> wrote:
> On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo Järvinen wrote:
> > On Thu, 2 May 2024, Andy Shevchenko wrote:
>
> ...
>
> > > // 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);
> > > + if (exar_ee_read_bit(priv))
> > > + data |= BIT(i);
> >
> > Does this end up reversing the order of bits? In the original, data was left
> > shifted which moved the existing bits and added the lsb but the replacement
> > adds highest bit on each iteration?
>
> Oh, seems a good catch!
>
> I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> so two loops has to be aligned.
>
I just tested this and Ilpo is correct, the read loop portion is backwards as
expected. This is the corrected loop:
// Read data 1 bit at a time
for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
if (exar_ee_read_bit(priv))
data |= BIT(i);
}
I know this looks wrong because its looping from 16->0 rather than the
more intuitive 15->0 for a 16bit value. This is actually correct however
because according to the AT93C46D datasheet there is always dummy 0 bit
before the actual 16 bits of data.
I hope that helps,
-Parker
On Fri, 3 May 2024 08:36:38 -0400
Parker Newman <[email protected]> wrote:
> On Thu, 2 May 2024 21:01:54 +0300
> Andy Shevchenko <[email protected]> wrote:
>
> > On Thu, May 02, 2024 at 01:49:49PM -0400, Parker Newman wrote:
> > > On Thu, 2 May 2024 20:22:47 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> > > > > On Thu, 2 May 2024 19:01:01 +0300
> > > > > Andy Shevchenko <[email protected]> wrote:
> > > > > > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > > > > > On Thu, 2 May 2024 17:43:54 +0300
> > > > > > > Andy Shevchenko <[email protected]> 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.
> > > > > > >
> > > > > > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > > > > > think it fell through the cracks (I know everyone is very busy!).
> > > > > > >
> > > > > > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > > > > > >
> > > > > > > I believe my previous patch series is no longer required. This one fixes
> > > > > > > everything.
> > > > > >
> > > > > > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > > > > > with yours if you insist.
> > > > > >
> > > > > > In any case it's better to reply there that you prefer this series to be
> > > > > > applied, so Greg will not pick it up.
> > > > > >
> > > > >
> > > > > I do not have a preference. I am fine with using yours if it is easier on
> > > > > the maintainers.
> > > >
> > > > Up to you, there is no issue to take your patches in case they are the same
> > > > (or quite similar) as mine. I can pick them up, just tell me if you want this
> > > > to happen with a list of the patches (as mail Message-Id).
> > >
> > > Just use yours.
> >
> > Okay, thanks!
> >
> > If you are going to test, better to pay attention to the BIT() conversion patch
> > as Ilpo noted an issue. I believe it's easy to drop (via local git-rebase run)
> > or move and test separately.
> >
>
> I am working on testing now but patches 7 and 12 did not apply with git am.
> Both failed around lines 1095/1096.
> I can apply them manually but I may be using the wrong branch (tty-next).
> Which branch/commit did you create your patches from? I don't see it in the
> patch submission.
I figured it out. git am was applying the typo fix patch out of order.
Sorry, I didn't notice that. Patches should be fine.
I can do a final test once you decide what to do with the BIT() conversion patch.
Parker
On Fri, May 03, 2024 at 10:26:32AM -0400, Parker Newman wrote:
> On Thu, 2 May 2024 20:20:01 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo J?rvinen wrote:
> > > On Thu, 2 May 2024, Andy Shevchenko wrote:
..
> > > > // 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);
> > > > + if (exar_ee_read_bit(priv))
> > > > + data |= BIT(i);
> > >
> > > Does this end up reversing the order of bits? In the original, data was left
> > > shifted which moved the existing bits and added the lsb but the replacement
> > > adds highest bit on each iteration?
> >
> > Oh, seems a good catch!
> >
> > I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> > so two loops has to be aligned.
> >
>
> I just tested this and Ilpo is correct, the read loop portion is backwards as
> expected. This is the corrected loop:
>
> // Read data 1 bit at a time
> for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
> if (exar_ee_read_bit(priv))
> data |= BIT(i);
> }
>
> I know this looks wrong because its looping from 16->0 rather than the
> more intuitive 15->0 for a 16bit value. This is actually correct however
> because according to the AT93C46D datasheet there is always dummy 0 bit
> before the actual 16 bits of data.
>
> I hope that helps,
Yes, it helps and means that we need that comment to be added to the code. Is
it the same applicable to the write part above (for address)? Because AFAIU
mine is one bit longer than yours. Maybe in the original code is a bug? Have
you tried to read high addresses?
--
With Best Regards,
Andy Shevchenko
On Fri, 3 May 2024 18:35:45 +0300
Andy Shevchenko <[email protected]> wrote:
> On Fri, May 03, 2024 at 10:26:32AM -0400, Parker Newman wrote:
> > On Thu, 2 May 2024 20:20:01 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo Järvinen wrote:
> > > > On Thu, 2 May 2024, Andy Shevchenko wrote:
>
> ...
>
> > > > > // 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);
> > > > > + if (exar_ee_read_bit(priv))
> > > > > + data |= BIT(i);
> > > >
> > > > Does this end up reversing the order of bits? In the original, data was left
> > > > shifted which moved the existing bits and added the lsb but the replacement
> > > > adds highest bit on each iteration?
> > >
> > > Oh, seems a good catch!
> > >
> > > I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> > > so two loops has to be aligned.
> > >
> >
> > I just tested this and Ilpo is correct, the read loop portion is backwards as
> > expected. This is the corrected loop:
> >
> > // Read data 1 bit at a time
> > for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
> > if (exar_ee_read_bit(priv))
> > data |= BIT(i);
> > }
> >
> > I know this looks wrong because its looping from 16->0 rather than the
> > more intuitive 15->0 for a 16bit value. This is actually correct however
> > because according to the AT93C46D datasheet there is always dummy 0 bit
> > before the actual 16 bits of data.
> >
> > I hope that helps,
>
> Yes, it helps and means that we need that comment to be added to the code Is
> it the same applicable to the write part above (for address)? Because AFAIU
> mine is one bit longer than yours. Maybe in the original code is a bug? Have
> you tried to read high addresses?
The address portion is 6 bits, nothing extra, so what you have is correct.
The original code was legacy, I cleaned it up but didn't change those loops
Your method works out the the same number of bits as the legacy method
which sets bit 5 and has to shift right 6 times to get i = 0 which ends the loop.
Parker
On Fri, May 03, 2024 at 10:47:30AM -0400, Parker Newman wrote:
> On Fri, 3 May 2024 08:36:38 -0400
> Parker Newman <[email protected]> wrote:
>
> > On Thu, 2 May 2024 21:01:54 +0300
> > Andy Shevchenko <[email protected]> wrote:
> >
> > > On Thu, May 02, 2024 at 01:49:49PM -0400, Parker Newman wrote:
> > > > On Thu, 2 May 2024 20:22:47 +0300
> > > > Andy Shevchenko <[email protected]> wrote:
> > > > > On Thu, May 02, 2024 at 12:08:40PM -0400, Parker Newman wrote:
> > > > > > On Thu, 2 May 2024 19:01:01 +0300
> > > > > > Andy Shevchenko <[email protected]> wrote:
> > > > > > > On Thu, May 02, 2024 at 11:46:45AM -0400, Parker Newman wrote:
> > > > > > > > On Thu, 2 May 2024 17:43:54 +0300
> > > > > > > > Andy Shevchenko <[email protected]> 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.
> > > > > > > >
> > > > > > > > Just an FYI I submitted a patch series that fixed several of these issues but I
> > > > > > > > think it fell through the cracks (I know everyone is very busy!).
> > > > > > > >
> > > > > > > > Link: https://lore.kernel.org/linux-serial/[email protected]/
> > > > > > > >
> > > > > > > > I believe my previous patch series is no longer required. This one fixes
> > > > > > > > everything.
> > > > > > >
> > > > > > > I haven't noticed that, if it contains duplicated patches, I may replace mine
> > > > > > > with yours if you insist.
> > > > > > >
> > > > > > > In any case it's better to reply there that you prefer this series to be
> > > > > > > applied, so Greg will not pick it up.
> > > > > > >
> > > > > >
> > > > > > I do not have a preference. I am fine with using yours if it is easier on
> > > > > > the maintainers.
> > > > >
> > > > > Up to you, there is no issue to take your patches in case they are the same
> > > > > (or quite similar) as mine. I can pick them up, just tell me if you want this
> > > > > to happen with a list of the patches (as mail Message-Id).
> > > >
> > > > Just use yours.
> > >
> > > Okay, thanks!
> > >
> > > If you are going to test, better to pay attention to the BIT() conversion patch
> > > as Ilpo noted an issue. I believe it's easy to drop (via local git-rebase run)
> > > or move and test separately.
> > >
> >
> > I am working on testing now but patches 7 and 12 did not apply with git am.
> > Both failed around lines 1095/1096.
> > I can apply them manually but I may be using the wrong branch (tty-next).
> > Which branch/commit did you create your patches from? I don't see it in the
> > patch submission.
>
> I figured it out. git am was applying the typo fix patch out of order.
> Sorry, I didn't notice that. Patches should be fine.
>
> I can do a final test once you decide what to do with the BIT() conversion patch.
Can you revert it and test the rest? So we will know that they are okay.
Or does the above implies that you already performed such a test?
--
With Best Regards,
Andy Shevchenko
On Fri, May 03, 2024 at 02:56:33PM -0400, Parker Newman wrote:
> On Fri, 3 May 2024 18:35:45 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Fri, May 03, 2024 at 10:26:32AM -0400, Parker Newman wrote:
> > > On Thu, 2 May 2024 20:20:01 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Thu, May 02, 2024 at 07:08:21PM +0300, Ilpo J?rvinen wrote:
> > > > > On Thu, 2 May 2024, Andy Shevchenko wrote:
..
> > > > > > // 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);
> > > > > > + if (exar_ee_read_bit(priv))
> > > > > > + data |= BIT(i);
> > > > >
> > > > > Does this end up reversing the order of bits? In the original, data was left
> > > > > shifted which moved the existing bits and added the lsb but the replacement
> > > > > adds highest bit on each iteration?
> > > >
> > > > Oh, seems a good catch!
> > > >
> > > > I was also wondering, but missed this somehow. Seems the EEPROM is in BE mode,
> > > > so two loops has to be aligned.
> > > >
> > >
> > > I just tested this and Ilpo is correct, the read loop portion is backwards as
> > > expected. This is the corrected loop:
> > >
> > > // Read data 1 bit at a time
> > > for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
> > > if (exar_ee_read_bit(priv))
> > > data |= BIT(i);
> > > }
> > >
> > > I know this looks wrong because its looping from 16->0 rather than the
> > > more intuitive 15->0 for a 16bit value. This is actually correct however
> > > because according to the AT93C46D datasheet there is always dummy 0 bit
> > > before the actual 16 bits of data.
> > >
> > > I hope that helps,
> >
> > Yes, it helps and means that we need that comment to be added to the code. Is
> > it the same applicable to the write part above (for address)? Because AFAIU
> > mine is one bit longer than yours. Maybe in the original code is a bug? Have
> > you tried to read high addresses?
>
> The address portion is 6 bits, nothing extra, so what you have is correct.
>
> The original code was legacy, I cleaned it up but didn't change those loops.
>
> Your method works out the the same number of bits as the legacy method
> which sets bit 5 and has to shift right 6 times to get i = 0 which ends the loop.
Okay, thank your for confirming the correctness of a new code!
--
With Best Regards,
Andy Shevchenko