lkml.org 
[lkml]   [2010]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] leds-lp3944: properly handle lp3944_configure fail in lp3944_probe
    2010/5/17 Antonio Ospite <ospite@studenti.unina.it>:
    > On Mon, 17 May 2010 09:34:02 +0800
    > Axel Lin <axel.lin@gmail.com> 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@gmail.com>
    >> ---
    >>  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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-05-17 11:55    [W:0.031 / U:2.272 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site