On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:

On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:

quoted

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 <redacted>
Reviewed-by: Heikki Krogerus <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Few more comments, mostly about style.

quoted

+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)

quoted

+	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++;

quoted

+	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

quoted

+	len -= 9;

This one could be part of previous evaluation:
 value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;

Great point.

quoted

+	len &= 3;
+	len <<= 3;

Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).

OK

quoted

+
+	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;

quoted

+				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.

quoted

+			}
+		}
+	}
+#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().

quoted

+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().

quoted

+     struct lp8841_serial_data *data;
+     struct resource *mmres, *mires;
+     int ret;
+
+     mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);

quoted

+     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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html