[PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Antony Pavlov]

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 drivers/serial/serial_ns16550.c |   87 ++++++++++++++++++++++++---------------
 1 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c
index 4ed2671..ec93750 100644
--- a/drivers/serial/serial_ns16550.c
+++ b/drivers/serial/serial_ns16550.c
@@ -48,6 +48,23 @@
 
 /*********** Private Functions **********************************/
 
+#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
+static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
+		unsigned char reg_idx)		\
+{						\
+	return cmdr((char *)base + reg_idx);	\
+}						\
+						\
+static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
+		unsigned char reg_idx)			\
+{							\
+	cmdw((mask val), (char *)base + reg_idx);	\
+}
+
+NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
+NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
+NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
+
 /**
  * @brief read register
  *
@@ -60,22 +77,10 @@ static uint32_t ns16550_read(struct console_device *cdev, uint32_t off)
 {
 	struct device_d *dev = cdev->dev;
 	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
-	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
 
 	off <<= plat->shift;
 
-	if (plat->reg_read)
-		return plat->reg_read((unsigned long)dev->priv, off);
-
-	switch (width) {
-	case IORESOURCE_MEM_8BIT:
-		return readb(dev->priv + off);
-	case IORESOURCE_MEM_16BIT:
-		return readw(dev->priv + off);
-	case IORESOURCE_MEM_32BIT:
-		return readl(dev->priv + off);
-	}
-	return -1;
+	return plat->reg_read((unsigned long)dev->priv, off);
 }
 
 /**
@@ -90,26 +95,10 @@ static void ns16550_write(struct console_device *cdev, uint32_t val,
 {
 	struct device_d *dev = cdev->dev;
 	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
-	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
 
 	off <<= plat->shift;
 
-	if (plat->reg_write) {
-		plat->reg_write(val, (unsigned long)dev->priv, off);
-		return;
-	}
-
-	switch (width) {
-	case IORESOURCE_MEM_8BIT:
-		writeb(val & 0xff, dev->priv + off);
-		break;
-	case IORESOURCE_MEM_16BIT:
-		writew(val & 0xffff, dev->priv + off);
-		break;
-	case IORESOURCE_MEM_32BIT:
-		writel(val, dev->priv + off);
-		break;
-	}
+	plat->reg_write(val, (unsigned long)dev->priv, off);
 }
 
 /**
@@ -239,9 +228,41 @@ static int ns16550_probe(struct device_d *dev)
 	/* we do expect platform specific data */
 	if (plat == NULL)
 		return -EINVAL;
-	if (!(dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK) &&
-	    ((plat->reg_read == NULL) || (plat->reg_write == NULL)))
-		return -EINVAL;
+
+	if (plat->reg_read == NULL) {
+		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
+
+		switch (width) {
+		default:
+		case IORESOURCE_MEM_8BIT:
+			plat->reg_read = ns16550_generic_8bit_read;
+			break;
+		case IORESOURCE_MEM_16BIT:
+			plat->reg_read = ns16550_generic_16bit_read;
+			break;
+		case IORESOURCE_MEM_32BIT:
+			plat->reg_read = ns16550_generic_32bit_read;
+			break;
+		}
+	}
+
+	if (plat->reg_write == NULL) {
+		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
+
+		switch (width) {
+		default:
+		case IORESOURCE_MEM_8BIT:
+			plat->reg_write = ns16550_generic_8bit_write;
+			break;
+		case IORESOURCE_MEM_16BIT:
+			plat->reg_write = ns16550_generic_16bit_write;
+			break;
+		case IORESOURCE_MEM_32BIT:
+			plat->reg_write = ns16550_generic_32bit_write;
+			break;
+		}
+	}
+
 	dev->priv = dev_request_mem_region(dev, 0);
 
 	cdev = xzalloc(sizeof(*cdev));
-- 
1.7.5.4


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

[PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'

[Antony Pavlov]

ns16550_read() and ns16550_write() functions are private functions
of the driver so there is no need to pass them 'struct console_device *'
pointer to get private device data.

Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 drivers/serial/serial_ns16550.c |   56 +++++++++++++++++++++-----------------
 1 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c
index ec93750..4cbab5e 100644
--- a/drivers/serial/serial_ns16550.c
+++ b/drivers/serial/serial_ns16550.c
@@ -73,9 +73,8 @@ NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
  *
  * @return value
  */
-static uint32_t ns16550_read(struct console_device *cdev, uint32_t off)
+static uint32_t ns16550_read(struct device_d *dev, uint32_t off)
 {
-	struct device_d *dev = cdev->dev;
 	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
 
 	off <<= plat->shift;
@@ -90,10 +89,9 @@ static uint32_t ns16550_read(struct console_device *cdev, uint32_t off)
  * @param[in] offset
  * @param[in] val
  */
-static void ns16550_write(struct console_device *cdev, uint32_t val,
+static void ns16550_write(struct device_d *dev, uint32_t val,
 			  uint32_t off)
 {
-	struct device_d *dev = cdev->dev;
 	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
 
 	off <<= plat->shift;
@@ -128,23 +126,24 @@ static unsigned int ns16550_calc_divisor(struct console_device *cdev,
 static void ns16550_serial_init_port(struct console_device *cdev)
 {
 	unsigned int baud_divisor;
+	struct device_d *dev = cdev->dev;
 
 	/* Setup the serial port with the defaults first */
 	baud_divisor = ns16550_calc_divisor(cdev, CONFIG_BAUDRATE);
 
 	/* initializing the device for the first time */
-	ns16550_write(cdev, 0x00, ier);
+	ns16550_write(dev, 0x00, ier);
 #ifdef CONFIG_DRIVER_SERIAL_NS16550_OMAP_EXTENSIONS
-	ns16550_write(cdev, 0x07, mdr1);	/* Disable */
+	ns16550_write(dev, 0x07, mdr1);	/* Disable */
 #endif
-	ns16550_write(cdev, LCR_BKSE | LCRVAL, lcr);
-	ns16550_write(cdev, baud_divisor & 0xFF, dll);
-	ns16550_write(cdev, (baud_divisor >> 8) & 0xff, dlm);
-	ns16550_write(cdev, LCRVAL, lcr);
-	ns16550_write(cdev, MCRVAL, mcr);
-	ns16550_write(cdev, FCRVAL, fcr);
+	ns16550_write(dev, LCR_BKSE | LCRVAL, lcr);
+	ns16550_write(dev, baud_divisor & 0xFF, dll);
+	ns16550_write(dev, (baud_divisor >> 8) & 0xff, dlm);
+	ns16550_write(dev, LCRVAL, lcr);
+	ns16550_write(dev, MCRVAL, mcr);
+	ns16550_write(dev, FCRVAL, fcr);
 #ifdef CONFIG_DRIVER_SERIAL_NS16550_OMAP_EXTENSIONS
-	ns16550_write(cdev, 0x00,  mdr1);
+	ns16550_write(dev, 0x00,  mdr1);
 #endif
 }
 
@@ -158,9 +157,11 @@ static void ns16550_serial_init_port(struct console_device *cdev)
  */
 static void ns16550_putc(struct console_device *cdev, char c)
 {
+	struct device_d *dev = cdev->dev;
+
 	/* Loop Doing Nothing */
-	while ((ns16550_read(cdev, lsr) & LSR_THRE) == 0) ;
-	ns16550_write(cdev, c, thr);
+	while ((ns16550_read(dev, lsr) & LSR_THRE) == 0) ;
+	ns16550_write(dev, c, thr);
 }
 
 /**
@@ -172,9 +173,11 @@ static void ns16550_putc(struct console_device *cdev, char c)
  */
 static int ns16550_getc(struct console_device *cdev)
 {
+	struct device_d *dev = cdev->dev;
+
 	/* Loop Doing Nothing */
-	while ((ns16550_read(cdev, lsr) & LSR_DR) == 0) ;
-	return ns16550_read(cdev, rbr);
+	while ((ns16550_read(dev, lsr) & LSR_DR) == 0) ;
+	return ns16550_read(dev, rbr);
 }
 
 /**
@@ -186,7 +189,9 @@ static int ns16550_getc(struct console_device *cdev)
  */
 static int ns16550_tstc(struct console_device *cdev)
 {
-	return ((ns16550_read(cdev, lsr) & LSR_DR) != 0);
+	struct device_d *dev = cdev->dev;
+
+	return ((ns16550_read(dev, lsr) & LSR_DR) != 0);
 }
 
 /**
@@ -200,14 +205,15 @@ static int ns16550_tstc(struct console_device *cdev)
 static int ns16550_setbaudrate(struct console_device *cdev, int baud_rate)
 {
 	unsigned int baud_divisor = ns16550_calc_divisor(cdev, baud_rate);
+	struct device_d *dev = cdev->dev;
 
-	ns16550_write(cdev, 0x00, ier);
-	ns16550_write(cdev, LCR_BKSE, lcr);
-	ns16550_write(cdev, baud_divisor & 0xff, dll);
-	ns16550_write(cdev, (baud_divisor >> 8) & 0xff, dlm);
-	ns16550_write(cdev, LCRVAL, lcr);
-	ns16550_write(cdev, MCRVAL, mcr);
-	ns16550_write(cdev, FCRVAL, fcr);
+	ns16550_write(dev, 0x00, ier);
+	ns16550_write(dev, LCR_BKSE, lcr);
+	ns16550_write(dev, baud_divisor & 0xff, dll);
+	ns16550_write(dev, (baud_divisor >> 8) & 0xff, dlm);
+	ns16550_write(dev, LCRVAL, lcr);
+	ns16550_write(dev, MCRVAL, mcr);
+	ns16550_write(dev, FCRVAL, fcr);
 	return 0;
 }
 
-- 
1.7.5.4


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

Re: [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:37 Wed 03 Aug     , Antony Pavlov wrote:
> ns16550_read() and ns16550_write() functions are private functions
> of the driver so there is no need to pass them 'struct console_device *'
> pointer to get private device data.
> 
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
Does not simplify the code

does not see the need

Best Regards,
J.

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

Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:37 Wed 03 Aug     , Antony Pavlov wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  drivers/serial/serial_ns16550.c |   87 ++++++++++++++++++++++++---------------
>  1 files changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c
> index 4ed2671..ec93750 100644
> --- a/drivers/serial/serial_ns16550.c
> +++ b/drivers/serial/serial_ns16550.c
> @@ -48,6 +48,23 @@
>  
>  /*********** Private Functions **********************************/
>  
> +#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
> +static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
> +		unsigned char reg_idx)		\
> +{						\
> +	return cmdr((char *)base + reg_idx);	\
> +}						\
> +						\
> +static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
> +		unsigned char reg_idx)			\
> +{							\
> +	cmdw((mask val), (char *)base + reg_idx);	\
> +}
> +
> +NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
> +NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
> +NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
> +
>  /**
>   * @brief read register
>   *
> @@ -60,22 +77,10 @@ static uint32_t ns16550_read(struct console_device *cdev, uint32_t off)
>  {
>  	struct device_d *dev = cdev->dev;
>  	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
> -	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>  
>  	off <<= plat->shift;
>  
> -	if (plat->reg_read)
> -		return plat->reg_read((unsigned long)dev->priv, off);
> -
> -	switch (width) {
> -	case IORESOURCE_MEM_8BIT:
> -		return readb(dev->priv + off);
> -	case IORESOURCE_MEM_16BIT:
> -		return readw(dev->priv + off);
> -	case IORESOURCE_MEM_32BIT:
> -		return readl(dev->priv + off);
> -	}
> -	return -1;
> +	return plat->reg_read((unsigned long)dev->priv, off);
>  }
>  
>  /**
> @@ -90,26 +95,10 @@ static void ns16550_write(struct console_device *cdev, uint32_t val,
>  {
>  	struct device_d *dev = cdev->dev;
>  	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
> -	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>  
>  	off <<= plat->shift;
>  
> -	if (plat->reg_write) {
> -		plat->reg_write(val, (unsigned long)dev->priv, off);
> -		return;
> -	}
> -
> -	switch (width) {
> -	case IORESOURCE_MEM_8BIT:
> -		writeb(val & 0xff, dev->priv + off);
> -		break;
> -	case IORESOURCE_MEM_16BIT:
> -		writew(val & 0xffff, dev->priv + off);
> -		break;
> -	case IORESOURCE_MEM_32BIT:
> -		writel(val, dev->priv + off);
> -		break;
> -	}
> +	plat->reg_write(val, (unsigned long)dev->priv, off);
no it must stay void __iomem*


>  }
>  
>  /**
> @@ -239,9 +228,41 @@ static int ns16550_probe(struct device_d *dev)
>  	/* we do expect platform specific data */
>  	if (plat == NULL)
>  		return -EINVAL;
> -	if (!(dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK) &&
> -	    ((plat->reg_read == NULL) || (plat->reg_write == NULL)))
> -		return -EINVAL;
we need to apply the fix fix and then change the behavior

for bisect
> +
> +	if (plat->reg_read == NULL) {
> +		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> +
> +		switch (width) {
> +		default:
> +		case IORESOURCE_MEM_8BIT:
> +			plat->reg_read = ns16550_generic_8bit_read;
> +			break;
> +		case IORESOURCE_MEM_16BIT:
> +			plat->reg_read = ns16550_generic_16bit_read;
> +			break;
> +		case IORESOURCE_MEM_32BIT:
> +			plat->reg_read = ns16550_generic_32bit_read;
> +			break;
> +		}
> +	}
> +
> +	if (plat->reg_write == NULL) {
> +		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
why do it twice?

Best Regards,
J.

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

Re: [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'

[Antony Pavlov]

On 4 August 2011 04:37, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 22:37 Wed 03 Aug     , Antony Pavlov wrote:
>> ns16550_read() and ns16550_write() functions are private functions
>> of the driver so there is no need to pass them 'struct console_device *'
>> pointer to get private device data.
>>
>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>> ---

> Does not simplify the code
>
> does not see the need

Just the opposite!

Without this patch we do the operation 'dev = cdev->dev' at _EVERY
SINGLE CALL_ of ns16550_read/ns16550_write.

In the driver, only in ns16550_tstc() we have single call of
ns16550_read(). In all other places we have multiple
ns16550_read/ns16550_write. So we can move 'dev = cdev->dev'
to caller without any damage.

Moreover, in ns16550_putc() we have cycle:
----
while ((ns16550_read(dev, lsr) & LSR_THRE) == 0) ;
----
On every cycle loop we do 'dev = cdev->dev' !

Disassemble the compiled barebox and you will see that you have many
memory memory accesses at the entrance to the
ns16550_read/ns16550_write functions.

So, removing this 'dev = cdev->dev' we remove one memory access.

This is a real optimization!

Also, confirmation of this patch comes from simple logic.

Simple question.
Have we need of 'console_device *cdev' pointer in
ns16550_read/ns16550_write at all?

My answer is 'No'.

Console_device is very high-level abstraction structure, but
ns16550_read/ns16550_write functions work at the low-level.
They work with the private driver data, but cdev can't directly supply
this private data.

-- 
Best regards,
  Antony Pavlov

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

Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Sascha Hauer]

On Wed, Aug 03, 2011 at 10:37:45PM +0400, Antony Pavlov wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  drivers/serial/serial_ns16550.c |   87 ++++++++++++++++++++++++---------------
>  1 files changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/serial/serial_ns16550.c b/drivers/serial/serial_ns16550.c
> index 4ed2671..ec93750 100644
> --- a/drivers/serial/serial_ns16550.c
> +++ b/drivers/serial/serial_ns16550.c
> @@ -48,6 +48,23 @@
>  
>  /*********** Private Functions **********************************/
>  
> +#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
> +static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
> +		unsigned char reg_idx)		\
> +{						\
> +	return cmdr((char *)base + reg_idx);	\
> +}						\
> +						\
> +static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
> +		unsigned char reg_idx)			\
> +{							\
> +	cmdw((mask val), (char *)base + reg_idx);	\
> +}
> +
> +NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
> +NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
> +NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )

Please don't do such preprocessor tricks if not necessary. we can afford
the 8 additional lines for making this readable.

> +
>  /**
>   * @brief read register
>   *
> @@ -60,22 +77,10 @@ static uint32_t ns16550_read(struct console_device *cdev, uint32_t off)
>  {
>  	struct device_d *dev = cdev->dev;
>  	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
> -	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>  
>  	off <<= plat->shift;
>  
> -	if (plat->reg_read)
> -		return plat->reg_read((unsigned long)dev->priv, off);
> -
> -	switch (width) {
> -	case IORESOURCE_MEM_8BIT:
> -		return readb(dev->priv + off);
> -	case IORESOURCE_MEM_16BIT:
> -		return readw(dev->priv + off);
> -	case IORESOURCE_MEM_32BIT:
> -		return readl(dev->priv + off);
> -	}
> -	return -1;
> +	return plat->reg_read((unsigned long)dev->priv, off);
>  }
>  
>  /**
> @@ -90,26 +95,10 @@ static void ns16550_write(struct console_device *cdev, uint32_t val,
>  {
>  	struct device_d *dev = cdev->dev;
>  	struct NS16550_plat *plat = (struct NS16550_plat *)dev->platform_data;
> -	int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>  
>  	off <<= plat->shift;
>  
> -	if (plat->reg_write) {
> -		plat->reg_write(val, (unsigned long)dev->priv, off);
> -		return;
> -	}
> -
> -	switch (width) {
> -	case IORESOURCE_MEM_8BIT:
> -		writeb(val & 0xff, dev->priv + off);
> -		break;
> -	case IORESOURCE_MEM_16BIT:
> -		writew(val & 0xffff, dev->priv + off);
> -		break;
> -	case IORESOURCE_MEM_32BIT:
> -		writel(val, dev->priv + off);
> -		break;
> -	}
> +	plat->reg_write(val, (unsigned long)dev->priv, off);
>  }
>  
>  /**
> @@ -239,9 +228,41 @@ static int ns16550_probe(struct device_d *dev)
>  	/* we do expect platform specific data */
>  	if (plat == NULL)
>  		return -EINVAL;
> -	if (!(dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK) &&
> -	    ((plat->reg_read == NULL) || (plat->reg_write == NULL)))
> -		return -EINVAL;
> +
> +	if (plat->reg_read == NULL) {
> +		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> +
> +		switch (width) {
> +		default:
> +		case IORESOURCE_MEM_8BIT:
> +			plat->reg_read = ns16550_generic_8bit_read;
> +			break;
> +		case IORESOURCE_MEM_16BIT:
> +			plat->reg_read = ns16550_generic_16bit_read;
> +			break;
> +		case IORESOURCE_MEM_32BIT:
> +			plat->reg_read = ns16550_generic_32bit_read;
> +			break;
> +		}
> +	}
> +
> +	if (plat->reg_write == NULL) {
> +		int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> +
> +		switch (width) {
> +		default:
> +		case IORESOURCE_MEM_8BIT:
> +			plat->reg_write = ns16550_generic_8bit_write;
> +			break;
> +		case IORESOURCE_MEM_16BIT:
> +			plat->reg_write = ns16550_generic_16bit_write;
> +			break;
> +		case IORESOURCE_MEM_32BIT:
> +			plat->reg_write = ns16550_generic_32bit_write;
> +			break;
> +		}
> +	}

Passing one of the register access function without the other shouldn't
be a valid usecase. Jean is right, there should be only one if().

Generally platform_data shouldn't be used after device probe and for
sure it shouldn't be modified. This may be the point where this driver
needs a private data struct.

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

Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Antony Pavlov]

On 4 August 2011 04:42, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 22:37 Wed 03 Aug     , Antony Pavlov wrote:
>> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>> ---
>>  drivers/serial/serial_ns16550.c |   87 ++++++++++++++++++++++++---------------
>>  1 files changed, 54 insertions(+), 33 deletions(-)
>>

>> +     plat->reg_write(val, (unsigned long)dev->priv, off);
> no it must stay void __iomem*

plat->reg_write() get the second argument 'unsigned long base'.

Moreover, using 'void *' in arithmetic is a poor practice.

Fortunally for us, in GCC incrementing a void pointer adds one to the
value (sizeof(void) == 1).

Add '-pedantic' flag to gcc and compile current 'next' barebox.

You will see something like this:
In function ‘ns16550_read’:
drivers/serial/serial_ns16550.c:72: warning: pointer of type ‘void *’
used in arithmetic

>> +     if (plat->reg_read == NULL) {
>> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>> +
>> +             switch (width) {
>> +             default:
...
>> +             }
>> +     }
>> +
>> +     if (plat->reg_write == NULL) {
>> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> why do it twice?

You are right.
width calculation or even separate reg_write & reg_read handling  can
be combined.

-- 
Best regards,
  Antony Pavlov

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

Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Antony Pavlov]

On 4 August 2011 11:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:

>>
>> +#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
>> +static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
>> +             unsigned char reg_idx)          \
>> +{                                            \
>> +     return cmdr((char *)base + reg_idx);    \
>> +}                                            \
>> +                                             \
>> +static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
>> +             unsigned char reg_idx)                  \
>> +{                                                    \
>> +     cmdw((mask val), (char *)base + reg_idx);       \
>> +}
>> +
>> +NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
>> +NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
>> +NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
>
> Please don't do such preprocessor tricks if not necessary. we can afford
> the 8 additional lines for making this readable.

approx. > 16 additional lines

>> +     if (plat->reg_read == NULL) {
>> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>> +
>> +             switch (width) {
...
>> +     }
>> +
>> +     if (plat->reg_write == NULL) {
>> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
>> +
>> +             switch (width) {
...
>> +             }
>> +     }
>
> Passing one of the register access function without the other shouldn't
> be a valid usecase. Jean is right, there should be only one if().

Agree.

> Generally platform_data shouldn't be used after device probe and for
> sure it shouldn't be modified. This may be the point where this driver
> needs a private data struct.

I like this point of view.
We must add private data struct and copy plat->width, plat->reg_read etc there.

-- 
Best regards,
  Antony Pavlov

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

Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

[Sascha Hauer]

On Thu, Aug 04, 2011 at 11:38:53AM +0400, Antony Pavlov wrote:
> On 4 August 2011 11:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> >>
> >> +#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
> >> +static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
> >> +             unsigned char reg_idx)          \
> >> +{                                            \
> >> +     return cmdr((char *)base + reg_idx);    \
> >> +}                                            \
> >> +                                             \
> >> +static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
> >> +             unsigned char reg_idx)                  \
> >> +{                                                    \
> >> +     cmdw((mask val), (char *)base + reg_idx);       \
> >> +}
> >> +
> >> +NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
> >> +NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
> >> +NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
> >
> > Please don't do such preprocessor tricks if not necessary. we can afford
> > the 8 additional lines for making this readable.
> 
> approx. > 16 additional lines

Whatever.

> 
> >> +     if (plat->reg_read == NULL) {
> >> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> >> +
> >> +             switch (width) {
> ...
> >> +     }
> >> +
> >> +     if (plat->reg_write == NULL) {
> >> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> >> +
> >> +             switch (width) {
> ...
> >> +             }
> >> +     }
> >
> > Passing one of the register access function without the other shouldn't
> > be a valid usecase. Jean is right, there should be only one if().
> 
> Agree.
> 
> > Generally platform_data shouldn't be used after device probe and for
> > sure it shouldn't be modified. This may be the point where this driver
> > needs a private data struct.
> 
> I like this point of view.
> We must add private data struct and copy plat->width, plat->reg_read etc there.

You won't need plat->width, only the correct the register accessors
depending on plat->width.

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