From: Hans de Goede <hdegoede@redhat.com>
To: Daniel Dadap <ddadap@nvidia.com>, platform-driver-x86@vger.kernel.org
Cc: andy.shevchenko@gmail.com, mario.limonciello@outlook.com,
pobrn@protonmail.com, dvhart@infradead.org, thomas@t-8ch.de,
Aaron Plattner <aplattner@nvidia.com>
Subject: Re: [PATCH v4] platform/x86: Add driver for ACPI WMAA EC-based backlight control
Date: Thu, 2 Sep 2021 17:38:57 +0200 [thread overview]
Message-ID: <c508437a-5140-00f3-6316-a30c1d806e48@redhat.com> (raw)
In-Reply-To: <20210831224906.1072-1-ddadap@nvidia.com>
Hi Daniel,
On 9/1/21 12:49 AM, Daniel Dadap wrote:
> A number of upcoming notebook computer designs drive the internal
> display panel's backlight PWM through the Embedded Controller (EC).
> This EC-based backlight control can be plumbed through to an ACPI
> "WMAA" method interface, which in turn can be wrapped by WMI with
> the GUID handle 603E9613-EF25-4338-A3D0-C46177516DB7.
>
> Add a new driver, aliased to the WMAA WMI GUID, to expose a sysfs
> backlight class driver to control backlight levels on systems with
> EC-driven backlights.
>
> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
> ---
>
> v2: Convert to WMI subsystem driver, suggested by Mario Limonciello
> <mario.limonciello@outlook.com>; various cleanups suggested by
> Barnabás Pőcze <pobrn@protonmail.com>
> v3: Address assorted style nits raised by Andy Shevchenko
> <andy.shevchenko@gmail.com> in response to a related patch; remove
> additional behavior change to WMI subsystem from patch series as
> recommended by Hans de Goede <hdegoede@redhat.com>
> v4: Use MODULE_DEVICE_TABLE() (Thomas Weißschuh <thomas@t-8ch.de>)
> Fix scope of internal driver state; various style fixes (Barnabás
> Pőcze, Andy Shevchenko)
> Use devm_backlight_device_register() (Barnabás Pőcze)
> Add kerneldoc comments for enums and structs (Andy Shevchenko)
Thank you for your patch, given Andy's (small) set of remarks
I believe it would be best to do a v5 addressing those, after that
I expect this to be ready for merging.
Regards,
Hans
>
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 16 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/wmaa-backlight-wmi.c | 185 ++++++++++++++++++++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/platform/x86/wmaa-backlight-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bbaecde94aa0..fd7362a86c6d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20008,6 +20008,12 @@ L: linux-wireless@vger.kernel.org
> S: Odd fixes
> F: drivers/net/wireless/wl3501*
>
> +WMAA BACKLIGHT DRIVER
> +M: Daniel Dadap <ddadap@nvidia.com>
> +L: platform-driver-x86@vger.kernel.org
> +S: Supported
> +F: drivers/platform/x86/wmaa-backlight-wmi.c
> +
> WOLFSON MICROELECTRONICS DRIVERS
> L: patches@opensource.cirrus.com
> S: Supported
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index d12db6c316ea..0df908ef8d7c 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -113,6 +113,22 @@ config PEAQ_WMI
> help
> Say Y here if you want to support WMI-based hotkeys on PEAQ 2-in-1s.
>
> +config WMAA_BACKLIGHT_WMI
> + tristate "ACPI WMAA Backlight Driver"
> + depends on ACPI_WMI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> + This driver provides a sysfs backlight interface for notebook
> + systems which expose the WMAA ACPI method and an associated WMI
> + wrapper to drive LCD backlight levels through the system's
> + Embedded Controller (EC).
> +
> + Say Y or M here if you want to control the backlight on a notebook
> + system with an EC-driven backlight using the ACPI WMAA method.
> +
> + If you choose to compile this driver as a module the module will be
> + called wmaa-backlight-wmi.
> +
> config XIAOMI_WMI
> tristate "Xiaomi WMI key driver"
> depends on ACPI_WMI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 7ee369aab10d..109c1714237d 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_INTEL_WMI_SBL_FW_UPDATE) += intel-wmi-sbl-fw-update.o
> obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> +obj-$(CONFIG_WMAA_BACKLIGHT_WMI) += wmaa-backlight-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>
> diff --git a/drivers/platform/x86/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c
> new file mode 100644
> index 000000000000..cb1a973803b1
> --- /dev/null
> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +/**
> + * enum wmaa_method - WMI method IDs for ACPI WMAA
> + *
> + * @WMAA_LEVEL: Get or set the brightness level,
> + * or get the maximum brightness level.
> + * @WMAA_SOURCE: Get the source for backlight control.
> + */
> +enum wmaa_method {
> + WMAA_LEVEL = 1,
> + WMAA_SOURCE = 2,
> +};
> +
> +/**
> + * enum wmaa_get_or_set - Operation mode for ACPI WMAA method
> + *
> + * @WMAA_GET: Get the current brightness level or backlight source.
> + * @WMAA_SET: Set the brightness level.
> + * @WMAA_GET_MAX: Get the maximum brightness level. This is only valid when the
> + * WMI method is %WMAA_LEVEL.
> + */
> +enum wmaa_get_or_set {
> + WMAA_GET = 0,
> + WMAA_SET = 1,
> + WMAA_GET_MAX = 2
> +};
> +
> +/**
> + * enum wmaa_source - Backlight brightness control source identification
> + *
> + * @WMAA_SOURCE_GPU: Backlight brightness is controlled by the GPU.
> + * @WMAA_SOURCE_EC: Backlight brightness is controlled by the system's
> + * Embedded Controller (EC).
> + * @WMAA_SOURCE_AUX: Backlight brightness is controlled over the DisplayPort
> + * AUX channel.
> + */
> +enum wmaa_source {
> + WMAA_SOURCE_GPU = 1,
> + WMAA_SOURCE_EC = 2,
> + WMAA_SOURCE_AUX = 3
> +};
> +
> +/**
> + * struct wmaa_args - arguments for the ACPI WMAA method
> + *
> + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or
> + * setting a value.
> + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not
> + * used in %WMAA_GET or %WMAA_GET_MAX mode.
> + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET
> + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode.
> + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct.
> + *
> + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI.
> + * The value passed in to @val or returned by @ret will be a brightness value
> + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when
> + * the WMI method ID is %WMAA_SOURCE.
> + */
> +struct wmaa_args {
> + u32 set;
> + u32 val;
> + u32 ret;
> + u32 ignored[3];
> +};
> +
> +static int wmi_call_wmaa(struct wmi_device *w, enum wmaa_method method, enum wmaa_get_or_set set, u32 *val)
> +{
> + struct wmaa_args args = {
> + .set = set,
> + .val = 0,
> + .ret = 0,
> + };
> + struct acpi_buffer buf = { (acpi_size)sizeof(args), &args };
> + acpi_status status;
> +
> + if (set == WMAA_SET)
> + args.val = *val;
> +
> + status = wmidev_evaluate_method(w, 0, method, &buf, &buf);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&w->dev, "ACPI WMAA failed with %s\n",
> + acpi_format_exception(status));
> + return -EIO;
> + }
> +
> + if (set != WMAA_SET)
> + *val = args.ret;
> +
> + return 0;
> +}
> +
> +static int wmaa_backlight_update_status(struct backlight_device *bd)
> +{
> + struct wmi_device *wdev = bl_get_data(bd);
> +
> + return wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_SET, &bd->props.brightness);
> +}
> +
> +static int wmaa_backlight_get_brightness(struct backlight_device *bd)
> +{
> + struct wmi_device *wdev = bl_get_data(bd);
> + u32 level = 0;
> + int ret;
> +
> + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level);
> +
> + if (ret)
> + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n");
> +
> + return level;
> +}
> +
> +static const struct backlight_ops wmaa_backlight_ops = {
> + .update_status = wmaa_backlight_update_status,
> + .get_brightness = wmaa_backlight_get_brightness
> +};
> +
> +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx)
> +{
> + struct backlight_properties props = {};
> + struct backlight_device *backlight;
> + u32 source;
> + int ret;
> +
> + ret = wmi_call_wmaa(w, WMAA_SOURCE, WMAA_GET, &source);
> + if (ret)
> + return ret;
> +
> + if (source != WMAA_SOURCE_EC) {
> + /* This driver is only to be used when brightness control is
> + * handled by the EC; otherwise, the GPU driver(s) should handle
> + * brightness control. */
> + return -ENODEV;
> + }
> +
> + /* Identify this backlight device as a platform device so that it can
> + * be prioritized over any exposed GPU-driven raw device(s). */
> + props.type = BACKLIGHT_PLATFORM;
> +
> + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET_MAX, &props.max_brightness);
> + if (ret)
> + return ret;
> +
> + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET, &props.brightness);
> + if (ret)
> + return ret;
> +
> + backlight = devm_backlight_device_register(
> + &w->dev, "wmaa_backlight",
> + &w->dev, w, &wmaa_backlight_ops, &props);
> + if (IS_ERR(backlight))
> + return PTR_ERR(backlight);
> +
> + return 0;
> +}
> +
> +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
> +
> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = {
> + { .guid_string = WMAA_WMI_GUID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table);
> +
> +static struct wmi_driver wmaa_backlight_wmi_driver = {
> + .driver = {
> + .name = "wmaa-backlight"
> + },
> + .probe = wmaa_backlight_wmi_probe,
> + .id_table = wmaa_backlight_wmi_id_table
> +};
> +module_wmi_driver(wmaa_backlight_wmi_driver);
> +
> +MODULE_AUTHOR("Aaron Plattner <aplattner@nvidia.com>");
> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>");
> +MODULE_DESCRIPTION("WMAA Backlight WMI driver");
> +MODULE_LICENSE("GPL v2");
>
prev parent reply other threads:[~2021-09-02 15:39 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <DM6PR19MB2636AB267CD321DE40EF324AFA730@DM6PR19MB2636.namprd19.prod.outlook.com>
[not found] ` <20200731202154.11382-1-ddadap@nvidia.com>
[not found] ` <20200731202154.11382-2-ddadap@nvidia.com>
2020-11-10 9:34 ` [PATCH v2 2/2] platform/x86: wmi: fail wmi_driver_register when no GUID is found Hans de Goede
2020-11-12 18:54 ` Daniel Dadap
2021-08-24 22:04 ` [PATCH v3] platform/x86: Add driver for ACPI WMAA EC-based backlight control Daniel Dadap
2021-08-24 22:47 ` Barnabás Pőcze
2021-08-25 16:36 ` Daniel Dadap
2021-08-25 22:26 ` Daniel Dadap
2021-08-27 16:47 ` Daniel Dadap
2021-08-27 17:12 ` Daniel Dadap
2021-08-25 9:05 ` Andy Shevchenko
2021-08-25 16:47 ` Daniel Dadap
2021-08-25 22:06 ` Thomas Weißschuh
2021-08-25 22:28 ` Daniel Dadap
2021-08-26 13:35 ` Andy Shevchenko
2021-08-26 15:39 ` Daniel Dadap
2021-08-31 22:49 ` [PATCH v4] " Daniel Dadap
2021-09-01 9:25 ` Thomas Weißschuh
2021-09-02 2:12 ` Daniel Dadap
2021-09-02 10:19 ` Thomas Weißschuh
2021-09-02 20:15 ` Daniel Dadap
2021-09-02 20:31 ` Hans de Goede
2021-09-02 21:47 ` [PATCH v5] " Daniel Dadap
2021-09-02 23:20 ` Thomas Weißschuh
2021-09-03 0:22 ` Daniel Dadap
2021-09-03 8:14 ` Andy Shevchenko
2021-09-03 17:55 ` Daniel Dadap
2021-09-03 0:38 ` [PATCH v6] " Daniel Dadap
2021-09-13 9:01 ` Hans de Goede
2021-09-20 13:29 ` Pali Rohár
2021-09-20 13:33 ` Hans de Goede
2021-09-20 13:51 ` Pali Rohár
2021-09-20 17:34 ` Daniel Dadap
2021-09-20 17:55 ` Pali Rohár
2021-09-20 19:38 ` Daniel Dadap
2021-09-20 19:52 ` Pali Rohár
2021-09-21 13:53 ` Hans de Goede
2021-09-21 14:02 ` Pali Rohár
2021-09-21 14:29 ` Daniel Dadap
2021-09-21 14:58 ` Hans de Goede
2021-09-27 20:23 ` [PATCH 1/2] platform/x86: Remove "WMAA" from identifier names in wmaa-backlight-wmi.c Daniel Dadap
2021-09-27 20:23 ` [PATCH 2/2] platform/x86: Rename wmaa-backlight-wmi to nvidia-wmi-ec-backlight Daniel Dadap
2021-10-11 13:00 ` [PATCH 1/2] platform/x86: Remove "WMAA" from identifier names in wmaa-backlight-wmi.c Hans de Goede
2021-09-27 22:52 ` [PATCH v6] platform/x86: Add driver for ACPI WMAA EC-based backlight control Daniel Dadap
2021-09-02 9:28 ` [PATCH v4] " Andy Shevchenko
2021-09-02 9:33 ` Thomas Weißschuh
2021-09-02 9:36 ` Andy Shevchenko
2021-09-01 15:57 ` Andy Shevchenko
2021-09-01 23:12 ` Aaron Plattner
2021-09-02 2:37 ` Daniel Dadap
2021-09-02 9:35 ` Andy Shevchenko
2021-09-02 20:15 ` Daniel Dadap
2021-09-03 8:10 ` Andy Shevchenko
2021-09-02 15:38 ` Hans de Goede [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c508437a-5140-00f3-6316-a30c1d806e48@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.shevchenko@gmail.com \
--cc=aplattner@nvidia.com \
--cc=ddadap@nvidia.com \
--cc=dvhart@infradead.org \
--cc=mario.limonciello@outlook.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
--cc=thomas@t-8ch.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).