MIME-Version: 1.0
In-Reply-To: <1456862903-12392-1-git-send-email-ynvich@gmail.com>
References: <1456862078-11795-1-git-send-email-ynvich@gmail.com>
	<1456862903-12392-1-git-send-email-ynvich@gmail.com>
Date: Tue, 1 Mar 2016 22:23:40 +0200
Message-ID: <CAHp75Ve-s+vP3BQcrT22PyQC34vFrM6Esz6sy2UoRCOH8tkdYQ@mail.gmail.com>
Subject: Re: [PATCH v9] serial: support for 16550A serial ports on LP-8x4x
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Sergei Ianovich <ynvich@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Arnd Bergmann <arnd@arndb.de>, Rob Herring <robh+dt@kernel.org>,
        Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Joachim Eastwood <manabian@gmail.com>,
        Paul Burton <paul.burton@imgtec.com>, Mans Rullgard <mans@mansr.com>,
        Scott Wood <scottwood@freescale.com>,
        Paul Gortmaker <paul.gortmaker@windriver.com>,
        Peter Ujfalusi <peter.ujfalusi@ti.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2656
Lines: 85

On Tue, Mar 1, 2016 at 10:08 PM, Sergei Ianovich <ynvich@gmail.com> 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>

Sorry, but still few nitpicks and then
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> +#define LP8841_DATA_LEN_MASK           0x3
> +#define LP8841_DATA_LEN_SHIFT_OFFSET   3

No need to have _OFFSET suffix.

> +
> +static void lp8841_serial_set_termios(struct uart_port *port,
> +               struct ktermios *termios, struct ktermios *old)
> +{
> +       unsigned int baud;
> +       u8 value;

> +       unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> +       struct lp8841_serial_data *data = port->private_data;

I would rearrange to have assignments first in the definition block.

Code duplication (no idea if it worth to fix):

+       case 2400:
+               value |= 1;
+               break;
+       default:
+               value |= 1;
+               tty_termios_encode_baud_rate(termios, 2400, 2400);
+               break;

> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> +       struct uart_8250_port uart = {};

No need {} since memset().

> +       struct lp8841_serial_data *data;
> +       struct resource *mmres, *mires;
> +       int ret;
> +
> +       memset(&uart, 0, sizeof(uart));

Move this closer to the first assignment of a field.

> +
> +       mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       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);
> +

+       memset(&uart, 0, sizeof(uart));

> +       uart.port.iotype = UPIO_MEM;
> +       uart.port.mapbase = mmres->start;
> +       uart.port.regshift = 1;
> +       uart.port.irq = platform_get_irq(pdev, 0);
> +       uart.port.flags = UPF_IOREMAP;
> +       uart.port.dev = &pdev->dev;
> +       uart.port.uartclk = 14745600;
> +       uart.port.set_termios = lp8841_serial_set_termios;
> +       uart.port.private_data = data;

-- 
With Best Regards,
Andy Shevchenko