Re: [PATCH 2.6.27-rc3] led: driver for LEDs on PCEngines ALIX.2 and ALIX.3 boards

Andrew Morton wrote:
On Sat, 23 Aug 2008 02:08:15 +0500
Constantin Baranov <const@xxxxxxxxxxxxxx> 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 :(

Well, this variant looks better and correct and still works.
Should I post again patch or fix-patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages