RE: [ANNOUNCE] [PATCH] [MTD] Flex-OneNAND MTD Driver available.

[Artem Bityutskiy <dedekind@infradead.org>]

> +/**
>   * onenand_get_density - [DEFAULT] Get OneNAND density
>   * @param dev_id	OneNAND device ID
>   *
> @@ -196,6 +280,7 @@ static int onenand_command(struct mtd_info *mtd, int
> cmd, loff_t addr, size_t le
>  {
>  	struct onenand_chip *this = mtd->priv;
>  	int value, block, page;
> +	unsigned slc = 0;
>  
>  	/* Address translation */
>  	switch (cmd) {
> @@ -207,15 +292,16 @@ static int onenand_command(struct mtd_info *mtd, int
> cmd, loff_t addr, size_t le
>  		page = -1;
>  		break;
>  
> +	case FLEXONENAND_CMD_PI_ACCESS:
>  	case ONENAND_CMD_ERASE:
>  	case ONENAND_CMD_BUFFERRAM:
>  	case ONENAND_CMD_OTP_ACCESS:
> -		block = (int) (addr >> this->erase_shift);
> +		block = onenand_get_block(mtd, addr, NULL);

Why do you make SLC slower by adding a function call it which it does
not need? Could you please make it in-line for SLC and a func for MLC.
IOW, I do not like that you make SLC degrade because of supporting MLC.

E.g., you may do it like this:

static inline onenand_get_block()
{
	if (SLC)
 		return addr >> this->erase_shift;
	else
		return flexonenand_get_block()
}

>  		page = -1;
>  		break;
>  
>  	default:
> -		block = (int) (addr >> this->erase_shift);
> +		block = onenand_get_block(mtd, addr, &slc);
>  		page = (int) (addr >> this->page_shift);
>  
>  		if (ONENAND_IS_2PLANE(this)) {
> @@ -227,6 +313,8 @@ static int onenand_command(struct mtd_info *mtd, int
> cmd, loff_t addr, size_t le
>  			page >>= 1;
>  		}
>  		page &= this->page_mask;
> +		if (FLEXONENAND(this) && slc)
> +			page &= (this->page_mask >> 1);

This is bad. Please, be consistent and use _one_ way to get the address.
Either a function or the if statement. Do not introduce mess by mixing
these approaches.

I tried to review the rest of the patch. But quite frankly, it is so
difficult to do, because you injected little pieces of

if (MLC) {
	do;
	various;
	stuff;
}

all over the place, and made the driver very difficult to follow. Could
you please work some more on the driver and try to improve the
readability? E.g., by making some functions to have 2 separate variants
- one for SLC, one for MLC. If you do not want to duplicate some code -
this is what fuctions exist for.

Also, the patch is line-wrapped. You should read this:
Documentation/SubmittingPatches

Thanks.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)