Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings

From: Russell Senior <russell@personaltelco.net>
To: Johan Hovold <johan@kernel.org>
Cc: Aidan Thornton <makosoft@gmail.com>,
	Linux USB Mailing List <linux-usb@vger.kernel.org>,
	Grigori Goronzy <greg@chown.ath.cx>,
	Karl Palsson <karlp@tweak.net.au>,
	Eddi De Pieri <eddi@depieri.net>, stable <stable@vger.kernel.org>
Subject: Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings
Date: Fri, 16 Dec 2016 08:04:18 -0800	[thread overview]
Message-ID: <CAHP3WfPmi2es-ZqJhx4NZr3fvorbgv5Yyy_v8chKLaoOiGutfQ@mail.gmail.com> (raw)
In-Reply-To: <20161216144614.GB21690@localhost>

Sorry, I got distracted.  I'm back now.  Do you want me to test your
13 patch series?  And what is that on top of?

Thanks

On Fri, Dec 16, 2016 at 6:46 AM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Dec 16, 2016 at 01:19:05PM +0000, Aidan Thornton wrote:
>> On Wed, Dec 14, 2016 at 3:28 PM, Johan Hovold <johan@kernel.org> wrote:
>> > The ch341 driver is based on reverse-engineering and contains some bits
>> > which appear to be redundant and some which appear incompatible which
>> > certain devices.
>> >
>> > Specifically, some CH340 devices seem unable to change the initial line
>> > settings, which have so far been set to 5N1. Let's use a more reasonable
>> > 8N1 default instead.
>> >
>> > Note that we also need to set the ENABLE_RX bit or receive will be
>> > disabled after a reset during resume.
>>
>> Lost track a little of the testing, but I don't think you ever got
>> Russel to test whether this worked using a driver that tried to change
>> this through direct register writes which seem to be the only thing
>> that has any effect on this strange hardware variant. Would be a
>> little surprised if it didn't at this point - changing the line
>> settings that way after initialization certainly works well enough to
>> cause us all a headache.
>
> I believe he did test this against my usb-linus branch, which did not
> have your recent changes. IIRC both removing that first LCR write and
> changing it to 0xc3 worked, but maybe you're right and we only verified
> removing it.
>
> In the same setup, changing the word size after successful
> initialisation using direct register writes did not work however, and
> the device appeared stuck at 5N1 or 8N1 depending on if the initial 0x50
> LCR write was left in or not.
>
>> If so, I suggest it might be clearer to drop this patch altogether in
>> favour of patch 10 in the series, since the underlying problem is that
>> setting the baudrate and LCR register didn't work and patch 10 fixes
>> that.
>
> These two patches are definitely related, and I struggled a bit about
> how to order things as I wanted to find a way to backport this minimal
> change (from 5N1 to 8N1) without having to depend on the upcoming
> changes in 4.10 (your changes) as that may be enough to enable support
> in older kernels.
>
> I'll respin this, once we learn more about how those quirky devices
> work.
>
> Thanks,
> Johan