Hi,
This patch fixes several dozen warnings spit out when compiling the hermes
wireless driver.
In file included from drivers/net/wireless/orinoco.c:448:
drivers/net/wireless/hermes.h: In function `hermes_present':
drivers/net/wireless/hermes.h:398: warning: passing arg 1 of `readw' makes pointer from integer without a cast
drivers/net/wireless/hermes.h: In function `hermes_set_irqmask':
drivers/net/wireless/hermes.h:404: warning: passing arg 2 of `writew' makes pointer from integer without a cast
...
thanks,
-- Cal
Signed-off-by: Cal Peake <[email protected]>
diff -Nru linux-2.6.9-rc4/drivers/net/wireless/hermes.h linux-2.6.9-rc4-1/drivers/net/wireless/hermes.h
--- linux-2.6.9-rc4/drivers/net/wireless/hermes.h 2004-10-11 02:38:38.000000000 -0400
+++ linux-2.6.9-rc4-1/drivers/net/wireless/hermes.h 2004-10-11 06:56:01.000000000 -0400
@@ -364,12 +364,12 @@
/* Register access convenience macros */
#define hermes_read_reg(hw, off) ((hw)->io_space ? \
inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
- readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
+ readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
#define hermes_write_reg(hw, off, val) do { \
if ((hw)->io_space) \
outw_p((val), (hw)->iobase + ((off) << (hw)->reg_spacing)); \
else \
- writew((val), (hw)->iobase + ((off) << (hw)->reg_spacing)); \
+ writew((val), (void __iomem *)(hw)->iobase + ((off) << (hw)->reg_spacing)); \
} while (0)
#define hermes_read_regn(hw, name) hermes_read_reg((hw), HERMES_##name)
#define hermes_write_regn(hw, name, val) hermes_write_reg((hw), HERMES_##name, (val))
@@ -442,7 +442,7 @@
* gcc is smart enough to fold away the two swaps on
* big-endian platforms. */
for (i = 0, p = buf; i < count; i++) {
- *p++ = cpu_to_le16(readw(hw->iobase + off));
+ *p++ = cpu_to_le16(readw((void __iomem *)hw->iobase + off));
}
}
}
@@ -462,7 +462,7 @@
* hope gcc is smart enough to fold away the two swaps
* on big-endian platforms. */
for (i = 0, p = buf; i < count; i++) {
- writew(le16_to_cpu(*p++), hw->iobase + off);
+ writew(le16_to_cpu(*p++), (void __iomem *)hw->iobase + off);
}
}
}
@@ -478,7 +478,7 @@
outw(0, hw->iobase + off);
} else {
for (i = 0; i < count; i++)
- writew(0, hw->iobase + off);
+ writew(0, (void __iomem *)hw->iobase + off);
}
}
Cal Peake wrote:
> Hi,
>
> This patch fixes several dozen warnings spit out when compiling the hermes
> wireless driver.
>
> In file included from drivers/net/wireless/orinoco.c:448:
> drivers/net/wireless/hermes.h: In function `hermes_present':
> drivers/net/wireless/hermes.h:398: warning: passing arg 1 of `readw' makes pointer from integer without a cast
> drivers/net/wireless/hermes.h: In function `hermes_set_irqmask':
> drivers/net/wireless/hermes.h:404: warning: passing arg 2 of `writew' makes pointer from integer without a cast
> ...
> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> - readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> + readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> #define hermes_write_reg(hw, off, val) do { \
Isn't the correct fix to declare iobase as (void __iomem *) ?
Thanks,
Jank
> Isn't the correct fix to declare iobase as (void __iomem *) ?
>
Earlier today i had posted a patch which mainly fixes this same
problem with lotsa scsi
drivers and tulip drivers. I wondered the same "shouldnt all the addrs
be declared as
void __iomem* ??".
--
-> Ricky
On Mon, 11 Oct 2004, Jan Dittmer wrote:
> Cal Peake wrote:
>
> > inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > - readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > + readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > #define hermes_write_reg(hw, off, val) do { \
>
> Isn't the correct fix to declare iobase as (void __iomem *) ?
iobase is an unsigned long, declaring it as a void pointer is prolly not
what we want to do here. The typecast seems proper. A lot of other drivers
do this as well thus it must be proper ;-)
-- Cal
Cal Peake wrote:
> On Mon, 11 Oct 2004, Jan Dittmer wrote:
>
>
>>Cal Peake wrote:
>>
>>
>>> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
>>>- readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>+ readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>> #define hermes_write_reg(hw, off, val) do { \
>>
>>Isn't the correct fix to declare iobase as (void __iomem *) ?
>
>
> iobase is an unsigned long, declaring it as a void pointer is prolly not
> what we want to do here. The typecast seems proper. A lot of other drivers
> do this as well thus it must be proper ;-)
Why is iobase a unsigned long in the first place? Isn't this broken for
64bit archs?
Thanks,
Jan
On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
>
> Earlier today i had posted a patch which mainly fixes this same
> problem with lotsa scsi
> drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> be declared as
> void __iomem* ??".
The trouble with that is that for some versions of the orinoco card,
the iobase refers to a legacy ISA IO address, not a memory-mapped IO
address (that's the inw()/outw() path in the macro). That needs an
integer, rather than a pointer.
It's not clear to me which way around the cast is less ugly.
--
David Gibson | For every complex problem there is a
david AT gibson.dropbear.id.au | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
> Cal Peake wrote:
> >On Mon, 11 Oct 2004, Jan Dittmer wrote:
> >
> >
> >>Cal Peake wrote:
> >>
> >>
> >>> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> >>>- readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>+ readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>#define hermes_write_reg(hw, off, val) do { \
> >>
> >>Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
> >
> >iobase is an unsigned long, declaring it as a void pointer is prolly not
> >what we want to do here. The typecast seems proper. A lot of other drivers
> >do this as well thus it must be proper ;-)
>
> Why is iobase a unsigned long in the first place? Isn't this broken for
> 64bit archs?
Um, no.
--
David Gibson | For every complex problem there is a
david AT gibson.dropbear.id.au | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
David Gibson wrote:
> On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
>
>>Cal Peake wrote:
>>
>>>On Mon, 11 Oct 2004, Jan Dittmer wrote:
>>>
>>>
>>>
>>>>Cal Peake wrote:
>>>>
>>>>
>>>>
>>>>> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
>>>>>- readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>>>+ readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
>>>>>#define hermes_write_reg(hw, off, val) do { \
>>>>
>>>>Isn't the correct fix to declare iobase as (void __iomem *) ?
>>>
>>>
>>>iobase is an unsigned long, declaring it as a void pointer is prolly not
>>>what we want to do here. The typecast seems proper. A lot of other drivers
>>>do this as well thus it must be proper ;-)
>>
>>Why is iobase a unsigned long in the first place? Isn't this broken for
>>64bit archs?
>
>
> Um, no.
>
Yeah, just rememberd when sending the mail ;-). Still, most drivers seem
to use (void __iomem *) in the declaration of their iobase.
Jan
On Mon, 11 Oct 2004, David Gibson wrote:
> On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> > >
> >
> > Earlier today i had posted a patch which mainly fixes this same
> > problem with lotsa scsi
> > drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> > be declared as
> > void __iomem* ??".
>
> The trouble with that is that for some versions of the orinoco card,
> the iobase refers to a legacy ISA IO address, not a memory-mapped IO
> address (that's the inw()/outw() path in the macro). That needs an
> integer, rather than a pointer.
>
> It's not clear to me which way around the cast is less ugly.
I guess the cast is kludgy. I just wanted to shut the warnings up, it
prints 68 of 'em I believe.
-- Cal
On Mon, Oct 11, 2004 at 02:39:00PM +0200, Jan Dittmer wrote:
> David Gibson wrote:
> >On Mon, Oct 11, 2004 at 02:29:39PM +0200, Jan Dittmer wrote:
> >
> >>Cal Peake wrote:
> >>
> >>>On Mon, 11 Oct 2004, Jan Dittmer wrote:
> >>>
> >>>
> >>>
> >>>>Cal Peake wrote:
> >>>>
> >>>>
> >>>>
> >>>>> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> >>>>>- readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>>>+ readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> >>>>>#define hermes_write_reg(hw, off, val) do { \
> >>>>
> >>>>Isn't the correct fix to declare iobase as (void __iomem *) ?
> >>>
> >>>
> >>>iobase is an unsigned long, declaring it as a void pointer is prolly not
> >>>what we want to do here. The typecast seems proper. A lot of other
> >>>drivers do this as well thus it must be proper ;-)
> >>
> >>Why is iobase a unsigned long in the first place? Isn't this broken for
> >>64bit archs?
> >
> >
> >Um, no.
> >
>
> Yeah, just rememberd when sending the mail ;-). Still, most drivers seem
> to use (void __iomem *) in the declaration of their iobase.
IIRC, ioremap() and friends all return (void __iomem *) or at least
(void *)
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Mon, Oct 11, 2004 at 08:23:35AM -0400, Cal Peake wrote:
> On Mon, 11 Oct 2004, Jan Dittmer wrote:
>
> > Cal Peake wrote:
> >
> > > inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > > - readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > + readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > #define hermes_write_reg(hw, off, val) do { \
> >
> > Isn't the correct fix to declare iobase as (void __iomem *) ?
>
> iobase is an unsigned long, declaring it as a void pointer is prolly not
> what we want to do here. The typecast seems proper. A lot of other drivers
> do this as well thus it must be proper ;-)
Typecast is not a proper solution here. Folks, there are cleanups underway
for all that mess, but it's not _that_ simple.
And adding casts to shut the warnings up is wrong in 99% of cases.
On Monday 11 October 2004 14:31, David Gibson wrote:
Hi all,
I'm having the same pointer casting warnings with my ymfpci soundchip...
> On Mon, Oct 11, 2004 at 05:34:20PM +0530, Ricky lloyd wrote:
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
> > Earlier today i had posted a patch which mainly fixes this same
> > problem with lotsa scsi
> > drivers and tulip drivers. I wondered the same "shouldnt all the addrs
> > be declared as
> > void __iomem* ??".
>
> The trouble with that is that for some versions of the orinoco card,
> the iobase refers to a legacy ISA IO address, not a memory-mapped IO
> address (that's the inw()/outw() path in the macro). That needs an
> integer, rather than a pointer.
however,
I don't think there should be any casting of IO addresses for the simple reason that the void __iomem *
is a part of an interface which does the right thing in both MMIO/PIO cases. The lkml
thread "Being more anal about iospace accesses.." contains the answer to
that in a great detail. As a result, the right thing to do here is, I think, to declare all addrs void __iomem*.
Which leaves a question: while compiling the following code fragment:
<sound/pci/ymfpci/ymfpci_main.c>
static inline u8 snd_ymfpci_readb(ymfpci_t *chip, u32 offset)
{
return readb(chip->reg_area_virt + offset);
}
gcc complains as so:
sound/pci/ymfpci/ymfpci_main.c: In function `snd_ymfpci_readb':
sound/pci/ymfpci/ymfpci_main.c:57: warning: passing arg 1 of `readb' makes pointer from integer without a cast
Do we have to cast here or use the new interface?
>
> It's not clear to me which way around the cast is less ugly.
Boris.
On Mon, Oct 11, 2004 at 03:18:38PM +0200, Borislav Petkov wrote:
> that in a great detail. As a result, the right thing to do here is, I think, to declare all addrs void __iomem*.
> Which leaves a question: while compiling the following code fragment:
>
> <sound/pci/ymfpci/ymfpci_main.c>
> static inline u8 snd_ymfpci_readb(ymfpci_t *chip, u32 offset)
> {
> return readb(chip->reg_area_virt + offset);
> }
>
> gcc complains as so:
>
> sound/pci/ymfpci/ymfpci_main.c: In function `snd_ymfpci_readb':
> sound/pci/ymfpci/ymfpci_main.c:57: warning: passing arg 1 of `readb' makes pointer from integer without a cast
>
> Do we have to cast here or use the new interface?
Make ->reg_area_virt void __iomem *. *However*, ALSA folks said that they
have already done iomem annotations in their tree, so that's an area best
left alone until they merge.
On Mon, 11 Oct 2004, Cal Peake wrote:
> This patch fixes several dozen warnings spit out when compiling the hermes
> wireless driver.
I noticed them too.
By the way, it would be nice to move the discussion to the mailing list of
the driver, [email protected]. Sorry that it wasn't
mentioned in the MAINTAINERS file. I've just submitted a patch to add the
mailing lists to that file.
> @@ -364,12 +364,12 @@
> /* Register access convenience macros */
> #define hermes_read_reg(hw, off) ((hw)->io_space ? \
> inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> - readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> + readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
The HEAD version of the driver aims to support Linux starting with version
2.4.10, so you need to add some magic in kcompat.h to define __iomem.
HostAP driver uses cast to (void *), which compiles without warnings. I
believe it's sufficient because the call to readw() would cast the
argument to whatever readw() needs.
Another, more sophisticated solution would be to use union for iobase:
typedef struct hermes {
union {
unsigned long io;
void *mem;
} base;
int io_space; /* 1 if we IO-mapped IO, 0 for memory-mapped IO? */
...
}
--
Regards,
Pavel Roskin
On Mon, Oct 11, 2004 at 09:12:03AM -0400, Pavel Roskin wrote:
> Another, more sophisticated solution would be to use union for iobase:
>
> typedef struct hermes {
> union {
> unsigned long io;
> void *mem;
> } base;
> int io_space; /* 1 if we IO-mapped IO, 0 for memory-mapped IO? */
> ...
> }
Not needed. Use ioread*/iowrite* family; it does what you need.
Al, putting together a patchset and documention on that sort of cleanups...
On Mon, 11 Oct 2004 [email protected] wrote:
> On Mon, Oct 11, 2004 at 08:23:35AM -0400, Cal Peake wrote:
> > On Mon, 11 Oct 2004, Jan Dittmer wrote:
> >
> > > Cal Peake wrote:
> > >
> > > > inw((hw)->iobase + ( (off) << (hw)->reg_spacing )) : \
> > > > - readw((hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > > + readw((void __iomem *)(hw)->iobase + ( (off) << (hw)->reg_spacing )))
> > > > #define hermes_write_reg(hw, off, val) do { \
> > >
> > > Isn't the correct fix to declare iobase as (void __iomem *) ?
> >
> > iobase is an unsigned long, declaring it as a void pointer is prolly not
> > what we want to do here. The typecast seems proper. A lot of other drivers
> > do this as well thus it must be proper ;-)
>
> Typecast is not a proper solution here. Folks, there are cleanups underway
> for all that mess, but it's not _that_ simple.
>
> And adding casts to shut the warnings up is wrong in 99% of cases.
ok, I'm retarded. I'll shut up for the moment and get a clue :^)
-- Cal