On Fri, Sep 08, 2017 at 05:01:03PM -0500, joshua stein wrote:
> Revision 1.50 of acpithinkpad.c made inteldrm defer to acpithinkpad
> for screen brightness adjustments instead of handling it natively.
> That was needed to avoid some synchronization issues on machines
> where the hardware buttons do the backlight adjustment on their own,
> and just notify acpithinkpad of the change.
>
> On brand new machines like the X1C5, the screen adjustment buttons
> don't do anything on their own and don't even notify acpithinkpad
> anymore, instead requiring a WMI driver to get notified. Since
> acpithinkpad is still forcing screen backlight changes to go through
> the proprietary ACPI interface, it is still limited to 10 levels of
> backlight adjustment which are oddly defined.
>
> I thought about just reverting 1.55 so that acpithinkpad wouldn't
> even bother attaching to these new machines which now use HID
> LEN0268, so that inteldrm could take over brightness adjustment and
> give you the full 100 levels of adjustment.
>
> However, controlling keyboard backlight through wscons still has to
> be done through acpithinkpad, so the driver still has to attach. As
> is, the driver was not getting notification of keyboard backlight
> changes either, so the "wsconsctl keyboard.backlight" value would
> get out of sync with the hardware if you adjusted the backlight with
> Fn+Space.
>
> This diff fixes the keyboard backlight events coming through, and
> also makes it not take over backlight adjustment on these new
> machines (LEN0268 and anything in the future).
>
> Eventually these machines will need a WMI driver to respond to other
> hardware keys and react accordingly, though many other kinds of
> laptops can also benefit from that driver.
>
> I'm not pushing to commit this before the lock, so it would be nice
> to have testing on a wide variety of ThinkPads to make sure the
> keyboard backlight change doesn't break anything.
No problems seen on my machines, but I only have one TP
with a keyboard backlight (helix2).
This diff is OK by me, though I have one small nit (see below).
> Index: sys/dev/acpi/acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.58
> diff -u -p -u -p -r1.58 acpithinkpad.c
> --- sys/dev/acpi/acpithinkpad.c 12 Aug 2017 17:33:51 -0000 1.58
> +++ sys/dev/acpi/acpithinkpad.c 8 Sep 2017 21:39:12 -0000
> @@ -41,6 +41,8 @@
> #define THINKPAD_HKEY_VERSION1 0x0100
> #define THINKPAD_HKEY_VERSION2 0x0200
>
> +#define THINKPAD_KEYLIGHT_MASK 0x20000
> +
> #define THINKPAD_CMOS_VOLUME_DOWN 0x00
> #define THINKPAD_CMOS_VOLUME_UP 0x01
> #define THINKPAD_CMOS_VOLUME_MUTE 0x02
> @@ -136,6 +138,7 @@ struct acpithinkpad_softc {
> const char *sc_thinklight_set;
>
> uint64_t sc_brightness;
> + int sc_fw_brightness;
> };
>
> extern void acpiec_read(struct acpiec_softc *, u_int8_t, int, u_int8_t *);
> @@ -195,6 +198,17 @@ const char *acpithinkpad_hids[] = {
> 0
> };
>
> +/*
> + * Older machines which need backlight control done in firmware/ACPI. Newer
> + * machines rely on inteldrm to do adjustments since hardware keys don't come
> + * through here.
> + */
In isolation it's not immediately obvious if the above comment is
talking about keyboard backlight or screen brightness.
s/backlight control/keyboard backlight control/ ?
> +const char *acpithinkpad_fw_hids[] = {
> + "IBM0068",
> + "LEN0068",
> + 0
> +};
> +
> int
> thinkpad_match(struct device *parent, void *match, void *aux)
> {
> @@ -272,6 +286,9 @@ thinkpad_attach(struct device *parent, s
> sc->sc_acpi = (struct acpi_softc *)parent;
> sc->sc_devnode = aa->aaa_node;
>
> + sc->sc_fw_brightness = acpi_matchhids(aa, acpithinkpad_fw_hids,
> + sc->sc_dev.dv_xname);
> +
> printf("\n");
>
> #if NAUDIO > 0 && NWSKBD > 0
> @@ -299,8 +316,8 @@ thinkpad_attach(struct device *parent, s
> wskbd_set_backlight = thinkpad_set_backlight;
> }
>
> - if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> - 0, NULL, &sc->sc_brightness) == 0) {
> + if (sc->sc_fw_brightness && aml_evalinteger(sc->sc_acpi,
> + sc->sc_devnode, "PBLG", 0, NULL, &sc->sc_brightness) == 0) {
> ws_get_param = thinkpad_get_param;
> ws_set_param = thinkpad_set_param;
> }
> @@ -323,6 +340,9 @@ thinkpad_enable_events(struct acpithinkp
> printf("%s: no MHKA\n", DEVNAME(sc));
> return (1);
> }
> +
> + /* Make sure keyboard backlight events are enabled */
> + mask |= THINKPAD_KEYLIGHT_MASK;
>
> /* Update hotkey mask */
> bzero(args, sizeof(args));
>
