Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Johan Hovold <johan@kernel.org>]

On Tue, Oct 20, 2015 at 09:19:05AM -0500, Konstantin Shkolnyy wrote:
> On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
> >> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
> >> [...]
> >> > Instead of adding the new helpers to read u16 as a prerequisite for
> >> > fixing the broken cp2108 support, just reuse the current config register
> >> > helpers for now (in order to keep the fixes minimal and potentially
> >> > backportable). Once the fixes are in place, feel free to clean up the
> >> > remaining register accesses.
> >>
> >> The current helpers take "port" as a parameter. You pointed out
> >> previously that port shouldn't be used in probe(). That made me
> >> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
> >> that don't use port. Probe() now calls cp210x_activate_workarounds
> >> which in turn calls these new helpers.
> >
> > Oh, that's right.
> >
> >> An alternative would be to call usb_control_msg from
> >> cp210x_activate_workarounds, but I think it would make it look pretty
> >> ugly.
> >
> > Or you move the quirk-detect (and private data allocation) to port_probe
> > instead (and remove startup/release). These devices have exactly one
> > port per interface, so you wouldn't introduce any redundancy.
> 
> OK, how about this:
> patch #1
> - attach/release replaced with port_probe/port_release;
> - changes in the code using the "private" data since it has moved from
> serial to port;
> - cp210x_activate_workarounds in the port_probe, using the current helpers;
> - a new helper cp210x_get_line_ctl that calls the current helper and
> swaps the bytes.
> patch #2
> - sending "purge" from close, using the current helper.
> 
> However, the Tx queue bug fixed by the "purge" doesn't have a
> detection method. We only know that it exists in the same cp2108 that
> has the "byte swap" bug. So the "purge" is activated by the same "byte
> swap" flag and  patch #2 depends on patch #1.

So you only see the hang after close with data in fifo on cp2108? Is
this something that might get fixed as well in later revisions?

But doing a purge as part of close should not be a problem on the other
products, right? I think doing this unconditionally would be best (just
like you eventually add tx_empty for all devices). No need to depend on
the broken-lcr-quirk.

> Also, there is no reason
> to only apply one of the patches. Do you think we can merge these
> patches?

No, again you want one logical change per patch.

I suggest you split it as follows

 - fix hang with non-empty tx fifo
 - use port data for private data
 - fix broken cp2108 lcr

Thanks,
Johan