[v1,0/4] serial: exar: fix kbuild warnings and code style

Message ID	cover.1713452766.git.pnewman@connecttech.com
Headers	show Received: from mout.perfora.net (mout.perfora.net [74.208.4.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3239016D33E; Thu, 18 Apr 2024 15:37:23 +0000 (UTC) From: Parker Newman <parker@finest.io> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org Cc: Parker Newman <pnewman@connecttech.com> Subject: [PATCH v1 0/4] serial: exar: fix kbuild warnings and code style Date: Thu, 18 Apr 2024 11:36:27 -0400 Message-ID: <cover.1713452766.git.pnewman@connecttech.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable UI-OutboundReport: notjunk:1;M01:P0:5PXZGpAllV0=;kSybSKQ4rlhqmr0RjpPiL/8Xjey SJqloJYmhkkQHPVzgomc9eOmw/PNVlof0XrooKfnUlEF0m3PE3OdbIy5NNBEzwrjeN+Ve+2T9 GgzpKYZbxL6YO3jAqjpQ8/31blPrfE8SN9v9EekgD9ju+hoqKXU+ojD7M4qFNJ7SdPkhLDju/ laTkF6N3Ig/C1rwwwenQAmCuuIETjPno80e4sX9/zdRGfqCvWF/y5OsxQlNuhcU+wJEwUOx+b 3BG4uK8A/qu3aYuY7LmH2vEJq9Olz7uSQj9ePZIhnvHoywv3W/GYeINsj8B+qQKWkfM7274d2 sbQCJlvlQan1B4MwjtgdFn56zTDoYVCKyr+cW+HZoRuru14wcW2wchSgFr+WNEhC1aQmLVZUh SUI50KeudfU/tl5lg2mn8N+EUgRrFrl6EdHfcKDJf9s4Emx6W5jQ4VQi2lo12dJxI3ozFqsiW fZNFHcddOxuxiuhmkj4vb3IzRHaQFnfnP1zer+v/B87Al5CQJHaAwbc1RCBvSGN5Ncwu2vHy7 E6iIBhFZj1K4elmYWuhPOM2VcoIdwWf7sa4rmfH1Sj5Zul/weLGkWMcrdbYJ5KcLP5BAXBHwW jgFwrl0f7KDEjA19EsmD/hwK+ayKfq+WJK4dzpxRAE1TL0on1bEMmGw2yFjnlwFbYZBLaGXhY dOq9IfY7Gsuf1yAjEKgK9wb5hGoPS+kQCYBHkJwTzvZgWqgpKOmeUJsLKApjZW5HNvtUQE3d9 Aq9+v7l43iTZDllB0mIQihLY7ukK0F96j/pnIhKviE8sVazKnGIdvs=
Series	serial: exar: fix kbuild warnings and code style \| expand [v1,0/4] serial: exar: fix kbuild warnings and code style [v1,1/4] serial: exar: add missing kernel doc function parameters [v1,2/4] serial: exar: use return dev_err_probe instead of returning error code [v1,3/4] serial: exar: remove unneeded parenthesis [v1,4/4] serial: exar: change port_type ternary line wrapping

Parker Newman April 18, 2024, 3:36 p.m. UTC

From: Parker Newman <pnewman@connecttech.com>

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/cover.1713382717.git.pnewman@connecttech.com/

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

Jiri Slaby April 19, 2024, 6:07 a.m. UTC | #1

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
>

Greg KH April 19, 2024, 6:57 a.m. UTC | #2

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

Greg KH April 19, 2024, 6:58 a.m. UTC | #3

On Thu, Apr 18, 2024 at 11:36:30AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> Remove unneeded parenthesis from several locations.
> 
> 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 | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 01748ddbf729..10725ad0f3ef 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -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 April 19, 2024, 7:01 a.m. UTC | #4

On 19. 04. 24, 8:58, Greg Kroah-Hartman wrote:
> On Thu, Apr 18, 2024 at 11:36:30AM -0400, Parker Newman wrote:
>> From: Parker Newman <pnewman@connecttech.com>
>>
>> Remove unneeded parenthesis from several locations.
>>
>> 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 | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
>> index 01748ddbf729..10725ad0f3ef 100644
>> --- a/drivers/tty/serial/8250/8250_exar.c
>> +++ b/drivers/tty/serial/8250/8250_exar.c
>> @@ -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 :)

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,

Parker Newman April 19, 2024, 12:17 p.m. UTC | #5

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

Parker Newman April 19, 2024, 12:50 p.m. UTC | #6

On Fri, 19 Apr 2024 09:01:22 +0200
Jiri Slaby <jirislaby@kernel.org> wrote:

> On 19. 04. 24, 8:58, Greg Kroah-Hartman wrote:
> > On Thu, Apr 18, 2024 at 11:36:30AM -0400, Parker Newman wrote:
> >> From: Parker Newman <pnewman@connecttech.com>
> >>
> >> Remove unneeded parenthesis from several locations.
> >>
> >> 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 | 28 ++++++++++++++--------------
> >>   1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> >> index 01748ddbf729..10725ad0f3ef 100644
> >> --- a/drivers/tty/serial/8250/8250_exar.c
> >> +++ b/drivers/tty/serial/8250/8250_exar.c
> >> @@ -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 :)
>
> 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

[v1,0/4] serial: exar: fix kbuild warnings and code style

Message

Comments