lkml.org 
[lkml]   [2009]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] input: keyboard: introduce lm8323 driver
    On Thu, 19 Feb 2009 14:14:14 +0200
    Felipe Balbi <me@felipebalbi.com> wrote:

    > From: Felipe Balbi <felipe.balbi@nokia.com>
    >
    > lm8323 is the keypad driver used in n810 device. This
    > driver has been sitting in linux-omap for quite a long
    > time, it's about time to get comments from upstream and
    > get it merged.
    >

    Please feed it through scripts/checkpatch.pl and review the resulting
    report?


    > +static ssize_t lm8323_pwm_store_time(struct device *dev,
    > + struct device_attribute *attr, const char *buf, size_t len)
    > +{
    > + struct led_classdev *led_cdev = dev_get_drvdata(dev);
    > + struct lm8323_pwm *pwm = cdev_to_pwm(led_cdev);
    > + int ret;
    > + int time;
    > +
    > + ret = sscanf(buf, "%d", &time);
    > + /* Numbers only, please. */
    > + if (ret)
    > + return -EINVAL;

    strict_strtoul() does a better job of this.

    > + pwm->fade_time = time;
    > +
    > + return strlen(buf);
    > +}
    > +static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, lm8323_pwm_store_time);
    > +
    > +static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
    > + const char *name)
    > +{
    > + struct lm8323_pwm *pwm = NULL;
    > +
    > + BUG_ON(id > 3);
    > +
    > + switch (id) {
    > + case 1:
    > + pwm = &lm->pwm1;
    > + break;
    > + case 2:
    > + pwm = &lm->pwm2;
    > + break;
    > + case 3:
    > + pwm = &lm->pwm3;
    > + break;
    > + }
    > +
    > + pwm->id = id;
    > + pwm->fade_time = 0;
    > + pwm->brightness = 0;
    > + pwm->desired_brightness = 0;
    > + pwm->running = 0;
    > + mutex_init(&pwm->lock);
    > + if (name) {
    > + pwm->cdev.name = name;
    > + pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
    > + if (led_classdev_register(dev, &pwm->cdev) < 0) {

    I did't see a dependency on LEDS in the Kconfig entry. Will it compile
    with LEDS=n?


    > + dev_err(dev, "couldn't register PWM %d\n", id);
    > + return -1;
    > + }
    > + if (device_create_file(pwm->cdev.dev,
    > + &dev_attr_time) < 0) {
    > + dev_err(dev, "couldn't register time attribute\n");
    > + led_classdev_unregister(&pwm->cdev);
    > + return -1;
    > + }
    > + INIT_WORK(&pwm->work, lm8323_pwm_work);
    > + pwm->enabled = 1;
    > + } else {
    > + pwm->enabled = 0;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static struct i2c_driver lm8323_i2c_driver;
    > +
    > +static ssize_t lm8323_show_disable(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > + struct lm8323_chip *lm = dev_get_drvdata(dev);
    > +
    > + return sprintf(buf, "%u\n", !lm->kp_enabled);
    > +}
    > +
    > +static ssize_t lm8323_set_disable(struct device *dev,
    > + struct device_attribute *attr,
    > + const char *buf, size_t count)
    > +{
    > + struct lm8323_chip *lm = dev_get_drvdata(dev);
    > + int ret;
    > + int i;
    > +
    > + i = sscanf(buf, "%d", &ret);

    Again, strict_strto*() would be better. It will disallow input of the
    form "42foo".

    > + mutex_lock(&lm->lock);
    > + lm->kp_enabled = !i;
    > + mutex_unlock(&lm->lock);
    > +
    > + return count;
    > +}
    >
    > ...
    >
    > +static struct i2c_driver lm8323_i2c_driver = {
    > + .driver = {
    > + .name = "lm8323",
    > + },
    > + .probe = lm8323_probe,
    > + .remove = lm8323_remove,
    > + .suspend = lm8323_suspend,
    > + .resume = lm8323_resume,
    > + .id_table = lm8323_id,
    > +};

    Does this compile and run OK with CONFIG_PM=n?

    >
    > ...
    >



    \
     
     \ /
      Last update: 2009-02-20 22:47    [W:0.033 / U:0.304 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site