| Message ID | 1389694438-18614-1-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Tue, Jan 14, 2014 at 5:13 AM, Mika Westerberg <mika.westerberg@linux.intel.com> 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 :( > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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? > + 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? 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. What if we just change i2c_hid_set_power in i2c_hid_power by the corresponding pm_runtime calls? > .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. I tested this today, and it works, so you should be right, but I'd like to have your opinion on this. Cheers, Benjamin > +#endif > + > +static const struct dev_pm_ops i2c_hid_pm = { > + SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_suspend, i2c_hid_resume) > + SET_RUNTIME_PM_OPS(i2c_hid_runtime_suspend, i2c_hid_runtime_resume, > + NULL) > +}; > > static const struct i2c_device_id i2c_hid_id_table[] = { > { "hid", 0 }, > -- > 1.8.5.2 -- 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
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@linux.intel.com> 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. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > 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. > > > + 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. > 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 ;-) -- 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
On Tue, Jan 28, 2014 at 4:12 AM, Mika Westerberg <mika.westerberg@linux.intel.com> 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@linux.intel.com> 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. > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> > --- >> > 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 :) > >> 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... Cheers, Benjamin -- 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
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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> > >> > --- > >> > 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 :-) -- 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
On Wed, Jan 29, 2014 at 4:13 AM, Mika Westerberg <mika.westerberg@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> >> >> > --- >> >> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 29, 2014 at 09:57:00AM -0500, Benjamin Tissoires wrote: > 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 :) Yeah, those devices use some ACPI features that are not well supported in Linux. Most notably GPIO/Serialbus operation regions are currently not supported at all. We are in progress of writing support for them. -- 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
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); + 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, .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; +} +#endif + +static const struct dev_pm_ops i2c_hid_pm = { + SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_suspend, i2c_hid_resume) + SET_RUNTIME_PM_OPS(i2c_hid_runtime_suspend, i2c_hid_runtime_resume, + NULL) +}; static const struct i2c_device_id i2c_hid_id_table[] = { { "hid", 0 },
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. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/hid/i2c-hid/i2c-hid.c | 81 ++++++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 27 deletions(-)