[08/10] mtd: add MTD_MLCNANDFLASH case for mtd_type_show()

The current mtd_type_show() misses the MTD_MLCNANDFLASH case.
This patch adds the case for it.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/mtdcore.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

On Mon, Aug 12, 2013 at 01:42:51PM +0800, Huang Shijie wrote:
> The current mtd_type_show() misses the MTD_MLCNANDFLASH case.
> This patch adds the case for it.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/mtdcore.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 6aa952b..c7cee29 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -157,6 +157,9 @@ static ssize_t mtd_type_show(struct device *dev,
>  	case MTD_UBIVOLUME:
>  		type = "ubi";
>  		break;
> +	case MTD_MLCNANDFLASH:
> +		type = "MLC nand";

The current convention uses lower-case, single-word names. And your
selection isn't very consistent with the SLC NAND case (but I see that
you're trying to change the SLC NAND case). I'd go with "mlc-nand" or
just "mlc".

But more importantly, this is probably a break in ABI. At a minimum,
this needs documentation here in Documentation/ABI. I know tools which
currently check only for "nand", and if the name suddely becomes
different for MLC, that is a breakage.

But really, does user-space need to know SLC vs. MLC? If so, this needs
a clear argument for why in the patch description. Perhaps to
differentiate whether or not JFFS2 support is even possible? I'm not
opposed to adding a new name, especially since the MTD_MLCNANDFLASH
macro has existed in mtd/mtd-abi.h for a long time (but it was rotten).
Just do it sensibly (i.e., better name string, proper ABI documentation
inlcuded in this patch, explain the reason for the ABI addition in this
patch, etc.).

Brian

于 2013年08月13日 09:05, Brian Norris 写道:
> The current convention uses lower-case, single-word names. And your
> selection isn't very consistent with the SLC NAND case (but I see that
> you're trying to change the SLC NAND case). I'd go with "mlc-nand" or
> just "mlc".
>
> But more importantly, this is probably a break in ABI. At a minimum,
> this needs documentation here in Documentation/ABI. I know tools which
> currently check only for "nand", and if the name suddely becomes
> different for MLC, that is a breakage.
thanks for pointing this. If the tool may check the "nand", I prefer to 
do not change the ABI.

> But really, does user-space need to know SLC vs. MLC? If so, this needs
I expose it to user-space only for mtd-info.
But if these two patches will break current tools (not the mtd-info), i
will abandon these two patches.

thanks
Huang Shijie


> a clear argument for why in the patch description. Perhaps to
> differentiate whether or not JFFS2 support is even possible? I'm not
> opposed to adding a new name, especially since the MTD_MLCNANDFLASH
> macro has existed in mtd/mtd-abi.h for a long time (but it was rotten).
> Just do it sensibly (i.e., better name string, proper ABI documentation
> inlcuded in this patch, explain the reason for the ABI addition in this
> patch, etc.).

On Mon, Aug 12, 2013 at 7:20 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年08月13日 09:05, Brian Norris 写道:
>
>> The current convention uses lower-case, single-word names. And your
>> selection isn't very consistent with the SLC NAND case (but I see that
>> you're trying to change the SLC NAND case). I'd go with "mlc-nand" or
>> just "mlc".
>>
>> But more importantly, this is probably a break in ABI. At a minimum,
>> this needs documentation here in Documentation/ABI. I know tools which
>> currently check only for "nand", and if the name suddely becomes
>> different for MLC, that is a breakage.
>
> thanks for pointing this. If the tool may check the "nand", I prefer to do
> not change the ABI.

OK. And I guess for a tool that wants to check the type for SLC vs.
MLC from userspace, they can still use ioctl(MEMGETINFO) which has
always documented the MTD_MLCNANDFLASH type (we just didn't use it
right).

>> But really, does user-space need to know SLC vs. MLC? If so, this needs
>
> I expose it to user-space only for mtd-info.
> But if these two patches will break current tools (not the mtd-info), i
> will abandon these two patches.

I think the "nand" to "SLC nand" patch should be dropped, but this
patch is a different story. Without this patch, MLC NAND will be
exported as "unknown". So you need a new patch which will combine the
'switch (mtd->type)' cases for MTD_NANDFLASH and MTD_MLCNANDFLASH.
Otherwise, your patch set leaves MLC exported as "unknown" (a
definite, undesirable breakage).

[Side note: onenand already uses MTD_MLCNANDLFASH, so this is already
exported as "unknkown" in the current upstream. I assume no one cares
about this entry for MLC onenand, if such a thing exists.]

>> a clear argument for why in the patch description. Perhaps to
>> differentiate whether or not JFFS2 support is even possible? I'm not
>> opposed to adding a new name, especially since the MTD_MLCNANDFLASH
>> macro has existed in mtd/mtd-abi.h for a long time (but it was rotten).
>> Just do it sensibly (i.e., better name string, proper ABI documentation
>> inlcuded in this patch, explain the reason for the ABI addition in this
>> patch, etc.).

Regards,
Brian

于 2013年08月13日 11:10, Brian Norris 写道:
> I think the "nand" to "SLC nand" patch should be dropped, but this
> patch is a different story. Without this patch, MLC NAND will be
> exported as "unknown". So you need a new patch which will combine the
> 'switch (mtd->type)' cases for MTD_NANDFLASH and MTD_MLCNANDFLASH.
> Otherwise, your patch set leaves MLC exported as "unknown" (a
ok, I will keep this patch, and update the document.

thanks
Huang Shijie