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@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
