From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755997AbcANSxM (ORCPT ); Thu, 14 Jan 2016 13:53:12 -0500 Received: from mail-lb0-f193.google.com ([209.85.217.193]:34647 "EHLO mail-lb0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754824AbcANSxK (ORCPT ); Thu, 14 Jan 2016 13:53:10 -0500 Date: Thu, 14 Jan 2016 19:53:11 +0100 From: Johan Hovold To: Konstantin Shkolnyy Cc: Martyn Welch , Johan Hovold , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers Message-ID: <20160114185311.GA7211@localhost> References: <1451704360-12011-1-git-send-email-konstantin.shkolnyy@gmail.com> <5697E03D.9040209@collabora.co.uk> 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 Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote: > The patching procedure enforced by maintainers dictates that each > separate patch addresses 1 issue. > It's much easier to review changes this way. So this particular patch > just converts from one register access function to another. > The bugfix patch will come later. Do one logical change per patch is a good rule of thumb, yes. But we must never introduce regression by taking that rule too far. I haven't had time to review your patches yet, but I remember thinking that those FIXMEs looked odd. How about adding the missing error handling before you introduce the new helpers? > While doing this cleanup, I also noticed another bug - this function > will always set the low 2 bits of byte 0 to 01b: "modem_ctl[0] |= > 0x01". > This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held > inactive"). The function will only write it when CRTSCTS changes. > So the device will start with 0, then, if CRTSCTS ever changes, it'll > become 1 and stay 1 forever. Looks wrong to me. > I'm still researching the subject when and how it should be set. > > * Wikipedia: "DTR and DSR are usually on all the time and, per the > * RS-232 standard and its successors, are used to signal from each > * end that the other equipment is actually present and powered-up." > * So perhaps DTR should be turned ON in open() and OFF in close()? > > I'm waiting on this patch series to be accepted, then submit other > improvements. Or it may be better to submit a longer patch series that > has further improvements appended... I'm new here and not really sure. Generally, you should wait for a series to be reviewed before sending (too many) follow ups. Unless you find any issues with it and ask for it to be dropped, that is. Thanks, Johan