Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423696Ab2KOUSs (ORCPT ); Thu, 15 Nov 2012 15:18:48 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:44898 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1423636Ab2KOUSr (ORCPT ); Thu, 15 Nov 2012 15:18:47 -0500 Date: Thu, 15 Nov 2012 23:18:17 +0300 From: Dan Carpenter To: YAMANE Toshiaki Cc: devel@driverdev.osuosl.org, Mauro Carvalho Chehab , Greg Kroah-Hartman , Rusty Russell , linux-kernel@vger.kernel.org, Alan Stern Subject: Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c Message-ID: <20121115201817.GO11515@mwanda> References: <1352496782-7246-1-git-send-email-yamanetoshi@gmail.com> <1352496836-7280-1-git-send-email-yamanetoshi@gmail.com> <20121114124110.GG11515@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: acsinet22.oracle.com [141.146.126.238] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3170 Lines: 85 On Fri, Nov 16, 2012 at 05:01:55AM +0900, YAMANE Toshiaki wrote: > On Wed, Nov 14, 2012 at 9:41 PM, Dan Carpenter wrote: > > > > Why can't we test whether i == (RxCount - 3) earlier and handle > > the errors there? That way we wouldn't need to pass the limit > > variable. > > > > In fact, this whole function is sort of nasty. We start by doing > > a switch (data[i + 2]) { then we combine the 0x00 and 0x01 and call > > this function which separates them out and sets a function pointer > > and then calls the function point? Get rid of this whole function. > > > > You shouldn't need to use function pointers to do this; that's too > > many levels of abstraction. > > I feel it so diffcult to consider the fixing this patch more. > > There are some reasons why I have become such a description. > - The purpose of this patch is the resolution of the > line over 80 characters issue > - I Wrote the code to be aware of the following: > -- Do not change the procedure > -- The shallow nest > -- To avoid the redundancy > > If I do not use a function pointer, which take the form below. > > if (0x00 == data[i + 2]) > dev_dbg(&port->dev, "Line status status.\n"); > else > dev_dbg(&port->dev, "Modem status status.\n"); > > if (i > limit) { > dev_dbg(&port->dev, > "Illegal escape seuences in received data\n"); > return 0; > } > > if (0x00 == data[i + 2]) > ProcessLineStatus(qt_port, data[i + 3]); > else > ProcessModemStatus(qt_port, data[i + 3]); > > return 1; > > I also feel it may be... > > And I am against to move the dev_dbg procedure call to > qt_status_change_check procedure because the nesting will be so deep. In the end, the new version is more confusing than the original code. Checkpatch.pl is not a king which must be obeyed. The only thing which matters is how easy it is for a human to understand the code. > > >> if (urb->status) { > >> qt_port->ReadBulkStopped = 1; > >> - dev_dbg(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n", > >> + dev_dbg(&urb->dev->dev, > >> + "%s - nonzero write bulk status received: %d\n", > >> __func__, urb->status); > > > > Don't mix in these unrelated 80 character limit changes. > > I think the purpose of refactoring is the resolution of the line over 80 > characters issue. I think that the separation of the patch should stop taking > because they are already applied in the linux-next tree. > Yes, once it is merged into linux-next then it is too late to send a version 2 patch. I'm explaining that as a reviewer it is confusing for me to figure out when you do unrelated things in the same patch and mix everything up. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/