On Tue, Mar 05, 2019 at 02:03:13PM -0600, joshua stein wrote:
> Here we go again...
>
> On at least the ThinkPad X1C6, the screen brightness keys (F5 and
> F6) do not work and "wsconsctl keyboard.backlight" doesn't report
> the correct value when the keyboard backlight is adjusted with
> Fn+Space.
>
> These are both caused by the default event mask not including these
> events, so explicitly enable them.
>
> But then acpithinkpad has to actually do something for the screen
> brightness keys, but it tries the very old CMOS method which doesn't
> work on these newer machines[0]. So make it use the ACPI method.
>
> I renamed thinkpad_[gs]et_backlight to thinkpad_[gs]et_kbd_backlight
> because it was confusing that they have nothing to do with screen
> backlight.
>
>
> 0. "newer machines" being those with MHKV reporting version 2. If
> this diff breaks on older "newer machines", this metric will have to
> be changed to something else.
>
Tested on an x240 and x270. Both work fine (actually fixes the x270).
Diff reads ok to me. OK claudio@
> Index: sys/dev/acpi/acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.61
> diff -u -p -u -p -r1.61 acpithinkpad.c
> --- sys/dev/acpi/acpithinkpad.c 1 Jul 2018 19:40:49 -0000 1.61
> +++ sys/dev/acpi/acpithinkpad.c 5 Mar 2019 20:00:23 -0000
> @@ -124,6 +124,10 @@
> #define THINKPAD_ADAPTIVE_MODE_HOME 1
> #define THINKPAD_ADAPTIVE_MODE_FUNCTION 3
>
> +#define THINKPAD_MASK_BRIGHTNESS_UP (1 << 15)
> +#define THINKPAD_MASK_BRIGHTNESS_DOWN (1 << 16)
> +#define THINKPAD_MASK_KBD_BACKLIGHT (1 << 17)
> +
> struct acpithinkpad_softc {
> struct device sc_dev;
>
> @@ -134,6 +138,8 @@ struct acpithinkpad_softc {
> struct ksensor sc_sens[THINKPAD_NSENSORS];
> struct ksensordev sc_sensdev;
>
> + uint64_t sc_hkey_version;
> +
> uint64_t sc_thinklight;
> const char *sc_thinklight_get;
> const char *sc_thinklight_set;
> @@ -161,8 +167,8 @@ int thinkpad_activate(struct device *, i
> /* wscons hook functions */
> void thinkpad_get_thinklight(struct acpithinkpad_softc *);
> void thinkpad_set_thinklight(void *, int);
> -int thinkpad_get_backlight(struct wskbd_backlight *);
> -int thinkpad_set_backlight(struct wskbd_backlight *);
> +int thinkpad_get_kbd_backlight(struct wskbd_backlight *);
> +int thinkpad_set_kbd_backlight(struct wskbd_backlight *);
> extern int (*wskbd_get_backlight)(struct wskbd_backlight *);
> extern int (*wskbd_set_backlight)(struct wskbd_backlight *);
> void thinkpad_get_brightness(struct acpithinkpad_softc *);
> @@ -284,6 +290,10 @@ thinkpad_attach(struct device *parent, s
>
> printf("\n");
>
> + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "MHKV", 0, NULL,
> + &sc->sc_hkey_version))
> + sc->sc_hkey_version = THINKPAD_HKEY_VERSION1;
> +
> #if NAUDIO > 0 && NWSKBD > 0
> /* Defer speaker mute */
> if (thinkpad_get_volume_mute(sc) == 1)
> @@ -299,14 +309,14 @@ thinkpad_attach(struct device *parent, s
> 0, NULL, &sc->sc_thinklight) == 0) {
> sc->sc_thinklight_get = "KLCG";
> sc->sc_thinklight_set = "KLCS";
> - wskbd_get_backlight = thinkpad_get_backlight;
> - wskbd_set_backlight = thinkpad_set_backlight;
> + wskbd_get_backlight = thinkpad_get_kbd_backlight;
> + wskbd_set_backlight = thinkpad_set_kbd_backlight;
> } else if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "MLCG",
> 0, NULL, &sc->sc_thinklight) == 0) {
> sc->sc_thinklight_get = "MLCG";
> sc->sc_thinklight_set = "MLCS";
> - wskbd_get_backlight = thinkpad_get_backlight;
> - wskbd_set_backlight = thinkpad_set_backlight;
> + wskbd_get_backlight = thinkpad_get_kbd_backlight;
> + wskbd_set_backlight = thinkpad_set_kbd_backlight;
> }
>
> if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> @@ -327,13 +337,19 @@ thinkpad_enable_events(struct acpithinkp
> int64_t mask;
> int i;
>
> - /* Get the supported event mask */
> + /* Get the default event mask */
> if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "MHKA",
> 0, NULL, &mask)) {
> printf("%s: no MHKA\n", DEVNAME(sc));
> return (1);
> }
>
> + /* Enable events we need to know about */
> + mask |= (THINKPAD_MASK_BRIGHTNESS_UP | THINKPAD_MASK_BRIGHTNESS_DOWN |
> + THINKPAD_MASK_KBD_BACKLIGHT);
> +
> + DPRINTF(("%s: setting event mask to 0x%llx\n", DEVNAME(sc), mask));
> +
> /* Update hotkey mask */
> bzero(args, sizeof(args));
> args[0].type = args[1].type = AML_OBJTYPE_INTEGER;
> @@ -380,6 +396,8 @@ thinkpad_hotkey(struct aml_node *node, i
> if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "MHKP",
> 0, NULL, &event))
> break;
> +
> + DPRINTF(("%s: event 0x%03llx\n", DEVNAME(sc), event));
> if (event == 0)
> break;
>
> @@ -535,13 +553,37 @@ thinkpad_volume_mute(struct acpithinkpad
> int
> thinkpad_brightness_up(struct acpithinkpad_softc *sc)
> {
> - return (thinkpad_cmos(sc, THINKPAD_CMOS_BRIGHTNESS_UP));
> + int b;
> +
> + if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION2) {
> + thinkpad_get_brightness(sc);
> + b = sc->sc_brightness & 0xff;
> + if (b < ((sc->sc_brightness >> 8) & 0xff)) {
> + sc->sc_brightness = b + 1;
> + thinkpad_set_brightness(sc, 0);
> + }
> +
> + return (0);
> + } else
> + return (thinkpad_cmos(sc, THINKPAD_CMOS_BRIGHTNESS_UP));
> }
>
> int
> thinkpad_brightness_down(struct acpithinkpad_softc *sc)
> {
> - return (thinkpad_cmos(sc, THINKPAD_CMOS_BRIGHTNESS_DOWN));
> + int b;
> +
> + if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION2) {
> + thinkpad_get_brightness(sc);
> + b = sc->sc_brightness & 0xff;
> + if (b > 0) {
> + sc->sc_brightness = b - 1;
> + thinkpad_set_brightness(sc, 0);
> + }
> +
> + return (0);
> + } else
> + return (thinkpad_cmos(sc, THINKPAD_CMOS_BRIGHTNESS_DOWN));
> }
>
> int
> @@ -620,7 +662,7 @@ thinkpad_set_thinklight(void *arg0, int
> }
>
> int
> -thinkpad_get_backlight(struct wskbd_backlight *kbl)
> +thinkpad_get_kbd_backlight(struct wskbd_backlight *kbl)
> {
> struct acpithinkpad_softc *sc = acpithinkpad_cd.cd_devs[0];
>
> @@ -637,7 +679,7 @@ thinkpad_get_backlight(struct wskbd_back
> }
>
> int
> -thinkpad_set_backlight(struct wskbd_backlight *kbl)
> +thinkpad_set_kbd_backlight(struct wskbd_backlight *kbl)
> {
> struct acpithinkpad_softc *sc = acpithinkpad_cd.cd_devs[0];
> int maxval;
> @@ -664,6 +706,8 @@ thinkpad_get_brightness(struct acpithink
> {
> aml_evalinteger(sc->sc_acpi, sc->sc_devnode,
> "PBLG", 0, NULL, &sc->sc_brightness);
> +
> + DPRINTF(("%s: %s: 0x%llx\n", DEVNAME(sc), __func__, sc->sc_brightness));
> }
>
> void
> @@ -672,11 +716,15 @@ thinkpad_set_brightness(void *arg0, int
> struct acpithinkpad_softc *sc = arg0;
> struct aml_value arg;
>
> + DPRINTF(("%s: %s: 0x%llx\n", DEVNAME(sc), __func__, sc->sc_brightness));
> +
> memset(&arg, 0, sizeof(arg));
> arg.type = AML_OBJTYPE_INTEGER;
> arg.v_integer = sc->sc_brightness & 0xff;
> aml_evalname(sc->sc_acpi, sc->sc_devnode,
> "PBLS", 1, &arg, NULL);
> +
> + thinkpad_get_brightness(sc);
> }
>
> int
>
--
:wq Claudio
