[PATCH] Random fixes for the ixp4xx mapping driver

[Alexey Zaytsev]

On 4/20/06, Jörn Engel <joern at wohnheim.fh-wedel.de> wrote:
> On Thu, 20 April 2006 16:58:39 +0400, Alexey Zaytsev wrote:
> > On 4/20/06, Jörn Engel <joern at wohnheim.fh-wedel.de> wrote:
> > > > @@ -151,7 +151,9 @@
> > > >        struct resource *res;
> > > >  };
> > > >
> > > > -static const char *probes[] = { "RedBoot", "cmdlinepart", NULL };
> > > > +#ifdef CONFIG_MTD_PARTITIONS
> > > > +static const char *probes[] __initdata = { "RedBoot", "cmdlinepart", NULL };
> > > > +#endif
> > >
> > > All this #ifdef mess doesn't make me happy.  In this case, it saves 32
> > > bytes for people using this driver but not having
> > > CONFIG_MTD_PARTITIONS enabled.  That's not a very small gain for a
> > > very small group.  Just make it unconditional.
> >
> > The gain is not big, but the price is small too. The thing I'm not
> > sure about is __initdata.
> > Now I see that it may be needed after initialization too, not sure.
>
> The price of a single #ifdef may (and I stress may) be small.  But
> once the numbers go up, changes become virtually impossible.  The only
> chance to change #ifdef-happy code is by adding new code and new
> #ifdefs.  Result is a lot more bloat than just 32 bytes.
>
> Take a look at the U-Boot source if you don't believe me.
Ok-ok, agree here.
>
> End result: stong NACK from me.
>
> > >
> > > >   static int ixp4xx_flash_remove(struct platform_device *dev)
> > > >  {
> > > > @@ -164,7 +166,9 @@
> > > >                return 0;
> > > >
> > > >        if (info->mtd) {
> > > > +#ifdef CONFIG_MTD_PARTITIONS
> > > >                del_mtd_partitions(info->mtd);
> > > > +#endif
> > >
> > > In this case, del_mtd_partitions() should be defined to a noop if
> > > CONFIG_MTD_PARTITIONS is not defined.
> > >
> > > > +#ifdef CONFIG_MTD_PARTITIONS
> > > >        err = parse_mtd_partitions(info->mtd, probes, &info->partitions, 0);
> > > >        if (err > 0) {
> > > >                err = add_mtd_partitions(info->mtd, info->partitions, err);
> > > > -               if(err)
> > > > -                       printk(KERN_ERR "Could not parse partitions\n");
> > > > +
> > > >        }
> > > > +       if (err < 0)
> > > > +               printk(KERN_ERR "Could not parse partitions\n");
> > > > +#endif
> > >
> > > Same here.  parse_mtd_partitions() should be a noop, the rest can just
> > > stay.  Gcc should be smart enough to optimize it away.
> > >
> > > The rest looks fine.
> >
> > Why do you think nooping functions is better than having #ifdefs? I
> > agree that lots of
> > #ifdefs make code hard to follow, but if you noop a function, defined
> > in some other place,
> > someone reading the code may not notice it, and refer to the un-nooped function.
>
> In the header where del_mtd_partitions() and parse_mtd_partitions()
> are declared, there should be a single #ifdef:
>
> #ifdef CONFIG_MTD_PARTITIONS
>
> void del_mtd_partitions(...);
> int parse_mtd_partitions(...);
>
> #else
>
> static inline void del_mtd_partitions(...) {}
> static inline int parse_mtd_partitions(...) { return 0; }
>
> #endif
>
> For the driver you touched alone, three #ifdefs could be removed after
> this one is added.  Then take a look at all the other drivers and do
> the math. ;)

Ok, I'll add them to include/linux/mtd/partitions.h, and clean the
other drivers.

> Jörn
>
> --
> /* Keep these two variables together */
> int bar;
>