| Message ID | 1413045778-5690-2-git-send-email-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On Sat, Oct 11, 2014 at 06:42:50PM +0200, Marek Vasut wrote: > The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the > _SYS is redundant. What makes that the correct name? The symbol is not documented anywhere, and while nothing currently tests for the SYS version, nothing currently sets the non-SYS version. What is SYS redundant with? Is this meant to be a user config knob or something that is fixed for a given board? -Scott
On Thursday, November 27, 2014 at 03:03:50 AM, Scott Wood wrote: > On Sat, Oct 11, 2014 at 06:42:50PM +0200, Marek Vasut wrote: > > The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the > > _SYS is redundant. > > What makes that the correct name? The symbol is not documented anywhere, > and while nothing currently tests for the SYS version, nothing currently > sets the non-SYS version. > > What is SYS redundant with? > > Is this meant to be a user config knob or something that is fixed for a > given board? u-boot$ git grep CONFIG_SYS_S3C2410_NAND_HWECC include/configs/VCMA9.h:#define CONFIG_SYS_S3C2410_NAND_HWECC include/configs/smdk2410.h:#define CONFIG_SYS_S3C2410_NAND_HWECC u-boot$ git grep CONFIG_S3C2410_NAND_HWECC drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC The driver checks the version without _SYS. This is a clear bugfix, so please apply. Best regards, Marek Vasut
On Sat, 2015-05-02 at 02:46 +0200, Marek Vasut wrote: > On Thursday, November 27, 2014 at 03:03:50 AM, Scott Wood wrote: > > On Sat, Oct 11, 2014 at 06:42:50PM +0200, Marek Vasut wrote: > > > The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the > > > _SYS is redundant. > > > > What makes that the correct name? The symbol is not documented anywhere, > > and while nothing currently tests for the SYS version, nothing currently > > sets the non-SYS version. > > > > What is SYS redundant with? > > > > Is this meant to be a user config knob or something that is fixed for a > > given board? > > u-boot$ git grep CONFIG_SYS_S3C2410_NAND_HWECC > include/configs/VCMA9.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > include/configs/smdk2410.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > > u-boot$ git grep CONFIG_S3C2410_NAND_HWECC > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > > The driver checks the version without _SYS. This is a clear bugfix, > so please apply. There's a clear bug. I asked questions to determine whether this is the proper fix (and encourage a better changelog), and you still haven't answered. It would also be nice if you'd document the symbol while you're at it. In any case, I don't know why you're asking me to apply a patch with an "arm:" subject line, which only touches ARM board config files, instead of asking an ARM custodian. -Scott
On Saturday, May 02, 2015 at 03:11:41 AM, Scott Wood wrote: > On Sat, 2015-05-02 at 02:46 +0200, Marek Vasut wrote: > > On Thursday, November 27, 2014 at 03:03:50 AM, Scott Wood wrote: > > > On Sat, Oct 11, 2014 at 06:42:50PM +0200, Marek Vasut wrote: > > > > The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the > > > > _SYS is redundant. > > > > > > What makes that the correct name? The symbol is not documented > > > anywhere, and while nothing currently tests for the SYS version, > > > nothing currently sets the non-SYS version. > > > > > > What is SYS redundant with? > > > > > > Is this meant to be a user config knob or something that is fixed for a > > > given board? > > > > u-boot$ git grep CONFIG_SYS_S3C2410_NAND_HWECC > > include/configs/VCMA9.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > > include/configs/smdk2410.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > > > > u-boot$ git grep CONFIG_S3C2410_NAND_HWECC > > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > > > > The driver checks the version without _SYS. This is a clear bugfix, > > so please apply. > > There's a clear bug. I asked questions to determine whether this is the > proper fix (and encourage a better changelog), and you still haven't > answered. It would also be nice if you'd document the symbol while > you're at it. Adding new stuff (like documentation) is now mandatory part of bugfix ? I'd expect bugfixes to be taken in to actually fix bugs and not kept out of the tree because the submitter didn't also do another random chore. Also, I do not know what else should I say besides that the symbol name is incorrect and you can clearly see it. If that is not enough ... > In any case, I don't know why you're asking me to apply a patch with an > "arm:" subject line, which only touches ARM board config files, instead > of asking an ARM custodian. You were the only one who responded, but I'm fine if Tom or whoever picks this. Best regards, Marek Vasut
On Sat, 2015-05-02 at 03:18 +0200, Marek Vasut wrote: > On Saturday, May 02, 2015 at 03:11:41 AM, Scott Wood wrote: > > On Sat, 2015-05-02 at 02:46 +0200, Marek Vasut wrote: > > > On Thursday, November 27, 2014 at 03:03:50 AM, Scott Wood wrote: > > > > On Sat, Oct 11, 2014 at 06:42:50PM +0200, Marek Vasut wrote: > > > > > The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the > > > > > _SYS is redundant. > > > > > > > > What makes that the correct name? The symbol is not documented > > > > anywhere, and while nothing currently tests for the SYS version, > > > > nothing currently sets the non-SYS version. > > > > > > > > What is SYS redundant with? > > > > > > > > Is this meant to be a user config knob or something that is fixed for a > > > > given board? > > > > > > u-boot$ git grep CONFIG_SYS_S3C2410_NAND_HWECC > > > include/configs/VCMA9.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > > > include/configs/smdk2410.h:#define CONFIG_SYS_S3C2410_NAND_HWECC > > > > > > u-boot$ git grep CONFIG_S3C2410_NAND_HWECC > > > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > > > drivers/mtd/nand/s3c2410_nand.c:#ifdef CONFIG_S3C2410_NAND_HWECC > > > > > > The driver checks the version without _SYS. This is a clear bugfix, > > > so please apply. > > > > There's a clear bug. I asked questions to determine whether this is the > > proper fix (and encourage a better changelog), and you still haven't > > answered. It would also be nice if you'd document the symbol while > > you're at it. > > Adding new stuff (like documentation) is now mandatory part of bugfix ? I didn't say mandatory. I said "would be nice". > I'd expect bugfixes to be taken in to actually fix bugs and not kept out > of the tree because the submitter didn't also do another random chore. In this case the lack of documentation feels related to the cause of the bug -- no authoritative place to say what the proper name is. I don't see it as "another random chore". > Also, I do not know what else should I say besides that the symbol name > is incorrect and you can clearly see it. If that is not enough ... The names don't match. That's a bug. I was wondering whether the right thing was to remove SYS in one place or add it in the other (or have we given up on maintaining the distinction?). I didn't expect asking that question to be a huge burden on the patch's progress. It was also bundled up in a patchset with a bunch of non-bugfixes, so it didn't seem like you were asking for urgent handling of this one. > > In any case, I don't know why you're asking me to apply a patch with an > > "arm:" subject line, which only touches ARM board config files, instead > > of asking an ARM custodian. > > You were the only one who responded, And look at what I get for doing so. :-) > but I'm fine if Tom or whoever picks this. While I would have liked an answer to the questions I asked, it's not worth arguing over, so: Acked-by: Scott Wood <scottwood@freescale.com> -Scott
diff --git a/include/configs/VCMA9.h b/include/configs/VCMA9.h index d40185e..a2ce7c5 100644 --- a/include/configs/VCMA9.h +++ b/include/configs/VCMA9.h @@ -201,7 +201,7 @@ /* NAND configuration */ #ifdef CONFIG_CMD_NAND #define CONFIG_NAND_S3C2410 -#define CONFIG_SYS_S3C2410_NAND_HWECC +#define CONFIG_S3C2410_NAND_HWECC #define CONFIG_SYS_MAX_NAND_DEVICE 1 #define CONFIG_SYS_NAND_BASE 0x4E000000 #define CONFIG_S3C24XX_CUSTOM_NAND_TIMING diff --git a/include/configs/smdk2410.h b/include/configs/smdk2410.h index d4ae19f..0d0da28 100644 --- a/include/configs/smdk2410.h +++ b/include/configs/smdk2410.h @@ -172,7 +172,7 @@ */ #ifdef CONFIG_CMD_NAND #define CONFIG_NAND_S3C2410 -#define CONFIG_SYS_S3C2410_NAND_HWECC +#define CONFIG_S3C2410_NAND_HWECC #define CONFIG_SYS_MAX_NAND_DEVICE 1 #define CONFIG_SYS_NAND_BASE 0x4E000000 #endif
The correct name of this symbol is CONFIG_S3C2410_NAND_HWECC , the _SYS is redundant. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Minkyu Kang <mk7.kang@samsung.com> Cc: Scott Wood <scottwood@freescale.com> Cc: Vladimir Zapolskiy <vz@mleia.com> --- include/configs/VCMA9.h | 2 +- include/configs/smdk2410.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)