From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752160AbbJTQWR (ORCPT ); Tue, 20 Oct 2015 12:22:17 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:36618 "EHLO mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750878AbbJTQWN (ORCPT ); Tue, 20 Oct 2015 12:22:13 -0400 Date: Tue, 20 Oct 2015 18:22:16 +0200 From: Johan Hovold To: Konstantin Shkolnyy Cc: Johan Hovold , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions Message-ID: <20151020162216.GC26805@localhost> References: <1445292084-27910-1-git-send-email-konstantin.shkolnyy@gmail.com> <20151020074516.GA20289@localhost> <20151020130256.GE20289@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 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 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