LinuxLists.cc - [PATCH] fujitsu-laptop: Support radio LED

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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?? <[email protected]>

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

2016-03-22 13:30:49

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

> 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.

--
Best regards,
Michał Kępień

2016-03-23 07:51:29

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

> > 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)
}

--
Best regards,
Michał Kępień

2016-03-24 11:38:12

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

2016-03-28 17:47:12

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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.
--
Darren Hart
Intel Open Source Technology Center

2016-04-10 02:33:21

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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?

--
Darren Hart
Intel Open Source Technology Center

2016-04-10 10:55:57

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

2016-04-10 18:29:12

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

2016-04-12 12:03:31

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

> > 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

--
Best regards,
Michał Kępień

2016-04-12 12:26:21

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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.

--
Best regards,
Michał Kępień

2016-04-12 12:39:26

[permalink] [raw]

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ń <[email protected]>
Acked-by: Jonathan Woithe <[email protected]>

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ń <[email protected]>

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ń <[email protected]>
Acked-by: Jonathan Woithe <[email protected]>
---
--- 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);

2016-04-12 12:49:57

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

2016-04-12 13:29:19

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

> > 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.

--
Best regards,
Michał Kępień

2016-04-14 12:39:49

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2016-04-15 04:43:04

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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ń <[email protected]>

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 <[email protected]>
>
> 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.

--
Darren Hart
Intel Open Source Technology Center

2016-04-15 05:36:20

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

On Thu, Apr 14, 2016 at 09:42:55PM -0700, Darren Hart wrote:
> > Signed-off-by: Michał Kępień <[email protected]>
>
> 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 <[email protected]>
> >
> > 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

2016-04-15 05:44:56

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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ń <[email protected]>
> >
> > 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 <[email protected]>
> > >
> > > 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

--
Darren Hart
Intel Open Source Technology Center

2016-04-15 06:00:51

[permalink] [raw]

Subject: Re: [PATCH] fujitsu-laptop: Support radio LED

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

2016-04-15 07:32:35