Re: [PATCH 1/3] input: keyboard: MCS5080: support suspend/resume.

["Kim, HeungJun" <riverful.kim@samsung.com>]

Hi Dmitry,

I'll fix the following points, and re-send patches, soon.
And, I gonna send the led-trigger patch, later.
Actually, the led-blink command & method is changed according to touchkey F/W, as you know.
The cause of my sending led-patch is that I think it's the just basic function.
But, If the touchkey-drv use led-trigger framework, the driver code is heavy, I guess.

After I consider that deeply, I'll re-send patch. 

Thansk to review.

2010-11-15 오후 5:32, Dmitry Torokhov 쓴 글:
> Hi Kim,
> 
> On Mon, Nov 15, 2010 at 01:31:45PM +0900, Kim, HeungJun wrote:
>> This patch supports suspend/resume functions for mcs5080 touchkey
>> driver.
>>
>> Signed-off-by: Heungjun Kim <riverful.kim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/input/keyboard/mcs_touchkey.c |   41 +++++++++++++++++++++++++++++++++
>>  include/linux/i2c/mcs.h               |    1 +
>>  2 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/mcs_touchkey.c b/drivers/input/keyboard/mcs_touchkey.c
>> index 63b849d..06385f5 100644
>> --- a/drivers/input/keyboard/mcs_touchkey.c
>> +++ b/drivers/input/keyboard/mcs_touchkey.c
>> @@ -45,6 +45,8 @@ struct mcs_touchkey_chip {
>>  };
>>
>>  struct mcs_touchkey_data {
>> +       void (*poweron)(int);
> 
> int (*poweron)(bool);
> 
> The method must be able to signal errors.
Ok, I'll fix this.

> 
>> +
>>         struct i2c_client *client;
>>         struct input_dev *input_dev;
>>         struct mcs_touchkey_chip chip;
>> @@ -168,6 +170,8 @@ static int __devinit mcs_touchkey_probe(struct i2c_client *client,
>>
>>         if (pdata->cfg_pin)
>>                 pdata->cfg_pin();
>> +       if (pdata->poweron)
>> +               data->poweron = pdata->poweron;
>>
>>         error = request_threaded_irq(client->irq, NULL, mcs_touchkey_interrupt,
>>                         IRQF_TRIGGER_FALLING, client->dev.driver->name, data);
>> @@ -202,6 +206,41 @@ static int __devexit mcs_touchkey_remove(struct i2c_client *client)
>>         return 0;
>>  }
>>
>> +#ifdef CONFIG_PM
>> +static int mcs_touchkey_suspend(struct i2c_client *client, pm_message_t mesg)
> 
> Newer code should use dev_pm_ops please.
I got it. I'll adapt new runtime-pm framework.

> `
>> +{
>> +       struct mcs_touchkey_data *data = i2c_get_clientdata(client);
>> +
>> +       /* Disable the work */
>> +       disable_irq(client->irq);
>> +
>> +       /* Don't I2C operation since we don't know I2C bus is dead already */
> 
> ? I am not quite sure what you are trying to say here...
It's kind of defensive(?) code for sure.
If don't use this code and the I2C operation is crashed once,
after I2C operation is crashed, too.
It happened very frequently in the MCS5080 chip.
But fortunatly, It affects just own bus, not any other one.

> 
>> +
>> +       /* Finally turn off the power */
>> +       if (data->poweron)
>> +               data->poweron(0);
>> +
>> +       return 0;
>> +}
>> +
>> +static int mcs_touchkey_resume(struct i2c_client *client)
>> +{
>> +       struct mcs_touchkey_data *data = i2c_get_clientdata(client);
>> +
>> +       /* Enable the device first */
>> +       if (data->poweron)
>> +               data->poweron(1);
>> +
>> +       /* Enable irq again */
>> +       enable_irq(client->irq);
>> +
>> +       return 0;
>> +}
>> +#else
>> +#define mcs_touchkey_suspend   NULL
>> +#define mcs_touchkey_resume    NULL
>> +#endif
>> +
>>  static const struct i2c_device_id mcs_touchkey_id[] = {
>>         { "mcs5000_touchkey", MCS5000_TOUCHKEY },
>>         { "mcs5080_touchkey", MCS5080_TOUCHKEY },
>> @@ -216,6 +255,8 @@ static struct i2c_driver mcs_touchkey_driver = {
>>         },
>>         .probe          = mcs_touchkey_probe,
>>         .remove         = __devexit_p(mcs_touchkey_remove),
>> +       .suspend        = mcs_touchkey_suspend,
>> +       .resume         = mcs_touchkey_resume,
>>         .id_table       = mcs_touchkey_id,
>>  };
>>
>> diff --git a/include/linux/i2c/mcs.h b/include/linux/i2c/mcs.h
>> index 725ae7c..c4a3869 100644
>> --- a/include/linux/i2c/mcs.h
>> +++ b/include/linux/i2c/mcs.h
>> @@ -18,6 +18,7 @@
>>  #define MCS_KEY_CODE(v)                ((v) & 0xffff)
>>
>>  struct mcs_platform_data {
>> +       void (*poweron)(int);
>>         void (*cfg_pin)(void);
>>
>>         /* touchscreen */
>> --
>> 1.7.0.4
> 
> Shouldn't you call poweron() at driver bind and unbind time (if you do not want to
> supply open() and close() methods which wold be the best option)?
I got it. I'll locate poweron() function at the bind & unbind time.

> 
> Thanks.
> 

Regards,
HeundJun Kim
--
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