On Fri, Sep 14, 2012 at 02:24:48PM -0500, Scott Wood wrote:
On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs. Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncalves@inov.pt
[snip]
+/*
- Hardware specific access to control-lines function
- */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl) +{
- struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
- struct nand_chip *this = mtd->priv;
- if (ctrl & NAND_CTRL_CHANGE) {
if (ctrl & NAND_CLE)this->IO_ADDR_W = &nand->nfcmmd;else if (ctrl & NAND_ALE)this->IO_ADDR_W = &nand->nfaddr;elsethis->IO_ADDR_W = &nand->nfdata;Why don't you use local variable here?
Because then you'll access the wrong data when the function is called again without NAND_CTRL_CHANGE. This is a common way of writing the hwcontrol function, though the way ndfc.c does it is simpler (you can use a local variable if you ignore NAND_CTRL_CHANGE altogether).
if (ctrl & NAND_NCE)s3c_nand_select_chip(mtd, *(int *)this->priv);Uh, how's this *(int *) supposed to work?
Usually driver-private data is a struct; apparently in this driver it's an int.
Shall we take both of these comments as requests to do things differently (struct like everyone else does, simpiler code like ndfc.c does) unless there's good reason to not change?