Re: [PATCH v2 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

["Pali Rohár" <pali@kernel.org>]

On Sunday 24 July 2022 14:06:38 Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:53:02PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > In preparation for following changes, extract divisor code for SIO chip
> > into new function ftdi_sio_baud_to_divisor(), as is done for other
> > chips.
> > 
> > No functional change.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Tested-by: Marek Behún <kabel@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/usb/serial/ftdi_sio.c | 36 ++++++++++++++++++++---------------
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 717b97f4e094..5b109714612f 100644
> > --- a/drivers/usb/serial/ftdi_sio.c
> > +++ b/drivers/usb/serial/ftdi_sio.c
> > @@ -1150,6 +1150,23 @@ static struct usb_serial_driver * const serial_drivers[] = {
> >   * ***************************************************************************
> >   */
> >  
> > +static int ftdi_sio_baud_to_divisor(int baud)
> > +{
> > +	switch (baud) {
> > +	case 300: return ftdi_sio_b300;
> > +	case 600: return ftdi_sio_b600;
> > +	case 1200: return ftdi_sio_b1200;
> > +	case 2400: return ftdi_sio_b2400;
> > +	case 4800: return ftdi_sio_b4800;
> > +	case 9600: return ftdi_sio_b9600;
> > +	case 19200: return ftdi_sio_b19200;
> > +	case 38400: return ftdi_sio_b38400;
> > +	case 57600: return ftdi_sio_b57600;
> > +	case 115200: return ftdi_sio_b115200;
> > +	default: return -1;
> 
> Please add a line break after the colons.
> 
> Adding another enum for the invalid value might be preferable.

Enum describe HW values, so it cannot be added to hw list.

> > +	}
> > +}
> > +
> >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> >  {
> >  	unsigned short int divisor;
> > @@ -1286,7 +1303,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
> >  {
> >  	struct ftdi_private *priv = usb_get_serial_port_data(port);
> >  	struct device *dev = &port->dev;
> > -	u32 div_value = 0;
> > +	int div_value = 0;
> 
> And don't change the type here.

This type change was explicitly asked during v1 review. v1 had u32.

> >  	int div_okay = 1;
> >  	int baud;
> >  
> > @@ -1309,23 +1326,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
> >  		baud = 9600;
> >  	switch (priv->chip_type) {
> >  	case SIO: /* SIO chip */
> > -		switch (baud) {
> > -		case 300: div_value = ftdi_sio_b300; break;
> > -		case 600: div_value = ftdi_sio_b600; break;
> > -		case 1200: div_value = ftdi_sio_b1200; break;
> > -		case 2400: div_value = ftdi_sio_b2400; break;
> > -		case 4800: div_value = ftdi_sio_b4800; break;
> > -		case 9600: div_value = ftdi_sio_b9600; break;
> > -		case 19200: div_value = ftdi_sio_b19200; break;
> > -		case 38400: div_value = ftdi_sio_b38400; break;
> > -		case 57600: div_value = ftdi_sio_b57600;  break;
> > -		case 115200: div_value = ftdi_sio_b115200; break;
> > -		} /* baud */
> > -		if (div_value == 0) {
> > +		div_value = ftdi_sio_baud_to_divisor(baud);
> > +		if (div_value == -1) {
> >  			dev_dbg(dev, "%s - Baudrate (%d) requested is not supported\n",
> >  				__func__,  baud);
> > -			div_value = ftdi_sio_b9600;
> >  			baud = 9600;
> > +			div_value = ftdi_sio_baud_to_divisor(baud);
> >  			div_okay = 0;
> >  		}
> >  		break;
> 
> Johan