Messages in this thread | | | Subject | Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x | From | Andy Shevchenko <> | Date | Tue, 01 Mar 2016 18:46:22 +0200 |
| |
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
| |