On Wed, Jan 29, 2014 at 4:13 AM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> On Tue, Jan 28, 2014 at 09:19:33AM -0500, Benjamin Tissoires wrote:
>> On Tue, Jan 28, 2014 at 4:12 AM, Mika Westerberg
>> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>> > On Mon, Jan 27, 2014 at 10:36:25PM -0500, Benjamin Tissoires wrote:
>> >> On Tue, Jan 14, 2014 at 5:13 AM, Mika Westerberg
>> >> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>> >> > This patch adds runtime PM support for the HID over I2C driver. When the
>> >> > i2c-hid device is first opened we power it on and on the last close we
>> >> > power it off.
>> >> >
>> >> > The implementation is not the most power efficient because it needs some
>> >> > interaction from the userspace (e.g close the device node whenever we are
>> >> > no more interested in getting events), nevertheless it allows us to save
>> >> > some power and works with devices that are not wake capable.
>> >> >
>> >>
>> >> Hi Mika,
>> >>
>> >> I am a little bit puzzled here. The commit message just says that you
>> >> changed the implementation of the power saving with the exact same
>> >> behavior... At least that's what I understand.
>> >> Currently, the devices should be put on sleep if nobody is reading,
>> >> and back alive if a reader arrives.
>> >> I think there is a gain with the patch, but my knowledge of the pm
>> >> subsystem is far too limited to see it :(
>> >
>> > Yes, there should be gain. If there is power domain involved like, ACPI in
>> > our case, runtime suspending the device on close will let ACPI to move the
>> > device to D3cold (e.g full off) which saves more power.
>>
>> Oh, right. So, if you could just put this last sentence in the commit
>> message, that would be great.
>
> Sure.
>
>>
>> >
>> >> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> >> > ---
>> >> > drivers/hid/i2c-hid/i2c-hid.c | 81 ++++++++++++++++++++++++++++---------------
>> >> > 1 file changed, 54 insertions(+), 27 deletions(-)
>> >> >
>> >> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> >> > index d1f81f52481a..ff767d03d60e 100644
>> >> > --- a/drivers/hid/i2c-hid/i2c-hid.c
>> >> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> >> > @@ -25,6 +25,7 @@
>> >> > #include <linux/delay.h>
>> >> > #include <linux/slab.h>
>> >> > #include <linux/pm.h>
>> >> > +#include <linux/pm_runtime.h>
>> >> > #include <linux/device.h>
>> >> > #include <linux/wait.h>
>> >> > #include <linux/err.h>
>> >> > @@ -454,10 +455,18 @@ static void i2c_hid_init_reports(struct hid_device *hid)
>> >> > return;
>> >> > }
>> >> >
>> >> > + /*
>> >> > + * The device must be powered on while we fetch initial reports
>> >> > + * from it.
>> >> > + */
>> >> > + pm_runtime_get_sync(&client->dev);
>> >> > +
>> >> > list_for_each_entry(report,
>> >> > &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
>> >> > i2c_hid_init_report(report, inbuf, ihid->bufsize);
>> >> >
>> >> > + pm_runtime_put(&client->dev);
>> >> > +
>> >> > kfree(inbuf);
>> >> > }
>> >> >
>> >> > @@ -703,8 +712,8 @@ static int i2c_hid_open(struct hid_device *hid)
>> >> >
>> >> > mutex_lock(&i2c_hid_open_mut);
>> >> > if (!hid->open++) {
>> >> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> >> > - if (ret) {
>> >> > + ret = pm_runtime_get_sync(&client->dev);
>> >>
>> >> dummy question (kind of late here...). Is there a counter of how many
>> >> get/put has been called in pm_runtime which could allow us to get rid
>> >> of the hid->open count?
>> >
>> > There is a counter but I'm not sure if drivers are supposed to use that. I
>> > would prefer not to use that.
>>
>> ok
>>
>> >
>> >>
>> >> > + if (ret < 0) {
>> >> > hid->open--;
>> >> > goto done;
>> >> > }
>> >> > @@ -712,7 +721,7 @@ static int i2c_hid_open(struct hid_device *hid)
>> >> > }
>> >> > done:
>> >> > mutex_unlock(&i2c_hid_open_mut);
>> >> > - return ret;
>> >> > + return ret < 0 ? ret : 0;
>> >> > }
>> >> >
>> >> > static void i2c_hid_close(struct hid_device *hid)
>> >> > @@ -729,37 +738,17 @@ static void i2c_hid_close(struct hid_device *hid)
>> >> > clear_bit(I2C_HID_STARTED, &ihid->flags);
>> >> >
>> >> > /* Save some power */
>> >> > - i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> >> > + pm_runtime_put(&client->dev);
>> >> > }
>> >> > mutex_unlock(&i2c_hid_open_mut);
>> >> > }
>> >> >
>> >> > -static int i2c_hid_power(struct hid_device *hid, int lvl)
>> >> > -{
>> >> > - struct i2c_client *client = hid->driver_data;
>> >> > - struct i2c_hid *ihid = i2c_get_clientdata(client);
>> >> > - int ret = 0;
>> >> > -
>> >> > - i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
>> >> > -
>> >> > - switch (lvl) {
>> >> > - case PM_HINT_FULLON:
>> >> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> >> > - break;
>> >> > - case PM_HINT_NORMAL:
>> >> > - ret = i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> >> > - break;
>> >> > - }
>> >> > - return ret;
>> >> > -}
>> >> > -
>> >> > static struct hid_ll_driver i2c_hid_ll_driver = {
>> >> > .parse = i2c_hid_parse,
>> >> > .start = i2c_hid_start,
>> >> > .stop = i2c_hid_stop,
>> >> > .open = i2c_hid_open,
>> >> > .close = i2c_hid_close,
>> >> > - .power = i2c_hid_power,
>> >>
>> >> If I understand correctly, here you are trying to fix hidraw (with
>> >> i2c_hid tramsport) which used to set_power on/off twice with the first
>> >> reader, right?
>> >
>> > Right.
>> >
>> >> I don't think we have other i2c_hid users of hid_hw_power, but I am a
>> >> little bit worried of simply removing the callback.
>> >
>> > OK.
>> >
>> >> What if we just change i2c_hid_set_power in i2c_hid_power by the
>> >> corresponding pm_runtime calls?
>> >
>> > Sure, I'll change that in the next version.
>> >
>> >>
>> >> > .request = i2c_hid_request,
>> >> > };
>> >> >
>> >> > @@ -973,13 +962,17 @@ static int i2c_hid_probe(struct i2c_client *client,
>> >> > if (ret < 0)
>> >> > goto err;
>> >> >
>> >> > + pm_runtime_get_noresume(&client->dev);
>> >> > + pm_runtime_set_active(&client->dev);
>> >> > + pm_runtime_enable(&client->dev);
>> >> > +
>> >> > ret = i2c_hid_fetch_hid_descriptor(ihid);
>> >> > if (ret < 0)
>> >> > - goto err;
>> >> > + goto err_pm;
>> >> >
>> >> > ret = i2c_hid_init_irq(client);
>> >> > if (ret < 0)
>> >> > - goto err;
>> >> > + goto err_pm;
>> >> >
>> >> > hid = hid_allocate_device();
>> >> > if (IS_ERR(hid)) {
>> >> > @@ -1010,6 +1003,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>> >> > goto err_mem_free;
>> >> > }
>> >> >
>> >> > + pm_runtime_put(&client->dev);
>> >> > return 0;
>> >> >
>> >> > err_mem_free:
>> >> > @@ -1018,6 +1012,10 @@ err_mem_free:
>> >> > err_irq:
>> >> > free_irq(client->irq, ihid);
>> >> >
>> >> > +err_pm:
>> >> > + pm_runtime_put_noidle(&client->dev);
>> >> > + pm_runtime_disable(&client->dev);
>> >> > +
>> >> > err:
>> >> > i2c_hid_free_buffers(ihid);
>> >> > kfree(ihid);
>> >> > @@ -1029,6 +1027,11 @@ static int i2c_hid_remove(struct i2c_client *client)
>> >> > struct i2c_hid *ihid = i2c_get_clientdata(client);
>> >> > struct hid_device *hid;
>> >> >
>> >> > + pm_runtime_get_sync(&client->dev);
>> >> > + pm_runtime_disable(&client->dev);
>> >> > + pm_runtime_set_suspended(&client->dev);
>> >> > + pm_runtime_put_noidle(&client->dev);
>> >> > +
>> >> > hid = ihid->hid;
>> >> > hid_destroy_device(hid);
>> >> >
>> >> > @@ -1074,7 +1077,31 @@ static int i2c_hid_resume(struct device *dev)
>> >> > }
>> >> > #endif
>> >> >
>> >> > -static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
>> >> > +#ifdef CONFIG_PM_RUNTIME
>> >> > +static int i2c_hid_runtime_suspend(struct device *dev)
>> >> > +{
>> >> > + struct i2c_client *client = to_i2c_client(dev);
>> >> > +
>> >> > + i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>> >> > + disable_irq(client->irq);
>> >> > + return 0;
>> >> > +}
>> >> > +
>> >> > +static int i2c_hid_runtime_resume(struct device *dev)
>> >> > +{
>> >> > + struct i2c_client *client = to_i2c_client(dev);
>> >> > +
>> >> > + enable_irq(client->irq);
>> >> > + i2c_hid_set_power(client, I2C_HID_PWR_ON);
>> >> > + return 0;
>> >> > +}
>> >>
>> >> These two functions looks very similar to i2c_hid_suspend and
>> >> i2c_hid_resume, without the reset and the irq_wake :(
>> >> So, my question here is can we use some common code for them?
>> >> It may not be possible regarding CONFIG_PM_RUNTIME and
>> >> CONFIG_PM_SLEEP, but it still looks ugly to me.
>> >
>> > It looks ugly, I agree and we can probably reuse some code in the
>> > callbacks. I'll look into that.
>>
>> well, anyway, if this involve something even more ugly, we can for
>> sure stick with this version :)
>
> It turned out that it is going to be ugly, no matter what :-( I'll keep the
> current version for now.
>
>>
>> >
>> >> I tested this today, and it works, so you should be right, but I'd
>> >> like to have your opinion on this.
>> >
>> > Thanks for testing and comments.
>> >
>> > I'll prepare a new version with the suggested changes if you are OK with my
>> > explanations ;-)
>>
>> Sure I am. Thanks for handling the whole ACPI part of this module. I
>> was really pleased the other day to see that the touchscreen of a Bay
>> trail tablet is now working out of the box :) I still have many other
>> problems with this tablet, but still, having the input working is
>> always good with a tablet...
>
> If that's the Asus T100 tablet, we are going to fix rest of the problems
> soon :-)
That's great. I hope this will also fix my Lenovo Miix 2 (the T100 is
too expensive compared to the Dell Venue pro 8 and the Lenovo one) :)
I think most of my problems are ACPI related (WIFI card not powered
up, SD-ext reader not working, suspend/resume not there, etc...), so I
expect great of you :)
I'll give a last shot to the v2, before giving the reviewed by.
Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
