Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932080AbcDKNIJ (ORCPT ); Mon, 11 Apr 2016 09:08:09 -0400 Received: from mga03.intel.com ([134.134.136.65]:21252 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754635AbcDKNIH (ORCPT ); Mon, 11 Apr 2016 09:08:07 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,462,1455004800"; d="scan'208";a="952515014" Message-ID: <1460380142.6620.75.camel@linux.intel.com> Subject: Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module From: Andy Shevchenko To: Peter Hurley , Andy Shevchenko Cc: Vinod Koul , "linux-kernel@vger.kernel.org" , dmaengine , Greg Kroah-Hartman , "Puustinen, Ismo" , Heikki Krogerus , "linux-serial@vger.kernel.org" Date: Mon, 11 Apr 2016 16:09:02 +0300 In-Reply-To: <57083445.9090009@hurleysoftware.com> References: <1460061433-63750-1-git-send-email-andriy.shevchenko@linux.intel.com> <1460061433-63750-10-git-send-email-andriy.shevchenko@linux.intel.com> <57070C93.2090000@hurleysoftware.com> <57083445.9090009@hurleysoftware.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4378 Lines: 132 On Fri, 2016-04-08 at 15:44 -0700, Peter Hurley wrote: > On 04/08/2016 01:17 AM, Andy Shevchenko wrote: > > > > On Fri, Apr 8, 2016 at 4:42 AM, Peter Hurley > om> wrote: > > > > > > On 04/07/2016 01:37 PM, Andy Shevchenko wrote: > > > > > > > > Intes SoCs, such as Braswell, have DesignWare UART. Split out to > > > > separate > > > > module which also will be used for Intel Quark later. > > > What's the rationale? > > 1. Not poison 8250_pci with too many quirks. > > 2. They all use same DMA engine, otherwise we might end up in all > > possible DMA engines included in one file. > > 3. All of them are actually DesignWare, so, in the future we might > > share code between 8250_dw and 8250_lpss. > Just my opinion, but I like to see the rationale in the changelog. Agreed. Already did locally. >  > > > And this really isn't a split; this patch introduces a number of > > > significant > > > changes from the pci version. > > Some style changes, yes, but "significant"? > > For example? > I'm just pointing out the changelog doesn't really match the > commit. I'm not suggesting necessarily to redo the series, but just > more > adequately reflect the change. See below. > +struct lpss8250_board { > > > > +     unsigned long freq; > > > > +     unsigned int base_baud; > > > > +     int (*setup)(struct lpss8250 *, struct uart_port *p); > > > > +}; > > > New concept. > > > +struct lpss8250 { > > > > +     int line; > > > > +     struct lpss8250_board *board; > > > > + > > > > +     /* DMA parameters */ > > > > +     struct uart_8250_dma dma; > > > > +     struct dw_dma_slave dma_param; > > > > +     u8 dma_maxburst; > > > > +}; > > > > +static int byt_serial_setup(struct lpss8250 *lpss, struct > > > > uart_port *port) > > > This would have been much easier to review if you had simply moved > > > it here > > > and done the rework in a follow-on patch. > > I didn't quite get this one. > Well, just comparing byt_serial_setup() from the two versions: > 1) dma setup is in a completely separate function > 2) the tx & rx dma parameters are folded together > 3) the port setup is split out > 4) introduction of struct lpss8250 > ... > > > > > How series should look like? > I would have just moved byt_serial_setup() without any of the other > changes > except perhaps replacing pciserial_board with lpss8250_board, and > then made the other changes on top before the Quark patches. > > There is no changelog describing the purpose of struct lpss8250_board, > or > struct lpss8250. Or of the dma changes. Or why dma_maxburst was > parameterized. > ... > > Naturally, I can figure all of that out on my own, but it would have > been > better to read your reasoning. > > It looks alot of work to split out now, so I guess what's done is > done, and I'll > just review this *really* carefully. But imagine if you hadn't wrote > it, and > were reviewing this: it's very difficult to mentally separate out the > changes > and keep track of them while reviewing. Side-by-side diff is nearly > useless... > I sent new version, please, review. > > > >  > > > > +             ret = lpss->board->setup(lpss, &uart.port); > > > > +             if (ret) > > > > +                     return ret; > > > > +     } > > > > + > > > > +     ret = lpss8250_dma_setup(lpss, &uart); > > > > +     if (ret) > > > > +             return ret; > > > I would fold this call into board setup which avoids the > > > ugliness when this error pathway is reworked in the > > > follow-on patches. > > Each of them? > I'm assuming there's just the two: byt_serial_setup() > and qrk_serial_setup()? For now yes. > > Did I overlook something? Perhaps I missed some design goal? Nope.  But I still prefer to have separate _dma_setup() helper. I didn't see too much ugliness in the next patches. > > > > +     .probe          = lpss8250_probe, > > > > +     .remove         = lpss8250_remove, > > > No power management? > > PCI does the trick, no *special* power management treatment > > required, yes. > I realized that later this am; sorry about that. > [Maybe just put a small note in the changelog about that though?] OK. -- Andy Shevchenko Intel Finland Oy