Messages in this thread | | | Date | Fri, 22 Aug 2008 14:31:43 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 2.6.27-rc3] led: driver for LEDs on PCEngines ALIX.2 and ALIX.3 boards |
| |
On Sat, 23 Aug 2008 02:08:15 +0500 Constantin Baranov <const@const.mimas.ru> wrote:
> > static int alix_led_resume(struct platform_device *dev) > > { > > int i; > > + > > for (i = 0; i < ARRAY_SIZE(alix_leds); i++) > > led_classdev_resume(&alix_leds[i].cdev); > > return 0; > > @@ -92,7 +95,7 @@ static int __init alix_led_probe(struct > > ret = led_classdev_register(&pdev->dev, &alix_leds[i].cdev); > > > > if (ret < 0) { > > - for (i = i - 2; i >= 0; i--) > > + while (--i >= 0) > > led_classdev_unregister(&alix_leds[i].cdev); > > } > > At the first iteration this while-loop will attempt to unregister > device that has failed and thus is not registered. > My for-loop starts from device immediately before failed one.
ug, OK, the complex expression in that for-loop is to blame.
For maintinability we should aim for code which is as simple and as straightfroward as possible and which adheres to oft-used and well-understood kernel idioms.
For example, this?
static int __init alix_led_probe(struct platform_device *pdev) { int i; int ret;
for (i = 0; i < ARRAY_SIZE(alix_leds); i++) { ret = led_classdev_register(&pdev->dev, &alix_leds[i].cdev); if (ret < 0) goto fail; } return 0;
fail: while (--i >= 0) led_classdev_unregister(&alix_leds[i].cdev); return ret; }
(note there's no longer a need for a fake initalisation of `ret')
> Note that probe routine originates from leds-ams-delta.c.
That's what you get for copying stuff :(
| |