* [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name
@ 2013-10-18 18:14 Ezequiel Garcia
2013-10-18 19:15 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2013-10-18 18:14 UTC (permalink / raw)
To: linux-mtd
Cc: Thomas Petazzoni, Lior Amsalem, Daniel Mack, Ezequiel Garcia,
Gregory Clement, Brian Norris, David Woodhouse, Willy Tarreau
In a recent commit:
commit f455578dd961087a5cf94730d9f6489bb1d355f0
Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Date: Mon Aug 12 14:14:53 2013 -0300
mtd: nand: pxa3xx: Remove hardcoded mtd name
There's no advantage in using a hardcoded name for the mtd device.
Instead use the provided by the platform_device.
The MTD name was changed to use the one provided by the platform_device.
However, this can be problematic as some users want to set partitions
using the kernel parameter 'mtdpart', where the name is needed.
Therefore, to avoid regressions in users relying in 'mtdpart' we revert
the change and use the previous one 'pxa3xx_nand-0'.
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
v1->v2:
* Use exactly the same name as before, and avoid regressions
instead of using any seemingly better or cleaner one.
drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index aa75adf..4d53e8e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -46,6 +46,8 @@
*/
#define INIT_BUFFER_SIZE 256
+#define DRIVER_NAME "pxa3xx_nand-0"
+
/* registers and bit definitions */
#define NDCR (0x00) /* Control register */
#define NDTR0CS0 (0x04) /* Timing Parameter 0 for CS0 */
@@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
for (cs = 0; cs < pdata->num_cs; cs++) {
struct mtd_info *mtd = info->host[cs]->mtd;
- mtd->name = pdev->name;
+ mtd->name = DRIVER_NAME;
info->cs = cs;
ret = pxa3xx_nand_scan(mtd);
if (ret) {
@@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
static struct platform_driver pxa3xx_nand_driver = {
.driver = {
- .name = "pxa3xx-nand",
+ .name = DRIVER_NAME,
.of_match_table = pxa3xx_nand_dt_ids,
},
.probe = pxa3xx_nand_probe,
--
1.8.1.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name
2013-10-18 18:14 [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name Ezequiel Garcia
@ 2013-10-18 19:15 ` Brian Norris
2013-10-18 19:32 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2013-10-18 19:15 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Thomas Petazzoni, Lior Amsalem, Daniel Mack,
linux-mtd@lists.infradead.org, Gregory Clement, David Woodhouse,
Willy Tarreau
On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> In a recent commit:
>
> commit f455578dd961087a5cf94730d9f6489bb1d355f0
> Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Date: Mon Aug 12 14:14:53 2013 -0300
>
> mtd: nand: pxa3xx: Remove hardcoded mtd name
>
> There's no advantage in using a hardcoded name for the mtd device.
> Instead use the provided by the platform_device.
>
> The MTD name was changed to use the one provided by the platform_device.
> However, this can be problematic as some users want to set partitions
> using the kernel parameter 'mtdpart', where the name is needed.
>
> Therefore, to avoid regressions in users relying in 'mtdpart' we revert
> the change and use the previous one 'pxa3xx_nand-0'.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Should this be marked for -stable (or squeezed into 3.12-rc now), so
we don't leave the regression in 3.12?
> ---
> v1->v2:
> * Use exactly the same name as before, and avoid regressions
> instead of using any seemingly better or cleaner one.
>
> drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index aa75adf..4d53e8e 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -46,6 +46,8 @@
> */
> #define INIT_BUFFER_SIZE 256
>
> +#define DRIVER_NAME "pxa3xx_nand-0"
> +
> /* registers and bit definitions */
> #define NDCR (0x00) /* Control register */
> #define NDTR0CS0 (0x04) /* Timing Parameter 0 for CS0 */
> @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> for (cs = 0; cs < pdata->num_cs; cs++) {
> struct mtd_info *mtd = info->host[cs]->mtd;
>
> - mtd->name = pdev->name;
> + mtd->name = DRIVER_NAME;
> info->cs = cs;
> ret = pxa3xx_nand_scan(mtd);
> if (ret) {
> @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
>
> static struct platform_driver pxa3xx_nand_driver = {
> .driver = {
> - .name = "pxa3xx-nand",
> + .name = DRIVER_NAME,
> .of_match_table = pxa3xx_nand_dt_ids,
> },
> .probe = pxa3xx_nand_probe,
For mtdparts' sake, do we need both the driver name and the MTD name
to be the same? Or could we just stick the "pxa3xx_nand-0" directly
in mtd->name only, where it's needed for compatibility, and allow the
driver name to be cleaner? Or is that a silly idea?
Brian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name
2013-10-18 19:15 ` Brian Norris
@ 2013-10-18 19:32 ` Ezequiel Garcia
2013-10-19 1:37 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2013-10-18 19:32 UTC (permalink / raw)
To: Brian Norris
Cc: Thomas Petazzoni, Lior Amsalem, Daniel Mack,
linux-mtd@lists.infradead.org, Gregory Clement, David Woodhouse,
Willy Tarreau
On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > In a recent commit:
> >
> > commit f455578dd961087a5cf94730d9f6489bb1d355f0
> > Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > Date: Mon Aug 12 14:14:53 2013 -0300
> >
> > mtd: nand: pxa3xx: Remove hardcoded mtd name
> >
> > There's no advantage in using a hardcoded name for the mtd device.
> > Instead use the provided by the platform_device.
> >
> > The MTD name was changed to use the one provided by the platform_device.
> > However, this can be problematic as some users want to set partitions
> > using the kernel parameter 'mtdpart', where the name is needed.
> >
> > Therefore, to avoid regressions in users relying in 'mtdpart' we revert
> > the change and use the previous one 'pxa3xx_nand-0'.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>
> Should this be marked for -stable (or squeezed into 3.12-rc now), so
> we don't leave the regression in 3.12?
>
Yes, I think that if we're still on time to fix v3.12 it would be great.
(Sorry for taking care of my own crap so late!).
> > ---
> > v1->v2:
> > * Use exactly the same name as before, and avoid regressions
> > instead of using any seemingly better or cleaner one.
> >
> > drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index aa75adf..4d53e8e 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -46,6 +46,8 @@
> > */
> > #define INIT_BUFFER_SIZE 256
> >
> > +#define DRIVER_NAME "pxa3xx_nand-0"
> > +
> > /* registers and bit definitions */
> > #define NDCR (0x00) /* Control register */
> > #define NDTR0CS0 (0x04) /* Timing Parameter 0 for CS0 */
> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> > for (cs = 0; cs < pdata->num_cs; cs++) {
> > struct mtd_info *mtd = info->host[cs]->mtd;
> >
> > - mtd->name = pdev->name;
> > + mtd->name = DRIVER_NAME;
> > info->cs = cs;
> > ret = pxa3xx_nand_scan(mtd);
> > if (ret) {
> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
> >
> > static struct platform_driver pxa3xx_nand_driver = {
> > .driver = {
> > - .name = "pxa3xx-nand",
> > + .name = DRIVER_NAME,
> > .of_match_table = pxa3xx_nand_dt_ids,
> > },
> > .probe = pxa3xx_nand_probe,
>
> For mtdparts' sake, do we need both the driver name and the MTD name
> to be the same? Or could we just stick the "pxa3xx_nand-0" directly
> in mtd->name only, where it's needed for compatibility, and allow the
> driver name to be cleaner? Or is that a silly idea?
>
Hm... What's the impact of the driver's name?
I know it's just a small bloat, but still I don't like to see two stupid
strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
that's why I chose a common macro :-)
What do you think?
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name
2013-10-18 19:32 ` Ezequiel Garcia
@ 2013-10-19 1:37 ` Brian Norris
2013-10-19 13:02 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2013-10-19 1:37 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Thomas Petazzoni, Lior Amsalem, Daniel Mack,
linux-mtd@lists.infradead.org, Gregory Clement, David Woodhouse,
Willy Tarreau
On Fri, Oct 18, 2013 at 12:32 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
>> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
>> <ezequiel.garcia@free-electrons.com> wrote:
[...]
>> > drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
>> > 1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> > index aa75adf..4d53e8e 100644
>> > --- a/drivers/mtd/nand/pxa3xx_nand.c
>> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> > @@ -46,6 +46,8 @@
>> > */
>> > #define INIT_BUFFER_SIZE 256
>> >
>> > +#define DRIVER_NAME "pxa3xx_nand-0"
>> > +
>> > /* registers and bit definitions */
>> > #define NDCR (0x00) /* Control register */
>> > #define NDTR0CS0 (0x04) /* Timing Parameter 0 for CS0 */
>> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
>> > for (cs = 0; cs < pdata->num_cs; cs++) {
>> > struct mtd_info *mtd = info->host[cs]->mtd;
>> >
>> > - mtd->name = pdev->name;
>> > + mtd->name = DRIVER_NAME;
>> > info->cs = cs;
>> > ret = pxa3xx_nand_scan(mtd);
>> > if (ret) {
>> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
>> >
>> > static struct platform_driver pxa3xx_nand_driver = {
>> > .driver = {
>> > - .name = "pxa3xx-nand",
>> > + .name = DRIVER_NAME,
>> > .of_match_table = pxa3xx_nand_dt_ids,
>> > },
>> > .probe = pxa3xx_nand_probe,
>>
>> For mtdparts' sake, do we need both the driver name and the MTD name
>> to be the same? Or could we just stick the "pxa3xx_nand-0" directly
>> in mtd->name only, where it's needed for compatibility, and allow the
>> driver name to be cleaner? Or is that a silly idea?
>>
>
> Hm... What's the impact of the driver's name?
I meant to ask you the exact same question, because I'm lazy :)
I'm not sure exactly. I thought it affected the device name (which is
important to users; it's exposed in sysfs in potentially important
ways), but that actually comes from the platform-device allocation or
from device tree. The most visible way I know of is that it prefixes
the messages seen via the dev_{printk,err,info,warn,..}() print
functions. This could be annoying, but not horrible.
The driver name does show up in /sys/bus/platform/drivers, but I'm not
sure if that's ever expected to be stable.
> I know it's just a small bloat, but still I don't like to see two stupid
> strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
> that's why I chose a common macro :-)
That's fine.
> What do you think?
I'm fine with just making both use DRIVER_NAME. So I'd take this patch as-is.
One possible consequence: it makes things a little more fragile if
people want to change one or both in the future, not noticing that
while the driver name may (?) be changed harmlessly, it can cause
mtdparts=... breakage.
Brian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name
2013-10-19 1:37 ` Brian Norris
@ 2013-10-19 13:02 ` Ezequiel Garcia
0 siblings, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2013-10-19 13:02 UTC (permalink / raw)
To: Brian Norris
Cc: Thomas Petazzoni, Lior Amsalem, Daniel Mack,
linux-mtd@lists.infradead.org, Gregory Clement, David Woodhouse,
Willy Tarreau
On Fri, Oct 18, 2013 at 06:37:06PM -0700, Brian Norris wrote:
> On Fri, Oct 18, 2013 at 12:32 PM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > On Fri, Oct 18, 2013 at 12:15:39PM -0700, Brian Norris wrote:
> >> On Fri, Oct 18, 2013 at 11:14 AM, Ezequiel Garcia
> >> <ezequiel.garcia@free-electrons.com> wrote:
> [...]
> >> > drivers/mtd/nand/pxa3xx_nand.c | 6 ++++--
> >> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> >> > index aa75adf..4d53e8e 100644
> >> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> >> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> >> > @@ -46,6 +46,8 @@
> >> > */
> >> > #define INIT_BUFFER_SIZE 256
> >> >
> >> > +#define DRIVER_NAME "pxa3xx_nand-0"
> >> > +
> >> > /* registers and bit definitions */
> >> > #define NDCR (0x00) /* Control register */
> >> > #define NDTR0CS0 (0x04) /* Timing Parameter 0 for CS0 */
> >> > @@ -1339,7 +1341,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
> >> > for (cs = 0; cs < pdata->num_cs; cs++) {
> >> > struct mtd_info *mtd = info->host[cs]->mtd;
> >> >
> >> > - mtd->name = pdev->name;
> >> > + mtd->name = DRIVER_NAME;
> >> > info->cs = cs;
> >> > ret = pxa3xx_nand_scan(mtd);
> >> > if (ret) {
> >> > @@ -1425,7 +1427,7 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
> >> >
> >> > static struct platform_driver pxa3xx_nand_driver = {
> >> > .driver = {
> >> > - .name = "pxa3xx-nand",
> >> > + .name = DRIVER_NAME,
> >> > .of_match_table = pxa3xx_nand_dt_ids,
> >> > },
> >> > .probe = pxa3xx_nand_probe,
> >>
> >> For mtdparts' sake, do we need both the driver name and the MTD name
> >> to be the same? Or could we just stick the "pxa3xx_nand-0" directly
> >> in mtd->name only, where it's needed for compatibility, and allow the
> >> driver name to be cleaner? Or is that a silly idea?
> >>
> >
> > Hm... What's the impact of the driver's name?
>
> I meant to ask you the exact same question, because I'm lazy :)
>
I guess that makes the two of us lazy :)
> I'm not sure exactly. I thought it affected the device name (which is
> important to users; it's exposed in sysfs in potentially important
> ways), but that actually comes from the platform-device allocation or
> from device tree. The most visible way I know of is that it prefixes
> the messages seen via the dev_{printk,err,info,warn,..}() print
> functions. This could be annoying, but not horrible.
>
Good point. I'm not sure my OCD will resist seeing "pxa3xx_nand-0: blabla"
printed.
> The driver name does show up in /sys/bus/platform/drivers, but I'm not
> sure if that's ever expected to be stable.
>
> > I know it's just a small bloat, but still I don't like to see two stupid
> > strings "pxa3xx-nand" and "pxa3xx_nand-0" bloating our kernels, so
> > that's why I chose a common macro :-)
>
> That's fine.
>
> > What do you think?
>
> I'm fine with just making both use DRIVER_NAME. So I'd take this patch as-is.
>
> One possible consequence: it makes things a little more fragile if
> people want to change one or both in the future, not noticing that
> while the driver name may (?) be changed harmlessly, it can cause
> mtdparts=... breakage.
>
Hm.. another good point. Let me send a v3.
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-19 13:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-18 18:14 [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name Ezequiel Garcia
2013-10-18 19:15 ` Brian Norris
2013-10-18 19:32 ` Ezequiel Garcia
2013-10-19 1:37 ` Brian Norris
2013-10-19 13:02 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).