Re: [PATCH 3/3] mtd: nand_base: Use readsb/readsw/writesb/writesw if possible

[Brian Norris <computersforpeace@gmail.com>]

Hi Alexander,

I'm not an expert on IO accessors, but since you asked...

On 02/28/2013 12:02 AM, Alexander Shiyan wrote:
> This patch provide using readsb/readsw/writesb/writesw functions
> if it possible for particular platform.

The commit description does not give enough motivation. Should this 
improve performance? Or is this just a "cleanup"? (If the latter, then 
it fails, as the resulting code is much less clean.)

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>   drivers/mtd/nand/nand_base.c |   29 ++++++++++++++++++++++++-----
>   1 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 382b857..4efde01 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr)
>    */
>   static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>   {
> -	int i;
>   	struct nand_chip *chip = mtd->priv;
>
> +#ifndef writesb

No one guarantees that 'writesb' will be a macro and not a static inline 
function, do they?

> +	int i;
> +
>   	for (i = 0; i < len; i++)
>   		writeb(buf[i], chip->IO_ADDR_W);
> +#else
> +	writesb(chip->IO_ADDR_W, buf, len);
> +#endif
>   }

These #ifndef's are very ugly and are likely the wrong way(TM). The 
first question I ask when I see this is: why isn't this interface 
available on all platforms? In fact, it seems like it has been dropped 
from the asm-generic headers and specifically recommended against. See:

commit b2656a138ab7bc4e7abd3b1cbd6d1f105c7a7186
Author: Will Deacon <will.deacon@arm.com>
Date:   Wed Oct 17 16:45:01 2012 +0100

     asm-generic: io: remove {read,write} string functions

Quote: "We should encourage driver writers to use the 
io{read,write}{8,16,32}_rep functions instead."

So, an alternative patch might use iowrite8_rep() here unconditionally.

>   /**

<snipped the rest; same comments apply>

Brian