Date:   Tue, 19 Jun 2018 09:50:54 +0000
From:   "Karoly Pados" <pados@...os.hu>
To:     "Johan Hovold" <johan@...nel.org>
Cc:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: serial: cp210x: Improve baudrate support for
 CP2102N

Hello,

> Pass in a struct usb_serial (or port) as a first argument instead which
> allows for more readable code as well as for this to be reused to handle
> other device type differences (e.g. only 2108 besides 2102n handles
> baudrates over 921.6k).
> 

Sure, will do.

> Add a static helper (looks like you add a define in the gpio patch)
> cp210x_is_cp2102n(serial) here.

Yes I have macro for that in the GPIO patch, and I will turn that into a
static function too.

To keep the baudrate and gpio patches independent,
do you think it is a good idea if I make a new patch which only adds the
partnum defines and the helper function first, then baudrate v2 and gpio v2
can build onto it?

> You can even test for bit 0x20 in the
> helper if you prefer (we can always change that later if needed).
>

If you wish, but personally I think that is asking for future bugs
in the long run. Even though the helper can be easily adjusted if needed,
when/if a new partnum shows up which has nothing to do with the cp2102n,
no one will think of having to adjust cp2102n-spacific code until bug reports
start coming in. So I'd prefer to explicitly check for the packages, but in
the end I'll use whatever you prefer.

What do you prefer?

> And even if the current code uses this odd formatting, your amendments
> should not.
> 

Of course. I also saw this is odd, but (apparently wrongly) decided to 
stay consistent inside the function with existing code. I will change
that too.

Greetings,
Karoly


June 19, 2018 11:16 AM, "Johan Hovold" <johan@...nel.org> wrote:

> On Fri, Jun 15, 2018 at 11:29:57PM +0200, Karoly Pados wrote:
> 
>> The CP2102N supports more baudrates than earlier chips by SiLabs.
>> This patch adds support for all rates documented in the datasheet
>> of this device.
>> 
>> Signed-off-by: Karoly Pados <pados@...os.hu>
>> ---
>> drivers/usb/serial/cp210x.c | 39 ++++++++++++++++++++++++++-----------
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
>> index b1849f657e01..793b86252c46 100644
>> --- a/drivers/usb/serial/cp210x.c
>> +++ b/drivers/usb/serial/cp210x.c
>> @@ -357,6 +357,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_PARTNUM_CP2104 0x04
>> #define CP210X_PARTNUM_CP2105 0x05
>> #define CP210X_PARTNUM_CP2108 0x08
>> +#define CP210X_PARTNUM_CP2102N_QFN28 0x20
>> +#define CP210X_PARTNUM_CP2102N_QFN24 0x21
>> +#define CP210X_PARTNUM_CP2102N_QFN20 0x22
>> #define CP210X_PARTNUM_UNKNOWN 0xFF
>> 
>> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>> @@ -758,8 +761,12 @@ static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
>> /*
>> * cp210x_quantise_baudrate
>> * Quantises the baud rate as per AN205 Table 1
>> + * The CP2102N is fully (except for baud rate aliasing) software-
>> + * compatible, but supports some additional baudrates. However, there is
>> + * no quantitisation table available for this model, so in this case we
>> + * take the supported baudrate which is closest to the requested one.
>> */
>> -static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>> +static unsigned int cp210x_quantise_baudrate(unsigned int baud, bool cp2102n)
> 
> Pass in a struct usb_serial (or port) as a first argument instead which
> allows for more readable code as well as for this to be reused to handle
> other device type differences (e.g. only 2108 besides 2102n handles
> baudrates over 921.6k).
> 
>> {
>> if (baud <= 300)
>> baud = 300;
>> @@ -790,10 +797,17 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>> else if (baud <= 491520) baud = 460800;
>> else if (baud <= 567138) baud = 500000;
>> else if (baud <= 670254) baud = 576000;
>> - else if (baud < 1000000)
>> - baud = 921600;
>> - else if (baud > 2000000)
>> - baud = 2000000;
>> + else if (cp2102n) {
> 
> Add a static helper (looks like you add a define in the gpio patch)
> cp210x_is_cp2102n(serial) here. You can even test for bit 0x20 in the
> helper if you prefer (we can always change that later if needed).
> 
>> + if (baud <= 960800) baud = 921600;
>> + else if (baud <= 1100000) baud = 1000000;
>> + else if (baud <= 1350000) baud = 1200000;
>> + else if (baud <= 1750000) baud = 1500000;
>> + else if (baud <= 2500000) baud = 2000000;
>> + else baud = 3000000;
> 
> And even if the current code uses this odd formatting, your amendments
> should not.
> 
>> + } else {
>> + if (baud < 1000000) baud = 921600;
>> + else if (baud > 2000000) baud = 2000000;
>> + }
>> return baud;
>> }
>> 
>> @@ -1045,16 +1059,19 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>> static void cp210x_change_speed(struct tty_struct *tty,
>> struct usb_serial_port *port, struct ktermios *old_termios)
>> {
>> - u32 baud;
>> -
>> - baud = tty->termios.c_ospeed;
>> + bool is_cp2102n;
>> + u32 baud = tty->termios.c_ospeed;
>> + struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
>> 
>> - /* This maps the requested rate to a rate valid on cp2102 or cp2103,
>> - * or to an arbitrary rate in [1M,2M].
>> + /* This maps the requested rate to a rate valid on cp2102(n) or
>> + * cp2103 or to an arbitrary rate in [1M,2M].
>> *
>> * NOTE: B0 is not implemented.
>> */
>> - baud = cp210x_quantise_baudrate(baud);
>> + is_cp2102n = (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
>> + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
>> + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
>> + baud = cp210x_quantise_baudrate(baud, is_cp2102n);
> 
> So most of these changes would not be needed. Just pass in port->serial
> to cp210x_quantise_baudrate().
> 
> Thanks,
> Johan

Powered by blists - more mailing lists