linux-kernel - Re: [PATCH] spi-nor:fsl-quadspi:Add LS1021 support for fsl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <201410142310.13832.marex@denx.de>
Date:	Tue, 14 Oct 2014 23:10:13 +0200
From:	Marek Vasut <marex@...x.de>
To:	Chao Fu <b44548@...escale.com>
Cc:	shijie.huang@...el.com, geert+renesas@...der.be,
	ryazanov.s.a@...il.com, computersforpeace@...il.com,
	zajec5@...il.com, dwmw2@...radead.org,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi-nor:fsl-quadspi:Add LS1021 support for fsl_quadspi

On Tuesday, October 14, 2014 at 08:36:21 AM, Chao Fu wrote:
> From: Chao Fu <B44548@...escale.com>
> 
> FSL Quadspi module register bitwise is big-endian, but on ohter paltform
> is little endian.
> Add functions for Quadspi register read/write for bitwise:
> qspi_readl
> qpsi_writel

The commit message really needs fixing ;-)

> Add devtype for LS1021:
> struct fsl_qspi_devtype_data ls1_data

A devtype ?

[...]

> @@ -242,6 +250,23 @@ static inline int is_imx6sx_qspi(struct fsl_qspi *q)
>  	return q->devtype_data->devtype == FSL_QUADSPI_IMX6SX;
>  }
> 
> +static inline int is_ls1_qspi(struct fsl_qspi *q)

Drop the inline and rename the function to be something like 
"fsl_qspi_is_bigendian()", so once a new LS2 enters the market,
this function won't have to be renamed . Future proof the driver ;-)

> +{
> +	return q->devtype_data->devtype == FSL_QUADSPI_LS1;
> +}
> +
> +static inline void qspi_writel(struct fsl_qspi *q, u32 val, void __iomem
> *addr) +{

Drop the inline.

> +	is_ls1_qspi(q) ? __raw_writel(cpu_to_be32(val), addr) :
> +			__raw_writel(cpu_to_le32(val), addr);

You're changing the behavior here, you're replacing writel() with __raw_writel() 
and you are not documenting it anywhere. Nonetheless, such a change should be in
a separate patch anyway .

Besides, it's only the value that's bitwise-swapped here, so just do:

if (fsl_qspi_is_be())
	val = cpu_to_be32()
else
	val = cpu_to_le32()
writel(val, addr);

That's much more readable.

> +}
> +
> +static inline u32 qspi_readl(struct fsl_qspi *q, void __iomem *addr)
> +{
> +	return is_ls1_qspi(q) ? cpu_to_be32((__force u32) __raw_readl(addr)) :
> +				cpu_to_le32((__force u32) __raw_readl(addr));

DTTO as above. Furthermore, drop the __force u32 nonsense. Furthermore, here
the bitwise swaps go the other way around, thus it should be [lb]e32_to_cpu()
instead.
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/