| Message ID | 20170209154417.19040-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thu, Feb 9, 2017, at 13:44, Hans de Goede wrote: > There is no need to set the led_classdev's brightness value from > its set_brightness callback, this is taken care of by the led-core and > thinkpad_acpi really should not be mucking with it. > > Note that kbdlight_set_level_and_update() is still used by the old > thinpad_acpi specific sysfs interface for the led, so we cannot > remove it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
On Thursday 09 February 2017 16:44:11 Hans de Goede wrote: > There is no need to set the led_classdev's brightness value from > its set_brightness callback, this is taken care of by the led-core and > thinkpad_acpi really should not be mucking with it. > > Note that kbdlight_set_level_and_update() is still used by the old > thinpad_acpi specific sysfs interface for the led, so we cannot > remove it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v8: > -New patch in v8 of this patch-set > --- > drivers/platform/x86/thinkpad_acpi.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index cacb43f..0680bb3 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -5095,8 +5095,6 @@ 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; > @@ -5164,7 +5162,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_and_update(data->new_state); > + kbdlight_set_level(data->new_state); > } > > static void kbdlight_sysfs_set(struct led_classdev *led_cdev, Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save kbdlight state on suspend and restore it on resume). It is really OK to revert that change? CCing Marco who is author of that commit.
Hi, On 01-03-17 12:27, Pali Rohár wrote: > On Thursday 09 February 2017 16:44:11 Hans de Goede wrote: >> There is no need to set the led_classdev's brightness value from >> its set_brightness callback, this is taken care of by the led-core and >> thinkpad_acpi really should not be mucking with it. >> >> Note that kbdlight_set_level_and_update() is still used by the old >> thinpad_acpi specific sysfs interface for the led, so we cannot >> remove it. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v8: >> -New patch in v8 of this patch-set >> --- >> drivers/platform/x86/thinkpad_acpi.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index cacb43f..0680bb3 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -5095,8 +5095,6 @@ 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; >> @@ -5164,7 +5162,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_and_update(data->new_state); >> + kbdlight_set_level(data->new_state); >> } >> >> static void kbdlight_sysfs_set(struct led_classdev *led_cdev, > > Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save > kbdlight state on suspend and restore it on resume). It is really OK to > revert that change? Yes, as explained on the commit msg, kbdlight_set_worker is only used from ledclass callbacks and the led core already update led_cdev->brightness when calling those. Regards, Hans
On Wednesday 01 March 2017 12:57:42 Hans de Goede wrote: > Hi, > > On 01-03-17 12:27, Pali Rohár wrote: > >On Thursday 09 February 2017 16:44:11 Hans de Goede wrote: > >>There is no need to set the led_classdev's brightness value from > >>its set_brightness callback, this is taken care of by the led-core and > >>thinkpad_acpi really should not be mucking with it. > >> > >>Note that kbdlight_set_level_and_update() is still used by the old > >>thinpad_acpi specific sysfs interface for the led, so we cannot > >>remove it. > >> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>--- > >>Changes in v8: > >>-New patch in v8 of this patch-set > >>--- > >> drivers/platform/x86/thinkpad_acpi.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > >> > >>diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > >>index cacb43f..0680bb3 100644 > >>--- a/drivers/platform/x86/thinkpad_acpi.c > >>+++ b/drivers/platform/x86/thinkpad_acpi.c > >>@@ -5095,8 +5095,6 @@ 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; > >>@@ -5164,7 +5162,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_and_update(data->new_state); > >>+ kbdlight_set_level(data->new_state); > >> } > >> > >> static void kbdlight_sysfs_set(struct led_classdev *led_cdev, > > > >Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save > >kbdlight state on suspend and restore it on resume). It is really OK to > >revert that change? > > Yes, as explained on the commit msg, kbdlight_set_worker is only used > from ledclass callbacks and the led core already update led_cdev->brightness > when calling those. Ok, I wrote it because of adding Marco to discussion so he has chance to comment this change -- as it is basically revert of his commit...
HI, On 01-03-17 13:00, Pali Rohár wrote: > On Wednesday 01 March 2017 12:57:42 Hans de Goede wrote: >> Hi, >> >> On 01-03-17 12:27, Pali Rohár wrote: >>> On Thursday 09 February 2017 16:44:11 Hans de Goede wrote: >>>> There is no need to set the led_classdev's brightness value from >>>> its set_brightness callback, this is taken care of by the led-core and >>>> thinkpad_acpi really should not be mucking with it. >>>> >>>> Note that kbdlight_set_level_and_update() is still used by the old >>>> thinpad_acpi specific sysfs interface for the led, so we cannot >>>> remove it. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Changes in v8: >>>> -New patch in v8 of this patch-set >>>> --- >>>> drivers/platform/x86/thinkpad_acpi.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >>>> index cacb43f..0680bb3 100644 >>>> --- a/drivers/platform/x86/thinkpad_acpi.c >>>> +++ b/drivers/platform/x86/thinkpad_acpi.c >>>> @@ -5095,8 +5095,6 @@ 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; >>>> @@ -5164,7 +5162,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_and_update(data->new_state); >>>> + kbdlight_set_level(data->new_state); >>>> } >>>> >>>> static void kbdlight_sysfs_set(struct led_classdev *led_cdev, >>> >>> Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save >>> kbdlight state on suspend and restore it on resume). It is really OK to >>> revert that change? >> >> Yes, as explained on the commit msg, kbdlight_set_worker is only used >> from ledclass callbacks and the led core already update led_cdev->brightness >> when calling those. > > Ok, I wrote it because of adding Marco to discussion so he has chance to > comment this change -- as it is basically revert of his commit... No it is not, it removes a one line change he made amongst many others, the important parts of his commit are in other parts of it. Regards, Hans
2017-03-01 12:27 GMT+01:00 Pali Rohár <pali.rohar@gmail.com>: >> @@ -5164,7 +5162,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_and_update(data->new_state); >> + kbdlight_set_level(data->new_state); >> } >> >> static void kbdlight_sysfs_set(struct led_classdev *led_cdev, > > Hi! This change was added in commit afcedebc6a0 (thinkpad_acpi: save > kbdlight state on suspend and restore it on resume). It is really OK to > revert that change? > > CCing Marco who is author of that commit. I think that as per commit c685e20df this is actually not needed anymore, as it will be userspace to handle that. So it's fine to me. Thanks for asking, though
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index cacb43f..0680bb3 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -5095,8 +5095,6 @@ 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; @@ -5164,7 +5162,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_and_update(data->new_state); + kbdlight_set_level(data->new_state); } static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
There is no need to set the led_classdev's brightness value from its set_brightness callback, this is taken care of by the led-core and thinkpad_acpi really should not be mucking with it. Note that kbdlight_set_level_and_update() is still used by the old thinpad_acpi specific sysfs interface for the led, so we cannot remove it. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v8: -New patch in v8 of this patch-set --- drivers/platform/x86/thinkpad_acpi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)