Re: [PATCH v2] mtd: nand: pxa3xx: Fix registered MTD name

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

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