Hi Boris,

Boris Brezillon [off-list ref] wrote on Sat, 12 Oct
2019 11:28:24 +0200:

On Thu, 19 Sep 2019 21:31:06 +0200
Miquel Raynal [off-list ref] wrote:

quoted

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 00a261284aad..ad0b892c2523 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c

@@ -4835,7 +4835,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 
 static const char * const nand_ecc_modes[] = {
 	[NAND_ECC_NONE]		= "none",
-	[NAND_ECC_SOFT]		= "soft",
+	[NAND_SOFT_ECC_ENGINE]		= "soft",  

Not sure why this one is changed. This string array still describes ECC
modes, not ECC engine types.

quoted

 	[NAND_ECC_HW]		= "hw",
 	[NAND_ECC_HW_SYNDROME]	= "hw_syndrome",
 	[NAND_ECC_HW_OOB_FIRST]	= "hw_oob_first",

@@ -4863,21 +4863,44 @@ static int of_get_nand_ecc_mode(struct device_node *np)
 	if (err < 0)
 		return err;
 
-	for (i = NAND_ECC_NONE; i < ARRAY_SIZE(nand_ecc_modes); i++)
-		if (!strcasecmp(pm, nand_ecc_modes[i]))
+	for (i = NAND_NO_ECC_ENGINE;
+	     i < ARRAY_SIZE(nand_ecc_engine_providers); i++)
+		if (!strcasecmp(pm, nand_ecc_engine_providers[i]))
 			return i;  

Hm, you still need to support the old bindings (I wonder how that can
work). What should be done instead is have a conversion table that turns
an ecc_mode string into a engine_type+placement pair, so you don't have
to update the DT bindings (though we might want to expose new props for
the new model, like ecc-placement and ecc-engine).

As you know this series is already quite big :p. My point here is to
clarify a bit but now rework the DT properties which are things that
you can never get entirely rid of. This is exactly why I kept the
string properties as before: not changing anything in the DT
representation. Anyone with the time and desire to do so is welcome,
but I'm not willing to do it in this series :)

I'm addressing most of your comments (mainly enum/name changes) but the
string definitions will remain the same, even if they are not entirely
accurate. With these bits kept intact, the below logic works, I know
it is not clean, and deserves more cleaning, but this is a distinct
work :)

quoted

 
+	for (i = NAND_ECC_SYNDROME_OOB_PLACEMENT;
+	     i < ARRAY_SIZE(nand_ecc_engine_oob_placement); i++)
+		if (!strcasecmp(pm, nand_ecc_engine_oob_placement[i]))
+			return NAND_HW_ECC_ENGINE;
+  

I also don't understand how this one works, placement does not give any
clue on the type of ECC engine (at least it shouldn't).

quoted

 	/*
 	 * For backward compatibility we support few obsoleted values that don't
-	 * have their mappings into the nand_ecc_mode enum anymore (they were
-	 * merged with other enums).
+	 * have their mappings into the nand_ecc_engine_providers enum anymore
+	 * (they were merged with other enums).
 	 */
 	if (!strcasecmp(pm, "soft_bch"))
-		return NAND_ECC_SOFT;
+		return NAND_SOFT_ECC_ENGINE;
 
 	return -ENODEV;
 }  

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel