Re: [PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Rafal,

On Tue, 22 Mar 2016 23:54:11 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> Some time ago I started working on a new "nand-ecc-algo" property to allow
> specifying ECC algorithm for hardware ECC mode as well.
> Boris pointed out it's becoming a bit messy that way as we already have value
> NAND_ECC_SOFT_BCH.
> I suggested deprecating "soft_bch" value from nand-ecc-mode property and got
> Boris agreed and no objections from others. So there is how I want to implement
> this.
> If you agree on this way & apply my patches, I'll start modifying NAND drivers
> (similarly to the nandsim) and then will try to drop NAND_ECC_SOFT_BCH except
> for handling backward compatibility.
> Then finally we should be able to add "nand-ecc-algo" property support properly.

I agree with patch 1 and 3, but I think we should create an
of_get_nand_ecc_algo() helper right now instead of relying on the
ecc->mode value to set ecc->algo (I'm referring to patch 2).

int of_get_nand_ecc_algo(struct device_node *np)
{
	const char *pm;
	int err, i;

	err = of_property_read_string(np, "nand-ecc-algo", &pm);
	if (err < 0) {
		if (!of_property_read_string(np, "nand-ecc-mode", &pm)){
			if (!strcasecmp(pm, "soft"))
				err = NAND_ECC_HAMMING;
			else if (!strcasecmp(pm, "soft_bch"))
				err = NAND_ECC_BCH;
		}

		return err;
	}

	for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++)
		if (!strcasecmp(pm, nand_ecc_algos[i]))
			return i;

	return -EINVAL;
}
EXPORT_SYMBOL_GPL(of_get_nand_ecc_algo);

This way you'll be able to completely drop NAND_ECC_SOFT_BCH value when
all drivers have been patched.
BTW, even if we deprecate the DT property, we still have to support the
old format, so this extra "nand-ecc-mode" parsing will remain anyway.

And has already said ealier, I think we should also move the ecc layout
info somewhere else, and only leave ECC_NONE, ECC_HW and ECC_SOFT in
nand_ecc_modes_t enum. I'm not asking you to do that though, just
clarifying the separation I'd like to see ;).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com