Re: Input: add MAX7359 key switch controller driver

Re: Input: add MAX7359 key switch controller driver

[Trilok Soni]

Hi Kim,

On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon <q1.kim@samsung.com> 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.

> +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.

> +
> +       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.

> +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.

> +

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


> +};
> +
> +
> +MODULE_AUTHOR("Kim Kyuwon <q1.kim@samsung.com>");
> +MODULE_DESCRIPTION("MAX7359 Key Switch Controller Driver");
> +MODULE_LICENSE("GPL v2");

MODULE_ALIAS ??

> diff --git a/include/linux/max7359_keypad.h b/include/linux/max7359_keypad.h

> +
> +#define KEY(row, col, val)     (((row) << 28) | ((col) << 24) | (val))

Looks like this macro is highly used by many input drivers, anyone
looking at it to place it at common location?


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Input: add MAX7359 key switch controller driver

[H Hartley Sweeten]

On Wednesday, May 06, 2009 6:38 AM, Trilok Soni wrote:
>> +
>> +#define KEY(row, col, val)     (((row) << 28) | ((col) << 24) | (val))
>
> Looks like this macro is highly used by many input drivers, anyone
> looking at it to place it at common location?

Hello,

I submitted a patch series around early Feb that did just that. The only
archive for linux-input that I can find to reference the original submit
is at http://markmail.org/browse/org.kernel.vger.linux-input/2009-01.
The first message in the series is $SUBJECT
"[PATCH 0/5] input: Common macro for keypad drivers".

At this point the series would probably need to be updated.  If wanted I
will create a new one.

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Input: add MAX7359 key switch controller driver

[Kim Kyuwon]

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@gmail.com> wrote:
> Hi Kim,
>
> On Wed, May 6, 2009 at 11:51 AM, Kim Kyuwon <q1.kim@samsung.com> 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@samsung.com>");
>> +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,

Re: Input: add MAX7359 key switch controller driver

[Trilok Soni]

Hi Kim,

>>
>> 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.

This helps reader of the commit text to not give much stretch on his eyes :)

>>
>>> +
>>> +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?

Sorry I have mistaken it as __init. This is fine.

>
>>> +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?

suspend and resume is only important if the user of this driver has
CONFIG_PM enabled in it's
defconfig, right? So, if CONFIG_PM is not defined suspend/resume
should equal to NULL.

>>
>>
>>> +};
>>> +
>>> +
>>> +MODULE_AUTHOR("Kim Kyuwon <q1.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
>>> +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?
>

Yes, it is not __must__.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni