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 <[email protected]>
> Reviewed-by: Heikki Krogerus <[email protected]>
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;
> + len &= 3;
> + len <<= 3;
Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
> +
> + 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(termios
> ,
> + baud, baud);
I think you can call this unconditionally together with case > 115200.
> + }
> + }
> + }
> +#endif
> +
> + /* We only support up to 115200 */
> + if (baud > 115200) {
> + baud = 115200;
> + tty_termios_encode_baud_rate(termios, baud, baud);
> + }
Btw, can we use uart_get_baud_rate() here?
> + writeb(len, data->ios_mem);
> +
> +}
> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> + struct uart_8250_port uart = {};
{0}
> + 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.
> + if (!mmres)
> + return -ENODEV;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
+ mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
> + if (IS_ERR(data->ios_mem))
> + return PTR_ERR(data->ios_mem);
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
