On Tue, Oct 20, 2015 at 11:22 AM, Johan Hovold <johan@xxxxxxxxxx> wrote: > 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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. Theoretically it shouldn't be a problem, but since it hasn't been there for other devices, not causing any complaints, I'm reluctant to do this. >> 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 Yes, this looks OK for an "unconditional purge" option. Otherwise it would be - use port data for private data - fix both bugs Since the purge workaround is almost a one-liner, it won't noticeably increase the diff, and nobody would ever want to apply just one workaround of the 2 and continue to experience device hangs. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
