| Message ID | 20160412123634.GA8195@marvin.atrad.com.au (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, Apr 12, 2016 at 10:06:34PM +0930, Jonathan Woithe wrote: > Hi Darren > > On Sun, Apr 10, 2016 at 08:22:58PM +0930, Jonathan Woithe wrote: > > On Sat, Apr 09, 2016 at 07:30:20PM -0700, Darren Hart wrote: > > > Jonathan, Micha??, > > > > > > Where are we with this? The above reads as "Doesn't appear to break existing > > > systems on hand". Jonathan, are you happy with this patch? > > > > Sorry, I got caught up over the last couple of weeks with other tasks and > > have not yet managed to confirm the lack of regressions on the S7020. It > > was on my list for this coming week though. My comments quoted above were > > theoretical rather than based on an actual test. The patch appears fairly > > innoculous given that BTNI bit 24 is not set on the S7020 but for > > completeness I would prefer to give it a run on the S7020 before we merge > > the patch. I will make an effort to fit this in over the next couple of > > days. > > I have now tested the patch on the S7020 and unsurprisingly I have not > observed any regressions. I'm therefore happy to take the patch more or > less as is. However, I would like to see a brief comment in > acpi_fujitsu_hotkey_add() about the use of bit 24 in BTNI to determine > whether the LED handler should be registered since the reasoning is not > obvious and is not evident from the code. A copy of the original patch with > such a comment inserted (no code changes) is below. > > Signed-off-by: MichaÅ? KÄ?pieÅ? <kernel@kempniu.pl> Jonathan, please check your character set, a few mangled characters here which I have to fix up to use. UTF-8 seems to work reliably. Your headers currently include: Content-Type: text/plain; charset=iso-8859-1 > Acked-by: Jonathan Woithe <jwoithe@just42.net> > > MichaÅ? hasn't indicated that a revised patch is in the works so I'm fine if > you proceed with the one below. I've selected the relevant parts of his > preamble for inclusion in the commit message, but feel free to edit further > to your taste. > Yeah, tough call, some guess work involved here, and where we are uncertain, we should document it. I don't think we need to include bits about uncertain future plans or speculation on how things might be done. Keep it to what this patch does and any qualifiers a developer should be aware of. I've made a couple cosmetic changes and queued to for-next. Please review and let me know if you have any concerns.
On Thu, Apr 14, 2016 at 09:42:55PM -0700, Darren Hart wrote: > > Signed-off-by: MichaÅ? KÄ?pieÅ? <kernel@kempniu.pl> > > Jonathan, please check your character set, a few mangled characters here > which I have to fix up to use. UTF-8 seems to work reliably. Sorry about that. Normally UTF-8 stuff goes through just fine so I'm not sure what happened there. I'll admit I've never touched the default mutt charset settings though so perhaps there's a corner case that's not being handled. I'll look into it. > > Acked-by: Jonathan Woithe <jwoithe@just42.net> > > > > MichaÅ? hasn't indicated that a revised patch is in the works so I'm fine if > > you proceed with the one below. I've selected the relevant parts of his > > preamble for inclusion in the commit message, but feel free to edit further > > to your taste. > > Yeah, tough call, some guess work involved here, and where we are > uncertain, we should document it. I don't think we need to include bits > about uncertain future plans or speculation on how things might be done. > Keep it to what this patch does and any qualifiers a developer should be > aware of. I agree. Also note that a followup post from the original submitter indicated they were happy to go with what I proposed. > I've made a couple cosmetic changes and queued to for-next. Please review > and let me know if you have any concerns. Sure. Pardon my ignorance of such things, but what is the most straight-forward way to review the queued commit (preferrably without cloning an entire kernel git repo)? jonathan -- 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, Apr 15, 2016 at 03:03:33PM +0930, Jonathan Woithe wrote: > On Thu, Apr 14, 2016 at 09:42:55PM -0700, Darren Hart wrote: > > > Signed-off-by: MichaÅ? KÄ?pieÅ? <kernel@kempniu.pl> > > > > Jonathan, please check your character set, a few mangled characters here > > which I have to fix up to use. UTF-8 seems to work reliably. > > Sorry about that. Normally UTF-8 stuff goes through just fine so I'm not sure > what happened there. I'll admit I've never touched the default mutt charset > settings though so perhaps there's a corner case that's not being handled. > I'll look into it. > > > > Acked-by: Jonathan Woithe <jwoithe@just42.net> > > > > > > MichaÅ? hasn't indicated that a revised patch is in the works so I'm fine if > > > you proceed with the one below. I've selected the relevant parts of his > > > preamble for inclusion in the commit message, but feel free to edit further > > > to your taste. > > > > Yeah, tough call, some guess work involved here, and where we are > > uncertain, we should document it. I don't think we need to include bits > > about uncertain future plans or speculation on how things might be done. > > Keep it to what this patch does and any qualifiers a developer should be > > aware of. > > I agree. Also note that a followup post from the original submitter > indicated they were happy to go with what I proposed. > > > I've made a couple cosmetic changes and queued to for-next. Please review > > and let me know if you have any concerns. > > Sure. Pardon my ignorance of such things, but what is the most > straight-forward way to review the queued commit (preferrably without > cloning an entire kernel git repo)? You can see the tree in the MAINTAINERS file, but I suppose that doesn't make it obvious how to browse it. You can do that via the git web interface on infradead. The direct link to this patch is here: http://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git/commit/34b5199d2b69f9be731233af2425c53cc7ff995b
Hi Darren On Thu, Apr 14, 2016 at 10:44:52PM -0700, Darren Hart wrote: > On Fri, Apr 15, 2016 at 03:03:33PM +0930, Jonathan Woithe wrote: > > > I've made a couple cosmetic changes and queued to for-next. Please review > > > and let me know if you have any concerns. > > > > Sure. Pardon my ignorance of such things, but what is the most > > straight-forward way to review the queued commit (preferrably without > > cloning an entire kernel git repo)? > > You can see the tree in the MAINTAINERS file, but I suppose that doesn't > make it obvious how to browse it. You can do that via the git web > interface on infradead. Oh, there is a git-web interface on that server. I should have just followed my instinct. Apologies for the noise. > The direct link to this patch is here: > > http://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git/commit/34b5199d2b69f9be731233af2425c53cc7ff995b The commit looks fine to me. My only question is whether Michal should be credited explicitly as the author since the patch originated from him (I essentially just added some comments). You know the process better than me though so I'll leave it up to you. Regards jonathan -- 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, Apr 15, 2016 at 03:30:15PM +0930, Jonathan Woithe wrote: > Hi Darren > > On Thu, Apr 14, 2016 at 10:44:52PM -0700, Darren Hart wrote: > > On Fri, Apr 15, 2016 at 03:03:33PM +0930, Jonathan Woithe wrote: > > > > I've made a couple cosmetic changes and queued to for-next. Please review > > > > and let me know if you have any concerns. > > > > > > Sure. Pardon my ignorance of such things, but what is the most > > > straight-forward way to review the queued commit (preferrably without > > > cloning an entire kernel git repo)? > > > > You can see the tree in the MAINTAINERS file, but I suppose that doesn't > > make it obvious how to browse it. You can do that via the git web > > interface on infradead. > > Oh, there is a git-web interface on that server. I should have just > followed my instinct. Apologies for the noise. > > > The direct link to this patch is here: > > > > http://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git/commit/34b5199d2b69f9be731233af2425c53cc7ff995b > > The commit looks fine to me. My only question is whether Michal should be > credited explicitly as the author since the patch originated from him (I > essentially just added some comments). You know the process better than me > though so I'll leave it up to you. Indeed he should. Thank you for catching that. Corrected.
--- a/drivers/platform/x86/fujitsu-laptop.c 2016-03-14 14:58:54.000000000 +1030 +++ b/drivers/platform/x86/fujitsu-laptop.c 2016-04-12 22:39:39.940448329 +0930 @@ -107,6 +107,7 @@ #define KEYBOARD_LAMPS 0x100 #define LOGOLAMP_POWERON 0x2000 #define LOGOLAMP_ALWAYS 0x4000 +#define RADIO_LED_ON 0x20 #endif /* Hotkey details */ @@ -173,6 +174,7 @@ struct fujitsu_hotkey_t { int rfkill_state; int logolamp_registered; int kblamps_registered; + int radio_led_registered; }; static struct fujitsu_hotkey_t *fujitsu_hotkey; @@ -199,6 +201,16 @@ static struct led_classdev kblamps_led = .brightness_get = kblamps_get, .brightness_set = kblamps_set }; + +static enum led_brightness radio_led_get(struct led_classdev *cdev); +static void radio_led_set(struct led_classdev *cdev, + enum led_brightness brightness); + +static struct led_classdev radio_led = { + .name = "fujitsu::radio_led", + .brightness_get = radio_led_get, + .brightness_set = radio_led_set +}; #endif #ifdef CONFIG_FUJITSU_LAPTOP_DEBUG @@ -274,6 +286,15 @@ static void kblamps_set(struct led_class call_fext_func(FUNC_LEDS, 0x1, KEYBOARD_LAMPS, FUNC_LED_OFF); } +static void radio_led_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + if (brightness >= LED_FULL) + call_fext_func(FUNC_RFKILL, 0x5, RADIO_LED_ON, RADIO_LED_ON); + else + call_fext_func(FUNC_RFKILL, 0x5, RADIO_LED_ON, 0x0); +} + static enum led_brightness logolamp_get(struct led_classdev *cdev) { enum led_brightness brightness = LED_OFF; @@ -298,6 +319,16 @@ static enum led_brightness kblamps_get(s return brightness; } + +static enum led_brightness radio_led_get(struct led_classdev *cdev) +{ + enum led_brightness brightness = LED_OFF; + + if (call_fext_func(FUNC_RFKILL, 0x4, 0x0, 0x0) & RADIO_LED_ON) + brightness = LED_FULL; + + return brightness; +} #endif /* Hardware access for LCD brightness control */ @@ -893,6 +924,22 @@ static int acpi_fujitsu_hotkey_add(struc result); } } + + /* BTNI bit 24 seems to indicate the presence of a radio toggle + * button in place of a slide switch, and all such machines appear + * to also have an RF LED. Therefore use bit 24 as an indicator + * that an RF LED is present. + */ + if (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) { + result = led_classdev_register(&fujitsu->pf_device->dev, + &radio_led); + if (result == 0) { + fujitsu_hotkey->radio_led_registered = 1; + } else { + pr_err("Could not register LED handler for radio LED, error %i\n", + result); + } + } #endif return result; @@ -919,6 +966,9 @@ static int acpi_fujitsu_hotkey_remove(st if (fujitsu_hotkey->kblamps_registered) led_classdev_unregister(&kblamps_led); + + if (fujitsu_hotkey->radio_led_registered) + led_classdev_unregister(&radio_led); #endif input_unregister_device(input);