On Sun, Jan 10, 2016 at 04:59:22AM +0200, Sviatoslav Chagaev wrote: > Hi, > > I'm running -current on Apple MacBook Air 6,2. I installed using Jasper's > instructions [1], OpenBSD is the only OS and boots via EFI. > > I'm experiencing a problem with LCD backlight: on wakeup from suspend, the > backlight brightness is not restored and becomes unadjustable. If adjusted > down, > it turns off, if adjusted up, it goes to 100%.
I also own a MacBook Air 6,2 and wakeup does not really work at all for me, as the screen just stays black. Basically I see the same as reported in [5] for the MacBook Pro. > An Intel developer claims [2] that it's not a bug in Intel KMS driver and > people > claim [3] the problem goes away when booting in legacy BIOS mode. That makes no sense to me. If the problem goes aways when booting legacy BIOS, so it must be a bug when not, right? > It turns out [4], a numbers of MacBook models [5], including mine, have a > Texas > Instruments LP8550 LED backlight controller on I2C bus. By default, it's > controlled by external PWM. But it can be configured to be controlled by it's > own internal register instead. > > So below is a driver for TI LP8550. > > With this driver, after wake up from suspend, the brightness level is restored > and can be freely adjusted. I've tested the driver and while it compiles and attaches fine it does not work for me. After switching to console the screen stays black. > The catch is that Intel KMS driver seems to control the chip's ``EN'' pin > (basically an on/off pin). So whenever Intel KMS toggles the backlight, namely > on power management (e.g. `xset dpms force off`) and on Xorg <--> VT switch, > the chip's brightness control register gets reset and you need to > `wsconsctl display.backlight=<non_zero>` to turn the backlight back on. > > But it beats rebooting. Not for me, with my machine -current works better without your patch, because I can switch from X to console and back and brightness is correctly adjusted. > If there's any chance for it to be commited, please tell me what I need to > fix/improve to get it commited (so I don't have to reapply it every time I > upgrade). IMHO, instead of adding another driver to the mix, I would prefer this to be handled through the associated asmc(4) keys instead of accessing the chip directly. The SMC is supposed to be the central point for such manipulations. Unfortunately, the keys are not documented and need some non-trivial effort to be figured out. If this is not possible through asmc(4) and if your new driver improves the situation then I'm fine with importing it, but for now it is just worse. > Next step could be to somehow integrate it with Intel KMS. Yes, for me this is must-have before it can be committed. Some minor style comments on the code inline below. > [1] https://blog.jasper.la/openbsd-uefi-bootloader-howto/ > [2] https://bugs.freedesktop.org/show_bug.cgi?id=67454#c103 > [3] https://bugs.freedesktop.org/show_bug.cgi?id=67454#c58 > [4] https://bugs.freedesktop.org/show_bug.cgi?id=67454#c78 > [5] http://marc.info/?l=openbsd-misc&m=144988400031788&w=2 > > > > Index: share/man/man4/iic.4 > =================================================================== > RCS file: /cvs/src/share/man/man4/iic.4,v > retrieving revision 1.84 > diff -u -p -u -p -r1.84 iic.4 > --- share/man/man4/iic.4 27 Dec 2014 16:08:03 -0000 1.84 > +++ share/man/man4/iic.4 10 Jan 2016 01:52:55 -0000 > @@ -153,6 +153,8 @@ National Semiconductor LM87 temperature, > National Semiconductor LM93 temperature, voltage, and fan sensor > .It Xr lmtemp 4 > National Semiconductor LM75/LM76/LM77 temperature sensor > +.It Xr lpbl 4 > +Texas Instruments LP8550 LED backlight controller > .It Xr maxds 4 > Maxim DS1624/DS1631/DS1721 temperature sensor > .It Xr maxtmp 4 > Index: share/man/man4/lpbl.4 > =================================================================== > RCS file: share/man/man4/lpbl.4 > diff -N share/man/man4/lpbl.4 > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ share/man/man4/lpbl.4 10 Jan 2016 01:52:55 -0000 > @@ -0,0 +1,48 @@ > +.\" > +.\" Copyright (c) 2016 Sviatoslav Chagaev <sviatoslav.chag...@gmail.com> > +.\" > +.\" Permission to use, copy, modify, and distribute this software for any > +.\" purpose with or without fee is hereby granted, provided that the above > +.\" copyright notice and this permission notice appear in all copies. > +.\" > +.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > +.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > +.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > +.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > +.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > +.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > +.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > +.\" > +.Dd $Mdocdate: January 7 2016 $ > +.Dt LPBL 4 > +.Os > +.Sh NAME > +.Nm lpbl > +.Nd Texas Instruments LP8550 LED backlight controller > +.Sh SYNOPSIS > +.Cd "lpbl0 at iic?" > +.Sh DESCRIPTION > +The > +.Nm > +driver provides support for the LP8550 LED backlight controller, which can be > +found in various models of Intel based Apple laptops. > +.Pp > +The driver allows to control the brightness of LCD backlight through > +.Xr wsconsctl 8 > +variable > +.Va display.backlight . > +.Sh SEE ALSO > +.Xr intro 4 , > +.Xr iic 4 , ^ AFAIK, these should be sorted. > +.Xr wsconsctl 8 > +.Sh HISTORY > +The > +.Nm > +driver first appeared in > +.Ox 5.9 . > +.Sh AUTHORS > +.An -nosplit > +The > +.Nm > +driver was written by > +.An Sviatoslav Chagaev Aq Mt sviatoslav.chag...@gmail.com . > Index: sys/dev/i2c/files.i2c > =================================================================== > RCS file: /cvs/src/sys/dev/i2c/files.i2c,v > retrieving revision 1.51 > diff -u -p -u -p -r1.51 files.i2c > --- sys/dev/i2c/files.i2c 31 Mar 2013 13:30:24 -0000 1.51 > +++ sys/dev/i2c/files.i2c 10 Jan 2016 01:52:58 -0000 > @@ -123,6 +123,11 @@ device glenv > attach glenv at i2c > file dev/i2c/gl518sm.c glenv > > +# Texas Instruments LP8550 LED backlight controller > +device lpbl > +attach lpbl at i2c > +file dev/i2c/lp8550.c lpbl > + > # RICOH RS5C372[AB] Real Time Clock > device ricohrtc > attach ricohrtc at i2c > Index: sys/dev/i2c/i2c_scan.c > =================================================================== > RCS file: /cvs/src/sys/dev/i2c/i2c_scan.c,v > retrieving revision 1.145 > diff -u -p -u -p -r1.145 i2c_scan.c > --- sys/dev/i2c/i2c_scan.c 29 May 2015 00:37:10 -0000 1.145 > +++ sys/dev/i2c/i2c_scan.c 10 Jan 2016 01:52:58 -0000 > @@ -874,6 +874,8 @@ iic_probe_sensor(struct device *self, u_ > } else if ((addr == 0x2c || addr == 0x2d || addr == 0x2e) && > iicprobe(0x16) == 0x41 && ((iicprobe(0x17) & 0xf0) == 0x40)) { > name = "adm1026"; > + } else if (addr == 0x2c && ((iicprobe(0x03) >> 3) & 0xf) == 0xf) { > + name = "lp8550"; > } else if ((addr & 0x78) == 0x18 && iicprobew(0x06) == 0x1131 && > (iicprobew(0x07) & 0xfffc) == 0xa200) { > name = "se97"; /* or se97b */ > Index: sys/dev/i2c/lp8550.c > =================================================================== > RCS file: sys/dev/i2c/lp8550.c > diff -N sys/dev/i2c/lp8550.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ sys/dev/i2c/lp8550.c 10 Jan 2016 01:52:58 -0000 > @@ -0,0 +1,347 @@ > +/* > + * Copyright (c) 2015 Sviatoslav Chagaev <sviatoslav.chag...@gmail.com> > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ You may want to start with some comment describing the purpose of file/driver. > +#include <sys/param.h> > +#include <sys/systm.h> > +#include <sys/device.h> > + > +#include <dev/i2c/i2cvar.h> > +#include <dev/wscons/wsconsio.h> > + > +#ifdef LPBL_DEBUG > +#define DUMP_REGS(sc) lpbl_dump_registers(sc) > +#define DPRINTF(...) printf(__VA_ARGS__) > +#else > +#define DUMP_REGS(sc) > +#define DPRINTF(...) > +#endif > + > +#define LPBL_WRITE(reg, data) \ > + iic_smbus_write_byte(sc->sc_tag, sc->sc_addr, reg, data, 0) > +#define LPBL_READ(reg, data) \ > + iic_smbus_read_byte(sc->sc_tag, sc->sc_addr, reg, data, 0) > + > +/* Registers */ > +#define LP8550_BRIGHTNESS_CTL 0x00 > +#define LP8550_MIN_BRIGHTNESS 0 > +#define LP8550_MAX_BRIGHTNESS 0xff > +#define LP8550_DEVICE_CTL 0x01 > +#define LP8550_BRT_MODE_REG 0x4 /* Control brightness using register. */ > +#define LP8550_BL_CTL_ENABLE 0x1 /* Enable backlight. */ > +#define LP8550_FAULT 0x02 > +#define LP8550_ID 0x03 > +#define LP8550_DIRECT_CTL 0x04 > +#define LP8550_TEMP_MSB 0x05 > +#define LP8550_TEMP_LSB 0x06 > +#define LP8550_EEPROM_CTL 0x72 > + > +struct lpbl_softc { > + struct device sc_dev; > + > + i2c_tag_t sc_tag; > + i2c_addr_t sc_addr; > + > + u_int8_t brightness; /* Used to restore brightness level on wake up. */ I would use uint8_t here as it is supposed to be more modern style. > +}; > + > +int lpbl_match(struct device *, void *, void *); > +void lpbl_attach(struct device *, struct device *, void *); > +int lpbl_detach(struct device *, int); > +int lpbl_activate(struct device *, int); > +int lpbl_reset(struct lpbl_softc *); > +struct lpbl_softc *lpbl_get_softc(void); > +int lpbl_set(struct lpbl_softc *, u_int8_t); > +int lpbl_get(struct lpbl_softc *, u_int8_t *); > +#ifdef LPBL_DEBUG > +int lpbl_dump_registers(struct lpbl_softc *); > +#endif > + > +/* wsconsole hook functions */ > +int lpbl_get_wsparam(struct wsdisplay_param *p); > +int lpbl_set_wsparam(struct wsdisplay_param *p); > +extern int (*ws_get_param)(struct wsdisplay_param *); > +extern int (*ws_set_param)(struct wsdisplay_param *); > + > +struct cfattach lpbl_ca = { > + sizeof(struct lpbl_softc), > + lpbl_match, > + lpbl_attach, > + lpbl_detach, > + lpbl_activate > +}; ^ AFAIK this struct should be static. > +struct cfdriver lpbl_cd = { > + NULL, "lpbl", DV_DULL > +}; > + > +int > +lpbl_match(struct device *parent, void *match, void *aux) > +{ > + struct i2c_attach_args *ia = aux; > + > + if (strcmp(ia->ia_name, "lp8550") == 0) > + return 1; > + return 0; > +} > + > +void > +lpbl_attach(struct device *parent, struct device *self, void *aux) > +{ > + struct lpbl_softc *sc = (struct lpbl_softc *)self; > + struct i2c_attach_args *ia = aux; > + u_int8_t data; > + > + sc->sc_tag = ia->ia_tag; > + sc->sc_addr = ia->ia_addr; > + > + if (iic_acquire_bus(sc->sc_tag, 0) != 0) { > + printf(": cannot acquire bus\n"); > + return; > + } > + if (LPBL_READ(LP8550_ID, &data) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf(": cannot read id register\n"); > + return; > + } > + iic_release_bus(sc->sc_tag, 0); > + > + printf(": LP8550 rev 0x%02x\n", data & 0x7); > + > + DUMP_REGS(sc); > + > + if (lpbl_set(sc, LP8550_MAX_BRIGHTNESS) != 0) > + return; > + > + /* > + * XXX > + * Other drivers, e.g. acpivout(4), set these too, but earlier. > + */ > + ws_get_param = lpbl_get_wsparam; > + ws_set_param = lpbl_set_wsparam; > +} > + > +/* > + * Restore registers we touch to their default values. > + */ > +int > +lpbl_reset(struct lpbl_softc *sc) > +{ > + if (iic_acquire_bus(sc->sc_tag, 0) != 0) { > + printf("%s: cannot acquire I2C bus\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + > + if (LPBL_WRITE(LP8550_DIRECT_CTL, 0) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write direct ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + if (LPBL_WRITE(LP8550_DEVICE_CTL, 0) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write device ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + if (LPBL_WRITE(LP8550_BRIGHTNESS_CTL, 0)) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write brightness ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + > + iic_release_bus(sc->sc_tag, 0); > + > + return 0; > +} > + > +int > +lpbl_detach(struct device *self, int flags) > +{ > + struct lpbl_softc *sc = (struct lpbl_softc *)self; > + > + lpbl_reset(sc); > + > + return 0; > +} > + > +int > +lpbl_activate(struct device *self, int act) > +{ > + struct lpbl_softc *sc = (struct lpbl_softc *)self; > + > + DPRINTF("%s: %s(): act=%d\n", sc->sc_dev.dv_xname, __func__, act); > + > + switch (act) { > + case DVACT_WAKEUP: > + (void)lpbl_set(sc, sc->brightness); > + break; > + case DVACT_POWERDOWN: > + (void)lpbl_reset(sc); > + break; > + } > + > + return 0; > +} > + > +struct lpbl_softc * > +lpbl_first_softc(void) > +{ > + struct lpbl_softc *sc = NULL; > + int i; > + > + for (i = 0; i < lpbl_cd.cd_ndevs && sc == NULL; i++) { > + if (lpbl_cd.cd_devs[i] == NULL) > + continue; > + sc = (struct lpbl_softc *)lpbl_cd.cd_devs[i]; > + } > + return sc; > +} > + > +int > +lpbl_get_wsparam(struct wsdisplay_param *p) > +{ > + struct lpbl_softc *sc = NULL; > + u_int8_t value; > + > + sc = lpbl_first_softc(); > + if (sc == NULL) > + return -1; > + > + if (p->param != WSDISPLAYIO_PARAM_BACKLIGHT) > + return -1; > + > + DPRINTF("%s: %s()\n", sc->sc_dev.dv_xname, __func__); > + > + if (lpbl_get(sc, &value) != 0) > + return -1; > + p->min = LP8550_MIN_BRIGHTNESS; > + p->max = LP8550_MAX_BRIGHTNESS; > + p->curval = value; > + return 0; > +} > + > +int > +lpbl_set_wsparam(struct wsdisplay_param *p) > +{ > + struct lpbl_softc *sc = NULL; > + > + sc = lpbl_first_softc(); > + if (sc == NULL) > + return -1; > + > + if (p->param != WSDISPLAYIO_PARAM_BACKLIGHT) > + return -1; > + > + DPRINTF("%s: %s(): p->curval=%d\n", > + sc->sc_dev.dv_xname, __func__, p->curval); > + > + return lpbl_set(sc, (u_int8_t)p->curval); > +} > + > +/* > + * Set backlight brightness level. > + */ > +int > +lpbl_set(struct lpbl_softc *sc, u_int8_t value) > +{ > + if (iic_acquire_bus(sc->sc_tag, 0) != 0) { > + printf("%s: cannot acquire I2C bus\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + > + if (LPBL_WRITE(LP8550_BRIGHTNESS_CTL, value) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write brightness ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + if (LPBL_WRITE(LP8550_DIRECT_CTL, 0) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write direct ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + if (LPBL_WRITE(LP8550_DEVICE_CTL, > + LP8550_BRT_MODE_REG | LP8550_BL_CTL_ENABLE) != 0) { > + iic_release_bus(sc->sc_tag, 0); > + printf("%s: cannot write device ctl register\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + > + sc->brightness = value; > + > + iic_release_bus(sc->sc_tag, 0); > + > + return 0; > +} > + > +/* > + * Get backlight brightness level. > + */ > +int > +lpbl_get(struct lpbl_softc *sc, u_int8_t *valuep) > +{ > + *valuep = sc->brightness; > + > + return 0; > +} > + > +#ifdef LPBL_DEBUG > +int > +lpbl_dump_registers(struct lpbl_softc *sc) > +{ > + u_int8_t value; > + > + printf("%s: register dump:\n", > + sc->sc_dev.dv_xname); > + > + if (iic_acquire_bus(sc->sc_tag, 0) != 0) { > + printf("%s: cannot acquire I2C bus\n", > + sc->sc_dev.dv_xname); > + return -1; > + } > + > +#define Y(register) \ > + if (LPBL_READ(register, &value) != 0) { \ > + iic_release_bus(sc->sc_tag, 0); \ > + printf("%s: cannot read " #register " register\n", \ > + sc->sc_dev.dv_xname); \ > + return -1; \ > + } else { \ > + printf("%s: " #register ": 0x%02x\n", \ > + sc->sc_dev.dv_xname, value); \ > + } > + > + Y(LP8550_BRIGHTNESS_CTL); > + Y(LP8550_DEVICE_CTL); > + Y(LP8550_FAULT); > + Y(LP8550_ID); > + Y(LP8550_DIRECT_CTL); > + Y(LP8550_TEMP_MSB); > + Y(LP8550_TEMP_LSB); > + Y(LP8550_EEPROM_CTL); > + > +#undef Y > + > + iic_release_bus(sc->sc_tag, 0); > + > + return 0; > +} > +#endif /* LPBL_DEBUG */ > Index: sys/arch/amd64/conf/GENERIC > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/conf/GENERIC,v > retrieving revision 1.408 > diff -u -p -u -p -r1.408 GENERIC > --- sys/arch/amd64/conf/GENERIC 8 Jan 2016 09:48:41 -0000 1.408 > +++ sys/arch/amd64/conf/GENERIC 10 Jan 2016 01:52:59 -0000 > @@ -147,6 +147,7 @@ spdmem* at iic? # SPD memory eeproms > sdtemp* at iic? # SO-DIMM (JC-42.4) temperature > wbng* at iic? # Winbond W83793G > nvt* at iic? # Novoton W83795G > +lpbl0 at iic? # Texas Instruments LP8550 LED > backlight controller > > skgpio0 at isa? port 0x680 # Soekris net6501 GPIO and LEDs > gpio* at skgpio? >
