Hi Dmitry,
Thank you for good comments. I fixed all as you recommended. More
detailed answers are below your comments.
In addition, I also added wakeup related statements
(device_init_wakeup, device_may_wakeup, [enable|disable]_irq_wake).
I'm sending this revised patch right now.
By the way, can I ask the same question which I ask to Trilok.
Even though I guard suspend/resume with #ifdef CONFIG_PM in the new
patch, Could I know the good reason for this protection? Because
'/Documentation/SubmittingPatches' says "ifdefs are ugly"
Thank you Dmitry again for your comments and time.
Kyuwon
On Fri, May 8, 2009 at 12:19 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> Hi Kim,
>
> On Wed, May 06, 2009 at 03:21:24PM +0900, Kim Kyuwon wrote:
>> @@ -332,4 +332,12 @@ config KEYBOARD_SH_KEYSC
>>
>> To compile this driver as a module, choose M here: the
>> module will be called sh_keysc.
>> +
>> +config KEYBOARD_MAX7359
>> + tristate "Maxim MAX7359 Key Switch Controller"
>> + depends on I2C
>> + help
>> + If you say yes here you get support for the Maxim MAX7359 Key
>> + Switch Controller chip. This providers microprocessors with
>> + management of up to 64 key switches
>
> "To compile this driver as a module..."
I added this sentence.
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/input.h>
>> +#include <linux/max7359_keypad.h>
>> +
>> +#define MAX_KEY_NUM (8 * 8)
>
> Please do:
>
> #define MAX_KEY_NUM (MAX_MATRIX_KEY_ROWS * MAX_MATRIX_KEY_COLS)
I did
#define MAX7359_MAX_KEY_NUM (MAX7359_MAX_KEY_ROWS * MAX7359_MAX_KEY_COLS)
Thanks.
>> +static int max7359_write_reg(struct i2c_client *client, u8 reg, u8 val)
>> +{
>> + int ret = i2c_smbus_write_byte_data(client, reg, val);
>
> Empty line after declarations please.
Fixed.
>> +static irqreturn_t max7359_interrupt(int irq, void *dev_id)
>> +{
>> + struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;
>> +
>> + if (!work_pending(&keypad->work)) {
>> + disable_irq_nosync(keypad->irq);
>> + schedule_work(&keypad->work);
>> + } else {
>> + dev_err(&keypad->client->dev,
>> + "Another job is currently pending \n");
>
> Is this truly an error?
I changed dev_err to dev_warn.
>> +
>> +static int __exit max7359_remove(struct i2c_client *client)
>
> __devexit, not __exit.
Fixed.
>> +
>> +static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
>> +{
>> + /* If no keys are pressed, enter sleep mode for 8192 ms */
>
> What happens if there are keys that are pressed?
I added more comments in new function(max7359_fall_deepsleep) as below:
/*
* Let MAX7359 fall into a deep sleep:
* If no keys are pressed, enter sleep mode for 8192 ms. And if any
* key is pressed, the MAX7359 returns to normal operating mode.
*/
>> + max7359_write_reg(client, MAX7359_REG_SLEEP, 0x01);
>> +
>> + return 0;
>> +}
>> +
>> +static int max7359_resume(struct i2c_client *client)
>> +{
>> + /* Restore the default setting (autosleep for 256 ms) */
>> + max7359_write_reg(client, MAX7359_REG_SLEEP, 0x07);
>> +
>> + return 0;
>> +}
>
> Could we also have similar open and close? It seems prudent to be in low
> power mode if there are no users of the device.
Sure we can. I added max7359_open, max7359_close
>> +
>> +static struct i2c_driver max7359_i2c_driver = {
>> + .driver = {
>> + .name = "max7359",
>> + },
>> + .probe = max7359_probe,
>> + .remove = __exit_p(max7359_remove),
>
> __devexit_p();
Fixed.
>> + .suspend = max7359_suspend,
>> + .resume = max7359_resume,
>
> Guard suspend/resume with #ifdef CONFIG_PM please.
I guarded them.
>> +
>> +#define MAX_MATRIX_KEY_ROWS 8
>> +#define MAX_MATRIX_KEY_COLS 8
>
> These names are too generic, may clash with other #includes
> eventially... adding MAX7359_ prefix seems prudent.
I added MAX7539_ prefix and these macros are moved to max7359_keypad.c file.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
