Re: [PATCH v2 07/22] usb: serial: ti_usb_3410_5052: Use macros instead of magic values

From: Johan Hovold <johan@kernel.org>
To: Mathieu OTHACEHE <m.othacehe@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: Re: [PATCH v2 07/22] usb: serial: ti_usb_3410_5052: Use macros instead of magic values
Date: Tue, 23 Aug 2016 10:19:50 +0200	[thread overview]
Message-ID: <20160823081950.GF16896@localhost> (raw)
In-Reply-To: <20160726180002.2398-8-m.othacehe@gmail.com>

On Tue, Jul 26, 2016 at 07:59:47PM +0200, Mathieu OTHACEHE wrote:
> Use macros to define 3410 and 5052 baud bases.
> Use macro to define usb download timeout.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> index 2b7fe89..b5f3328 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -270,10 +270,12 @@ struct ti_firmware_header {
>  #define TI_DRIVER_AUTHOR	"Al Borchers <alborchers@steinerpoint.com>"
>  #define TI_DRIVER_DESC		"TI USB 3410/5052 Serial Driver"
>  
> -#define TI_FIRMWARE_BUF_SIZE	16284
> +#define TI_3410_BAUD_BASE       923077
> +#define TI_5052_BAUD_BASE       461538
>  
> +#define TI_FIRMWARE_BUF_SIZE	16284
>  #define TI_TRANSFER_TIMEOUT	2
> -
> +#define TI_DOWNLOAD_TIMEOUT	1000
>  #define TI_DEFAULT_CLOSING_WAIT	4000		/* in .01 secs */
>  
>  /* supported setserial flags */
> @@ -1016,9 +1018,9 @@ static void ti_set_termios(struct tty_struct *tty,
>  	if (!baud)
>  		baud = 9600;
>  	if (tport->tp_tdev->td_is_3410)
> -		wbaudrate = (923077 + baud/2) / baud;
> +		wbaudrate = (TI_3410_BAUD_BASE + baud / 2) / baud;
>  	else
> -		wbaudrate = (461538 + baud/2) / baud;
> +		wbaudrate = (TI_5052_BAUD_BASE + baud / 2) / baud;
>  
>  	/* FIXME: Should calculate resulting baud here and report it back */
>  	if ((C_BAUD(tty)) != B0)
> @@ -1434,6 +1436,7 @@ static int ti_get_serial_info(struct ti_port *tport,
>  	struct usb_serial_port *port = tport->tp_port;
>  	struct serial_struct ret_serial;
>  	unsigned cwait;
> +	int baud_base;
>  
>  	if (!ret_arg)
>  		return -EFAULT;
> @@ -1444,11 +1447,16 @@ static int ti_get_serial_info(struct ti_port *tport,
>  
>  	memset(&ret_serial, 0, sizeof(ret_serial));
>  
> +	if (tport->tp_tdev->td_is_3410)
> +		baud_base = TI_3410_BAUD_BASE;
> +	else
> +		baud_base = TI_5052_BAUD_BASE;
> +
>  	ret_serial.type = PORT_16550A;
>  	ret_serial.line = port->minor;
>  	ret_serial.port = port->port_number;
>  	ret_serial.xmit_fifo_size = kfifo_size(&port->write_fifo);
> -	ret_serial.baud_base = tport->tp_tdev->td_is_3410 ? 921600 : 460800;
> +	ret_serial.baud_base = baud_base;

The above is not functionally equivalent, since your now returning a
different baud_base.

This may be fine, but again you're hiding changes in the code without
describing them properly in the commit messages.

Please go through the rest of the series, and make sure that the commit
messages are correct. I'm not gonna look at the rest of the series.

Thanks,
Johan