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.
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?
if (!lm8323_pdata) {
kfree(lm);
return -EINVAL;
}
fixing tomorrow.
> > +fail8:
> > + device_remove_file(&client->dev, &dev_attr_disable_kp);
> > +fail7:
> > + free_irq(lm->irq, lm);
> > +fail6:
> > + if (lm->pwm3.enabled)
> > + led_classdev_unregister(&lm->pwm3.cdev);
> > +fail5:
> > + if (lm->pwm2.enabled)
> > + led_classdev_unregister(&lm->pwm2.cdev);
> > +fail4:
> > + if (lm->pwm1.enabled)
> > + led_classdev_unregister(&lm->pwm1.cdev);
> > +fail3:
> > +fail2:
> > + kfree(lm);
> > + return err;
> > +}
> ...
>
> Question not really related to this patchset, but in the TSC2005
> driver some averaging and limit checks are done to the ADC values.
> Shouldn't that be done in userspace instead? (The ADS784x and TSC2301
> drivers do the same evidently.)
Now that's not really up to me, but if we move it now it probably won't
provide the feature n810 needs. I mean, n810 is designed on top of the
features provided by driver, the application probably expects the driver
to do the averaging and limit checks so at least for tsc2005 I think
it's not a good idea to change due to application requirements.
--
Best Regards,
Felipe Balbi
me@xxxxxxxxxxxxxxx
http://blog.felipebalbi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
