On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> > The patch adds support for 3 additional LP-8x4x built-in serial
> > ports.
> >
> > The device can also host up to 8 extension cards with 4 serial
> > ports
> > on each card for a total of 35 ports. However, I don't have
> > the hardware to test extension cards, so they are not supported,
> > yet.
> >
> > Signed-off-by: Sergei Ianovich <ynvich@xxxxxxxxx>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>
> Few more comments, mostly about style.
>
> > +static void lp8841_serial_set_termios(struct uart_port *port,
> > + struct ktermios *termios, struct ktermios *old)
> > +{
> > +#ifdef BOTHER
> > + unsigned int cbaud;
> > +#endif
> > + unsigned int baud;
> > + unsigned int len;
>
> Since you are writing this to the register at the end maybe
> - unsigned int -> u8 (write*b* — exactly one byte)
> - len -> value (it's not only about data length)
>
> > + unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> > + struct lp8841_serial_data *data = port->private_data;
> > +
> > + /* We only support CS7 and CS8 */
> > + while ((termios->c_cflag & CSIZE) != CS7 &&
> > + (termios->c_cflag & CSIZE) != CS8) {
> > + termios->c_cflag &= ~CSIZE;
> > + termios->c_cflag |= old_csize;
> > + old_csize = CS8;
> > + }
> > +
> > + serial8250_do_set_termios(port, termios, old);
> > +
> > + if ((termios->c_cflag & CSIZE) == CS7)
> > + len = 9;
> > + else
> > + len = 10;
> > +
> > + if (termios->c_cflag & CSTOPB)
> > + len++;
>
> > + if (termios->c_cflag & PARENB)
> > + len++;
> > + if (!(termios->c_cflag & PARODD))
> > + len++;
> > +#ifdef CMSPAR
> > + if (termios->c_cflag & CMSPAR)
> > + len++;
> > +#endif
>
> I don't know if someone likes it or not (up to you), but for me looks
> better to have ternary operators here:
>
> value += (termios->c_cflag & CSTOPB) ? 1 : 0;
> value += (termios->c_cflag & PARENB) ? 1 : 0;
> value += (termios->c_cflag & PARODD) ? 0 : 1;
>
> #ifdef CMSPAR
> value += (termios->c_cflag & CMSPAR) ? 1 : 0;
> #endif
>
> > + len -= 9;
>
> This one could be part of previous evaluation:
> value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
Great point.
> > + len &= 3;
> > + 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 */
> > + cbaud = termios->c_cflag & CBAUD;
> > +
> > + if (cbaud == BOTHER) {
> > + termios->c_cflag &= ~BOTHER;
> > +
> > + /* Don't rewrite B0 */
> > + if (baud) {
> > + tty_termios_encode_baud_rate(termios,
> > baud,
> > baud);
> > + baud = tty_termios_baud_rate(termios);
> > +
> > + /* Set sane default speed if we get 0 */
> > + if (!baud) {
> > + baud = 9600;
>
> > + tty_termios_encode_baud_rate(termi
> > os
> > ,
> > + baud, baud);
>
> 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.
> > + }
> > + }
> > + }
> > +#endif
> > +
> > + /* 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.
> Btw, can we use uart_get_baud_rate() here?
uart_get_baud_rate() has already been called
in serial8250_do_set_termios(). uart_get_baud_rate()
calls tty_termios_encode_baud_rate(). uart_get_baud_rate() won't help
us if BOTHER is set. Once BOTHER is cleared, we don't need any special
processing of uart_get_baud_rate().
> > +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')
---
Zero triggers a warning. I'll use memset().
> > + struct lp8841_serial_data *data;
> > + struct resource *mmres, *mires;
> > + int ret;
> > +
> > + mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> > + mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>
> Perhaps move it down to be closer to devm_ioremap_resource() call.
OK
Thanks for lightning fast reviews. I'll resubmit v8 if there is no
objections to the points above.
--
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
