Message-ID: <b82f656ba4c2c05d86ae7149be4109c6@pados.hu>
Date:   Wed, 20 Jun 2018 09:51:41 +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

> By the way, have you tried setting other baudrates except the ones you
> explicitly allow for here? According to the data sheet more rates should
> be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
> trying to report back the exact rate used) or by actually calculating
> the resulting rate could be another option?
> 
> Can be done later of course, just curious if you tried it.
> 

Yeah I know, I was thinking about this too while developing the patch. 
Officially the cp2102 and the cp2102n are fully software compatible (aside
from baudrate aliasing), but if the cp2102n chooses different baudrates for 
the same inputs than the older devices would then they couldn't/wouldn't be
compatible. So I concluded it must also be doing the quantisation.

Maybe I am too naive and trust the datasheet to much. I'll do some measurements
with my scope and let you know the results.

Best,
Karoly

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

> On Tue, Jun 19, 2018 at 09:50:54AM +0000, Karoly Pados wrote:
> 
>> 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?
> 
> No, that's fine. And you can submit it as a series, where the first
> patch using the helper includes it (e.g. the baud rate one).
> 
>> 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?
> 
> Sure, I have no strong preference. You can keep the explicit package type
> enumeration if you want, but move it to a static helper. We'll see how
> this ends up being used in the series.
> 
>> 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.
> 
> Yeah, that's admittedly a bit arbitrary. I should probably just clean
> this up somehow once and for all.
> 
> By the way, have you tried setting other baudrates except the ones you
> explicitly allow for here? According to the data sheet more rates should
> be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
> trying to report back the exact rate used) or by actually calculating
> the resulting rate could be another option?
> 
> Can be done later of course, just curious if you tried it.
> 
> Thanks,
> Johan

Powered by blists - more mailing lists