serial: exar: fix kbuild warnings and code style

PATCH Superseded Parker Newman 3 participants 11 messages Apr 18, 2024 – Apr 19, 2024 TTY layer and serial drivers

lore

Share & tools

lore

Parker Newman

author

2024-04-18 15:37 UTC #0 lore raw

cover serial: exar: fix kbuild warnings and code style

From: Parker Newman <[email protected]>

This is a series of small patches fixing kbuilds error and code style issues based on feedback during review of main patches.

Original patches thread:

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Thank you,
Parker Newman (4):
serial: exar: add missing kernel doc function parameters
serial: exar: use return dev_err_probe instead of returning error code
serial: exar: remove unneeded parenthesis
serial: exar: change port_type ternary line wrapping

 drivers/tty/serial/8250/8250_exar.c | 47 +++++++++++++++--------------
 1 file changed, 24 insertions(+), 23 deletions(-)

base-commit: c6795fbffc4547b40933ec368200bd4926a41b44 --
2.43.2

Parker Newman

author

2024-04-18 15:37 UTC #1 lore raw

PATCH 1/4 serial: exar: add missing kernel doc function parameters

From: Parker Newman <[email protected]>

Adds missing kernel doc function parameters to 3 functions.

Reported-by kernel test robot <[email protected]>

Closes:
https://lore.kernel.org/oe-kbuild-all/[email protected]/

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/tty/serial/8250/8250_exar.c +3 -0

index 5e42558cbb01..a180741da634 100644
@@ -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: PCI device pointer
  * @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: PCI device pointer
  * @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: PCI device pointer
  * @port_num: port offset
  *
  * CTI XR17V35X based cards have the port types stored in the EEPROM.

--
2.43.2

Parker Newman

author

2024-04-18 15:37 UTC #2 lore raw

PATCH 2/4 serial: exar: use return dev_err_probe instead of returning error code

In reply to Parker Newman

From: Parker Newman <[email protected]>

Change to returning dev_err_probe() instead of returning the error code after calling dev_err_probe().

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/tty/serial/8250/8250_exar.c +2 -4

index a180741da634..01748ddbf729 100644
@@ -1551,9 +1551,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 
 	nr_ports = exar_get_nr_ports(board, pcidev);
 	if (nr_ports == 0) {
-		dev_err_probe(&pcidev->dev, -ENODEV,
+		return dev_err_probe(&pcidev->dev, -ENODEV,
 				"failed to get number of ports\n");
-		return -ENODEV;
 	}
 
 	priv = devm_kzalloc(&pcidev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);

@@ -1587,9 +1586,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (board->board_init) {
 		rc = board->board_init(priv, pcidev);
 		if (rc) {
-			dev_err_probe(&pcidev->dev, rc,
+			return dev_err_probe(&pcidev->dev, rc,
 					"failed to init serial board\n");
-			return rc;
 		}
 	}

--
2.43.2

Parker Newman

author

2024-04-18 15:37 UTC #3 lore raw

PATCH 3/4 serial: exar: remove unneeded parenthesis

In reply to Parker Newman

From: Parker Newman <[email protected]>

Remove unneeded parenthesis from several locations.

Based on feedback from:

Link https://lore.kernel.org/linux-serial/[email protected] open here

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

drivers/tty/serial/8250/8250_exar.c +14 ~~-14~~

index 01748ddbf729..10725ad0f3ef 100644
@@ -317,7 +317,7 @@ static inline u8 exar_ee_read_bit(struct exar8250 *priv)
 
 	regb = exar_read_reg(priv, UART_EXAR_REGB);
 
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+	return regb & UART_EXAR_REGB_EEDO ? 1 : 0;
 }
 
 /**

@@ -741,18 +741,18 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 		break;
 	// 1x RS232, 1x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
-		port_type = (port_num == 0) ?
+		port_type = port_num == 0 ?
 			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 		break;
 	// 2x RS232, 2x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
-		port_type = (port_num < 2) ?
+		port_type = port_num < 2 ?
 			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 		break;
 	// 4x RS232, 4x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
-		port_type = (port_num < 4) ?
+		port_type = port_num < 4 ?
 			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 		break;
 	// RS232/RS422/RS485 HW (jumper) selectable

@@ -789,12 +789,12 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 		break;
 	// 6x RS232, 2x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
-		port_type = (port_num < 6) ?
+		port_type = port_num < 6 ?
 			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 		break;
 	// 2x RS232, 6x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
-		port_type = (port_num < 2) ?
+		port_type = port_num < 2 ?
 			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
 		break;
 	default:

@@ -981,10 +981,10 @@ static int cti_port_setup_xr17v35x(struct exar8250 *priv,
 	if (ret)
 		return ret;
 
-	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
-	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
-	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
-	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);
+	exar_write_reg(priv, offset + UART_EXAR_8XMODE, 0x00);
+	exar_write_reg(priv, offset + UART_EXAR_FCTR, UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, offset + UART_EXAR_TXTRG, 128);
+	exar_write_reg(priv, offset + UART_EXAR_RXTRG, 128);
 
 	return 0;
 }

@@ -1037,10 +1037,10 @@ static int cti_port_setup_xr17v25x(struct exar8250 *priv,
 	if (ret)
 		return ret;
 
-	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
-	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
-	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
-	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);
+	exar_write_reg(priv, offset + UART_EXAR_8XMODE, 0x00);
+	exar_write_reg(priv, offset + UART_EXAR_FCTR, UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, offset + UART_EXAR_TXTRG, 32);
+	exar_write_reg(priv, offset + UART_EXAR_RXTRG, 32);
 
 	return 0;
 }

--
2.43.2

Greg Kroah-Hartman

maintainer

2024-04-19 06:58 UTC #4 lore raw

Re: [PATCH v1 3/4] serial: exar: remove unneeded parenthesis

Quoted 17 lines · Parker Newman

From: Parker Newman <[email protected]>
Remove unneeded parenthesis from several locations.
Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here
Signed-off-by Parker Newman <[email protected]>
---
 drivers/tty/serial/8250/8250_exar.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 01748ddbf729..10725ad0f3ef 100644

drivers/tty/serial/8250/8250_exar.c static inline u8 exar_ee_read_bit(struct exar8250 *priv) · line 320

Quoted 5 lines

drivers/tty/serial/8250/8250_exar.c +1 -1

@@ -317,7 +317,7 @@ static inline u8 exar_ee_read_bit(struct exar8250 *priv)
 
 	regb = exar_read_reg(priv, UART_EXAR_REGB);
 
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+	return regb & UART_EXAR_REGB_EEDO ? 1 : 0;

Again, spell out the ? : stuff here please. Using () isn't the problem :)

thanks,

greg k-h

Jiri Slaby

maintainer

2024-04-19 07:01 UTC #5 lore raw

Re: [PATCH v1 3/4] serial: exar: remove unneeded parenthesis

Quoted 19 lines · Greg Kroah-Hartman

Quoted 18 lines · Parker Newman
From: Parker Newman <[email protected]>
Remove unneeded parenthesis from several locations.
Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here
Signed-off-by Parker Newman <[email protected]>
---
  drivers/tty/serial/8250/8250_exar.c | 28 ++++++++++++++--------------
  1 file changed, 14 insertions(+), 14 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 01748ddbf729..10725ad0f3ef 100644
@@ -317,7 +317,7 @@ static inline u8 exar_ee_read_bit(struct exar8250 *priv)
 

Quoted 6 lines

drivers/tty/serial/8250/8250_exar.c static inline u8 exar_ee_read_bit(struct exar8250 *priv) · line 320
Quoted 4 lines
drivers/tty/serial/8250/8250_exar.c +1 -1
  	regb = exar_read_reg(priv, UART_EXAR_REGB);
 
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+	return regb & UART_EXAR_REGB_EEDO ? 1 : 0;
Again, spell out the ? : stuff here please. Using () isn't the problem :)

Could this in fact be inline bool exar_is_ee_set() (or alike) and return that regb & UART_EXAR_REGB_EEDO directly (w/o using ternary at all)?

thanks,

Signature · 2 lines

js
suse labs

Parker Newman

author

2024-04-19 12:50 UTC #6 lore raw

Re: [PATCH v1 3/4] serial: exar: remove unneeded parenthesis

Quoted 27 lines · On Fri, 19 Apr 2024 09:01:22 +0200 Jiri Slaby <[email protected]> wrote:

Quoted 19 lines · Greg Kroah-Hartman
Quoted 18 lines · Parker Newman
From: Parker Newman <[email protected]>
Remove unneeded parenthesis from several locations.
Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here
Signed-off-by Parker Newman <[email protected]>
---
  drivers/tty/serial/8250/8250_exar.c | 28 ++++++++++++++--------------
  1 file changed, 14 insertions(+), 14 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 01748ddbf729..10725ad0f3ef 100644
@@ -317,7 +317,7 @@ static inline u8 exar_ee_read_bit(struct exar8250 *priv)
 
Quoted 6 lines
drivers/tty/serial/8250/8250_exar.c static inline u8 exar_ee_read_bit(struct exar8250 *priv) · line 320
Quoted 4 lines
drivers/tty/serial/8250/8250_exar.c +1 -1
  	regb = exar_read_reg(priv, UART_EXAR_REGB);
 
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+	return regb & UART_EXAR_REGB_EEDO ? 1 : 0;
Again, spell out the ? : stuff here please. Using () isn't the problem :)

Quoted 4 lines

Could this in fact be inline bool exar_is_ee_set() (or alike) and return that regb & UART_EXAR_REGB_EEDO directly (w/o using ternary at all)?
thanks,

That would work. I will update.
Thanks,
Parker

Parker Newman

author

2024-04-18 15:37 UTC #7 lore raw

PATCH 4/4 serial: exar: change port_type ternary line wrapping

In reply to Parker Newman

From: Parker Newman <[email protected]>

Change line wrapping of ternary operators in cti_get_port_type_xr17c15x_xr17v25x() for better readability.

Old example:

	port_type = port_num == 0 ?
			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;

New:

	port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
				CTI_PORT_TYPE_RS422_485;

Based on feedback from:

Link https://lore.kernel.org/linux-serial/[email protected] open here

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/tty/serial/8250/8250_exar.c +10 ~~-10~~

index 10725ad0f3ef..a76b4e5bab4e 100644
@@ -741,19 +741,19 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 		break;
 	// 1x RS232, 1x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
-		port_type = port_num == 0 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
 		break;
 	// 2x RS232, 2x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
 		break;
 	// 4x RS232, 4x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
-		port_type = port_num < 4 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 4 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
 		break;
 	// RS232/RS422/RS485 HW (jumper) selectable
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:

@@ -789,13 +789,13 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 		break;
 	// 6x RS232, 2x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
-		port_type = port_num < 6 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 6 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
 		break;
 	// 2x RS232, 6x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
 		break;
 	default:
 		dev_err(&pcidev->dev, "unknown/unsupported device\n");

--
2.43.2

Jiri Slaby

maintainer

2024-04-19 06:07 UTC #8 lore raw

Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping

Quoted 5 lines · Parker Newman

From: Parker Newman <[email protected]>
Change line wrapping of ternary operators in cti_get_port_type_xr17c15x_xr17v25x() for better readability.

Quoted 8 lines

Old example:

	port_type = port_num == 0 ?
			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;

New:

	port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
				CTI_PORT_TYPE_RS422_485;

This is worse IMO. Ilpo suggested a bit different alignment. But still...

Quoted 2 lines

Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here

You should have CCed the author.

Quoted 10 lines

Signed-off-by Parker Newman <[email protected]>
---
  drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 10725ad0f3ef..a76b4e5bab4e 100644

drivers/tty/serial/8250/8250_exar.c static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p · line 745

Quoted 7 lines

drivers/tty/serial/8250/8250_exar.c +2 -2

@@ -741,19 +741,19 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
  		break;
  	// 1x RS232, 1x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
-		port_type = port_num == 0 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;

Well, could you initialize port_type = CTI_PORT_TYPE_RS232? And here do only:
if (port_num > 0)

    return CTI_PORT_TYPE_RS422_485;

drivers/tty/serial/8250/8250_exar.c static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p · line 750

Quoted 7 lines

drivers/tty/serial/8250/8250_exar.c +2 -2

  		break;
  	// 2x RS232, 2x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;

And so on.

Quoted 32 lines

drivers/tty/serial/8250/8250_exar.c +6 -7

  		break;
  	// 4x RS232, 4x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
-		port_type = port_num < 4 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 4 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	// RS232/RS422/RS485 HW (jumper) selectable
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:

@@ -789,13 +789,13 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
  		break;
  	// 6x RS232, 2x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
-		port_type = port_num < 6 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 6 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	// 2x RS232, 6x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	default:
  		dev_err(&pcidev->dev, "unknown/unsupported device\n");
--

2.43.2

Signature · 2 lines

js
suse labs

Parker Newman

author

2024-04-19 12:17 UTC #9 lore raw

Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping

Quoted 44 lines · On Fri, 19 Apr 2024 08:07:50 +0200 Jiri Slaby <[email protected]> wrote:

Quoted 5 lines · Parker Newman
From: Parker Newman <[email protected]>
Change line wrapping of ternary operators in cti_get_port_type_xr17c15x_xr17v25x() for better readability.
Quoted 8 lines
Old example:
	port_type = port_num == 0 ?
			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
New:
	port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
				CTI_PORT_TYPE_RS422_485;
This is worse IMO. Ilpo suggested a bit different alignment. But still...
Quoted 2 lines
Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here
You should have CCed the author.
Quoted 10 lines
Signed-off-by Parker Newman <[email protected]>
---
  drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 10725ad0f3ef..a76b4e5bab4e 100644
drivers/tty/serial/8250/8250_exar.c static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p · line 745
Quoted 7 lines
drivers/tty/serial/8250/8250_exar.c +2 -2
@@ -741,19 +741,19 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
  		break;
  	// 1x RS232, 1x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
-		port_type = port_num == 0 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
Well, could you initialize port_type = CTI_PORT_TYPE_RS232? And here do only:
if (port_num > 0)

Quoted 3 lines

     return CTI_PORT_TYPE_RS422_485;

I like this idea I will move to that. Thanks.

Quoted 44 lines

drivers/tty/serial/8250/8250_exar.c static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p · line 750

Quoted 8 lines

drivers/tty/serial/8250/8250_exar.c +2 -2

  		break;
  	// 2x RS232, 2x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;

And so on.

Quoted 32 lines

drivers/tty/serial/8250/8250_exar.c +6 -7

  		break;
  	// 4x RS232, 4x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
-		port_type = port_num < 4 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 4 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	// RS232/RS422/RS485 HW (jumper) selectable
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:

@@ -789,13 +789,13 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
  		break;
  	// 6x RS232, 2x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
-		port_type = port_num < 6 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 6 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	// 2x RS232, 6x RS422/RS485
  	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
-		port_type = port_num < 2 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num < 2 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;
  		break;
  	default:
  		dev_err(&pcidev->dev, "unknown/unsupported device\n");
--

2.43.2

Greg Kroah-Hartman

maintainer

2024-04-19 06:57 UTC #10 lore raw

Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping

In reply to Parker Newman

Quoted 27 lines · Parker Newman

From: Parker Newman <[email protected]>
Change line wrapping of ternary operators in cti_get_port_type_xr17c15x_xr17v25x() for better readability.
Old example:
	port_type = port_num == 0 ?
			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
New:
	port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
				CTI_PORT_TYPE_RS422_485;
Based on feedback from:
Link https://lore.kernel.org/linux-serial/[email protected] open here
Signed-off-by Parker Newman <[email protected]>
---
 drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/tty/serial/8250/8250_exar.c +0 -0
index 10725ad0f3ef..a76b4e5bab4e 100644

drivers/tty/serial/8250/8250_exar.c static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p · line 745

Quoted 7 lines

drivers/tty/serial/8250/8250_exar.c +2 -2

@@ -741,19 +741,19 @@ static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *p
 		break;
 	// 1x RS232, 1x RS422/RS485
 	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
-		port_type = port_num == 0 ?
-			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
+					CTI_PORT_TYPE_RS422_485;

I missed this the first time, PLEASE never use ? : unless you have to. Spell it out and use an if statement, the compiler doesn't care, and you write code for people to read it first, cpus second. So this should look like:

		if (port_num)
			port_type = CTI_PORT_TYPE_RS422_485;
		else
			port_type = CTI_PORT_TYPE_RS232;

Much simpler and easier to understand when you look at this in 10 years and try to scan to figure out what went wrong with the logic here...

thanks,

greg k-h