| Message ID | 1463620982-21058-1-git-send-email-mail@3v1n0.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thursday 19 May 2016 03:23:02 Marco Trevisan wrote: > From: Marco Trevisan (Treviño) <mail@3v1n0.net> > > Override default LED class suspend/resume handles, by keeping track of > the brightness level before suspending so that it can be automatically > restored on resume by calling default resume handler. > > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > --- > drivers/platform/x86/thinkpad_acpi.c | 43 +++++++++++++++++++++++++++++++++--- > 1 file changed, 40 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 1f9783c..10111c2 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -5041,6 +5041,8 @@ static int kbdlight_set_level(int level) > return 0; > } > > +static int kbdlight_set_level_and_update(int level); > + > static int kbdlight_get_level(void) > { > int status = 0; > @@ -5108,7 +5110,7 @@ static void kbdlight_set_worker(struct work_struct *work) > container_of(work, struct tpacpi_led_classdev, work); > > if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING)) > - kbdlight_set_level(data->new_state); > + kbdlight_set_level_and_update(data->new_state); > } > > static void kbdlight_sysfs_set(struct led_classdev *led_cdev, > @@ -5139,7 +5141,6 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = { > .max_brightness = 2, > .brightness_set = &kbdlight_sysfs_set, > .brightness_get = &kbdlight_sysfs_get, > - .flags = LED_CORE_SUSPENDRESUME, > } > }; > > @@ -5177,6 +5178,20 @@ static void kbdlight_exit(void) > flush_workqueue(tpacpi_wq); > } > > +static int kbdlight_set_level_and_update(int level) > +{ > + int ret; > + struct led_classdev *led_cdev; > + > + ret = kbdlight_set_level(level); > + led_cdev = &tpacpi_led_kbdlight.led_classdev; > + > + if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED)) > + led_cdev->brightness = level; > + > + return ret; > +} > + > static int kbdlight_read(struct seq_file *m) > { > int level; > @@ -5217,13 +5232,35 @@ static int kbdlight_write(char *buf) > if (level == -1) > return -EINVAL; > > - return kbdlight_set_level(level); > + return kbdlight_set_level_and_update(level); > +} > + > +static void kbdlight_suspend(void) > +{ > + struct led_classdev *led_cdev; > + > + if (!tp_features.kbdlight) > + return; > + > + led_cdev = &tpacpi_led_kbdlight.led_classdev; > + led_update_brightness(led_cdev); > + led_classdev_suspend(led_cdev); > +} > + > +static void kbdlight_resume(void) > +{ > + if (!tp_features.kbdlight) > + return; > + > + led_classdev_resume(&tpacpi_led_kbdlight.led_classdev); > } > > static struct ibm_struct kbdlight_driver_data = { > .name = "kbdlight", > .read = kbdlight_read, > .write = kbdlight_write, > + .suspend = kbdlight_suspend, > + .resume = kbdlight_resume, > .exit = kbdlight_exit, > }; > For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME does not work correctly... I would rather see fixed support for flag LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if possible. Any idea?
2016-05-19 9:52 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>: > For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME > does not work correctly... I would rather see fixed support for flag > LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if > possible. Any idea? As I wrote you in private mail I think the only way we have is to keep the led_cdev->brightness in sync with actual state. And to do this, we can listen the ACPI event, keeping this like a private event that we just consume to update led brightness value, without delivering it to userspace. However, the custom code needed for getting this done, is probably way more than the one I added here. Maybe it could be useful to others to read the brightness level before suspending, and in this case we could add a new flag to the led subsystem, but not sure it's really worth. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote: > From: Marco Trevisan (Treviño) <mail@3v1n0.net> > > Override default LED class suspend/resume handles, by keeping track of > the brightness level before suspending so that it can be automatically > restored on resume by calling default resume handler. > > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> The patch requires a Signed-off-by. I presume that should be from Marco? Henrique, is this Acked-by from you accurate? I need to see maintainers respond on list as they would likely object to me just taking other people's word for it in other scenarios ;-)
On Monday 23 May 2016 23:57:52 Darren Hart wrote: > On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote: > > From: Marco Trevisan (Treviño) <mail@3v1n0.net> > > > > Override default LED class suspend/resume handles, by keeping track > > of the brightness level before suspending so that it can be > > automatically restored on resume by calling default resume > > handler. > > > > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > > The patch requires a Signed-off-by. I presume that should be from > Marco? > > Henrique, is this Acked-by from you accurate? I need to see > maintainers respond on list as they would likely object to me just > taking other people's word for it in other scenarios ;-) Darren, I'm not sure if this is really correct approach in current patch... Anyway as Marco wrote, there is ACPI event when keyboard backlight change and maybe we should handle it...
2016-05-23 23:57 GMT+02:00 Darren Hart <dvhart@infradead.org>: > On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote: >> From: Marco Trevisan (Treviño) <mail@3v1n0.net> >> >> Override default LED class suspend/resume handles, by keeping track of >> the brightness level before suspending so that it can be automatically >> restored on resume by calling default resume handler. >> >> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > > The patch requires a Signed-off-by. I presume that should be from Marco? I'm quite new to this, so please let me know how I should proceed. > Henrique, is this Acked-by from you accurate? I need to see maintainers respond > on list as they would likely object to me just taking other people's word for it > in other scenarios ;-) I've added it based on this reply (on ibm-acpi ML): - http://thread.gmane.org/gmane.linux.acpi.ibm-acpi.devel/4006/focus=4007 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 20, 2016 at 05:03:52AM +0200, Marco Trevisan (Treviño) wrote: > 2016-05-19 9:52 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>: > > For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME > > does not work correctly... I would rather see fixed support for flag > > LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if > > possible. Any idea? > > As I wrote you in private mail I think the only way we have is to keep > the led_cdev->brightness in sync with actual state. And to do this, we > can listen the ACPI event, keeping this like a private event that we > just consume to update led brightness value, without delivering it to > userspace. > > However, the custom code needed for getting this done, is probably way > more than the one I added here. > Maybe it could be useful to others to read the brightness level before > suspending, and in this case we could add a new flag to the led > subsystem, but not sure it's really worth. > At the risk of contributing to "the platform platform", I agree with a local fix here in the near term. If this becomes a pattern, we can look to move this into the LED subsystem. As it is, this is working as designed, overriding when necessary.
2016-05-24 0:04 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>: > Darren, I'm not sure if this is really correct approach in current > patch... Anyway as Marco wrote, there is ACPI event when keyboard > backlight change and maybe we should handle it... I'll be happy to work on that as well, but I've not much time at the moment. However I think this could be an ok way to handle things in the mean time. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2016 at 12:08:46AM +0200, Marco Trevisan (Treviño) wrote: > 2016-05-23 23:57 GMT+02:00 Darren Hart <dvhart@infradead.org>: > > On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote: > >> From: Marco Trevisan (Treviño) <mail@3v1n0.net> > >> > >> Override default LED class suspend/resume handles, by keeping track of > >> the brightness level before suspending so that it can be automatically > >> restored on resume by calling default resume handler. > >> > >> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > > > > The patch requires a Signed-off-by. I presume that should be from Marco? > > I'm quite new to this, so please let me know how I should proceed. Any time you submit a patch, you must provide a Signed-off-by in the commit message. Git will do this for you with "git commit --signoff". See: Documentation/SubmittingPatches, Section 11: Sign your work > > > Henrique, is this Acked-by from you accurate? I need to see maintainers respond > > on list as they would likely object to me just taking other people's word for it > > in other scenarios ;-) > > I've added it based on this reply (on ibm-acpi ML): > - http://thread.gmane.org/gmane.linux.acpi.ibm-acpi.devel/4006/focus=4007 > Ah, excellent. I didn't have the original since I wasn't on Cc initially. That's all I need from Henrique.
On Tue, May 24, 2016 at 12:10:44AM +0200, Marco Trevisan (Treviño) wrote: > 2016-05-24 0:04 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>: > > Darren, I'm not sure if this is really correct approach in current > > patch... Anyway as Marco wrote, there is ACPI event when keyboard > > backlight change and maybe we should handle it... > > I'll be happy to work on that as well, but I've not much time at the moment. > However I think this could be an ok way to handle things in the mean time. > I believe so, and Henrique has approved.
On Mon, May 23, 2016, at 18:57, Darren Hart wrote: > On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote: > > From: Marco Trevisan (Treviño) <mail@3v1n0.net> > > > > Override default LED class suspend/resume handles, by keeping track of > > the brightness level before suspending so that it can be automatically > > restored on resume by calling default resume handler. > > > > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > > The patch requires a Signed-off-by. I presume that should be from Marco? > > Henrique, is this Acked-by from you accurate? I need to see maintainers > respond > on list as they would likely object to me just taking other people's word > for it > in other scenarios ;-) Yeah, I acked it. Sorry it took a few days to reply to this email. This is not the "best" fix, but it is by far the less troublesome one: hooking to the internal event from the thinkpad DSDT and updating the brightness shadow register in the driver should work, but that will require testing as we cannot really take for granted that we will get that event on every firmware out there without going over the DSDT AML of a lot of models.
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 1f9783c..10111c2 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -5041,6 +5041,8 @@ static int kbdlight_set_level(int level) return 0; } +static int kbdlight_set_level_and_update(int level); + static int kbdlight_get_level(void) { int status = 0; @@ -5108,7 +5110,7 @@ static void kbdlight_set_worker(struct work_struct *work) container_of(work, struct tpacpi_led_classdev, work); if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING)) - kbdlight_set_level(data->new_state); + kbdlight_set_level_and_update(data->new_state); } static void kbdlight_sysfs_set(struct led_classdev *led_cdev, @@ -5139,7 +5141,6 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = { .max_brightness = 2, .brightness_set = &kbdlight_sysfs_set, .brightness_get = &kbdlight_sysfs_get, - .flags = LED_CORE_SUSPENDRESUME, } }; @@ -5177,6 +5178,20 @@ static void kbdlight_exit(void) flush_workqueue(tpacpi_wq); } +static int kbdlight_set_level_and_update(int level) +{ + int ret; + struct led_classdev *led_cdev; + + ret = kbdlight_set_level(level); + led_cdev = &tpacpi_led_kbdlight.led_classdev; + + if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED)) + led_cdev->brightness = level; + + return ret; +} + static int kbdlight_read(struct seq_file *m) { int level; @@ -5217,13 +5232,35 @@ static int kbdlight_write(char *buf) if (level == -1) return -EINVAL; - return kbdlight_set_level(level); + return kbdlight_set_level_and_update(level); +} + +static void kbdlight_suspend(void) +{ + struct led_classdev *led_cdev; + + if (!tp_features.kbdlight) + return; + + led_cdev = &tpacpi_led_kbdlight.led_classdev; + led_update_brightness(led_cdev); + led_classdev_suspend(led_cdev); +} + +static void kbdlight_resume(void) +{ + if (!tp_features.kbdlight) + return; + + led_classdev_resume(&tpacpi_led_kbdlight.led_classdev); } static struct ibm_struct kbdlight_driver_data = { .name = "kbdlight", .read = kbdlight_read, .write = kbdlight_write, + .suspend = kbdlight_suspend, + .resume = kbdlight_resume, .exit = kbdlight_exit, };