linux-kernel - Re: Input: add MAX7359 key switch controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 9 May 2009 10:58:45 +0900
From:	Kim Kyuwon <chammoru@...il.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Kim Kyuwon <q1.kim@...sung.com>,
	LKML <linux-kernel@...r.kernel.org>, linux-input@...r.kernel.org,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: Input: add MAX7359 key switch controller driver

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@...il.com> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/