RE: [PATCH 2/4] mtd: devices: elm: Add support for ELM errorcorrection

[Philip, Avinash]

On Wed, Oct 03, 2012 at 20:40:46, Peter Meerwald wrote:
> 
> some minor nitpicks below
> 
> > + *
> > + * On completion of processing by elm module, error location status
> > + * register updated with correctable/uncorrectable error information.
> > + * In case of correctable errors, number of errors located from
> > + * elm location status register & read the these many positions from
> 
> should probably be: "... & read the positions from ..."?

Ok I will correct it.

> 
> > + * elm error location register.
> > + */
> > +static void elm_error_correction(struct elm_info *info,
> > +		struct elm_errorvec *err_vec)
> > +{
> > +	int i, j, errors = 0;
> > +	void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0;
> > +	elm_error_t *err_loc;
> > +	void *offset;
> > +	u32 reg_val;
> > +
> > +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> > +		/* check error reported */
> > +		if (err_vec[i].error_reported) {
> > +			offset = info->elm_base + ELM_LOCATION_STATUS +
> > +				i * ERROR_LOCATION_SIZE;
> > +			reg_val = elm_read_reg(offset);
> > +			/* check correctable error or not */
> > +			if (reg_val & ECC_CORRECTABLE_MASK) {
> > +				err_loc = err_loc_base +
> > +					i * ERROR_LOCATION_SIZE;
> > +				/* read count of correctable errors */
> > +				err_vec[i].error_count = reg_val &
> > +					ECC_NB_ERRORS_MASK;
> > +
> > +				/* update the error locations in error vector */
> > +				for (j = 0; j < err_vec[i].error_count; j++) {
> > +
> > +					reg_val = elm_read_reg(*err_loc + j);
> > +					err_vec[i].error_loc[j] = reg_val &
> > +						ECC_ERROR_LOCATION_MASK;
> > +				}
> > +
> > +				errors += err_vec[i].error_count;
> > +			} else {
> > +				err_vec[i].error_uncorrectable++;
> > +			}
> 
> extra braces above

As per coding style
This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

> 
> > +
> > +			/* clearing interrupts for processed error vectors */
> > +			elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i));
> > +
> > +			/* disable page mode */
> > +			elm_configure_page_mode(info, i, false);
> > +		}
> > +	}
> > +
> > +	return;
> > +}
> > +
> > +/**
> > + * elm_decode_bch_error_page - Locate error position
> > + * @info:	elm info
> 
> should be dev, not info

Ok I will correct it.

> 
> > + * @ecc_calc:	calculated ECC bytes from GPMC
> > + * @err_vec:	elm error vectors
> > + *
> > + * Called with one or greater reported and is vectors with error reported
> > + * is updated in err_vec[].error_reported
> > + */
> 
> I do not understand the statement "Called with one ..."

elm_decode_bch_error_page() API will called from nand driver if and only if
errors are present while reading page. Errors can be reported in one or
multiple error vectors.

I will reword it as.

Called with one or more error reported vectors & vectors with error reported
is updated in err_vec[].error_reported

> 
[snip]
> > +
> > +#define BCH8_ECC_OOB_BYTES		13
> > +/* RBL requires 14 byte even though BCH8 uses only 13 byte */
> 
> not sure what RBL is?

RBL stands for Rom boot Loader

> 
> > +#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
> > +#define BCH4_SIZE			7
> > +
> > +#define	BCH8_SYNDROME_SIZE		4	/* 13 bytes of ecc */
> > +#define	BCH4_SYNDROME_SIZE		2	/* 7 bytes of ecc */
> > +
> > +/**
> > + * struct elm_errorvec - error vector for elm
> > + * @error_reported:		set true for vectors error is reported
> > + *
> > + * @error_count:		number of correctable errors in the sector
> > + * @error_uncorrectable:	number of uncorrectable errors
> > + * @error_loc:			buffer for error location
> > + *
> > + */
> > +struct elm_errorvec {
> > +	bool error_reported;
> > +	int error_count;
> > +	int error_uncorrectable;
> > +	int error_loc[ERROR_VECTOR_MAX];
> > +};
> > +
> > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> > +		struct elm_errorvec *err_vec);
> > +struct device *elm_request(enum bch_ecc bch_type);
> > +#endif /* __ELM_H */
> > 
> 
> -- 
> 
> Peter Meerwald
> +43-664-2444418 (mobile)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/