Message-ID: <20160412123634.GA8195@marvin.atrad.com.au>
Date:	Tue, 12 Apr 2016 22:06:34 +0930
From:	Jonathan Woithe <jwoithe@...t42.net>
To:	Darren Hart <dvhart@...radead.org>
Cc:	Micha?? K??pie?? <kernel@...pniu.pl>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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@...pniu.pl>
Acked-by: Jonathan Woithe <jwoithe@...t42.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.

Regards
  jonathan

From: Michał Kępień <kernel@...pniu.pl>

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ń <kernel@...pniu.pl>
Acked-by: Jonathan Woithe <jwoithe@...t42.net>
---
--- 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);

Powered by blists - more mailing lists