From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Woodhouse Subject: Re: [PATCH 1/2] i2c-i801: Handle multiple instances instead of keeping global state Date: Sun, 31 Oct 2010 10:11:43 -0400 Message-ID: <1288534303.4570.32.camel@macbook.infradead.org> References: <20101030182458.0849f295@endymion.delvare> <1288481663.4570.19.camel@macbook.infradead.org> <20101030233930.GQ21564@trinity.fluff.org> <1288482478.4570.23.camel@macbook.infradead.org> <20101031101953.45b3dabf@endymion.delvare> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20101031101953.45b3dabf-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jean Delvare Cc: Ben Dooks , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-i2c@vger.kernel.org On Sun, 2010-10-31 at 10:19 +0100, Jean Delvare wrote: > On Sat, 30 Oct 2010 19:47:58 -0400, David Woodhouse wrote: > > On Sun, 2010-10-31 at 00:39 +0100, Ben Dooks wrote: > > > On Sat, Oct 30, 2010 at 07:34:23PM -0400, David Woodhouse wrote: > > > > On Sat, 2010-10-30 at 18:24 +0200, Jean Delvare wrote: > > > > > An explanation why this change is needed would be nice. > > > > > > > > Um, does it really need explaining? It's really poor form to keep driver > > > > state in global variables rather than per-instance, even if you *don't* > > > > actually have more than one device. > > This is actually a good second half for an explanation paragraph. > > > > I always like to fill it in, it makes it easier for lazy folks who can't > > > be bothered to read the patch itself. > > > > The explanation fits in one line: > > "Handle multiple instances instead of keeping global state" > > No, this isn't en explanation. This is a statement of _what_ your patch > does. This is needed but not sufficient. What is additionally requested > from you is an explanation _why_ it does it. > > > I could try adding a paragraph explaining further that this patch will > > make the driver handle multiple instances of the device, by keeping its > > state per-device instead of in global variables. But that seems > > somewhat... redundant. > > You want to explain that the assumption of the original driver author > that there can be only one supported device with one SMBus interface on > a given system no longer holds true for new devices to be released soon. Pfft. That was covered by this line in the patch itself: -/* Note: we assume there can only be one I801, with one SMBus interface */ I cannot imagine a realistic case where any further comment in the changelog would really be useful -- where a competent person glancing at the resulting commit would be in *any* doubt as to what was being done and why. But since it's easier to provide a redundant 'explanation' than to argue about it, I'll include that in the next version too. -- David Woodhouse Open Source Technology Centre David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org Intel Corporation