Re: [PATCH v1 09/12] serial: 8250_lpss: split LPSS driver to separate module

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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 <peter@hurleysoftware.c
> > 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy