linux-kernel - Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1456830401.13244.189.camel@linux.intel.com>
Date:	Tue, 01 Mar 2016 13:06:41 +0200
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Sergei Ianovich <ynvich@...il.com>, linux-kernel@...r.kernel.org
Cc:	Alan Cox <gnomes@...rguk.ukuu.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Masahiro Yamada <yamada.masahiro@...ionext.com>,
	Paul Burton <paul.burton@...tec.com>,
	Mans Rullgard <mans@...sr.com>,
	Joachim Eastwood <manabian@...il.com>,
	Scott Wood <scottwood@...escale.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Peter Ujfalusi <peter.ujfalusi@...com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@...r.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x

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@...il.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.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@...ux.intel.com>
Intel Finland Oy