Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-30 Thread Joerg Jung 
On Wed, Jan 27, 2016 at 05:20:59AM +0200, Sviatoslav Chagaev wrote:
> On Sun, 10 Jan 2016 21:28:31 +0100 Joerg Jung wrote:
> > On Sun, Jan 10, 2016 at 04:59:22AM +0200, Sviatoslav Chagaev wrote: 
> > > So below is a driver for TI LP8550.
> ...
> > > 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.
> > 
> 
> Using knowledge acquired from [1]

[1] contains some "wrong" (obsolete?) information, e.g. the Errors list.

> and [2] made a diff, posted below for
> reference, which adds:
>  * ioctl interface to asmc(4) to allow reading/writing of ``keys'' from
>userspace;
>  * companion smcprobe(1) which reads/writes ``keys'' using the ioctl;

Wow..., much effort for simple key dumping.  Please, find below an
(older/may not apply) diff which just dumps the keys directly from
kernel into dmesg.

Are you aware that keys can be all kind of types, e.g. structs?  Have
you seen the ASMC_INFO (0x13) command which might be used to determine
the type?  The key flags (read/write/atomic...) are mostly documented
somewhere.

>  * smcdumpkeys(1): scans an SMC textual firmware file and dumps the ``key''
>table that it contains.

Are you aware of the ASMC_INDEX (0x12) command, which can easily be used
to just iterate ALL keys based on index?

> Downloaded SMC FW [3], unpacked it using Google Drive and used smcdumpkeys(1) 
> +
> smcprobe(1) to create the table of keys and their current values before 
> suspend
> [4] and after wake up [5]. Diffed them [6], 

The diff approach is interesting... :) From your diff output I would try
to play with IPBF, MSXk, SAPN, and DM0T.

> filtering out keys which change over
> time as they are probably sensors (some might have gotten through).

If they start with:

'A'... usually ambient light sensor related,
'F'... fan sensors,
'T'... temperature sensors,
'P'... power related,
'V'... voltage sensors.

> Tried modifying all keys whose names start with 'B': no effect.

'B'... is likely not "backlight" but "battery".

> Tried modifying keys which are different after wake up: no effect.

Please find a more exhaustive/descriptive list of keys here:
http://web.archive.org/web/20151103144947/http://www.parhelia.ch/blog/statics/k3_keys.html

Have you seen this: https://github.com/gcsgithub/smc_util it contains
more details for the various structs.

You may also want to have a look into the existing FreeBSD and Linux
drivers.

> [1] 
> https://reverse.put.as/wp-content/uploads/2015/12/D1_02_Alex_Ninjas_and_Harry_Potter.pdf
> [2] 
> https://dev.inversepath.com/download/public/embedded_systems_exploitation.pdf
> [3] https://support.apple.com/kb/DL1748?locale=en_US
> [4] https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-before_suspend-txt
> [5] https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-after_wakeup-txt
> [6] 
> https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-suspend_wakeup_key-diff


Index: asmc.c
===
RCS file: /cvs/src/sys/dev/isa/asmc.c,v
retrieving revision 1.17
diff -u -p -r1.17 asmc.c
--- asmc.c  12 Dec 2015 12:34:05 -  1.17
+++ asmc.c  12 Dec 2015 13:30:12 -
@@ -32,6 +32,8 @@
 #include 
 #include 
 
+#define ASMC_DEBUG 1
+
 #define ASMC_BASE  0x300   /* SMC base address */
 #define ASMC_IOSIZE32  /* I/O region size 0x300-0x31f */
 
@@ -42,6 +44,7 @@
 
 #define ASMC_READ  0x10/* SMC read command */
 #define ASMC_WRITE 0x11/* SMC write command */
+#define ASMC_INDEX 0x12/* SMC index command */
 #define ASMC_INFO  0x13/* SMC info/type command */
 
 #define ASMC_RETRY 10
@@ -85,6 +88,9 @@ struct asmc_softc {
 };
 
 intasmc_try(struct asmc_softc *, int, const char *, uint8_t *, uint8_t);
+#ifdef ASMC_DEBUG
+void   asmc_dump(struct asmc_softc *, uint32_t);
+#endif
 
 void   asmc_init(void *);
 void   asmc_refresh(void *);
@@ -292,7 +298,9 @@ asmc_attach(struct device *parent, struc
}
printf(", %u key%s\n", ntohl(*(uint32_t *)buf),
(ntohl(*(uint32_t *)buf) == 1) ? "" : "s");
-
+#ifdef ASMC_DEBUG
+   asmc_dump(sc, ntohl(*(uint32_t *)buf));
+#endif
/* keyboard backlight led is optional */
sc->sc_kbdled = buf[0] = 127, buf[1] = 0;
if ((r = asmc_try(sc, ASMC_WRITE, "LKSB", buf, 2))) {
@@ -479,25 +487,25 @@ asmc_command(struct asmc_softc *sc, int 
if

Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-26 Thread Sviatoslav Chagaev 
On Sun, 10 Jan 2016 21:28:31 +0100 Joerg Jung wrote:
> On Sun, Jan 10, 2016 at 04:59:22AM +0200, Sviatoslav Chagaev wrote: 
> > So below is a driver for TI LP8550.
...
> > 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.
> 

Using knowledge acquired from [1] and [2] made a diff, posted below for
reference, which adds:
 * ioctl interface to asmc(4) to allow reading/writing of ``keys'' from
   userspace;
 * companion smcprobe(1) which reads/writes ``keys'' using the ioctl;
 * smcdumpkeys(1): scans an SMC textual firmware file and dumps the ``key''
   table that it contains.

Downloaded SMC FW [3], unpacked it using Google Drive and used smcdumpkeys(1) +
smcprobe(1) to create the table of keys and their current values before suspend
[4] and after wake up [5]. Diffed them [6], filtering out keys which change over
time as they are probably sensors (some might have gotten through).

Tried modifying all keys whose names start with 'B': no effect.
Tried modifying keys which are different after wake up: no effect.

[1] 
https://reverse.put.as/wp-content/uploads/2015/12/D1_02_Alex_Ninjas_and_Harry_Potter.pdf
[2] 
https://dev.inversepath.com/download/public/embedded_systems_exploitation.pdf
[3] https://support.apple.com/kb/DL1748?locale=en_US
[4] https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-before_suspend-txt
[5] https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-after_wakeup-txt
[6] 
https://gist.github.com/S010/26145f4446fcc0b8a0d6#file-suspend_wakeup_key-diff



Index: sys/dev/isa/asmc.c
===
RCS file: /cvs/src/sys/dev/isa/asmc.c,v
retrieving revision 1.28
diff -u -p -r1.28 asmc.c
--- sys/dev/isa/asmc.c  27 Dec 2015 20:54:53 -  1.28
+++ sys/dev/isa/asmc.c  27 Jan 2016 02:43:15 -
@@ -26,6 +26,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include 
 
@@ -705,4 +707,47 @@ asmc_update(void *arg)
if (!(sc->sc_sensor_motion[i].flags & SENSOR_FINVALID))
asmc_motion(sc, i, 0);
 #endif
+}
+
+int
+asmcopen(dev_t dev, int flag, int mode, struct proc *p)
+{
+   /* FIXME */
+   return 0;
+}
+
+int
+asmcclose(dev_t dev, int flag, int mode, struct proc *p)
+{
+   /* FIXME */
+   return 0;
+}
+
+/*
+ * An ioctl interface to poke SMC ``keys'' from userspace.
+ */
+int
+asmcioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p)
+{
+   struct asmc_softc *sc;
+   struct asmc_key_buf *buf = (struct asmc_key_buf *)addr;
+   int error;
+
+   sc = asmc_cd.cd_devs[minor(dev)];
+
+   switch (cmd) {
+   case ASMC_READ_KEY:
+   error = asmc_try(sc, ASMC_READ, buf->key, buf->data, buf->len);
+   break;
+   case ASMC_WRITE_KEY:
+   error = asmc_try(sc, ASMC_WRITE, buf->key, buf->data, buf->len);
+   break;
+   default:
+   return ENOTTY;
+   }
+
+   if (error)
+   return EIO;
+   else
+   return 0;
 }
Index: sys/dev/isa/asmc.h
===
RCS file: sys/dev/isa/asmc.h
diff -N sys/dev/isa/asmc.h
--- /dev/null   1 Jan 1970 00:00:00 -
+++ sys/dev/isa/asmc.h  27 Jan 2016 02:43:15 -
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2016 Sviatoslav Chagaev 
+ *
+ * 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.
+ */
+
+#ifndef _ASMC_H_
+#define _ASMC_H_
+
+#include 
+#include 
+
+struct asmc_key_buf {
+   char key[4];
+   size_t len;
+   uint8_t data[32];
+};
+
+#define ASMC_READ_KEY  _IOWR('a', 1, struct asmc_key_buf)
+#define ASMC_WRITE_KEY _IOWR('a', 2, struct asmc_key_buf)
+

Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-12 Thread Sviatoslav Chagaev 
On Tue, 12 Jan 2016 01:37:39 +0200
Sviatoslav Chagaev  wrote:
> On Sun, 10 Jan 2016 21:28:31 +0100
> Joerg Jung  wrote:
> > On Sun, Jan 10, 2016 at 04:59:22AM +0200, Sviatoslav Chagaev wrote:
> >  
> > > 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?
> 
> Judging by the code at e.g. sys/dev/pci/drm/i915/intel_panel.c:984, it
> seems Intel KMS driver expects BIOS or EFI to configure backlight,
> then reads these values and uses them itself.
> 
> I found documentation for Intel Haswell [1] and glanced over
> descriptions of relevant registers (BLC_PWM_CTL, BLC_PWM2_CTL,
> BLC_PWM_DATA, BLC_MISC_CTL, BLM_HIST_CTL, BLM_HIST_BIN, BLM_HIST_GUARD,
> UTIL_PIN_CTL, SBLC_PWM_CTL1, SBLC_PWM_CTL2), they are confusing...
> 
> Based on these, I agree, it might be a bug in Intel KMS.
> 
> I'll add debug prints and play with these registers, maybe I'll spot
> some incorrect values.
> 
> [1] 
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-commandreference-registers_0_0.pdf
> 

Had no luck with Intel KMS:

Added register dump to every backlight operation, see first diff.

State of registers I get from EFI:

pch_dump_backlight_registers(): pch_setup_backlight
MY_BLC_PWM_CTL= 0x; MY_BLC_PWM_DATA   = 0x
MY_BLC_PWM_DATA2  = 0x; MY_BLM_HIST_CTL   = 0x
MY_BLM_HIST_BIN   = 0x; MY_BLM_HIST_GUARD = 0x
MY_BLC_PWM2_CTL   = 0x6000; MY_BLC_MISC_CTL   = 0x
MY_UTIL_PIN_CTL   = 0x; MY_SBLC_PWM_CTL1  = 0xc000
MY_SBLC_PWM_CTL2  = 0x0ad901c3; 

State of registers after Intel KMS configures them:

pch_dump_backlight_registers(): pch_set_backlight
MY_BLC_PWM_CTL= 0xe000; MY_BLC_PWM_DATA   = 0x0ad9
MY_BLC_PWM_DATA2  = 0x; MY_BLM_HIST_CTL   = 0x0006
MY_BLM_HIST_BIN   = 0x; MY_BLM_HIST_GUARD = 0x
MY_BLC_PWM2_CTL   = 0x6000; MY_BLC_MISC_CTL   = 0x
MY_UTIL_PIN_CTL   = 0x; MY_SBLC_PWM_CTL1  = 0x8000
MY_SBLC_PWM_CTL2  = 0x0ad9; 

BLC_PWM_CTL change seems strange.
Modified pch_*_backlight() functions so they do not change the
configuration set by EFI, see second diff, but observed no change in
behaviour.


Index: sys/dev/pci/drm/i915/i915_reg.h
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/i915_reg.h,v
retrieving revision 1.11
diff -u -p -r1.11 i915_reg.h
--- sys/dev/pci/drm/i915/i915_reg.h 25 Sep 2015 16:15:19 -  1.11
+++ sys/dev/pci/drm/i915/i915_reg.h 12 Jan 2016 02:54:49 -
@@ -2493,6 +2493,18 @@
 #define   BACKLIGHT_DUTY_CYCLE_MASK_PNV(0xfffe)
 #define   BLM_POLARITY_PNV (1 << 0) /* pnv only */
 
+#define MY_BLC_PWM_CTL 0x48250
+#define MY_BLC_PWM_DATA0x48254
+#define MY_BLC_PWM_DATA2   0x48354
+#define MY_BLM_HIST_CTL0x48260
+#define MY_BLM_HIST_BIN0x48264
+#define MY_BLM_HIST_GUARD  0x48268
+#define MY_BLC_PWM2_CTL0x48350
+#define MY_BLC_MISC_CTL0x48360
+#define MY_UTIL_PIN_CTL0x48400
+#define MY_SBLC_PWM_CTL1   0xc8250
+#define MY_SBLC_PWM_CTL2   0xc8254
+
 #define BLC_HIST_CTL   (dev_priv->info->display_mmio_offset + 0x61260)
 
 /* New registers for PCH-split platforms. Safe where new bits show up, the
Index: sys/dev/pci/drm/i915/intel_panel.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_panel.c,v
retrieving revision 1.11
diff -u -p -r1.11 intel_panel.c
--- sys/dev/pci/drm/i915/intel_panel.c  23 Sep 2015 23:12:12 -  1.11
+++ sys/dev/pci/drm/i915/intel_panel.c  12 Jan 2016 02:54:49 -
@@ -36,6 +36,38 @@
 
 #define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
 
+void pch_dump_backlight_registers(struct intel_connector *connector, const 
char *msg)
+{
+   struct drm_device *dev = connector->base.dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   u32 i = 0;
+   u32 value;
+
+   printf("%s(): %s\n", __func__, msg);
+
+#define Y(register)\
+   value = I915_READ(register);\
+   printf("%-17s = 0x%08x%s",  \
+   #register, value, (i++ & 1) ? "\n" : "; ");
+
+   Y(MY_BLC_PWM_CTL);
+   Y(MY_BLC_PWM_DATA);
+   Y(MY_BLC_PWM_DATA2);
+   Y(MY_BLM_HIST_CTL);
+   Y(MY_BLM_HIST_BIN);
+   Y(MY_BLM_HIST_GUARD);
+   Y(MY_BLC_PWM2_CTL);
+   Y(MY_BLC_MISC_CTL);
+   Y(MY_UTIL_PIN_CTL);
+   Y(MY_SBLC_PWM_CTL1);
+   Y(MY_SBLC_PWM_CTL2);
+
+   if (i & 1)
+   printf("\n");
+
+#undef Y
+}
+
 void

Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-11 Thread Sviatoslav Chagaev 
On Sun, 10 Jan 2016 21:28:31 +0100
Joerg Jung  wrote:
> 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.

Try the following:

1) Upgrade to latest snapshot.
2) Bind `wsconsctl display.brightness+=5` to some key combination in
   your X window manager.
3) On wake up, start adjusting the brightness up.

At some magical point (something like 87% for me IIRC) you should
observe the backlight turning on and going to 100% brightness. From
this point, if you adjust down, the backlight turns off, if you adjust
up, the backlight goes to 100%.

If you don't observe this, we might be experiencing different issues.

>  
> > 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?

Judging by the code at e.g. sys/dev/pci/drm/i915/intel_panel.c:984, it
seems Intel KMS driver expects BIOS or EFI to configure backlight,
then reads these values and uses them itself.

I found documentation for Intel Haswell [1] and glanced over
descriptions of relevant registers (BLC_PWM_CTL, BLC_PWM2_CTL,
BLC_PWM_DATA, BLC_MISC_CTL, BLM_HIST_CTL, BLM_HIST_BIN, BLM_HIST_GUARD,
UTIL_PIN_CTL, SBLC_PWM_CTL1, SBLC_PWM_CTL2), they are confusing...

Based on these, I agree, it might be a bug in Intel KMS.

I'll add debug prints and play with these registers, maybe I'll spot
some incorrect values.

[1] 
https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-commandreference-registers_0_0.pdf

> 
> > 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.

Yep, it needs to be integrated with Intel KMS to not break anything...

> 
> > 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=` 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.

Thanks, if there will be a next version of this diff, I'll fix them.

Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-10 Thread Mark Kettenis 
> Date: Sun, 10 Jan 2016 21:28:31 +0100
> From: Joerg Jung 
> 
> 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=` 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.

Also, I'dlike to see the acpidump output for this machine before we
decide things are indeed broken.

Re: [diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-10 Thread Joerg Jung 
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=` 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=144988400031788=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 -  1.84
> +++ share/man/man4/iic.4  10 Jan 2016 01:52:55 -
> @@ -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 -
> +++ share/man/man4/lpbl.4 10 Jan 2016 01:52:55 -
> @@ -0,0 +1,48 @@
> +.\"
> +.\" Copyright (c) 2016 Sviatoslav Chagaev 
> +.\"
> +.\" 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

[diff] lpbl(4): driver for TI LP8550 backlight controller (found in e.g. MacBook Air 6,2)

2016-01-09 Thread Sviatoslav Chagaev 
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%.

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.

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.

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=` to turn the backlight back on.

But it beats rebooting.

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).

Next step could be to somehow integrate it with Intel KMS.

[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=144988400031788=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.427 Dec 2014 16:08:03 -  1.84
+++ share/man/man4/iic.410 Jan 2016 01:52:55 -
@@ -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 -
+++ share/man/man4/lpbl.4   10 Jan 2016 01:52:55 -
@@ -0,0 +1,48 @@
+.\"
+.\" Copyright (c) 2016 Sviatoslav Chagaev 
+.\"
+.\" 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 ,
+.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 -  1.51
+++ sys/dev/i2c/files.i2c   10 Jan 2016 01:52:58 -
@@ -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.clpbl
+
 # RICOH RS5C372[AB] Real Time Clock
 device ricohrtc
 attach ricohrtc at i2c
Index: sys/dev/i2c/i2c_scan.c