Re: [PATCH] input: keyboard: introduce lm8323 driver

[Felipe Balbi]

Hi,

On Thu, Feb 19, 2009 at 11:12:07PM +0530, Trilok Soni wrote:
> Hi Flipe,
> 
> > diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
> > new file mode 100644
> > index 0000000..27da93c
> > --- /dev/null
> > +++ b/drivers/input/keyboard/lm8323.c
> > @@ -0,0 +1,913 @@
> > +/*
> > + * drivers/i2c/chips/lm8323.c
> > + *
> > + * Copyright (C) 2007 Nokia Corporation
> 
> You might want to update this if you have updated some code before
> submitting to kernel list.
> 
> > +
> > +       err = request_irq(client->irq, lm8323_irq,
> > +                         IRQF_TRIGGER_FALLING | IRQF_DISABLED |
> > +                         IRQF_SAMPLE_RANDOM, "lm8323", lm);
> 
> No need to add IRQF_SAMPLE_RANDOM, it is done input subsystem. Please
> check input_report_event.
> 
> > +
> > +       set_irq_wake(client->irq, 1);
> 
> Shall we move to device_init_wakup + enable_irq_wake friends ?
> 
> > +
> > +       lm->kp_enabled = 1;
> > +       err = device_create_file(&client->dev, &dev_attr_disable_kp);
> > +       if (err < 0)
> > +               goto fail7;
> > +
> > +       idev = input_allocate_device();
> > +       if (!idev) {
> > +               err = -ENOMEM;
> > +               goto fail8;
> > +       }
> > +
> > +       if (pdata->name)
> > +               idev->name = pdata->name;
> > +       else
> > +               idev->name = "LM8323 keypad";
> > +       snprintf(lm->phys, sizeof(lm->phys), "%s/input-kp", client->dev.bus_id);
> > +       idev->phys = lm->phys;
> > +
> > +       lm->keys_down = 0;
> > +       idev->evbit[0] = BIT(EV_KEY);
> > +       for (i = 0; i < LM8323_KEYMAP_SIZE; i++) {
> > +               if (pdata->keymap[i] > 0)
> > +                       set_bit(pdata->keymap[i], idev->keybit);
> 
> No difference but better to use __set_bit, as atomicity is not required here.
> 
> > +
> > +               lm->keymap[i] = pdata->keymap[i];
> > +       }
> > +
> > +       if (pdata->repeat)
> > +               set_bit(EV_REP, idev->evbit);
> 
> Ditto.
> 
> Also in lm8323_remove we need to set_irq_wake(..., 0) right?
> 
> > +
> > diff --git a/include/linux/i2c/lm8323.h b/include/linux/i2c/lm8323.h
> > new file mode 100644
> > index 0000000..5eae5cc
> > --- /dev/null
> > +++ b/include/linux/i2c/lm8323.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * include/lm8323.h
> 
> Path problem...better to remove it.

thanks for reviewing it, here's the updated version:

==== cut here ====