Re: [PATCH v2] mtd: spinand: Add support for AllianceMemory AS5F34G04SND

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Mario,

dev@kicherer.org wrote on Wed, 25 Jan 2023 13:13:13 +0100:

> Hello Miquel,
> 
> On 2023-01-23 11:33, Miquel Raynal wrote:
> >> +static int as5f34g04snd_ooblayout_ecc(struct mtd_info *mtd, int >> section,
> >> +				      struct mtd_oob_region *region)
> >> +{
> >> +	if (section > 3)
> >> +		return -ERANGE;
> >> +
> >> +	region->offset = 0x48;
> >> +	region->length = 0x38;  
> > 
> > Why would you have three sections if you have static offsets and
> > length? That does not look correct.  
> 
> Hm, I do not remember. I guess this is a leftover I forgot to remove.
> 
> >> +	/*
> >> +	 * It is unclear how many bytes are used for the bad block marker. >> We
> >> +	 * reserve one byte here.  
> > 
> > So far we reserved two bytes in Linux to mimic the raw NAND BBM.  
> 
> Ok, changed.
> 
> >> +static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
> >> +{
> >> +	switch (status & AM_STATUS_ECC_BITMASK) {
> >> +	case AM_STATUS_ECC_NONE_DETECTED:
> >> +		return 0;
> >> +
> >> +	case AM_STATUS_ECC_1_CORRECTED:
> >> +		return 1;
> >> +
> >> +	case AM_STATUS_ECC_MAX_CORRECTED:
> >> +		return 8;
> >> +
> >> +	case AM_STATUS_ECC_1_DETECTED:  
> > 
> > What does this mean "1 detected"?  
> 
> According to the manual, the chip can report that a bit error was detected
> but could not be corrected.

That does not make a lot of sense. Either you use a Hamming algorithm,
you will be able to correct 1 bit error and to detect 2 bit errors, or
you use another algorithm (BCH, usually) with a strength of X (X > 1)
and you'll be able to correct up to X errors and detect X+1 errors.

But only being able to detect a single bit flip without being able to
correct it is strange (even useless?). So I don't understand how it
should be used.

> 
> >> +static const struct spinand_manufacturer_ops >> alliancememory_spinand_manuf_ops = {
> >> +	.init = alliancememory_spinand_init,  
> > 
> > I don't think .init is mandatory, so if you do nothing in it, you
> > can just drop it.  
> 
> Done.
> 
> Thanks for the review!
> Mario


Thanks,
Miquèl

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