LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
@ 2015-01-29 14:51 Ricardo Ribalda Delgado
  2015-01-29 15:56 ` Geert Uytterhoeven
  2015-01-29 16:04 ` Arnd Bergmann
  0 siblings, 2 replies; 8+ messages in thread
From: Ricardo Ribalda Delgado @ 2015-01-29 14:51 UTC (permalink / raw)
  To: Mark Brown, Michal Simek, Sören Brinkmann, linux-spi,
	linux-arm-kernel, linux-kernel
  Cc: 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 14:51 [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers Ricardo Ribalda Delgado
@ 2015-01-29 15:56 ` Geert Uytterhoeven
  2015-01-29 16:39   ` Ricardo Ribalda Delgado
  2015-01-29 16:04 ` Arnd Bergmann
  1 sibling, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2015-01-29 15:56 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Mark Brown, Michal Simek, Sören Brinkmann, linux-spi,
	linux-arm-kernel, linux-kernel, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 14:51 [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers Ricardo Ribalda Delgado
  2015-01-29 15:56 ` Geert Uytterhoeven
@ 2015-01-29 16:04 ` Arnd Bergmann
  2015-01-29 16:39   ` Ricardo Ribalda Delgado
  1 sibling, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2015-01-29 16:04 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Ricardo Ribalda Delgado, Mark Brown, Michal Simek,
	Sören Brinkmann, linux-spi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 15:56 ` Geert Uytterhoeven
@ 2015-01-29 16:39   ` Ricardo Ribalda Delgado
  2015-01-29 22:11     ` Arnd Bergmann
  0 siblings, 1 reply; 8+ messages in thread
From: Ricardo Ribalda Delgado @ 2015-01-29 16:39 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Michal Simek, Sören Brinkmann, linux-spi,
	linux-arm-kernel, linux-kernel, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 16:04 ` Arnd Bergmann
@ 2015-01-29 16:39   ` Ricardo Ribalda Delgado
  0 siblings, 0 replies; 8+ messages in thread
From: Ricardo Ribalda Delgado @ 2015-01-29 16:39 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: moderated list:ARM/S5P EXYNOS AR...,
	Mark Brown, Michal Simek, Sören Brinkmann, linux-spi, LKML

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 16:39   ` Ricardo Ribalda Delgado
@ 2015-01-29 22:11     ` Arnd Bergmann
  2015-01-29 22:14       ` Ricardo Ribalda Delgado
  0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2015-01-29 22:11 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Ricardo Ribalda Delgado, Geert Uytterhoeven, linux-kernel,
	Michal Simek, Mark Brown, Sören Brinkmann, linux-spi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 22:11     ` Arnd Bergmann
@ 2015-01-29 22:14       ` Ricardo Ribalda Delgado
  2015-01-29 22:28         ` Arnd Bergmann
  0 siblings, 1 reply; 8+ messages in thread
From: Ricardo Ribalda Delgado @ 2015-01-29 22:14 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: moderated list:ARM/S5P EXYNOS AR...,
	Geert Uytterhoeven, linux-kernel, Michal Simek, Mark Brown,
	Sören Brinkmann, linux-spi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers
  2015-01-29 22:14       ` Ricardo Ribalda Delgado
@ 2015-01-29 22:28         ` Arnd Bergmann
  0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2015-01-29 22:28 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: moderated list:ARM/S5P EXYNOS AR...,
	Geert Uytterhoeven, linux-kernel, Michal Simek, Mark Brown,
	Sören Brinkmann, linux-spi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-01-29 22:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 14:51 [PATCH] spi/xilinx: Cast ioread32/iowrite32 function pointers Ricardo Ribalda Delgado
2015-01-29 15:56 ` Geert Uytterhoeven
2015-01-29 16:39   ` Ricardo Ribalda Delgado
2015-01-29 22:11     ` Arnd Bergmann
2015-01-29 22:14       ` Ricardo Ribalda Delgado
2015-01-29 22:28         ` Arnd Bergmann
2015-01-29 16:04 ` Arnd Bergmann
2015-01-29 16:39   ` Ricardo Ribalda Delgado

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