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@xxxxxxxxxxxxxxxxxxxxx>
>
> >>>--- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
