| Message ID | 1422543073-21895-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 29, 2015 at 3:51 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > IO functions prototypes may have different argument qualifiers > on different architectures. > > This patch cast the assignment of the function, to match the one defined > at iomap.h. Adding casts is (usually) not the solution to the problem. I think the definitions in include/asm-generic/iomap.h are actually wrong, as they lack a const: extern unsigned int ioread8(void __iomem *); extern unsigned int ioread16(void __iomem *); extern unsigned int ioread16be(void __iomem *); extern unsigned int ioread32(void __iomem *); extern unsigned int ioread32be(void __iomem *); Note that the definitions in include/asm-generic/io.h do have the const: static inline u8 ioread8(const volatile void __iomem *addr) static inline u16 ioread16(const volatile void __iomem *addr) static inline u32 ioread32(const volatile void __iomem *addr) static inline u16 ioread16be(const volatile void __iomem *addr) static inline u32 ioread32be(const volatile void __iomem *addr) > Fixes: 99082eab63449f9d spi/xilinx: Remove iowrite/ioread wrappers > Reported-by: kbuild test robot <fengguang.wu@intel.com> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > drivers/spi/spi-xilinx.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c > index 0e8962c..418e730 100644 > --- a/drivers/spi/spi-xilinx.c > +++ b/drivers/spi/spi-xilinx.c > @@ -91,7 +91,7 @@ struct xilinx_spi { > u8 bytes_per_word; > int buffer_size; /* buffer size in words */ > u32 cs_inactive; /* Level of the CS pins when inactive*/ > - unsigned int (*read_fn)(void __iomem *); > + u32 (*read_fn)(void __iomem *); > void (*write_fn)(u32, void __iomem *); > }; > > @@ -378,15 +378,15 @@ static int xilinx_spi_probe(struct platform_device *pdev) > * Setup little endian helper functions first and try to use them > * and check if bit was correctly setup or not. > */ > - xspi->read_fn = ioread32; > - xspi->write_fn = iowrite32; > + xspi->read_fn = (u32 (*)(void __iomem *)) ioread32; > + xspi->write_fn = (void (*)(u32, void __iomem *)) iowrite32; > > xspi->write_fn(XSPI_CR_LOOP, xspi->regs + XSPI_CR_OFFSET); > tmp = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET); > tmp &= XSPI_CR_LOOP; > if (tmp != XSPI_CR_LOOP) { > - xspi->read_fn = ioread32be; > - xspi->write_fn = iowrite32be; > + xspi->read_fn = (u32 (*)(void __iomem *)) ioread32be; > + xspi->write_fn = (void (*)(u32, void __iomem *)) iowrite32be; > } > > master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word); > -- > 2.1.4 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thursday 29 January 2015 15:51:13 Ricardo Ribalda Delgado wrote: > * Setup little endian helper functions first and try to use them > * and check if bit was correctly setup or not. > */ > - xspi->read_fn = ioread32; > - xspi->write_fn = iowrite32; > + xspi->read_fn = (u32 (*)(void __iomem *)) ioread32; > + xspi->write_fn = (void (*)(u32, void __iomem *)) iowrite32; > > xspi->write_fn(XSPI_CR_LOOP, xspi->regs + XSPI_CR_OFFSET); > Casting the type of a function you call seems rather dangerous. Why not add an inline function in this driver as a wrapper? Arnd
Hello Geert On Thu, Jan 29, 2015 at 4:56 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Jan 29, 2015 at 3:51 PM, Ricardo Ribalda Delgado > <ricardo.ribalda@gmail.com> wrote: >> IO functions prototypes may have different argument qualifiers >> on different architectures. >> >> This patch cast the assignment of the function, to match the one defined >> at iomap.h. > > Adding casts is (usually) not the solution to the problem. > > I think the definitions in include/asm-generic/iomap.h are actually wrong, > as they lack a const: > > extern unsigned int ioread8(void __iomem *); > extern unsigned int ioread16(void __iomem *); > extern unsigned int ioread16be(void __iomem *); > extern unsigned int ioread32(void __iomem *); > extern unsigned int ioread32be(void __iomem *); > > Note that the definitions in include/asm-generic/io.h do have the const: > > static inline u8 ioread8(const volatile void __iomem *addr) > static inline u16 ioread16(const volatile void __iomem *addr) > static inline u32 ioread32(const volatile void __iomem *addr) > static inline u16 ioread16be(const volatile void __iomem *addr) > static inline u32 ioread32be(const volatile void __iomem *addr) I think you are right: ioread/iowrite is poorly implemented in many arches :S Would you mind sending the patch for asm-generic/iomap ? Or you want me to do it. Until this is fixed on iomap and sparc64 I think 99082eab63449f9d spi/xilinx: Remove iowrite/ioread wrappers should be reverted and this patch ignored. Sorry for the noise. Thanks!
Hello Arnd On Thu, Jan 29, 2015 at 5:04 PM, Arnd Bergmann <arnd@arndb.de> wrote: > Casting the type of a function you call seems rather dangerous. Why not > add an inline function in this driver as a wrapper? > > Arnd Agree, please ignore this patch. Sorry for the noise
On Thursday 29 January 2015 17:39:08 Ricardo Ribalda Delgado wrote: > > I think the definitions in include/asm-generic/iomap.h are actually wrong, > > as they lack a const: > > > > extern unsigned int ioread8(void __iomem *); > > extern unsigned int ioread16(void __iomem *); > > extern unsigned int ioread16be(void __iomem *); > > extern unsigned int ioread32(void __iomem *); > > extern unsigned int ioread32be(void __iomem *); > > > > Note that the definitions in include/asm-generic/io.h do have the const: > > > > static inline u8 ioread8(const volatile void __iomem *addr) > > static inline u16 ioread16(const volatile void __iomem *addr) > > static inline u32 ioread32(const volatile void __iomem *addr) > > static inline u16 ioread16be(const volatile void __iomem *addr) > > static inline u32 ioread32be(const volatile void __iomem *addr) > > > I think you are right: ioread/iowrite is poorly implemented in many arches > > Would you mind sending the patch for asm-generic/iomap ? Or you want > me to do it. > I think we don't need the 'volatile' keyword here. The main reason we have it on readl() is to shut up the compiler when dealing with ancient driver code that annotates iomem pointers as volatile. This is generally considered a (minor) driver mistake though, and modern drivers that for some reason use ioread*() typically don't do that (or they get a warning). Arnd
Hello Arnd On Thu, Jan 29, 2015 at 11:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > I think the definitions in include/asm-generic/iomap.h are actually wrong, >> > as they lack a const: >> > >> > extern unsigned int ioread8(void __iomem *); >> > extern unsigned int ioread16(void __iomem *); >> > extern unsigned int ioread16be(void __iomem *); >> > extern unsigned int ioread32(void __iomem *); >> > extern unsigned int ioread32be(void __iomem *); >> > >> > Note that the definitions in include/asm-generic/io.h do have the const: >> > >> > static inline u8 ioread8(const volatile void __iomem *addr) >> > static inline u16 ioread16(const volatile void __iomem *addr) >> > static inline u32 ioread32(const volatile void __iomem *addr) >> > static inline u16 ioread16be(const volatile void __iomem *addr) >> > static inline u32 ioread32be(const volatile void __iomem *addr) >> > I think we don't need the 'volatile' keyword here. The main reason > we have it on readl() is to shut up the compiler when dealing with > ancient driver code that annotates iomem pointers as volatile. > > This is generally considered a (minor) driver mistake though, and > modern drivers that for some reason use ioread*() typically don't > do that (or they get a warning). What about the different return type? u8 vs int Thanks
On Thursday 29 January 2015 23:14:46 Ricardo Ribalda Delgado wrote:
> What about the different return type? u8 vs int
Too many drivers use all sorts of different types, I've given up
hope of changing drivers to agree on the same type here. Basically
you can think of 'void __iomem *' as the magic keyword for something
that gets returned from ioremap, can take an integer offset and gets
passed into readb/readw/readl/write..., but any other assumption beyond
that is dangerous.
Arnd
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c index 0e8962c..418e730 100644 --- a/drivers/spi/spi-xilinx.c +++ b/drivers/spi/spi-xilinx.c @@ -91,7 +91,7 @@ struct xilinx_spi { u8 bytes_per_word; int buffer_size; /* buffer size in words */ u32 cs_inactive; /* Level of the CS pins when inactive*/ - unsigned int (*read_fn)(void __iomem *); + u32 (*read_fn)(void __iomem *); void (*write_fn)(u32, void __iomem *); }; @@ -378,15 +378,15 @@ static int xilinx_spi_probe(struct platform_device *pdev) * Setup little endian helper functions first and try to use them * and check if bit was correctly setup or not. */ - xspi->read_fn = ioread32; - xspi->write_fn = iowrite32; + xspi->read_fn = (u32 (*)(void __iomem *)) ioread32; + xspi->write_fn = (void (*)(u32, void __iomem *)) iowrite32; xspi->write_fn(XSPI_CR_LOOP, xspi->regs + XSPI_CR_OFFSET); tmp = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET); tmp &= XSPI_CR_LOOP; if (tmp != XSPI_CR_LOOP) { - xspi->read_fn = ioread32be; - xspi->write_fn = iowrite32be; + xspi->read_fn = (u32 (*)(void __iomem *)) ioread32be; + xspi->write_fn = (void (*)(u32, void __iomem *)) iowrite32be; } master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word);
IO functions prototypes may have different argument qualifiers on different architectures. This patch cast the assignment of the function, to match the one defined at iomap.h. Fixes: 99082eab63449f9d spi/xilinx: Remove iowrite/ioread wrappers Reported-by: kbuild test robot <fengguang.wu@intel.com> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- drivers/spi/spi-xilinx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)