| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMeQTsZXWwetYK7SRVkbrVTnrq05nSGqhFRvFqnV=1yYC1zMng@mail.gmail.com>
Date: Tue, 29 Apr 2014 15:11:28 +0200
From: Patrik Jakobsson <patrik.r.jakobsson@...il.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: platform-driver-x86@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>,
matthew.garrett@...ula.com
Subject: Re: [RFC PATCH] mba6x_bl: Backlight driver for mid 2013 MacBook Air
On Tue, Apr 29, 2014 at 1:10 PM, Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi,
>
> Why is this patch an RFC? If it is ready for upstreaming please drop
> the RFC prefix when you post the next version.
I'll do that
> On 04/27/2014 10:56 PM, Patrik Jakobsson wrote:
> > This driver takes control over the LP8550 backlight driver chip found
> > in the mid 2013 and newer MacBook Air (6,1 and 6,2). The i915 GPU driver
> > cannot properly restore the backlight after resume, but with this driver
> > we can hijack the LP8550 and get fully functional backlight support.
> >
> > Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@...il.com>
> > ---
> > MAINTAINERS | 6 +
> > drivers/platform/x86/Kconfig | 13 ++
> > drivers/platform/x86/Makefile | 1 +
> > drivers/platform/x86/mba6x_bl.c | 351 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 371 insertions(+)
> > create mode 100644 drivers/platform/x86/mba6x_bl.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e67ea24..cad3e82 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5576,6 +5576,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git
> > S: Maintained
> > F: net/mac80211/rc80211_pid*
> >
> > +MACBOOK AIR 6,X BACKLIGHT DRIVER
> > +M: Patrik Jakobsson <patrik.r.jakobsson@...il.com>
> > +L: platform-driver-x86@...r.kernel.org
> > +S: Maintained
> > +F: drivers/platform/x86/mba6x_bl.c
> > +
> > MACVLAN DRIVER
> > M: Patrick McHardy <kaber@...sh.net>
> > L: netdev@...r.kernel.org
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 27df2c5..1308924 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -795,6 +795,19 @@ config APPLE_GMUX
> > graphics as well as the backlight. Currently only backlight
> > control is supported by the driver.
> >
> > +config MBA6X_BL
> > + tristate "MacBook Air 6,x backlight driver"
> > + depends on ACPI
> > + depends on BACKLIGHT_CLASS_DEVICE
> > + select ACPI_VIDEO if ACPI
>
> You can drop the if ACPI here, as you already DEPEND on it above
Ok, will do
> > + help
> > + This driver takes control over the LP8550 backlight driver found in
> > + some MacBook Air models. Say Y here if you have a MacBook Air from mid
> > + 2013 or newer.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called mba6x_bl.
> > +
> > config INTEL_RST
> > tristate "Intel Rapid Start Technology Driver"
> > depends on ACPI
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 1a2eafc..9a182fe 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -56,3 +56,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT) += intel-smartconnect.o
> >
> > obj-$(CONFIG_PVPANIC) += pvpanic.o
> > obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> > +obj-$(CONFIG_MBA6X_BL) += mba6x_bl.o
> > diff --git a/drivers/platform/x86/mba6x_bl.c b/drivers/platform/x86/mba6x_bl.c
> > new file mode 100644
> > index 0000000..022bc8d
> > --- /dev/null
> > +++ b/drivers/platform/x86/mba6x_bl.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * MacBook Air 6,1 and 6,2 (mid 2013) backlight driver
> > + *
> > + * Copyright (C) 2014 Patrik Jakobsson (patrik.r.jakobsson@...il.com)
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/backlight.h>
> > +#include <linux/acpi.h>
> > +#include <acpi/acpi.h>
> > +#include <acpi/video.h>
> > +
> > +#define LP8550_SMBUS_ADDR (0x58 >> 1)
> > +#define LP8550_REG_BRIGHTNESS 0
> > +#define LP8550_REG_DEV_CTL 1
> > +#define LP8550_REG_FAULT 2
> > +#define LP8550_REG_IDENT 3
> > +#define LP8550_REG_DIRECT_CTL 4
> > +#define LP8550_REG_TEMP_MSB 5 /* Must be read before TEMP_LSB */
> > +#define LP8550_REG_TEMP_LSB 6
> > +
> > +#define INIT_BRIGHTNESS 150
> > +
> > +static struct {
> > + u8 brightness; /* Brightness control */
> > + u8 dev_ctl; /* Device control */
> > + u8 fault; /* Fault indication */
> > + u8 ident; /* Identification */
> > + u8 direct_ctl; /* Direct control */
> > + u8 temp_msb; /* Temperature MSB */
> > + u8 temp_lsb; /* Temperature LSB */
> > +} lp8550_regs;
> > +
> > +static struct platform_device *platform_device;
> > +static struct backlight_device *backlight_device;
> > +
> > +static int lp8550_reg_read(u8 reg, u8 *val)
> > +{
> > + acpi_status status;
> > + acpi_handle handle;
> > + struct acpi_object_list arg_list;
> > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > + union acpi_object args[2];
> > + union acpi_object *result;
> > + int ret = 0;
> > +
> > + status = acpi_get_handle(NULL, "\\_SB.PCI0.SBUS.SRDB", &handle);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("mba6x_bl: Failed to get acpi handle\n");
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + args[0].type = ACPI_TYPE_INTEGER;
> > + args[0].integer.value = (LP8550_SMBUS_ADDR << 1) | 1;
> > +
> > + args[1].type = ACPI_TYPE_INTEGER;
> > + args[1].integer.value = reg;
> > +
> > + arg_list.count = 2;
> > + arg_list.pointer = args;
> > +
> > + status = acpi_evaluate_object(handle, NULL, &arg_list, &buffer);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("mba6x_bl: Failed to read reg: 0x%x\n", reg);
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + result = buffer.pointer;
> > +
> > + if (result->type != ACPI_TYPE_INTEGER) {
> > + pr_debug("mba6x_bl: Invalid response in reg: 0x%x (len: %Ld)\n",
> > + reg, buffer.length);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + *val = (u8)result->integer.value;
> > +out:
> > + kfree(buffer.pointer);
> > + return ret;
> > +}
> > +
> > +static int lp8550_reg_write(u8 reg, u8 val)
> > +{
> > + acpi_status status;
> > + acpi_handle handle;
> > + struct acpi_object_list arg_list;
> > + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > + union acpi_object args[3];
> > + union acpi_object *result;
> > + int ret = 0;
> > +
> > + status = acpi_get_handle(NULL, "\\_SB.PCI0.SBUS.SWRB", &handle);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("mba6x_bl: Failed to get acpi handle\n");
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + args[0].type = ACPI_TYPE_INTEGER;
> > + args[0].integer.value = (LP8550_SMBUS_ADDR << 1) & ~1;
> > +
> > + args[1].type = ACPI_TYPE_INTEGER;
> > + args[1].integer.value = reg;
> > +
> > + args[2].type = ACPI_TYPE_INTEGER;
> > + args[2].integer.value = val;
> > +
> > + arg_list.count = 3;
> > + arg_list.pointer = args;
> > +
> > + status = acpi_evaluate_object(handle, NULL, &arg_list, &buffer);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("mba6x_bl: Failed to write reg: 0x%x\n", reg);
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + result = buffer.pointer;
> > +
> > + if (result->type != ACPI_TYPE_INTEGER || result->integer.value != 1) {
> > + pr_debug("mba6x_bl: Invalid response at reg: 0x%x (len: %Ld)\n",
> > + reg, buffer.length);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > +out:
> > + kfree(buffer.pointer);
> > + return ret;
> > +}
> > +
> > +static inline int map_brightness(int b)
> > +{
> > + return ((b * b + 254) / 255);
> > +}
>
> What does this do ? it looks like some weird
> rounding code, is this really needed ?
>
> At a minimum please add a comment explaining what this does.
This makes the actual strength of the backlight more linear. It takes the
original value and maps it onto an exponential curve. What looks like rounding
is just there to make sure 0 is 0 and 255 is 255. I'll add a comment.
> > +
> > +static int lp8550_init(void);
> > +
> > +static int set_brightness(int brightness)
> > +{
> > + int ret;
> > +
> > + if (brightness < 0 || brightness > 255)
> > + return -EINVAL;
> > +
> > + lp8550_init();
>
> hmm, do we really need to re-init the lp8550 on each
> brightness change ?
I think it's just left-over paranoia from when I debugged some other issue.
I'll take a look at it again.
> > + brightness = map_brightness(brightness);
> > + ret = lp8550_reg_write(LP8550_REG_BRIGHTNESS, (u8)brightness);
> > +
> > + return ret;
> > +}
> > +
> > +static int get_brightness(struct backlight_device *bdev)
> > +{
> > + return bdev->props.brightness;
> > +}
> > +
> > +static int update_status(struct backlight_device *bdev)
> > +{
> > + return set_brightness(bdev->props.brightness);
> > +}
> > +
> > +static int lp8550_probe(void)
> > +{
> > + u8 val;
> > + int ret;
> > +
> > + ret = lp8550_reg_read(LP8550_REG_IDENT, &val);
> > + if (ret)
> > + return ret;
> > +
> > + if (val != 0xfc)
> > + return -ENODEV;
> > +
> > + pr_info("mba6x_bl: Found LP8550 backlight driver\n");
> > + return 0;
> > +}
> > +
> > +static int lp8550_save(void)
> > +{
> > + int ret;
> > +
> > + ret = lp8550_reg_read(LP8550_REG_DEV_CTL, &lp8550_regs.dev_ctl);
> > + if (ret)
> > + return ret;
> > +
> > + ret = lp8550_reg_read(LP8550_REG_BRIGHTNESS, &lp8550_regs.brightness);
> > + return ret;
> > +}
> > +
> > +
> > +static int lp8550_restore(void)
> > +{
> > + int ret;
> > +
> > + ret = lp8550_reg_write(LP8550_REG_BRIGHTNESS, lp8550_regs.brightness);
> > + if (ret)
> > + return ret;
> > +
> > + ret = lp8550_reg_write(LP8550_REG_DEV_CTL, lp8550_regs.dev_ctl);
> > + return ret;
> > +}
> > +
> > +static int lp8550_init(void)
> > +{
> > + int ret, i;
> > +
> > + for (i = 0; i < 10; i++) {
> > + ret = lp8550_reg_write(LP8550_REG_BRIGHTNESS, INIT_BRIGHTNESS);
> > + if (!ret)
> > + break;
> > + }
> > +
> > + if (i > 0)
> > + pr_err("mba6x_bl: Init retries: %d\n", i);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + ret = lp8550_reg_write(LP8550_REG_DEV_CTL, 0x05);
> > + return ret;
> > +}
> > +
> > +static struct backlight_ops backlight_ops = {
> > + .update_status = update_status,
> > + .get_brightness = get_brightness,
> > +};
> > +
> > +static struct platform_driver drv;
> > +
> > +static int platform_probe(struct platform_device *dev)
> > +{
> > + struct backlight_properties props;
> > + int ret;
> > +
> > + ret = lp8550_probe();
> > + if (ret)
> > + return ret;
> > +
> > + ret = lp8550_save();
> > + if (ret)
> > + return ret;
> > +
> > + ret = lp8550_init();
> > + if (ret)
> > + return ret;
> > +
> > + memset(&props, 0, sizeof(struct backlight_properties));
> > + props.max_brightness = 255;
> > + props.brightness = INIT_BRIGHTNESS;
> > + props.type = BACKLIGHT_FIRMWARE;
> > +
> > + backlight_device = backlight_device_register("mba6x_backlight",
> > + NULL, NULL,
> > + &backlight_ops, &props);
> > + if (IS_ERR(backlight_device)) {
> > + pr_err("mba6x_bl: Failed to register backlight device\n");
> > + return PTR_ERR(backlight_device);
> > + }
> > +
> > + acpi_video_dmi_promote_vendor();
> > + acpi_video_unregister();
> > +
> > + backlight_update_status(backlight_device);
> > +
> > + return 0;
> > +}
> > +
> > +static int platform_remove(struct platform_device *dev)
> > +{
> > + backlight_device_unregister(backlight_device);
> > + lp8550_restore();
> > + pr_info("mba6x_bl: Restored old configuration\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int platform_resume(struct platform_device *dev)
> > +{
> > + /*
> > + * Firmware restores the LP8550 for us but we might need some tweaking
> > + * in the future if firmware behaviour is changed.
> > + */
> > + return 0;
> > +}
> > +
> > +static void platform_shutdown(struct platform_device *dev)
> > +{
> > + /* We must restore or screen might go black on reboot */
> > + lp8550_restore();
> > +}
> > +
> > +static struct platform_driver drv = {
> > + .probe = platform_probe,
> > + .remove = platform_remove,
> > + .resume = platform_resume,
> > + .shutdown = platform_shutdown,
> > + .driver = {
> > + .name = "mba6x_bl",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init mba6x_bl_init(void)
> > +{
> > + int ret;
> > +
> > + ret = platform_driver_register(&drv);
> > + if (ret) {
> > + pr_err("Failed to register platform driver\n");
> > + return ret;
> > + }
> > +
> > + platform_device = platform_device_register_simple("mba6x_bl", -1, NULL,
> > + 0);
> > + return 0;
> > +}
> > +
> > +static void __exit mba6x_bl_exit(void)
> > +{
> > + platform_device_unregister(platform_device);
> > + platform_driver_unregister(&drv);
> > +}
> > +
> > +module_init(mba6x_bl_init);
> > +module_exit(mba6x_bl_exit);
> > +
> > +MODULE_AUTHOR("Patrik Jakobsson <patrik.r.jakobsson@...il.com>");
> > +MODULE_DESCRIPTION("MacBook Air 6,1 and 6,2 backlight driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +MODULE_ALIAS("dmi:*:pnMacBookAir6*");
> >
>
> Besides my few small comments this looks good overall.
>
> Regards,
>
> Hans
I missed a couple of checkpatch issues. I'll fix them as well.
Thanks for the review
Cheers
Patrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists