lkml.org 
[lkml]   [2016]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x
    From
    Date
    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@gmail.com>
    > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

    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 <andriy.shevchenko@linux.intel.com>
    Intel Finland Oy

    \
     
     \ /
      Last update: 2016-03-01 12:41    [W:4.361 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site