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

[Marek Vasut <marex@denx.de>]

On Tuesday, October 14, 2014 at 08:36:21 AM, Chao Fu wrote:
> From: Chao Fu <B44548@freescale.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.
[...]