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@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
