Re: [PATCH] leds-lp3944: properly handle lp3944_configure fail in lp3944_probe

[Axel Lin]

2010/5/17 Antonio Ospite <ospite@xxxxxxxxxxxxxxxxx>:
> On Mon, 17 May 2010 09:34:02 +0800
> Axel Lin <axel.lin@xxxxxxxxx> wrote:
>
>> In current implementation, lp3944_probe return 0 even if lp3944_configure fail.
>> Therefore, led_classdev_unregister will be executed twice ( in error handling of lp3944_configure and lp3944_remove ).
>> This patch properly handles lp3944_configure fail in lp3944_probe.
>>
>
> Hi Axel, thanks for the fix, I agree it's needed.
> There are some minor comments inlined below.
>
> Plus, when possible, I prefer commit messages to be wrapped to 72/80
> characters per line. Please, consider this if you are sending a v2.
ok. done in v2.

>
> Ah, may I ask where you are using this driver? Just curious.
>
>> Signed-off-by: Axel Lin <axel.lin@xxxxxxxxx>
>> ---
>>  drivers/leds/leds-lp3944.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lp3944.c b/drivers/leds/leds-lp3944.c
>> index 8d5ecce..03a24d0 100644
>> --- a/drivers/leds/leds-lp3944.c
>> +++ b/drivers/leds/leds-lp3944.c
>> @@ -379,6 +379,7 @@ static int __devinit lp3944_probe(struct i2c_client *client,
>>  {
>>       struct lp3944_platform_data *lp3944_pdata = client->dev.platform_data;
>>       struct lp3944_data *data;
>> +     int err;
>>
>>       if (lp3944_pdata == NULL) {
>>               dev_err(&client->dev, "no platform data\n");
>> @@ -403,8 +404,15 @@ static int __devinit lp3944_probe(struct i2c_client *client,
>>
>>       dev_info(&client->dev, "lp3944 enabled\n");
>>
>> -     lp3944_configure(client, data, lp3944_pdata);
>> +     err = lp3944_configure(client, data, lp3944_pdata);
>> +     if (err < 0)
>> +             goto err_configure;
>> +
>
> The dev_info(&client->dev, "lp3944 enabled\n"); could now go right
> here, before the return 0, so we don't report the driver as enabled
> even in the case its probe fails.
done in v2.
>
>>       return 0;
>> +
>> +err_configure:
>> +     kfree(data);
>
> add i2c_set_clientdata(client, NULL) here just like in lp3944_remove()

per Wolfram Sang's comment, I skip this part in v2.

>
>> +     return err;
>>  }
>>
>>  static int __devexit lp3944_remove(struct i2c_client *client)
>> --
>> 1.5.4.3
>>
>
> Regards,
>   Antonio
>
> --
> Antonio Ospite
> http://ao2.it
>
> PGP public key ID: 0x4553B001
>
> A: Because it messes up the order in which people normally read text.
>   See http://en.wikipedia.org/wiki/Posting_style
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
--
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/