Re: [PATCH v2 2/3] ARM: clps711x: Add serial driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sat, Oct 20, 2012 at 05:50:58PM +0400, Alexander Shiyan wrote:
> On Tue, 16 Oct 2012 23:39:56 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> ...
> > > > We already had this. This SoC has a very strange register layout. The
> > > > registers for the UARTs are spread around the SoC and the registers
> > > > are not even exclusively used for the UARTs. They have bits which are
> > > > also used for the timer and other stuff.
> > > use the device id to select the register is wrong
> > > 
> > > we need to pass the register via resource (without request) or platform data
> > > > 
> > > > > > +static int clps711x_tstc(struct console_device *cdev)
> > > > > > +{
> > > > > > +	return !(readl(SYSFLG(cdev->dev->id)) & SYSFLG_URXFE);
> > > > > factorise those pull status as
> > > > > 
> > > > > status int xxx(data, mask) {}
> > > > 
> > > > I don'r understand this. This is a single register read, there's not
> > > > much to factorize and the code is easy to read.
> > > he use the code at a least 2 place
> > 
> > Testing for a bit like above is done a thousand times all around the
> > kernel and barebox. There is nothing to factorize. The way Alexander did
> > it is perfectly fine.
> Please look on new version below. If it looks OK, I will make complete v2 patch.
> Thanks!
> 
> /*
>  * Simple CLPS711X serial driver
>  *
>  * (C) Copyright 2012 Alexander Shiyan <shc_work@mail.ru>
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License as
>  * published by the Free Software Foundation; either version 2 of
>  * the License, or (at your option) any later version.
>  */
> 
> #include <common.h>
> #include <malloc.h>
> #include <init.h>
> #include <io.h>
> #include <linux/clk.h>
> 
> #include <mach/clps711x.h>
> 
> struct clps711x_uart {
> 	void __iomem		*UBRLCR;
> 	void __iomem		*SYSCON;
> 	void __iomem		*SYSFLG;
> 	void __iomem		*UARTDR;
> 	struct clk		*uart_clk;
> 	struct console_device	cdev;
> };
> 
> static int clps711x_setbaudrate(struct console_device *cdev, int baudrate)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 	int divisor;
> 	u32 tmp;
> 
> 	divisor = (clk_get_rate(s->uart_clk) / 16) / baudrate;
> 
> 	tmp = readl(s->UBRLCR) & ~UBRLCR_BAUD_MASK;
> 	tmp |= divisor - 1;
> 	writel(tmp, s->UBRLCR);
> 
> 	return 0;
> }
> 
> static void clps711x_init_port(struct console_device *cdev)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 	u32 tmp;
> 
> 	/* Disable the UART */
> 	writel(readl(s->SYSCON) & ~SYSCON_UARTEN, s->SYSCON);
> 
> 	/* Setup Line Control Register */
> 	tmp = readl(s->UBRLCR) & UBRLCR_BAUD_MASK;
> 	tmp |= UBRLCR_FIFOEN | UBRLCR_WRDLEN8; /* FIFO on, 8N1 mode */
> 	writel(tmp, s->UBRLCR);
> 
> 	/* Set default baudrate on initialization */
> 	clps711x_setbaudrate(cdev, CONFIG_BAUDRATE);
> 
> 	/* Enable the UART */
> 	writel(readl(s->SYSCON) | SYSCON_UARTEN, s->SYSCON);
> }
> 
> static void clps711x_putc(struct console_device *cdev, char c)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 
> 	/* Wait until there is space in the FIFO */
> 	while (readl(s->SYSFLG) & SYSFLG_UTXFF)
> 		barrier();
> 
> 	/* Send the character */
> 	writew(c, s->UARTDR);
> }
> 
> static int clps711x_getc(struct console_device *cdev)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 	u16 data;
> 
> 	/* Wait until there is data in the FIFO */
> 	while (readl(s->SYSFLG) & SYSFLG_URXFE)
> 		barrier();
> 
> 	data = readw(s->UARTDR);
> 
> 	/* Check for an error flag */
> 	if (data & (UARTDR_FRMERR | UARTDR_PARERR | UARTDR_OVERR))
> 		return -1;
> 
> 	return (int)data;
> }
> 
> static int clps711x_tstc(struct console_device *cdev)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 
> 	return !(readl(s->SYSFLG) & SYSFLG_URXFE);
> }
> 
> static void clps711x_flush(struct console_device *cdev)
> {
> 	struct clps711x_uart *s = cdev->dev->priv;
> 
> 	while (readl(s->SYSFLG) & SYSFLG_UBUSY)
> 		barrier();
> }
> 
> static int clps711x_probe(struct device_d *dev)
> {
> 	struct clps711x_uart *s;
> 
> 	BUG_ON(dev->num_resources != 4);
> 
> 	s = xzalloc(sizeof(struct clps711x_uart));
> 	s->uart_clk = clk_get(dev, NULL);
> 	BUG_ON(!s->uart_clk);

clk_get returns error pointers, you have to use IS_ERR() to test for
errors.

> 
> 	s->UBRLCR = dev_get_mem_region(dev, 0);
> 	s->SYSCON = dev_get_mem_region(dev, 1);
> 	s->SYSFLG = dev_get_mem_region(dev, 2);
> 	s->UARTDR = dev_get_mem_region(dev, 3);

From my point of view this looks ok.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox