lkml.org 
[lkml]   [2019]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name
    From
    Date
    Hi Pavel,

    On 4/6/19 11:53 AM, Pavel Machek wrote:
    > Hi!
    >
    >> The patch set introduces also a set of predefined LED_FUNCTION
    >> names to be used in DT bindings. This along with the removal
    >> of devicename section from LED naming pattern will help to keep
    >> LED sysfs interface more uniform and not varying depending on
    >> underlaying hardware driving the LEDs.
    >>
    >> Regarding the problem discussed in this thread - I would not necessarily
    >> go for "platform" in place of devicename LED name section in the
    >> cros_kbd_led_backlight driver. If we change it (should we at all - it is
    >> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
    >> part. It believe it will be possible to retrieve this name with
    >> get_led_device_info.sh script. It would be good exercise to check
    >> it out.
    >
    > I am not sure about existing driver. Important thing for me is that
    > new drivers use consistent naming.
    >
    >> In cases like above:
    >>
    >> keyboardist::kbd_backlight
    >> tclnumpad::kbd_backlight
    >>
    >> we could do with the following:
    >>
    >> :kbd-backlight
    >> :numpad-backlight
    >>
    >> I used hyphens instead of underscores since we will have this convention
    >> in the LED_FUNCTION names, which is more common for Device Tree, and
    >> some of existing LED triggers.
    >
    > Existing userspace already searches for *:kbd_backlight", AFAICT, so
    > we probably want to keep the "_".

    OK, but it should be an exception but not a rule.
    This "kbd-*" naming is used in input and tty subsystems which register
    keyboard triggers with this style:

    ~/kernel$ git grep ".*[\":]kbd-" -- "*.c"
    drivers/input/input-leds.c: [LED_NUML] = { "numlock",
    VT_TRIGGER("kbd-numlock") },
    drivers/input/input-leds.c: [LED_CAPSL] = { "capslock",
    VT_TRIGGER("kbd-capslock") },
    drivers/input/input-leds.c: [LED_SCROLLL] = { "scrolllock",
    VT_TRIGGER("kbd-scrolllock") },
    drivers/input/input-leds.c: [LED_KANA] = { "kana",
    VT_TRIGGER("kbd-kanalock") },
    drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_SCROLLOCK,
    "kbd-scrolllock"),
    drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_NUMLOCK,
    "kbd-numlock"),
    drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_CAPSLOCK,
    "kbd-capslock"),
    drivers/tty/vt/keyboard.c: KBD_LED_TRIGGER(VC_KANALOCK,
    "kbd-kanalock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTLOCK,
    "kbd-shiftlock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_ALTGRLOCK,
    "kbd-altgrlock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLLOCK,
    "kbd-ctrllock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_ALTLOCK,
    "kbd-altlock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK,
    "kbd-shiftllock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK,
    "kbd-shiftrlock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLLLOCK,
    "kbd-ctrlllock"),
    drivers/tty/vt/keyboard.c: KBD_LOCKSTATE_TRIGGER(VC_CTRLRLOCK,
    "kbd-ctrlrlock"),

    "kbd_" naming is used only in case of backlight LEDs:

    ~/kernel$ git grep ".*[\":]kbd_" -- "*.c"
    drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name =
    "asus::kbd_backlight";
    drivers/hid/hid-google-hammer.c: kbd_backlight->cdev.name =
    "hammer::kbd_backlight";
    drivers/hwmon/applesmc.c: .name =
    "smc::kbd_backlight",
    drivers/input/misc/ims-pcu.c: "pcu%d::kbd_backlight",
    pcu->device_no);
    drivers/platform/chrome/cros_kbd_led_backlight.c: cdev->name =
    "chromeos::kbd_backlight";
    drivers/platform/x86/asus-laptop.c: cdev->name =
    "asus::kbd_backlight";
    drivers/platform/x86/asus-wmi.c: asus->kbd_led.name =
    "asus::kbd_backlight";
    drivers/platform/x86/dell-laptop.c: .name =
    "dell::kbd_backlight",
    drivers/platform/x86/samsung-laptop.c: samsung->kbd_led.name =
    "samsung::kbd_backlight";
    drivers/platform/x86/sony-laptop.c: kbdbl_ctl->mode_attr.attr.name =
    "kbd_backlight";
    drivers/platform/x86/sony-laptop.c:
    kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout";
    drivers/platform/x86/thinkpad_acpi.c: .name =
    "tpacpi::kbd_backlight",
    drivers/platform/x86/toshiba_acpi.c: dev->kbd_led.name =
    "toshiba::kbd_backlight";

    With LEDs in platform drivers is that problem that we have the name
    compiled into the kernel. Maybe to make it more flexible we could
    use kernel config to choose between new "kbd-" and legacy "kbd_"
    naming.

    > I don't care much if we use "platform:" or no prefix at all for
    > backlight of internal keyboard, as long as it is consistent across all
    > devices.
    >
    > We certainly want to use some prefix (probably inputX:) for backlight
    > on USB keyboards.

    For LEDs exposed through the input-LED bridge my get_led_device_info.sh
    script nicely reports:

    associated input node: input1

    It just does:

    readlink input1\:\:numlock/device

    which prints: "../../input1 "

    And I believe that for backlight LEDs exposed by input
    subsystem it should be similarly since the input device
    related to USB keyboard is a child of USB device:

    /sys/class/leds# readlink input1::numlock
    ../../devices/pci0000:00/0000:00:14.0/usb2/2-4/2-4:1.0/0003:1C4F:0002.0002/input/input1/input1::numlock

    --
    Best regards,
    Jacek Anaszewski

    \
     
     \ /
      Last update: 2019-04-06 16:17    [W:3.963 / U:1.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site