* [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
@ 2011-08-03 18:37 Antony Pavlov
2011-08-03 18:37 ` [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *' Antony Pavlov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Antony Pavlov @ 2011-08-03 18:37 UTC (permalink / raw)
To: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'
2011-08-03 18:37 [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Antony Pavlov
@ 2011-08-03 18:37 ` Antony Pavlov
2011-08-04 0:37 ` Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 0:42 ` [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 7:19 ` Sascha Hauer
2 siblings, 1 reply; 9+ messages in thread
From: Antony Pavlov @ 2011-08-03 18:37 UTC (permalink / raw)
To: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'
2011-08-03 18:37 ` [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *' Antony Pavlov
@ 2011-08-04 0:37 ` Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 4:54 ` Antony Pavlov
0 siblings, 1 reply; 9+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-08-04 0:37 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
2011-08-03 18:37 [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Antony Pavlov
2011-08-03 18:37 ` [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *' Antony Pavlov
@ 2011-08-04 0:42 ` Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 7:30 ` Antony Pavlov
2011-08-04 7:19 ` Sascha Hauer
2 siblings, 1 reply; 9+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-08-04 0:42 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *'
2011-08-04 0:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2011-08-04 4:54 ` Antony Pavlov
0 siblings, 0 replies; 9+ messages in thread
From: Antony Pavlov @ 2011-08-04 4:54 UTC (permalink / raw)
To: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
2011-08-03 18:37 [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Antony Pavlov
2011-08-03 18:37 ` [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *' Antony Pavlov
2011-08-04 0:42 ` [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Jean-Christophe PLAGNIOL-VILLARD
@ 2011-08-04 7:19 ` Sascha Hauer
2011-08-04 7:38 ` Antony Pavlov
2 siblings, 1 reply; 9+ messages in thread
From: Sascha Hauer @ 2011-08-04 7:19 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
2011-08-04 0:42 ` [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Jean-Christophe PLAGNIOL-VILLARD
@ 2011-08-04 7:30 ` Antony Pavlov
0 siblings, 0 replies; 9+ messages in thread
From: Antony Pavlov @ 2011-08-04 7:30 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
2011-08-04 7:19 ` Sascha Hauer
@ 2011-08-04 7:38 ` Antony Pavlov
2011-08-04 7:51 ` Sascha Hauer
0 siblings, 1 reply; 9+ messages in thread
From: Antony Pavlov @ 2011-08-04 7:38 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()
2011-08-04 7:38 ` Antony Pavlov
@ 2011-08-04 7:51 ` Sascha Hauer
0 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2011-08-04 7:51 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-08-04 7:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-03 18:37 [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Antony Pavlov
2011-08-03 18:37 ` [PATCH 2/2] serial_ns16550: use 'struct device_d *' instead of 'struct console_device *' Antony Pavlov
2011-08-04 0:37 ` Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 4:54 ` Antony Pavlov
2011-08-04 0:42 ` [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe() Jean-Christophe PLAGNIOL-VILLARD
2011-08-04 7:30 ` Antony Pavlov
2011-08-04 7:19 ` Sascha Hauer
2011-08-04 7:38 ` Antony Pavlov
2011-08-04 7:51 ` Sascha Hauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox