Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360557 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] fujitsu-laptop: Support radio LED Jonathan Woithe <jwoithe@just42.net> - 2016-03-18 13:10 +0100
    Re: [PATCH] fujitsu-laptop: Support radio LED Michał Kępień <kernel@kempniu.pl> - 2016-03-22 14:40 +0100
      Re: [PATCH] fujitsu-laptop: Support radio LED Michał Kępień <kernel@kempniu.pl> - 2016-03-23 09:00 +0100
      Re: [PATCH] fujitsu-laptop: Support radio LED Jonathan Woithe <jwoithe@just42.net> - 2016-03-24 12:40 +0100
      Re: [PATCH] fujitsu-laptop: Support radio LED Darren Hart <dvhart@infradead.org> - 2016-03-28 19:50 +0200

#1360557 — 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?? <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

[toc] | [next] | [standalone]

#1362748

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

[toc] | [prev] | [next] | [standalone]

#1363231

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

[toc] | [prev] | [next] | [standalone]

#1364099

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

[toc] | [prev] | [next] | [standalone]

#1365606

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web