* [patch 2.6.29-rc2] palm_bk3710 buildfix
@ 2009-01-18 16:33 David Brownell
2009-01-18 17:14 ` Sergei Shtylyov
0 siblings, 1 reply; 11+ messages in thread
From: David Brownell @ 2009-01-18 16:33 UTC (permalink / raw)
To: bzolnier; +Cc: linux-ide, Kevin Hilman, Sergei Shtylyov
From: David Brownell <dbrownell@users.sourceforge.net>
Subject: drivers/ide/palm_bk3710.c buildfix
CC drivers/ide/palm_bk3710.o
drivers/ide/palm_bk3710.c: In function 'palm_bk3710_probe':
drivers/ide/palm_bk3710.c:382: warning: assignment makes integer from pointer without a cast
Someone should fix hw_regs_t to neither be a typedef, nor
use "unsigned long" where it should use "void __iomem *".
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
drivers/ide/palm_bk3710.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
--- a/drivers/ide/palm_bk3710.c
+++ b/drivers/ide/palm_bk3710.c
@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
{
struct clk *clk;
struct resource *mem, *irq;
- unsigned long base, rate;
+ void __iomem *base;
+ unsigned long rate;
int i, rc;
hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
@@ -382,11 +383,13 @@ static int __init palm_bk3710_probe(stru
base = IO_ADDRESS(mem->start);
/* Configure the Palm Chip controller */
- palm_bk3710_chipinit((void __iomem *)base);
+ palm_bk3710_chipinit(base);
for (i = 0; i < IDE_NR_PORTS - 2; i++)
- hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
- hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
+ hw.io_ports_array[i] = (unsigned long)
+ (base + IDE_PALM_ATA_PRI_REG_OFFSET + i);
+ hw.io_ports.ctl_addr = (unsigned long)
+ (base + IDE_PALM_ATA_PRI_CTL_OFFSET);
hw.irq = irq->start;
hw.dev = &pdev->dev;
hw.chipset = ide_palm3710;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-18 16:33 [patch 2.6.29-rc2] palm_bk3710 buildfix David Brownell
@ 2009-01-18 17:14 ` Sergei Shtylyov
2009-01-19 0:29 ` David Brownell
2009-01-19 12:00 ` Bartlomiej Zolnierkiewicz
0 siblings, 2 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-18 17:14 UTC (permalink / raw)
To: David Brownell; +Cc: bzolnier, linux-ide, Kevin Hilman
Hello.
David Brownell wrote:
> CC drivers/ide/palm_bk3710.o
> drivers/ide/palm_bk3710.c: In function 'palm_bk3710_probe':
> drivers/ide/palm_bk3710.c:382: warning: assignment makes integer from pointer without a cast
> Someone should fix hw_regs_t to neither be a typedef, nor
> use "unsigned long" where it should use "void __iomem *".
It cannot use pointers of course -- as the addresses can be I/O ports.
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> --- a/drivers/ide/palm_bk3710.c
> +++ b/drivers/ide/palm_bk3710.c
> @@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
> {
> struct clk *clk;
> struct resource *mem, *irq;
> - unsigned long base, rate;
> + void __iomem *base;
> + unsigned long rate;
> int i, rc;
> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>
> @@ -382,11 +383,13 @@ static int __init palm_bk3710_probe(stru
> base = IO_ADDRESS(mem->start);
>
> /* Configure the Palm Chip controller */
> - palm_bk3710_chipinit((void __iomem *)base);
> + palm_bk3710_chipinit(base);
>
> for (i = 0; i < IDE_NR_PORTS - 2; i++)
> - hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
> - hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
> + hw.io_ports_array[i] = (unsigned long)
> + (base + IDE_PALM_ATA_PRI_REG_OFFSET + i);
> + hw.io_ports.ctl_addr = (unsigned long)
> + (base + IDE_PALM_ATA_PRI_CTL_OFFSET);
Ugh. I suggest adding another variable...
--- a/drivers/ide/palm_bk3710.c
+++ b/drivers/ide/palm_bk3710.c
@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
{
struct clk *clk;
struct resource *mem, *irq;
unsigned long base, rate;
+ void __iomem *regs;
int i, rc;
hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
@@ -379,11 +380,11 @@ static int __init palm_bk3710_probe(stru
return -EBUSY;
}
- base = IO_ADDRESS(mem->start);
+ regs = IO_ADDRESS(mem->start);
/* Configure the Palm Chip controller */
- palm_bk3710_chipinit((void __iomem *)base);
+ palm_bk3710_chipinit(regs);
+ base = (unsigned long)regs;
for (i = 0; i < IDE_NR_PORTS - 2; i++)
hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-18 17:14 ` Sergei Shtylyov
@ 2009-01-19 0:29 ` David Brownell
2009-01-19 8:35 ` Sergei Shtylyov
2009-01-19 12:00 ` Bartlomiej Zolnierkiewicz
1 sibling, 1 reply; 11+ messages in thread
From: David Brownell @ 2009-01-19 0:29 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: bzolnier, linux-ide, Kevin Hilman
On Sunday 18 January 2009, Sergei Shtylyov wrote:
> > Someone should fix hw_regs_t to neither be a typedef, nor
> > use "unsigned long" where it should use "void __iomem *".
>
> It cannot use pointers of course -- as the addresses can be I/O ports.
It could use the ioread*() calls, which take pointers
and handle both types of I/O in a type-safe manner.
- Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 0:29 ` David Brownell
@ 2009-01-19 8:35 ` Sergei Shtylyov
2009-01-19 8:57 ` Sergei Shtylyov
0 siblings, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-19 8:35 UTC (permalink / raw)
To: David Brownell; +Cc: bzolnier, linux-ide, Kevin Hilman
Hello.
David Brownell wrote:
> On Sunday 18 January 2009, Sergei Shtylyov wrote:
>
>>> Someone should fix hw_regs_t to neither be a typedef, nor
>>> use "unsigned long" where it should use "void __iomem *".
>>>
>> It cannot use pointers of course -- as the addresses can be I/O ports.
>>
>
> It could use the ioread*() calls, which take pointers
> and handle both types of I/O in a type-safe manner.
>
Probably they can... though using those would only slow down register
access on x86.
> - Dave
WBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 8:35 ` Sergei Shtylyov
@ 2009-01-19 8:57 ` Sergei Shtylyov
2009-01-19 12:07 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-19 8:57 UTC (permalink / raw)
To: David Brownell; +Cc: bzolnier, linux-ide, Kevin Hilman
Hello, I wrote:
>>>> Someone should fix hw_regs_t to neither be a typedef, nor
>>>> use "unsigned long" where it should use "void __iomem *".
>>>>
>>> It cannot use pointers of course -- as the addresses can be I/O
>>> ports.
>>>
>>
>> It could use the ioread*() calls, which take pointers
>> and handle both types of I/O in a type-safe manner.
>>
>
> Probably they can... though using those would only slow down
> register access on x86.
... and cause code bloat due to switching to real function calls from
inlines which now should render into in/out isns.
> - Dave
WBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 8:57 ` Sergei Shtylyov
@ 2009-01-19 12:07 ` Bartlomiej Zolnierkiewicz
2009-01-19 13:06 ` Sergei Shtylyov
0 siblings, 1 reply; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-01-19 12:07 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: David Brownell, linux-ide, Kevin Hilman
On Monday 19 January 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
>
> >>>> Someone should fix hw_regs_t to neither be a typedef, nor
> >>>> use "unsigned long" where it should use "void __iomem *".
> >>>>
> >>> It cannot use pointers of course -- as the addresses can be I/O
> >>> ports.
> >>>
> >>
> >> It could use the ioread*() calls, which take pointers
> >> and handle both types of I/O in a type-safe manner.
> >>
> >
> > Probably they can... though using those would only slow down
> > register access on x86.
>
> ... and cause code bloat due to switching to real function calls from
> inlines which now should render into in/out isns.
OTOH it would allow to unify a lot of 'if (hwif->host_flags & IDE_HFLAG_MMIO)'
cases so the code may even shrink. I also suspect that register access slow
down is most likely negligable (we can always measure and check it).
The real reason for not using ioread*() and co. is that not all archs support
them (though I checked some time ago, maybe they do now?) and we don't want to
break IDE support for them.
Thanks,
Bart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 12:07 ` Bartlomiej Zolnierkiewicz
@ 2009-01-19 13:06 ` Sergei Shtylyov
2009-01-19 14:14 ` Sergei Shtylyov
0 siblings, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-19 13:06 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Brownell, linux-ide, Kevin Hilman
Bartlomiej Zolnierkiewicz wrote:
>>>>>>Someone should fix hw_regs_t to neither be a typedef, nor
>>>>>>use "unsigned long" where it should use "void __iomem *".
>>>>> It cannot use pointers of course -- as the addresses can be I/O
>>>>>ports.
>>>>It could use the ioread*() calls, which take pointers
>>>>and handle both types of I/O in a type-safe manner.
>>> Probably they can... though using those would only slow down
>>>register access on x86.
>> ... and cause code bloat due to switching to real function calls from
>>inlines which now should render into in/out isns.
> OTOH it would allow to unify a lot of 'if (hwif->host_flags & IDE_HFLAG_MMIO)'
> cases so the code may even shrink.
It won't shrink as much as it will grow.
> I also suspect that register access slow
> down is most likely negligable (we can always measure and check it).
I'd agree here but code bloat won't be as negligible considering the IDE
code's predominant use of I/O ports on x86.
> The real reason for not using ioread*() and co. is that not all archs support
> them (though I checked some time ago, maybe they do now?) and we don't want to
> break IDE support for them.
Sure. :-)
> Thanks,
> Bart
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 13:06 ` Sergei Shtylyov
@ 2009-01-19 14:14 ` Sergei Shtylyov
0 siblings, 0 replies; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-19 14:14 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Bartlomiej Zolnierkiewicz, David Brownell, linux-ide,
Kevin Hilman
Hello, I wrote:
>>>>>>> Someone should fix hw_regs_t to neither be a typedef, nor
>>>>>>> use "unsigned long" where it should use "void __iomem *".
>>>>>> It cannot use pointers of course -- as the addresses can be I/O
>>>>>> ports.
>>>>> It could use the ioread*() calls, which take pointers
>>>>> and handle both types of I/O in a type-safe manner.
>>>> Probably they can... though using those would only slow down
>>>> register access on x86.
>>> ... and cause code bloat due to switching to real function calls
>>> from inlines which now should render into in/out isns.
>> OTOH it would allow to unify a lot of 'if (hwif->host_flags &
>> IDE_HFLAG_MMIO)'
>> cases so the code may even shrink.
> It won't shrink as much as it will grow.
>> I also suspect that register access slow
>> down is most likely negligable (we can always measure and check it).
> I'd agree here but code bloat won't be as negligible considering the
> IDE code's predominant use of I/O ports on x86.
After looking at the current code, the generic IDE code would actually
benefit from such change, while the most of the controller drivers will only
be subject to the code bloat.
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-18 17:14 ` Sergei Shtylyov
2009-01-19 0:29 ` David Brownell
@ 2009-01-19 12:00 ` Bartlomiej Zolnierkiewicz
2009-01-19 13:03 ` Sergei Shtylyov
1 sibling, 1 reply; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-01-19 12:00 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: David Brownell, linux-ide, Kevin Hilman
On Sunday 18 January 2009, Sergei Shtylyov wrote:
> Hello.
>
> David Brownell wrote:
>
> > CC drivers/ide/palm_bk3710.o
> > drivers/ide/palm_bk3710.c: In function 'palm_bk3710_probe':
> > drivers/ide/palm_bk3710.c:382: warning: assignment makes integer from pointer without a cast
>
> > Someone should fix hw_regs_t to neither be a typedef, nor
> > use "unsigned long" where it should use "void __iomem *".
>
> It cannot use pointers of course -- as the addresses can be I/O ports.
>
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>
> > --- a/drivers/ide/palm_bk3710.c
> > +++ b/drivers/ide/palm_bk3710.c
> > @@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
> > {
> > struct clk *clk;
> > struct resource *mem, *irq;
> > - unsigned long base, rate;
> > + void __iomem *base;
> > + unsigned long rate;
> > int i, rc;
> > hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
> >
> > @@ -382,11 +383,13 @@ static int __init palm_bk3710_probe(stru
> > base = IO_ADDRESS(mem->start);
> >
> > /* Configure the Palm Chip controller */
> > - palm_bk3710_chipinit((void __iomem *)base);
> > + palm_bk3710_chipinit(base);
> >
> > for (i = 0; i < IDE_NR_PORTS - 2; i++)
> > - hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
> > - hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
> > + hw.io_ports_array[i] = (unsigned long)
> > + (base + IDE_PALM_ATA_PRI_REG_OFFSET + i);
> > + hw.io_ports.ctl_addr = (unsigned long)
> > + (base + IDE_PALM_ATA_PRI_CTL_OFFSET);
>
> Ugh. I suggest adding another variable...
>
> --- a/drivers/ide/palm_bk3710.c
> +++ b/drivers/ide/palm_bk3710.c
> @@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
> {
> struct clk *clk;
> struct resource *mem, *irq;
> unsigned long base, rate;
> + void __iomem *regs;
> int i, rc;
> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>
> @@ -379,11 +380,11 @@ static int __init palm_bk3710_probe(stru
> return -EBUSY;
> }
>
> - base = IO_ADDRESS(mem->start);
> + regs = IO_ADDRESS(mem->start);
>
> /* Configure the Palm Chip controller */
> - palm_bk3710_chipinit((void __iomem *)base);
> + palm_bk3710_chipinit(regs);
>
> + base = (unsigned long)regs;
> for (i = 0; i < IDE_NR_PORTS - 2; i++)
> hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
> hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
I applied original patch since having one additional cast has overally lower
complexity than having an additional variable...
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 12:00 ` Bartlomiej Zolnierkiewicz
@ 2009-01-19 13:03 ` Sergei Shtylyov
2009-01-19 13:21 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2009-01-19 13:03 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Brownell, linux-ide, Kevin Hilman
Hello.
Bartlomiej Zolnierkiewicz wrote:
>>> CC drivers/ide/palm_bk3710.o
>>>drivers/ide/palm_bk3710.c: In function 'palm_bk3710_probe':
>>>drivers/ide/palm_bk3710.c:382: warning: assignment makes integer from pointer without a cast
>>>Someone should fix hw_regs_t to neither be a typedef, nor
>>>use "unsigned long" where it should use "void __iomem *".
>> It cannot use pointers of course -- as the addresses can be I/O ports.
>>>Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>>>--- a/drivers/ide/palm_bk3710.c
>>>+++ b/drivers/ide/palm_bk3710.c
>>>@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
>>> {
>>> struct clk *clk;
>>> struct resource *mem, *irq;
>>>- unsigned long base, rate;
>>>+ void __iomem *base;
>>>+ unsigned long rate;
>>> int i, rc;
>>> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>>>
>>>@@ -382,11 +383,13 @@ static int __init palm_bk3710_probe(stru
>>> base = IO_ADDRESS(mem->start);
>>>
>>> /* Configure the Palm Chip controller */
>>>- palm_bk3710_chipinit((void __iomem *)base);
>>>+ palm_bk3710_chipinit(base);
>>>
>>> for (i = 0; i < IDE_NR_PORTS - 2; i++)
>>>- hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
>>>- hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
>>>+ hw.io_ports_array[i] = (unsigned long)
>>>+ (base + IDE_PALM_ATA_PRI_REG_OFFSET + i);
>>>+ hw.io_ports.ctl_addr = (unsigned long)
>>>+ (base + IDE_PALM_ATA_PRI_CTL_OFFSET);
>> Ugh. I suggest adding another variable...
>>--- a/drivers/ide/palm_bk3710.c
>>+++ b/drivers/ide/palm_bk3710.c
>>@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
>> {
>> struct clk *clk;
>> struct resource *mem, *irq;
>> unsigned long base, rate;
>>+ void __iomem *regs;
>> int i, rc;
>> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
>>
>>@@ -379,11 +380,11 @@ static int __init palm_bk3710_probe(stru
>> return -EBUSY;
>> }
>>
>>- base = IO_ADDRESS(mem->start);
>>+ regs = IO_ADDRESS(mem->start);
>>
>> /* Configure the Palm Chip controller */
>>- palm_bk3710_chipinit((void __iomem *)base);
>>+ palm_bk3710_chipinit(regs);
>>
>>+ base = (unsigned long)regs;
>> for (i = 0; i < IDE_NR_PORTS - 2; i++)
>> hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
>> hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
> I applied original patch since having one additional cast has overally lower
> complexity than having an additional variable...
Grr... as if the stack use aren't optimized by gcc based on the variable
lifetimes. I can recast the patch myself BTW.
MBR, Sergei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 2.6.29-rc2] palm_bk3710 buildfix
2009-01-19 13:03 ` Sergei Shtylyov
@ 2009-01-19 13:21 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 11+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-01-19 13:21 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: David Brownell, linux-ide, Kevin Hilman
On Monday 19 January 2009, Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> >>> CC drivers/ide/palm_bk3710.o
> >>>drivers/ide/palm_bk3710.c: In function 'palm_bk3710_probe':
> >>>drivers/ide/palm_bk3710.c:382: warning: assignment makes integer from pointer without a cast
>
> >>>Someone should fix hw_regs_t to neither be a typedef, nor
> >>>use "unsigned long" where it should use "void __iomem *".
>
> >> It cannot use pointers of course -- as the addresses can be I/O ports.
>
> >>>Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>
> >>>--- a/drivers/ide/palm_bk3710.c
> >>>+++ b/drivers/ide/palm_bk3710.c
> >>>@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
> >>> {
> >>> struct clk *clk;
> >>> struct resource *mem, *irq;
> >>>- unsigned long base, rate;
> >>>+ void __iomem *base;
> >>>+ unsigned long rate;
> >>> int i, rc;
> >>> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
> >>>
> >>>@@ -382,11 +383,13 @@ static int __init palm_bk3710_probe(stru
> >>> base = IO_ADDRESS(mem->start);
> >>>
> >>> /* Configure the Palm Chip controller */
> >>>- palm_bk3710_chipinit((void __iomem *)base);
> >>>+ palm_bk3710_chipinit(base);
> >>>
> >>> for (i = 0; i < IDE_NR_PORTS - 2; i++)
> >>>- hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
> >>>- hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
> >>>+ hw.io_ports_array[i] = (unsigned long)
> >>>+ (base + IDE_PALM_ATA_PRI_REG_OFFSET + i);
> >>>+ hw.io_ports.ctl_addr = (unsigned long)
> >>>+ (base + IDE_PALM_ATA_PRI_CTL_OFFSET);
>
> >> Ugh. I suggest adding another variable...
>
> >>--- a/drivers/ide/palm_bk3710.c
> >>+++ b/drivers/ide/palm_bk3710.c
> >>@@ -346,7 +346,8 @@ static int __init palm_bk3710_probe(stru
> >> {
> >> struct clk *clk;
> >> struct resource *mem, *irq;
> >> unsigned long base, rate;
> >>+ void __iomem *regs;
> >> int i, rc;
> >> hw_regs_t hw, *hws[] = { &hw, NULL, NULL, NULL };
> >>
> >>@@ -379,11 +380,11 @@ static int __init palm_bk3710_probe(stru
> >> return -EBUSY;
> >> }
> >>
> >>- base = IO_ADDRESS(mem->start);
> >>+ regs = IO_ADDRESS(mem->start);
> >>
> >> /* Configure the Palm Chip controller */
> >>- palm_bk3710_chipinit((void __iomem *)base);
> >>+ palm_bk3710_chipinit(regs);
> >>
> >>+ base = (unsigned long)regs;
> >> for (i = 0; i < IDE_NR_PORTS - 2; i++)
> >> hw.io_ports_array[i] = base + IDE_PALM_ATA_PRI_REG_OFFSET + i;
> >> hw.io_ports.ctl_addr = base + IDE_PALM_ATA_PRI_CTL_OFFSET;
>
> > I applied original patch since having one additional cast has overally lower
> > complexity than having an additional variable...
>
> Grr... as if the stack use aren't optimized by gcc based on the variable
> lifetimes. I can recast the patch myself BTW.
I meant code complexity in the maintainance / readability context here.
You know what this variable is for (at least now but a year from now it may
result in a brief WTF moment when looking at base/regs) and that it is short
lived but some random person who will need to update this driver won't...
Thanks,
Bart
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-01-19 14:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-18 16:33 [patch 2.6.29-rc2] palm_bk3710 buildfix David Brownell
2009-01-18 17:14 ` Sergei Shtylyov
2009-01-19 0:29 ` David Brownell
2009-01-19 8:35 ` Sergei Shtylyov
2009-01-19 8:57 ` Sergei Shtylyov
2009-01-19 12:07 ` Bartlomiej Zolnierkiewicz
2009-01-19 13:06 ` Sergei Shtylyov
2009-01-19 14:14 ` Sergei Shtylyov
2009-01-19 12:00 ` Bartlomiej Zolnierkiewicz
2009-01-19 13:03 ` Sergei Shtylyov
2009-01-19 13:21 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).