| Message ID | 1458127687-25366-1-git-send-email-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Mar 16, 2016 at 12:28:07PM +0100, Micha?? K??pie?? wrote: > 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). > > Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl> Wow, that is a comprehensive explanation. In principle the patch looks good, but I wonder whether the heuristics you have developed for button detection needs wider testing. I can test on my S7020 but only in a few days time (this week is a very busy week) which would give us one more data point. > First of all, this patch raises a couple of checkpatch warnings. The code on the whole reads well so I would be happy with it as is. Making it (and the existing code) fully compliant with checkpatch results in harder to read code - at least that was the consensus when it was initially merged, which is why it was left in the current state. Darren may have an alternative view on this though, in which case I'm happy to defer to his preference. > 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. The other question is how consistent the bit layout is across all devices which might make use of this driver. The set of potential devices spans nearly 10 years, and in many ways it would be surprising if the bit definitions were kept the same over that time. Testing would be the only way to get a feeling for that. If you could let me know how you went about acquiring the values on your machine I could try the exact same steps on the S7020 to see what we get. > 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. I think this would only be necessary for those machines with the RF button in place of the hard slider switch, right? > One last remark is that I think this LED would best be driven by an > inverted airplane mode LED trigger ... In addition to the button interaction, presumedly. > Perhaps it's a candidate for a follow-up patch in the future. Could be. > And finally, perhaps some of the remarks above belong in the commit > message for future reference. Please advise. I think so - there's useful information in there which would be particularly relevant if the button detection heuristics ever need to be revised. Due to the necessarily arbitrary feel of the detection logic a brief in-source comment may be justified too. 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
Hi! > Sadly, this implementation is unsuitable for use with "heavy" LED > triggers, like phy0rx. Once blinking frequency achieves a certain > level, the system hangs. I'm not sure how much of an issue this is as > I'm pretty sure other LEDs registered by fujitsu-laptop would also cause > a hang when assigned to a similar trigger as they are also controlled > using ACPI. Something to do with triggers working in interrupt context? Cc led people? > + > +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 > +}; Is the naming consistent with other drivers? Should there be default trigger so that it works out of the box? Best regards, Pavel -- 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
> Wow, that is a comprehensive explanation. In principle the patch looks > good, but I wonder whether the heuristics you have developed for button > detection needs wider testing. This is indeed my primary concern. > I can test on my S7020 but only in a few > days time (this week is a very busy week) which would give us one more data > point. That would be nice, thanks. The more we know, the better. > > First of all, this patch raises a couple of checkpatch warnings. > > The code on the whole reads well so I would be happy with it as is. Making > it (and the existing code) fully compliant with checkpatch results in harder > to read code - at least that was the consensus when it was initially merged, > which is why it was left in the current state. Darren may have an > alternative view on this though, in which case I'm happy to defer to his > preference. Thanks for the explanation. It's just something that crossed my mind. Darren, feel free to let me know if you would like to get this done. > > 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. > > The other question is how consistent the bit layout is across all devices > which might make use of this driver. The set of potential devices spans > nearly 10 years, and in many ways it would be surprising if the bit > definitions were kept the same over that time. Testing would be the only > way to get a feeling for that. My thoughts exactly. > If you could let me know how you went about > acquiring the values on your machine I could try the exact same steps on the > S7020 to see what we get. The BTNI value is printed to the kernel log buffer by acpi_fujitsu_hotkey_add(), so all it takes to retrieve it is: dmesg | grep BTNI > > 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. > > I think this would only be necessary for those machines with the RF button > in place of the hard slider switch, right? Yes. On the E8420 I tested, moving the slider switch to "off" position caused the Bluetooth device to be removed from the system altogether while iwlwifi reacted by hard-blocking phy0. > > One last remark is that I think this LED would best be driven by an > > inverted airplane mode LED trigger ... > > In addition to the button interaction, presumedly. I wanted to reach three objectives: 1) make the LED indicate current rfkill state by default, 2) allow rfkill state to be persisted between reboots on models with an rfkill button instead of a slider, preferably also ensuring /sys/devices/platform/fujitsu-laptop/radios is always consistent with actual rfkill state, 3) allow the user to freely repurpose the LED to their liking. To achieve all of the above, I decided to, respectively: 1) assign the LED to an "inverted airplane mode" trigger by default, 2) consult rfkill_state upon module initialization and set the soft rfkill state for all devices appropriately, 3) refrain from calling radio_led_set() and/or FUNC_RFKILL with argument 0x5 from any function inside fujitsu-laptop.c. The code which could make the first point happen is not yet merged, so for now the user would probably have to assign the desired trigger from userspace. I also failed to implement the second point within fujitsu-laptop, so I suggested delegating this task to userspace. Could you please explain how the solution you had on your mind compares to the above? Are your objectives in line with mine or am I barking up the wrong tree? > > And finally, perhaps some of the remarks above belong in the commit > > message for future reference. Please advise. > > I think so - there's useful information in there which would be particularly > relevant if the button detection heuristics ever need to be revised. Due to > the necessarily arbitrary feel of the detection logic a brief in-source > comment may be justified too. I'll give this some more thought after you test the patch on the S7020.
> > If you could let me know how you went about > > acquiring the values on your machine I could try the exact same steps on the > > S7020 to see what we get. > > The BTNI value is printed to the kernel log buffer by > acpi_fujitsu_hotkey_add(), so all it takes to retrieve it is: > > dmesg | grep BTNI > I forgot to write that the other value I suggested could perhaps be used to determine whether a radio toggle button is present on a given model (0x00020320 on a Lifebook E744) is the return value of: call_fext_func(FUNC_RFKILL, 0x0, 0x0, 0x0); It is stored in the rfkill_supported field of struct fujitsu_hotkey_t. You can also look it up in a DSDT dump. On a Lifebook E744: Method (S000, 3, Serialized) { Name (_T_0, Zero) // _T_x: Emitted by ASL Compiler Local0 = Zero While (One) { _T_0 = Arg0 If ((_T_0 == Zero)) { >> Local0 |= 0x00020000 Local0 |= 0x0200 Local0 |= 0x0100 Local0 |= 0x20 } ... Break } Return (Local0) } On an E8420: Method (S000, 3, NotSerialized) { Local0 = 0x80000000 If ((Arg0 == 0x00)) { Local0 = Zero Local0 |= 0x20 Local0 |= 0x0100 Local0 |= 0x0200 } ... Return (Local0) }
This is a quick reply with preliminary information. I'll follow up in the next few days with further details. On Tue, Mar 22, 2016 at 02:30:51PM +0100, Micha?? K??pie?? wrote: > > > 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. > > > > The other question is how consistent the bit layout is across all devices > > which might make use of this driver. The set of potential devices spans > > nearly 10 years, and in many ways it would be surprising if the bit > > definitions were kept the same over that time. Testing would be the only > > way to get a feeling for that. > > My thoughts exactly. > > > If you could let me know how you went about > > acquiring the values on your machine I could try the exact same steps on the > > S7020 to see what we get. > > The BTNI value is printed to the kernel log buffer by > acpi_fujitsu_hotkey_add(), so all it takes to retrieve it is: > > dmesg | grep BTNI Here's what's reported by the S7020: fujitsu_laptop: BTNI: [0xf0001] The S7020 doesn't have any LEDs. It also has a physical slider to enable RF and an "RF enabled" indicator in the LCD panel. The LCD indicator is under hardware control; software cannot influence it. Clearly bit 24 is *not* set on the S7020. Using this bit as a test for the button's presence therefore should not cause trouble for the S7020 and probably other similar models from that time. Obviously we don't have access to every single model, but the apparent consistency back to the S7020 is encouraging. > > > 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. > > > > I think this would only be necessary for those machines with the RF button > > in place of the hard slider switch, right? > > Yes. On the E8420 I tested, moving the slider switch to "off" position > caused the Bluetooth device to be removed from the system altogether > while iwlwifi reacted by hard-blocking phy0. I haven't noticed anything that dramatic on the S7020, but anything's possible. 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 Tue, Mar 22, 2016 at 02:30:51PM +0100, Micha? K?pie? wrote: ... > > > First of all, this patch raises a couple of checkpatch warnings. > > > > The code on the whole reads well so I would be happy with it as is. Making > > it (and the existing code) fully compliant with checkpatch results in harder > > to read code - at least that was the consensus when it was initially merged, > > which is why it was left in the current state. Darren may have an > > alternative view on this though, in which case I'm happy to defer to his > > preference. > > Thanks for the explanation. It's just something that crossed my mind. > > Darren, feel free to let me know if you would like to get this done. I primarily care about Errors getting fixed, Warnings we take on a case by case basis, but err on the side of legibility. In the case of a driver with an active maintainer like Johnathan, I also weigh their input heavily. I haven't applied it yet, so if I see something particularly concerning, I'll raise it at that point.
On Thu, Mar 24, 2016 at 10:05:14PM +1030, Jonathan Woithe wrote: > This is a quick reply with preliminary information. I'll follow up in the > next few days with further details. > > On Tue, Mar 22, 2016 at 02:30:51PM +0100, Micha?? K??pie?? wrote: > > > > 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. > > > > > > The other question is how consistent the bit layout is across all devices > > > which might make use of this driver. The set of potential devices spans > > > nearly 10 years, and in many ways it would be surprising if the bit > > > definitions were kept the same over that time. Testing would be the only > > > way to get a feeling for that. > > > > My thoughts exactly. > > > > > If you could let me know how you went about > > > acquiring the values on your machine I could try the exact same steps on the > > > S7020 to see what we get. > > > > The BTNI value is printed to the kernel log buffer by > > acpi_fujitsu_hotkey_add(), so all it takes to retrieve it is: > > > > dmesg | grep BTNI > > Here's what's reported by the S7020: > > fujitsu_laptop: BTNI: [0xf0001] > > The S7020 doesn't have any LEDs. It also has a physical slider to enable RF > and an "RF enabled" indicator in the LCD panel. The LCD indicator is under > hardware control; software cannot influence it. > > Clearly bit 24 is *not* set on the S7020. Using this bit as a test for the > button's presence therefore should not cause trouble for the S7020 and > probably other similar models from that time. Obviously we don't have > access to every single model, but the apparent consistency back to the S7020 > is encouraging. > > > > > 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. > > > > > > I think this would only be necessary for those machines with the RF button > > > in place of the hard slider switch, right? > > > > Yes. On the E8420 I tested, moving the slider switch to "off" position > > caused the Bluetooth device to be removed from the system altogether > > while iwlwifi reacted by hard-blocking phy0. > > I haven't noticed anything that dramatic on the S7020, but anything's > possible. 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? Micha?, do you have plans for a v2?
Hi Darren Thanks for the ping. On Sat, Apr 09, 2016 at 07:30:20PM -0700, Darren Hart wrote: > On Thu, Mar 24, 2016 at 10:05:14PM +1030, Jonathan Woithe wrote: > > This is a quick reply with preliminary information. I'll follow up in the > > next few days with further details. > > > > On Tue, Mar 22, 2016 at 02:30:51PM +0100, Micha?? K??pie?? wrote: > > > > > 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. > > > > > > > > The other question is how consistent the bit layout is across all devices > > > > which might make use of this driver. The set of potential devices spans > > > > nearly 10 years, and in many ways it would be surprising if the bit > > > > definitions were kept the same over that time. Testing would be the only > > > > way to get a feeling for that. > > > > > > My thoughts exactly. > > > > > > > If you could let me know how you went about > > > > acquiring the values on your machine I could try the exact same steps on the > > > > S7020 to see what we get. > > > > > > The BTNI value is printed to the kernel log buffer by > > > acpi_fujitsu_hotkey_add(), so all it takes to retrieve it is: > > > > > > dmesg | grep BTNI > > > > Here's what's reported by the S7020: > > > > fujitsu_laptop: BTNI: [0xf0001] > > > > The S7020 doesn't have any LEDs. It also has a physical slider to enable RF > > and an "RF enabled" indicator in the LCD panel. The LCD indicator is under > > hardware control; software cannot influence it. > > > > Clearly bit 24 is *not* set on the S7020. Using this bit as a test for the > > button's presence therefore should not cause trouble for the S7020 and > > probably other similar models from that time. Obviously we don't have > > access to every single model, but the apparent consistency back to the S7020 > > is encouraging. > > > > > > > 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. > > > > > > > > I think this would only be necessary for those machines with the RF button > > > > in place of the hard slider switch, right? > > > > > > Yes. On the E8420 I tested, moving the slider switch to "off" position > > > caused the Bluetooth device to be removed from the system altogether > > > while iwlwifi reacted by hard-blocking phy0. > > > > I haven't noticed anything that dramatic on the S7020, but anything's > > possible. > > 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. > Micha??, do you have plans for a v2? I wasn't clear on this myself. I suspect, given the lack of a v2 in the time since the last discussion, that there is no v2 planned at this stage and I will proceed with my testing accordingly. 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
> > 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. > > > Micha?, do you have plans for a v2? > > I wasn't clear on this myself. I suspect, given the lack of a v2 in the > time since the last discussion, that there is no v2 planned at this stage > and I will proceed with my testing accordingly. Jonathan, in one of my previous messages [1] I laid out my motivation for implementing this patch the way I did. In your response [2], you wrote: > This is a quick reply with preliminary information. I'll follow up in the > next few days with further details. Thus, I have been patiently awaiting your response ever since as I see no reason to rush this patch. Darren, even if Jonathan is happy with v1 code-wise, I was planning to post a v2 with an improved commit message (as hinted before [1]). Yet, the contents of that message may depend on the results of Jonathan's tests. The logical path forward for me would be to wait until Jonathan finds time to respond to the issues I raised and only then post a v2 with at least an updated commit message. Let me know if you would like to handle this differently. [1] https://www.spinics.net/lists/platform-driver-x86/msg08657.html [2] https://www.spinics.net/lists/platform-driver-x86/msg08662.html
Pavel, Either your clock is really off or it took you 3 weeks to get this message out ;) Just letting you know. > > + > > +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 > > +}; > > Is the naming consistent with other drivers? I am not entirely clear what you are referring to. If it is the double colon, that seems to be the convention used throughout the platform-driver-x86 tree. If it is the LED's name ("radio_led"), I failed to find a similarly purposed LED in the platform-driver-x86 tree with a name I could reuse. I decided to use the _led suffix to differentiate this LED from the "lamps" already implemented by fujitsu-laptop. > Should there be default trigger so that it works out of the box? I have covered this issue in the lengthy comment attached to this patch: > One last remark is that 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. I haven't found a way to make this work the intended way out of the box, not with the currently available set of LED triggers. That being said, I would be happy if someone proved me wrong.
Our posts have crossed in transit. On Tue, Apr 12, 2016 at 02:03:23PM +0200, Micha?? K??pie?? wrote: > > > 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. > > > > > Micha??, do you have plans for a v2? > > > > I wasn't clear on this myself. I suspect, given the lack of a v2 in the > > time since the last discussion, that there is no v2 planned at this stage > > and I will proceed with my testing accordingly. > > Jonathan, in one of my previous messages [1] I laid out my motivation > for implementing this patch the way I did. In your response [2], you > wrote: > > > This is a quick reply with preliminary information. I'll follow up in the > > next few days with further details. > > Thus, I have been patiently awaiting your response ever since as I see > no reason to rush this patch. Thanks - I appreciate that. My response is in the post I sent a few minutes ago. Code-wise I don't see a problem: it doesn't regress on the S7020 so I think we're good to go in that respect. > Darren, even if Jonathan is happy with v1 code-wise, I was planning to > post a v2 with an improved commit message (as hinted before [1]). Yet, > the contents of that message may depend on the results of Jonathan's > tests. No problem. You can use my posted suggestions as they are or edit as appropriate. Note the suggested comment in the code regarding the use of BTNI bit 24 - again, feel free to use it as is or make changes as you see fit. Darren: I'm happy to wait for the v2 patch. A couple of further comments based on your original explanation: > Note that when called with Arg0 set to 0x00, S000 returns 0x00020320 on > a Lifebook E744 (this is the value saved in the rfkill_supported field > of struct fujitsu_hotkey_t). Bit 16 is not set on a Lifebook E8420, so > it might mean that this is an indicator of a radio toggle button being > present. I feel that the BTNI bit 24 route is better at this stage: the above seems to require more assumptions which we are not in a position to definitely prove. > 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). Is this possible/desired or > should this task simply be delegated to userspace? Agreed that this would be nice. As you said though, without the rfkill function it could be difficult to do. In any case, addressing this can be left for a follow up patch. I have no objection to the idea if you were to discover a way to do this. > One last remark is that 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. Agreed. 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
> > Darren, even if Jonathan is happy with v1 code-wise, I was planning to > > post a v2 with an improved commit message (as hinted before [1]). Yet, > > the contents of that message may depend on the results of Jonathan's > > tests. > > No problem. You can use my posted suggestions as they are or edit as > appropriate. Note the suggested comment in the code regarding the use of > BTNI bit 24 - again, feel free to use it as is or make changes as you see > fit. Darren: I'm happy to wait for the v2 patch. Jonathan, thanks so much for taking the time to edit the commit message. I like the end result, so if you're happy with it as it is, so am I. Darren, feel free to use the version of this patch suggested by Jonathan. I am at your disposal in case you'd like me to take any further action regarding this patch.
Hi! > > Either your clock is really off or it took you 3 weeks to get this > message out ;) Just letting you know. Clock is off. > > > + > > > +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 > > > +}; > > > > Is the naming consistent with other drivers? > > I am not entirely clear what you are referring to. If it is the double > colon, that seems to be the convention used throughout the > platform-driver-x86 tree. If it is the LED's name ("radio_led"), I > failed to find a similarly purposed LED in the platform-driver-x86 tree > with a name I could reuse. I decided to use the _led suffix to > differentiate this LED from the "lamps" already implemented by > fujitsu-laptop. I'd expected the led to be called "fujitsu::rfkill" but it looks that you are first one in tree with something similar, so I guess you get to pick the name. It would be nice to have easily-available list of all the suffixes. We have keyboard backlights, keyboard frontlights, LED flashes, ... > > Should there be default trigger so that it works out of the box? > > I have covered this issue in the lengthy comment attached to this patch: > > > One last remark is that 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. > > I haven't found a way to make this work the intended way out of the box, > not with the currently available set of LED triggers. That being said, > I would be happy if someone proved me wrong. Aha, ok. Thanks, Pavel
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index ffc84cc..7813e482 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -107,6 +107,7 @@ #define KEYBOARD_LAMPS 0x100 #define LOGOLAMP_POWERON 0x2000 #define LOGOLAMP_ALWAYS 0x4000 +#define RADIO_LED_ON 0x20 #endif /* Hotkey details */ @@ -174,6 +175,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; @@ -200,6 +202,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 @@ -275,6 +287,15 @@ static void kblamps_set(struct led_classdev *cdev, 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; @@ -299,6 +320,16 @@ static enum led_brightness kblamps_get(struct led_classdev *cdev) 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 */ @@ -895,6 +926,17 @@ static int acpi_fujitsu_hotkey_add(struct acpi_device *device) result); } } + + 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; @@ -921,6 +963,9 @@ static int acpi_fujitsu_hotkey_remove(struct acpi_device *device) 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);
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). Signed-off-by: Micha? K?pie? <kernel@kempniu.pl> --- First of all, this patch raises a couple of checkpatch warnings. I opted for consistency with existing code, which checkpatch doesn't like as well. Please let me know if you'd like me to fix the warnings (if that's desired, I could also clean up the whole driver to make checkpatch happy). Now, about the LED itself. As 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 change the hardware state of the transmitters. Consider the following scenario: 1. Power the machine up. Radio LED is on. 2. Press the radio toggle button before the bootloader kicks in. 3. Radio LED is turned off. 4. Wait for Linux to boot. Reading /sys/devices/platform/fujitsu-laptop/radios then yields "killed", but you can still connect to Bluetooth devices and wireless networks. So it looks like this machine only 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. I might be completely wrong and this needs testing on a broader set of devices. See also three paragraphs below for an alternative. 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). Here is the relevant part: Method (S000, 3, Serialized) { Name (_T_0, Zero) // _T_x: Emitted by ASL Compiler Local0 = Zero While (One) { _T_0 = Arg0 If ((_T_0 == Zero)) { Local0 |= 0x00020000 Local0 |= 0x0200 Local0 |= 0x0100 Local0 |= 0x20 } ... ElseIf ((_T_0 == 0x04)) { Arg1 = ((Arg2 << 0x10) | (Arg1 & 0xFFFF)) Local0 = FSMI (0x91, Arg0, Arg1) If ((Local0 & 0x20)) { RFSW = One } Else { RFSW = Zero } Local0 |= (RFSW << 0x05) Local0 |= (DKON << 0x09) Local0 |= (^^LID._LID () << 0x08) } ElseIf ((_T_0 == 0x05)) { If ((Arg1 & 0x20)) { If ((Arg2 & 0x20)) { RFSW = One } Else { RFSW = Zero } } Arg1 = ((Arg2 << 0x10) | (Arg1 & 0xFFFF)) Local0 = FSMI (0x91, Arg0, Arg1) } ... Break } Return (Local0) } When Arg0 is 0x04, current hardware state is queried and returned (this is already done by the driver). When Arg0 is 0x05 and Arg1 is 0x20, one can change the contents of RFSW (RF SWitch?), which eventually results in turning the LED on or off (depending on the value of Arg2). The 0x05 branch is not present in a DSDT dump from a Lifebook E8420, which has a slider instead of a radio toggle button. Note that when called with Arg0 set to 0x00, S000 returns 0x00020320 on a Lifebook E744 (this is the value saved in the rfkill_supported field of struct fujitsu_hotkey_t). Bit 16 is not set on a Lifebook E8420, so it might mean that this is an indicator of a radio toggle button being present. Sadly, this implementation is unsuitable for use with "heavy" LED triggers, like phy0rx. Once blinking frequency achieves a certain level, the system hangs. I'm not sure how much of an issue this is as I'm pretty sure other LEDs registered by fujitsu-laptop would also cause a hang when assigned to a similar trigger as they are also controlled using ACPI. 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). Is this possible/desired or should this task simply be delegated to userspace? One last remark is that 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. And finally, perhaps some of the remarks above belong in the commit message for future reference. Please advise. drivers/platform/x86/fujitsu-laptop.c | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)