| Message ID | 20220712115306.26471-4-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ftdi_sio driver improvements | expand |
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. > + } > +} > + > 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. > 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
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
On Tuesday 13 September 2022 16:49:01 Johan Hovold wrote: > > > > @@ -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. As I said, I changed it just because I was explicitly asked for it during v1 review. I'm just doing what I was asked. Maybe... I can change it into union? union { s32 signed; u32 unsigned; } div_value; I really do not see any other option how to achieve requirement that it should be type of "int" with your another requirement that "do not do it".
On Tue, Sep 13, 2022 at 05:30:51PM +0200, Pali Rohár wrote: > On Tuesday 13 September 2022 16:49:01 Johan Hovold wrote: > > > > > @@ -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. > > As I said, I changed it just because I was explicitly asked for it > during v1 review. I'm just doing what I was asked. > > Maybe... I can change it into union? > > union { > s32 signed; > u32 unsigned; > } div_value; > > I really do not see any other option how to achieve requirement that it > should be type of "int" with your another requirement that "do not do it". I'm pretty sure I didn't review v1. Perhaps you're referring to this: https://lore.kernel.org/all/Ysb3ORyUAPEOntqK@kroah.com/ But that doesn't mean you should change the type of div_value. Johan
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; + } +} + 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; 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;