Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers

From: Johan Hovold <johan@kernel.org>
To: Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>,
	Johan Hovold <johan@kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@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
Date: Thu, 14 Jan 2016 19:53:11 +0100	[thread overview]
Message-ID: <20160114185311.GA7211@localhost> (raw)
In-Reply-To: <BLUPR0701MB157297A9CB514F414CCA583691CC0@BLUPR0701MB1572.namprd07.prod.outlook.com>

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