Re: [V2] mtd: spinand: Add support for Etron EM73D044VCx

From: Bert Vermeulen <bert@biot.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: linux-mtd@lists.infradead.org,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Patrice Chotard <patrice.chotard@foss.st.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Christophe Kerello <christophe.kerello@foss.st.com>,
	Mark Brown <broonie@kernel.org>,
	Alexander Lobakin <alobakin@pm.me>
Subject: Re: [V2] mtd: spinand: Add support for Etron EM73D044VCx
Date: Sat, 7 Aug 2021 09:31:55 +0200	[thread overview]
Message-ID: <ba6637f8-6ed0-262e-0229-1b449334cdd2@biot.com> (raw)
In-Reply-To: <20210806213410.7b1f74bd@xps13>

On 8/6/21 9:34 PM, Miquel Raynal wrote:

Hi Miquel,

Thanks for reviewing. I'll send in a v2 with fixes. However...

> Bert Vermeulen <bert@biot.com> wrote on Thu, 29 Jul 2021 17:18:23 +0200:
>> +static int etron_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +					struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 72;
>> +	oobregion->length = 56;
> 
> This looks highly dependent on a single chip, could you make these
> values (at least the offset) dependent on the MTD device?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int etron_ooblayout_free(struct mtd_info *mtd, int section,
>> +			   struct mtd_oob_region *oobregion)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = 1;
>> +	oobregion->length = 71;
> 
> Same here.

I was rather hoping for some guidance on the _ecc and _free
offset/length numbers. There's not much info around on what's best
practice here, and I haven't played with NAND chips enough to actually
test the in-kernel ECC stuff.

The _free offset of 1 is for the bad block byte, which I saw in another
NAND chip driver, but I don't remember where I got the other numbers
from, and really need somebody to double-check that.

thanks,

-- 
Bert Vermeulen
bert@biot.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/