Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220523090822.3035189-1-tzungbi@kernel.org> <20220523090822.3035189-3-tzungbi@kernel.org>
In-Reply-To: <20220523090822.3035189-3-tzungbi@kernel.org>
From:   Guenter Roeck <groeck@google.com>
Date:   Thu, 9 Jun 2022 05:44:36 -0700
Message-ID: <CABXOdTeXB8V7ma9W6ZEQJeedvDg2xQOtJ5TduDmND-=BeDJpRA@mail.gmail.com>
Subject: Re: [PATCH v4 2/5] platform/chrome: cros_kbd_led_backlight: separate
 ACPI backend
To:     Tzung-Bi Shih <tzungbi@kernel.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:CHROME HARDWARE PLATFORM SUPPORT" 
        <chrome-platform@lists.linux.dev>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" 
        <devicetree@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 23, 2022 at 2:08 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> cros_kbd_led_backlight uses ACPI_KEYBOARD_BACKLIGHT_WRITE and
> ACPI_KEYBOARD_BACKLIGHT_READ for setting and getting the brightness
> respectively.
>
> Separate ACPI operations for preparing the driver to support other
> backends.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
> Changes from v3:
> - Remove CROS_KBD_LED_BACKLIGHT_ACPI Kconfig.
> - Remove stub function.
>
> Changes from v2:
> - Use #ifdef for boolean CONFIG_CROS_KBD_LED_BACKLIGHT_ACPI.
>
> Changes from v1:
> - Update email address accordingly.
> - Use CONFIG_ACPI guard per "kernel test robot <lkp@intel.com>" reported an
>   unused variable issue.
>
>  .../platform/chrome/cros_kbd_led_backlight.c  | 82 ++++++++++++++++---
>  1 file changed, 69 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index f9587a562bb7..a86d664854ae 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -13,6 +13,33 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>
> +/**
> + * struct keyboard_led_drvdata - keyboard LED driver data.
> + * @init:                      Init function.
> + * @brightness_get:            Get LED brightness level.
> + * @brightness_set:            Set LED brightness level.  Must not sleep.
> + * @brightness_set_blocking:   Set LED brightness level.  It can block the
> + *                             caller for the time required for accessing a
> + *                             LED device register
> + * @max_brightness:            Maximum brightness.
> + *
> + * See struct led_classdev in include/linux/leds.h for more details.
> + */
> +struct keyboard_led_drvdata {
> +       int (*init)(struct platform_device *pdev);
> +
> +       enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
> +
> +       void (*brightness_set)(struct led_classdev *led_cdev,
> +                              enum led_brightness brightness);
> +       int (*brightness_set_blocking)(struct led_classdev *led_cdev,
> +                                      enum led_brightness brightness);
> +
> +       enum led_brightness max_brightness;
> +};
> +
> +#ifdef CONFIG_ACPI
> +
>  /* Keyboard LED ACPI Device must be defined in firmware */
>  #define ACPI_KEYBOARD_BACKLIGHT_DEVICE "\\_SB.KBLT"
>  #define ACPI_KEYBOARD_BACKLIGHT_READ   ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
> @@ -20,8 +47,8 @@
>
>  #define ACPI_KEYBOARD_BACKLIGHT_MAX            100
>
> -static void keyboard_led_set_brightness(struct led_classdev *cdev,
> -                                       enum led_brightness brightness)
> +static void keyboard_led_set_brightness_acpi(struct led_classdev *cdev,
> +                                            enum led_brightness brightness)
>  {
>         union acpi_object param;
>         struct acpi_object_list input;
> @@ -40,7 +67,7 @@ static void keyboard_led_set_brightness(struct led_classdev *cdev,
>  }
>
>  static enum led_brightness
> -keyboard_led_get_brightness(struct led_classdev *cdev)
> +keyboard_led_get_brightness_acpi(struct led_classdev *cdev)
>  {
>         unsigned long long brightness;
>         acpi_status status;
> @@ -56,12 +83,10 @@ keyboard_led_get_brightness(struct led_classdev *cdev)
>         return brightness;
>  }
>
> -static int keyboard_led_probe(struct platform_device *pdev)
> +static int keyboard_led_init_acpi(struct platform_device *pdev)
>  {
> -       struct led_classdev *cdev;
>         acpi_handle handle;
>         acpi_status status;
> -       int error;
>
>         /* Look for the keyboard LED ACPI Device */
>         status = acpi_get_handle(ACPI_ROOT_OBJECT,
> @@ -73,15 +98,44 @@ static int keyboard_led_probe(struct platform_device *pdev)
>                 return -ENXIO;
>         }
>
> +       return 0;
> +}
> +
> +static const struct keyboard_led_drvdata keyboard_led_drvdata_acpi = {
> +       .init = keyboard_led_init_acpi,
> +       .brightness_set = keyboard_led_set_brightness_acpi,
> +       .brightness_get = keyboard_led_get_brightness_acpi,
> +       .max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX,
> +};
> +
> +#endif /* CONFIG_ACPI */
> +
> +static int keyboard_led_probe(struct platform_device *pdev)
> +{
> +       struct led_classdev *cdev;
> +       const struct keyboard_led_drvdata *drvdata;
> +       int error;
> +
> +       drvdata = acpi_device_get_match_data(&pdev->dev);
> +       if (!drvdata)
> +               return -EINVAL;
> +
> +       if (drvdata->init) {
> +               error = drvdata->init(pdev);
> +               if (error)
> +                       return error;
> +       }
> +
>         cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
>         if (!cdev)
>                 return -ENOMEM;
>
>         cdev->name = "chromeos::kbd_backlight";
> -       cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
>         cdev->flags |= LED_CORE_SUSPENDRESUME;
> -       cdev->brightness_set = keyboard_led_set_brightness;
> -       cdev->brightness_get = keyboard_led_get_brightness;
> +       cdev->max_brightness = drvdata->max_brightness;
> +       cdev->brightness_set = drvdata->brightness_set;
> +       cdev->brightness_set_blocking = drvdata->brightness_set_blocking;
> +       cdev->brightness_get = drvdata->brightness_get;
>
>         error = devm_led_classdev_register(&pdev->dev, cdev);
>         if (error)
> @@ -90,16 +144,18 @@ static int keyboard_led_probe(struct platform_device *pdev)
>         return 0;
>  }
>
> -static const struct acpi_device_id keyboard_led_id[] = {
> -       { "GOOG0002", 0 },
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id keyboard_led_acpi_match[] = {
> +       { "GOOG0002", (kernel_ulong_t)&keyboard_led_drvdata_acpi },
>         { }
>  };
> -MODULE_DEVICE_TABLE(acpi, keyboard_led_id);
> +MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
> +#endif
>
>  static struct platform_driver keyboard_led_driver = {
>         .driver         = {
>                 .name   = "chromeos-keyboard-leds",
> -               .acpi_match_table = ACPI_PTR(keyboard_led_id),
> +               .acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
>         },
>         .probe          = keyboard_led_probe,
>  };
> --
> 2.36.1.124.g0e6072fb45-goog
>