Re: [PATCH 1/5] I2C: LM8323: Introduce lm8323 keypad driver

["andrzej zaborowski" <balrogg@gmail.com>]

On 09/04/2008, Felipe Balbi <me@felipebalbi.com> wrote:
> On Wed, Apr 09, 2008 at 07:26:31PM +0200, andrzej zaborowski wrote:
>  [...]
>
>  > >  +       time = strict_strtoul(buf, 10, &res);
>  > >  +       /* Numbers only, please. */
>  > >  +       if (buf && *buf != '\n' && *(buf + 1) != '\0')
>  > >  +               return -EINVAL;
>  >
>  > The condition doesn't look correct, for example it rejects "55\n\0"
>  > but accepts NULL.
>
>
> Well, this is not my driver and i just moved to strict_stroul cuz
>  checkpatch asked me to. I'll take a closer look tomorrow but if you can
>  come up with a better solution for this, I'll ack.

Turns out the result is returned in the buffer in third parameter, and
the checking is already "strict" (rejects trailing non-numbers), so
this should look something like:

ret = strict_strtoul(buf, 0, &time);
if (ret)
        return ret;

>
>  btw, good catch, i didn't had too much time to work on this but still
>  the driver is working fine.
>
>
>  > >  +       i2c_set_clientdata(client, lm);
>  > >  +       lm->client = client;
>  > >  +       lm8323_pdata = client->dev.platform_data;
>  > >  +       if (!lm8323_pdata)
>  > >  +               return -EINVAL; /* ? */
>  >
>  > Here lm remains allocated and is lost.  Not that it bothers me, but
>  > probably not what was intended.
>
>
> You mean the error handling?

Yep.

>
>  if (!lm8323_pdata) {
>         kfree(lm);
>         return -EINVAL;
>  }
>
>  fixing tomorrow.

Thanks
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.