Hi Trilok,

Thank you for reviewing this driver and I have something to make sure
for the next better patch :)

On Wed, May 6, 2009 at 10:38 PM, Trilok Soni [off-list ref] wrote:

Hi Kim,

On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon [off-list ref] wrote:

quoted

The Maxim MAX7359 is a I2C interfaced key switch controller which provides microprocessors with management of up to 64 key switches.
This patch adds support for the MAX7359 key switch controller.

Could you please restrict the commit text line to 80/72 columns? It
will look good.

Is this restriction for commit text mandatory? It seems like I got an
email from Andrew Morton that my patch was wordwraped even commit
text.

quoted

+static irqreturn_t max7359_interrupt(int irq, void *dev_id)
+{
+       struct max7359_keypad *keypad = (struct max7359_keypad *) dev_id;

No need of casting. Please remove.

Ok, Thanks I'll remove it.

quoted

+
+       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");
+       }
+
+       return IRQ_HANDLED;
+}
+

quoted

+
+static int __devinit max7359_probe(struct i2c_client *client,
+                                       const struct i2c_device_id *id)

You may want to remove __devinit as it is i2c client driver . I have
added linux-i2c for i2c specific review.

Actually I don't want to remove it :), Is there any reason why you think so?

quoted

+static int max7359_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+       /* If no keys are pressed, enter sleep mode for 8192 ms */
+       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;
+}

Protect these two function with #ifdef CONFIG_PM please.

This is what I really wanted to ask. I'm trying not to use '#ifdef'.
By the way, why most of drivers use protect suspend/resume functions
with #ifdef? Just for saving code size?

quoted

+

quoted

+
+static struct i2c_driver max7359_i2c_driver = {
+       .driver = {
+               .name = "max7359",
+       },
+       .probe          = max7359_probe,
+       .remove         = __exit_p(max7359_remove),

quoted

+};
+
+
+MODULE_AUTHOR("Kim Kyuwon [off-list ref]");
+MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
+MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

In 'include/linux/module.h'

 96 /* For userspace: you can also call me... */
 97 #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias)

From comment above, I think MODULE_ALIAS is optional. Do you still

think MODULE_ALIAS needs?

And when I send new version of patch, I will add [PATCH] in front of subject.
Thank you for reviewing and letting me ask.

Regards,