Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Linux>
  3. Kernel
[PATCH v1 4/4] serial: exar: change port_type ternary line wrapping
parker at finest
Apr 18, 2024, 8:36 AM
Post #1 of 4 (11 views)
Permalink
From: Parker Newman <pnewman@connecttech.com>

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/f2353b8c-2079-b895-2707-f6be83161288@linux.intel.com

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

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 10725ad0f3ef..a76b4e5bab4e 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -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
Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping [ In reply to ]
jirislaby at kernel
Apr 18, 2024, 11:07 PM
Post #2 of 4 (11 views)
Permalink
On 18. 04. 24, 17:36, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
>
> 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;

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

> Based on feedback from:
> Link: https://lore.kernel.org/linux-serial/f2353b8c-2079-b895-2707-f6be83161288@linux.intel.com

You should have CCed the author.

> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 10725ad0f3ef..a76b4e5bab4e 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -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;
?


> 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.

> 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
>

--
js
suse labs
Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping [ In reply to ]
gregkh at linuxfoundation
Apr 18, 2024, 11:57 PM
Post #3 of 4 (11 views)
Permalink
On Thu, Apr 18, 2024 at 11:36:31AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
>
> 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/f2353b8c-2079-b895-2707-f6be83161288@linux.intel.com
>
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 10725ad0f3ef..a76b4e5bab4e 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -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
Re: [PATCH v1 4/4] serial: exar: change port_type ternary line wrapping [ In reply to ]
parker at finest
Apr 19, 2024, 5:17 AM
Post #4 of 4 (9 views)
Permalink
On Fri, 19 Apr 2024 08:07:50 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 18. 04. 24, 17:36, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > 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;
>
> This is worse IMO. Ilpo suggested a bit different alignment. But still...
>
> > Based on feedback from:
> > Link: https://lore.kernel.org/linux-serial/f2353b8c-2079-b895-2707-f6be83161288@linux.intel.com
>
> You should have CCed the author.
>
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> > drivers/tty/serial/8250/8250_exar.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 10725ad0f3ef..a76b4e5bab4e 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -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;
> ?
>

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

>
> > 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.
>
> > 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
> >
>

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal