Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756807AbcDLMj0 (ORCPT ); Tue, 12 Apr 2016 08:39:26 -0400 Received: from server.atrad.com.au ([150.101.241.2]:35589 "EHLO server.atrad.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756685AbcDLMjY (ORCPT ); Tue, 12 Apr 2016 08:39:24 -0400 Date: Tue, 12 Apr 2016 22:06:34 +0930 From: Jonathan Woithe To: Darren Hart Cc: Micha?? K??pie?? , platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] fujitsu-laptop: Support radio LED Message-ID: <20160412123634.GA8195@marvin.atrad.com.au> References: <1458127687-25366-1-git-send-email-kernel@kempniu.pl> <20160318120418.GA29889@marvin.atrad.com.au> <20160322133051.GA3379@eudyptula.hq.kempniu.pl> <20160324113513.GA5129@marvin.atrad.com.au> <20160410023020.GA27081@dvhart-mobl5.amr.corp.intel.com> <20160410105258.GA2922@marvin.atrad.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160410105258.GA2922@marvin.atrad.com.au> User-Agent: Mutt/1.5.23 (2014-03-12) X-MIMEDefang-action: accept Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7034 Lines: 185 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ń Acked-by: Jonathan Woithe 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. Regards jonathan From: Michał Kępień Lifebook E734/E744/E754 has a LED which the manual calls "radio components indicator". It should be lit when any radio transmitter is enabled. Its state can be read and set using ACPI (FUNC interface, RFKILL method). Since the Lifebook E734/E744/E754 only has a button (as compared to a slider) for enabling/disabling radio transmitters, I believe the LED in question is meant to indicate whether all radio transmitters are currently on or off. However, pressing the radio toggle button doesn't automatically change the hardware state of the transmitters: it looks like this machine relies on soft rfkill. As for detecting whether the LED is present on a given machine, I had to resort to educated guesswork. I assumed this LED is present on all devices which have a radio toggle button instead of a slider. My Lifebook E744 holds 0x01010001 in BTNI. By comparing the bits and buttons with those of a Lifebook E8420 (BTNI=0x000F0101, has a slider), I put my money on bit 24 as the indicator of the radio toggle button being present. Furthermore, bit 24 is also clear on the S7020 which does not have the toggle button or an RF LED. Figuring out how the LED is controlled was more deterministic as all it took was decompiling the DSDT and taking a look at method S000 (the RFKILL method of the FUNC interface). The LED control method implemented here is unsuitable for use with "heavy" LED triggers, like phy0rx. Once blinking frequency achieves a certain level, the system hangs. While it's not essential, it would be nice to initialize soft rfkill state of all radio transmitters to the value of RFSW upon boot. Note that this value is persisted between reboots until the battery is removed, but can be changed before the kernel is booted. I haven't found an rfkill function which would enable one to set all rfkill devices to a chosen initial soft state (note that fujitsu-laptop doesn't create any rfkill devices on its own). This is probably a task best delegated to userspace. I think this LED would best be driven by an inverted airplane mode LED trigger (as proposed by João Paulo Rechi Vita). As the code for that trigger is not yet merged, I refrained from setting the default_trigger field in struct led_classdev radio_led. Perhaps it's a candidate for a follow-up patch in the future. Signed-off-by: Michał Kępień Acked-by: Jonathan Woithe --- --- 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);