Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Johan Hovold]

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