Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754871AbcDKSZ1 (ORCPT ); Mon, 11 Apr 2016 14:25:27 -0400 Received: from pygmy.kinoho.net ([134.0.27.24]:44709 "EHLO pygmy.kinoho.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751085AbcDKSZZ (ORCPT ); Mon, 11 Apr 2016 14:25:25 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 11 Apr 2016 20:25:22 +0200 From: Grigori Goronzy To: Karl Palsson Cc: Greg Kroah-Hartman , Johan Hovold , Linux Kernel Mailing List , Linux USB Mailing List Subject: Re: [PATCH v3 06/13] USB: ch341: add support for parity, frame length, stop bits In-Reply-To: References: <1460331833-19836-7-git-send-email-greg@chown.ath.cx> Message-ID: User-Agent: Roundcube Webmail/1.1.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2545 Lines: 80 On 2016-04-11 19:25, Karl Palsson wrote: > Sorry if you get this twice, I was having some client problems, > but wanted to make sure you got this one... > > > Grigori Goronzy wrote: >> With the new reinitialization method, configuring parity, >> different frame lengths and different stop bit settings work as >> expected on both CH340G and CH341A. This has been extensively >> tested with a logic analyzer. >> >> v2: only set mark/space when parity is enabled, >> simplifications, patch termios HW flags. >> >> Tested-by: Ryan Barber >> Signed-off-by: Grigori Goronzy >> --- >> drivers/usb/serial/ch341.c | 40 >> ++++++++++++++++++++++++++++++---------- >> 1 file changed, 30 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/usb/serial/ch341.c >> b/drivers/usb/serial/ch341.c index 6181616..99b4621 100644 >> --- a/drivers/usb/serial/ch341.c >> +++ b/drivers/usb/serial/ch341.c >> @@ -341,7 +341,6 @@ static void ch341_set_termios(struct tty_struct >> *tty, >> struct usb_serial_port *port, struct ktermios *old_termios) >> { >> struct ch341_private *priv = usb_get_serial_port_data(port); >> - unsigned baud_rate; >> unsigned long flags; >> unsigned char ctrl; >> int r; >> @@ -350,13 +349,39 @@ static void ch341_set_termios(struct tty_struct >> *tty, >> if (old_termios && !tty_termios_hw_change(&tty->termios, >> old_termios)) >> return; >> >> - baud_rate = tty_get_baud_rate(tty); >> + priv->baud_rate = tty_get_baud_rate(tty); >> >> - priv->baud_rate = baud_rate; >> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX; >> >> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8; >> + switch (C_CSIZE(tty)) { >> + case CS5: >> + ctrl |= CH341_LCR_CS5; >> + break; >> + case CS6: >> + ctrl |= CH341_LCR_CS6; >> + break; >> + case CS7: >> + ctrl |= CH341_LCR_CS7; >> + break; >> + default: >> + tty->termios.c_cflag |= CS8; >> + case CS8: >> + ctrl |= CH341_LCR_CS8; >> + break; >> + } >> + >> + if (C_PARENB(tty)) { >> + ctrl |= CH341_LCR_ENABLE_PAR; >> + if (C_PARODD(tty)) >> + ctrl |= CH341_LCR_PAR_EVEN; > > Are you sure this does the right thing now? this is, as best as I > can tell, the inverse of what you had earlier, and doesn't read > right, if this is working, then I suggest renaming _LCR_PAR_EVEN > to LCR_PAR_ODD? > No, this is absolutely wrong, of course. I only did some sporadic testing because my refactoring wasn't supposed to change functionality. Thanks for pointing it out! Grigori