[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Ricardo Ribalda Delgado]

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(-)

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

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Geert Uytterhoeven]

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 at 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

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Ricardo Ribalda Delgado]

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!



-- 
Ricardo Ribalda

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Arnd Bergmann]

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

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Ricardo Ribalda Delgado]

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

-- 
Ricardo Ribalda

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Arnd Bergmann]

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

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Arnd Bergmann]

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

[PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers

[Ricardo Ribalda Delgado]

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


-- 
Ricardo Ribalda