lkml.org 
[lkml]   [2018]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
    On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
    > Hi,
    >
    > On 04-06-18 15:51, Daniel Drake wrote:
    > > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <hdegoede@redhat.com> wrote:
    > > > Is this really a case of the hardware itself processing the
    > > > keypress and then changing the brightness *itself* ?
    > > >
    > > > From the "[PATCH 2/2] platform/x86: asus-wmi: Add keyboard backlight
    > > > toggle support" patch I get the impression that the driver is
    > > > modifying the brightness from within the kernel rather then the
    > > > keyboard controller are ACPI embeddec-controller doing it itself.
    > > >
    > > > If that is the case then the right fix is for the driver to stop
    > > > mucking with the brighness itself, it should simply report the
    > > > right keyboard events and export a led interface and then userspace
    > > > will do the right thing (and be able to offer flexible policies
    > > > to the user).
    > >
    > > Before this modification, the driver reports the brightness keypresses
    > > to userspace and then userspace can respond by changing the brightness
    > > level, as you describe.
    > >
    > > You are right in that the hardware doesn't change the brightness
    > > directly itself, which is the normal usage of LED_BRIGHT_HW_CHANGED.
    > >
    > > However this approach was suggested by Benjamin Berg and Bastien
    > > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi: Add
    > > keyboard backlight toggle support
    > > https://marc.info/?l=linux-kernel&m=152639169210655&w=2
    > >
    > > The issue is that we need to support a new "keyboard backlight
    > > brightness cycle" key (in the patch that follows this one) which
    > > doesn't fit into any definitions of keys recognised by the kernel and
    > > likewise there's no userspace code to handle it.
    > >
    > > If preferred we could leave the standard brightness keys behaving as
    > > they are (input events) and make the new special key type directly
    > > handled by the kernel?
    >
    > I'm sorry that Benjamin and Bastien steered you in this direction,
    > IMHO none of it should be handled in the kernel.
    >
    > Anytime any sort of input is directly responded to by the kernel
    > it is a huge PITA to deal with from userspace. The kernel will have
    > a simplistic implementation which almost always is wrong.
    >
    > Benjamin, remember the pain we went through with rfkill hotkey
    > presses being handled in the kernel ?
    >
    > And then there is the whole acpi_video.brightness_switch_enabled
    > debacle, which is an option which defaults to true which causes
    > the kernel to handle LCD brightness key presses, which all distros
    > have been patching to default to off for ages.
    >
    > To give a concrete example, we may want to implement software
    > dimming / auto-off of the kbd backlight when the no keys are
    > touched for x seconds. This would seriously get in the way of that.
    >
    > So sorry, but NACK to this series.

    So if instead of modifying the LED value, the kernel platform drivers
    converted the TOGGLE into a cycle even by converting to an UP event
    based on awareness of the plaform specific max value and the read
    current value, leaving userspace to act on the TOGGLE/UP events - would
    that be preferable?

    Something like:

    if (code == TOGGLE && ledval < ledmax)
    code = UP;

    sparse_keymap_report_event(..., code, ...)

    }
    --
    Darren Hart
    VMware Open Source Technology Center

    \
     
     \ /
      Last update: 2018-06-05 04:31    [W:3.643 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site