lkml.org 
[lkml]   [2016]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x
    From
    Date
    On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
    > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
    > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:

    > > > + len &= 3;

    Mask as well to be defined.

    > > > + len <<= 3;
    > >
    > > Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
    >
    > OK


    > + baud = tty_termios_baud_rate(termios);
    > > > +
    > > > +#ifdef BOTHER
    > > > + /* We only support fixed rates */

    So, but if you support only fixed rates, why do you care about BOTHER
    at all?

    > > > 
    > > I think you can call this unconditionally together with case >
    > > 115200.
    >
    > The calls are orthogonal. This one deals with the case when BOTHER is
    > defined and set, and we have non-zero rate with BOTHER, but we have
    > zero rate after BOTHER is cleared. So we set 9600 as a sane default
    > speed.

    > +
    > > > + /* We only support up to 115200 */
    > > > + if (baud > 115200) {
    > > > + baud = 115200;
    > > > + tty_termios_encode_baud_rate(termios, baud,
    > > > baud);
    > > > + }
    >
    > This one deals with the case when the rate is over 115200. If the
    > previous case has been triggered, this one won't be.

    Yeah, but I meant to unconditionally call it just once here every time.
     tty_termios_encode_baud_rate(termios, baud, baud);

    > +static int lp8841_serial_probe(struct platform_device *pdev)
    > > > +{
    > > > +     struct uart_8250_port uart = {};
    > >
    > > {0}
    >
    > ---
    > drivers/tty/serial/8250/8250_lp8841.c: In function
    > 'lp8841_serial_probe':
    > drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess
    > elements in struct initializer
    >   struct uart_8250_port uart = {0};
    >                                 ^
    > drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near
    > initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')

    Do you have any warning verbosity enabled? I see a lot of stuff like
    this in the code

    $ git grep -n 'struct .* = {0};' | wc -l
    338

    $ git grep -n 'struct .* = { \?0 \?};' | wc -l
    550

    ( '… = { 0 };' included)

    > ---
    >
    > Zero triggers a warning. I'll use memset().

    Either will work.

    > Thanks for lightning fast reviews. I'll resubmit v8 if there is no
    > objections to the points above.

    See above.

    --
    Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Intel Finland Oy

    \
     
     \ /
      Last update: 2016-03-01 18:21    [W:3.467 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site