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

[Johan Hovold <johan@kernel.org>]

On Thu, Aug 18, 2022 at 04:11:40PM +0200, Pali Rohár wrote:
> 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.

Of course you can, but perhaps you'll figure out some other way to do
this.

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

Ok, but don't. This variable is used in other parts of this function.

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

Looks like there's another bug here in the current driver as b300 is
defined as 0.

I'll fix that up separately first.

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