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 <soni.trilok@xxxxxxxxx> wrote:
> Hi Kim,
>
> On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon <q1.kim@xxxxxxxxxxx> wrote:
>> 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.
>> +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.
>> +
>> + 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;
>> +}
>> +
>
>> +
>> +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?
>> +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?
>> +
>
>> +
>> +static struct i2c_driver max7359_i2c_driver = {
>> + .driver = {
>> + .name = "max7359",
>> + },
>> + .probe = max7359_probe,
>> + .remove = __exit_p(max7359_remove),
>
>
>> +};
>> +
>> +
>> +MODULE_AUTHOR("Kim Kyuwon <q1.kim@xxxxxxxxxxx>");
>> +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,
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
