Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1768966Ab2KOUcE (ORCPT ); Thu, 15 Nov 2012 15:32:04 -0500 Received: from mail-vb0-f46.google.com ([209.85.212.46]:63016 "EHLO mail-vb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1768923Ab2KOUcC (ORCPT ); Thu, 15 Nov 2012 15:32:02 -0500 MIME-Version: 1.0 In-Reply-To: <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> <20121115201817.GO11515@mwanda> Date: Fri, 16 Nov 2012 05:32:00 +0900 Message-ID: Subject: Re: [PATCH 2/4] staging/serqt_usb2: refactor qt_read_bulk_callback() in serqt_usb2.c From: YAMANE Toshiaki To: Dan Carpenter Cc: devel@driverdev.osuosl.org, Mauro Carvalho Chehab , Greg Kroah-Hartman , Rusty Russell , linux-kernel@vger.kernel.org, Alan Stern Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3438 Lines: 94 On Fri, Nov 16, 2012 at 5:18 AM, Dan Carpenter wrote: > 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. Yes. I understand it. I wil condider the improvement this. >> >> 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. I understand it. Thanks for your comments. -- Regards, YAMANE Toshiaki -- 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/