[PATCH] Random fixes for the ixp4xx mapping driver

[PATCH] Random fixes for the ixp4xx mapping driver

[Alexey Zaytsev]

Hi all.

I've made a few fixes to the driver, including:

- Fix build with partitioning disabled.
- Change the error handling path in a cleaner way.
- Some coding style.


Hope gmail won't mess with it.

--- linux-2.6.16.1.orig/drivers/mtd/maps/ixp4xx.c       2006-04-19
21:03:06.000000000 +0400
+++ linux-2.6.16.1/drivers/mtd/maps/ixp4xx.c    2006-04-19
22:13:37.000000000 +0400
@@ -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

  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
               map_destroy(info->mtd);
       }
       if (info->map.virt)
@@ -187,7 +191,7 @@
 {
       struct flash_platform_data *plat = dev->dev.platform_data;
       struct ixp4xx_flash_info *info;
-       int err = -1;
+       int err = 0;

       if (!plat)
               return -ENODEV;
@@ -201,7 +205,7 @@
       info = kmalloc(sizeof(struct ixp4xx_flash_info), GFP_KERNEL);
       if(!info) {
               err = -ENOMEM;
-               goto Error;
+               goto out;
       }
       memzero(info, sizeof(struct ixp4xx_flash_info));

@@ -221,9 +225,9 @@
        */
       info->map.bankwidth = 2;
       info->map.name = dev->dev.bus_id;
-       info->map.read = ixp4xx_read16,
-       info->map.write = ixp4xx_probe_write16,
-       info->map.copy_from = ixp4xx_copy_from,
+       info->map.read = ixp4xx_read16;
+       info->map.write = ixp4xx_probe_write16;
+       info->map.copy_from = ixp4xx_copy_from;

       info->res = request_mem_region(dev->resource->start,
                       dev->resource->end - dev->resource->start + 1,
@@ -231,7 +235,7 @@
       if (!info->res) {
               printk(KERN_ERR "IXP4XXFlash: Could not reserve memory
region\n");
               err = -ENOMEM;
-               goto Error;
+               goto out;
       }

       info->map.virt = ioremap(dev->resource->start,
@@ -239,35 +243,36 @@
       if (!info->map.virt) {
               printk(KERN_ERR "IXP4XXFlash: Failed to ioremap region\n");
               err = -EIO;
-               goto Error;
+               goto out;
       }

       info->mtd = do_map_probe(plat->map_name, &info->map);
       if (!info->mtd) {
               printk(KERN_ERR "IXP4XXFlash: map_probe failed\n");
               err = -ENXIO;
-               goto Error;
+               goto out;
       }
       info->mtd->owner = THIS_MODULE;

       /* Use the fast version */
-       info->map.write = ixp4xx_write16,
+       info->map.write = ixp4xx_write16;

+#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

+out:
       if (err)
-               goto Error;
-
+               ixp4xx_flash_remove(dev);
+
       return 0;

-Error:
-       ixp4xx_flash_remove(dev);
-       return err;
 }

  static struct platform_driver ixp4xx_flash_driver = {

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Vitaly Wool]

Alexey,

Alexey Zaytsev wrote:

>Hi all.
>
>I've made a few fixes to the driver, including:
>
>- Fix build with partitioning disabled.
>- Change the error handling path in a cleaner way.
>- Some coding style.
>  
>
can you please resend your patches with tabs not spaces?

Thx,

Vitaly

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Alexey Zaytsev]

Both patches can be found here:

http://80.253.205.252/stuff/mtd-patches/

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Jörn Engel]

On Wed, 19 April 2006 23:04:25 +0400, Alexey Zaytsev wrote:
> 
> I've made a few fixes to the driver, including:
> 
> - Fix build with partitioning disabled.
> - Change the error handling path in a cleaner way.
> - Some coding style.
> 
> 
> Hope gmail won't mess with it.
> 
> --- linux-2.6.16.1.orig/drivers/mtd/maps/ixp4xx.c       2006-04-19
> 21:03:06.000000000 +0400
> +++ linux-2.6.16.1/drivers/mtd/maps/ixp4xx.c    2006-04-19
> 22:13:37.000000000 +0400
> @@ -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.

>   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.

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Alexey Zaytsev]

On 4/20/06, Jörn Engel <joern@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.

>
> >   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.

> Jörn
>
> --
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein
>

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Jörn Engel]

On Thu, 20 April 2006 16:58:39 +0400, Alexey Zaytsev wrote:
> On 4/20/06, Jörn Engel <joern@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.

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. ;)

Jörn

-- 
/* Keep these two variables together */
int bar;

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Alexey Zaytsev]

On 4/20/06, Jörn Engel <joern@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@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;
>

Re: [PATCH] Random fixes for the ixp4xx mapping driver

[Jörn Engel]

On Thu, 20 April 2006 17:47:35 +0400, Alexey Zaytsev wrote:
> >
> > Take a look at the U-Boot source if you don't believe me.
> Ok-ok, agree here.

:)

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

Excellent!

I have a tiny amount of time this weekend, so you can send patches to
me and I'll try to integrate them to git.  "Try" because things are
still quite new and the workflow isn't exactly established yet.

Jörn

-- 
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens