[PATCH] spi: Mediatek: fix endian warnings

[PATCH] spi: Mediatek: fix endian warnings

[Leilk Liu]

This patch fixes endian warnings detected by sparse:
- sparse: incorrect type in argument 1 (different base types)
	  expected unsigned int [unsigned] val
	  got restricted __le32 [usertype] <noident>
- sparse: incorrect type in argument 1 (different base types)
	  expected unsigned int [unsigned] val
	  got restricted __le32 [usertype] <noident>

Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
---
 drivers/spi/spi-mt65xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 4676b01..ae645fa 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 
 	if (mdata->tx_sgl)
-		writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
+		writel((__force u32)cpu_to_le32(xfer->tx_dma),
+		       mdata->base + SPI_TX_SRC_REG);
 	if (mdata->rx_sgl)
-		writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
+		writel((__force u32)cpu_to_le32(xfer->rx_dma),
+		       mdata->base + SPI_RX_DST_REG);
 }
 
 static int mtk_spi_fifo_transfer(struct spi_master *master,
-- 
1.8.1.1.dirty

Applied "spi: mediatek: fix endian warnings" to the spi tree

[Mark Brown]

The patch

   spi: mediatek: fix endian warnings

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7abc01b346e67534cc0307544e563e5497280296 Mon Sep 17 00:00:00 2001
From: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Date: Tue, 11 Aug 2015 18:43:09 +0800
Subject: [PATCH] spi: mediatek: fix endian warnings

This patch fixes endian warnings detected by sparse:
- sparse: incorrect type in argument 1 (different base types)
	  expected unsigned int [unsigned] val
	  got restricted __le32 [usertype] <noident>
- sparse: incorrect type in argument 1 (different base types)
	  expected unsigned int [unsigned] val
	  got restricted __le32 [usertype] <noident>

Signed-off-by: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-mt65xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 08da77e..2c41dcf 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 
 	if (mdata->tx_sgl)
-		writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
+		writel((__force u32)cpu_to_le32(xfer->tx_dma),
+		       mdata->base + SPI_TX_SRC_REG);
 	if (mdata->rx_sgl)
-		writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
+		writel((__force u32)cpu_to_le32(xfer->rx_dma),
+		       mdata->base + SPI_RX_DST_REG);
 }
 
 static int mtk_spi_fifo_transfer(struct spi_master *master,
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Mediatek: fix endian warnings

[Jonas Gorski]

Hi,

On Tue, Aug 11, 2015 at 12:43 PM, Leilk Liu <leilk.liu@mediatek.com> wrote:
> This patch fixes endian warnings detected by sparse:
> - sparse: incorrect type in argument 1 (different base types)
>           expected unsigned int [unsigned] val
>           got restricted __le32 [usertype] <noident>
> - sparse: incorrect type in argument 1 (different base types)
>           expected unsigned int [unsigned] val
>           got restricted __le32 [usertype] <noident>

This doesn't "fix" the warning, it only hides the warning and leaves
the actual issue unfixed.

>
> Signed-off-by: Leilk Liu <leilk.liu@mediatek.com>
> ---
>  drivers/spi/spi-mt65xx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 4676b01..ae645fa 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
>         struct mtk_spi *mdata = spi_master_get_devdata(master);
>
>         if (mdata->tx_sgl)
> -               writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> +               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> +                      mdata->base + SPI_TX_SRC_REG);
>         if (mdata->rx_sgl)
> -               writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> +               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> +                      mdata->base + SPI_RX_DST_REG);

The issue here is that writel already does a cpu_to_le32 conversion,
so the extra cpu_to_le32 calls are actually bogus and need to be
removed. Else it will do a double conversion on big endian systems,
resulting in the data being written in big endian.


Jonas

Re: [PATCH] spi: Mediatek: fix endian warnings

[lei liu]

Hello Mark,

 This patch is applied, should I append a new patch or you rollback it?
Thanks.

On Wed, 2015-08-12 at 16:45 +0200, Jonas Gorski wrote:
> Hi,
> 
> On Tue, Aug 11, 2015 at 12:43 PM, Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > This patch fixes endian warnings detected by sparse:
> > - sparse: incorrect type in argument 1 (different base types)
> >           expected unsigned int [unsigned] val
> >           got restricted __le32 [usertype] <noident>
> > - sparse: incorrect type in argument 1 (different base types)
> >           expected unsigned int [unsigned] val
> >           got restricted __le32 [usertype] <noident>
> 
> This doesn't "fix" the warning, it only hides the warning and leaves
> the actual issue unfixed.
> 
> >
> > Signed-off-by: Leilk Liu <leilk.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/spi/spi-mt65xx.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> > index 4676b01..ae645fa 100644
> > --- a/drivers/spi/spi-mt65xx.c
> > +++ b/drivers/spi/spi-mt65xx.c
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> >         struct mtk_spi *mdata = spi_master_get_devdata(master);
> >
> >         if (mdata->tx_sgl)
> > -               writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > +                      mdata->base + SPI_TX_SRC_REG);
> >         if (mdata->rx_sgl)
> > -               writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > +                      mdata->base + SPI_RX_DST_REG);
> 
> The issue here is that writel already does a cpu_to_le32 conversion,
> so the extra cpu_to_le32 calls are actually bogus and need to be
> removed. Else it will do a double conversion on big endian systems,
> resulting in the data being written in big endian.
> 
> 
> Jonas


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Mediatek: fix endian warnings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

On Thu, Aug 13, 2015 at 10:11:59AM +0800, lei liu wrote:

>  This patch is applied, should I append a new patch or you rollback it?
> Thanks.

Please send an incremental patch on top of what's applied and don't top
post.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: Mediatek: fix endian warnings

[Arnd Bergmann]

On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
>         struct mtk_spi *mdata = spi_master_get_devdata(master);
>  
>         if (mdata->tx_sgl)
> -               writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> +               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> +                      mdata->base + SPI_TX_SRC_REG);
>         if (mdata->rx_sgl)
> -               writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> +               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> +                      mdata->base + SPI_RX_DST_REG);
>  }
> 

This looks wrong: writel takes a CPU-endian argument, so the value returned
from cpu_to_le32() is not appropriate.

The warning is correct, and you have to remove the cpu_to_le32() conversion
in order to get the driver to behave correctly when the kernel is built
as big-endian.

	Arnd

Re: [PATCH] spi: Mediatek: fix endian warnings

[Arnd Bergmann]

On Saturday 15 August 2015 22:16:03 Arnd Bergmann wrote:
> On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> >         struct mtk_spi *mdata = spi_master_get_devdata(master);
> >  
> >         if (mdata->tx_sgl)
> > -               writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > +                      mdata->base + SPI_TX_SRC_REG);
> >         if (mdata->rx_sgl)
> > -               writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > +                      mdata->base + SPI_RX_DST_REG);
> >  }
> > 
> 
> This looks wrong: writel takes a CPU-endian argument, so the value returned
> from cpu_to_le32() is not appropriate.
> 
> The warning is correct, and you have to remove the cpu_to_le32() conversion
> in order to get the driver to behave correctly when the kernel is built
> as big-endian.

Nevermind, I now saw the issue has already been raised.

	Arnd

Re: [PATCH] spi: Mediatek: fix endian warnings

[Russell King - ARM Linux]

On Sat, Aug 15, 2015 at 10:16:03PM +0200, Arnd Bergmann wrote:
> On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> >         struct mtk_spi *mdata = spi_master_get_devdata(master);
> >  
> >         if (mdata->tx_sgl)
> > -               writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > +                      mdata->base + SPI_TX_SRC_REG);
> >         if (mdata->rx_sgl)
> > -               writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > +               writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > +                      mdata->base + SPI_RX_DST_REG);
> >  }
> > 
> 
> This looks wrong: writel takes a CPU-endian argument, so the value returned
> from cpu_to_le32() is not appropriate.
> 
> The warning is correct, and you have to remove the cpu_to_le32() conversion
> in order to get the driver to behave correctly when the kernel is built
> as big-endian.

Indeed, it's about time people started thinking more about the warnings
and why we have coded things in the way we have.

Look people.  cpu_to_le32() takes a value in the CPU endian, and
converts it to a little endian 32-bit number.  See, the clue is in
the name.

All writel() implementations take a CPU number and write it in little
endian format.  Hence, writel() almost always uses cpu_to_le32()
internally.

Now think about what you're saying with "cpu_to_le32(cpu_to_le32())".
It's utter rubbish, total crap.  It's wrong no matter which way you
look at it.

We have le32_to_cpu() which does what it says on the tin.  Same with
be32_to_cpu() and cpu_to_be32().

Don't hack around this stuff with __force.  If you're having to use
__force to get rid of a warning here, you _ARE_ doing something wrong,
no questions about that.  __force in driver code is a definite sign
that you are doing something wrong.  Don't do it.  Ask the question
if you think you need it.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Mediatek: fix endian warnings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Sun, Aug 16, 2015 at 03:06:39PM +0100, Russell King - ARM Linux wrote:

> All writel() implementations take a CPU number and write it in little
> endian format.  Hence, writel() almost always uses cpu_to_le32()
> internally.

I think the reason this keeps coming up is that people aren't expecting
writel() to be doing endianness conversion, it's easy to get caught out
by it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]