mbox series

[0/5] platform/chrome: cros_kbd_led_backlight: add EC PWM backend

Message ID 20220214053646.3088298-1-tzungbi@google.com
Headers show
Series platform/chrome: cros_kbd_led_backlight: add EC PWM backend | expand

Message

Tzung-Bi Shih Feb. 14, 2022, 5:36 a.m. UTC 
The series adds EC PWM as an backend option for ChromeOS keyboard LED
backlight.

The 1st patch reorder the headers alphabetically.

The 2nd patch separates the ACPI backend as an independent option.

The 3rd patch is the DT binding document for the proposed compatible string.

The 4th patch supports OF match.

The 5th patch adds EC PWM as another backend option.

Tzung-Bi Shih (5):
  platform/chrome: cros_kbd_led_backlight: sort headers alphabetically
  platform/chrome: cros_kbd_led_backlight: separate ACPI backend
  dt-bindings: add google,cros-kbd-led-backlight
  platform/chrome: cros_kbd_led_backlight: support OF match
  platform/chrome: cros_kbd_led_backlight: support EC PWM backend

 .../chrome/google,cros-kbd-led-backlight.yaml |  35 +++
 .../bindings/mfd/google,cros-ec.yaml          |   3 +
 drivers/platform/chrome/Kconfig               |  14 +-
 .../platform/chrome/cros_kbd_led_backlight.c  | 218 ++++++++++++++++--
 4 files changed, 247 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-kbd-led-backlight.yaml

Comments

Prashant Malani Feb. 16, 2022, 1:10 a.m. UTC | #1 
On Sun, Feb 13, 2022 at 9:37 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
> ---
>  .../platform/chrome/cros_kbd_led_backlight.c    | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index 814f2b74c602..ba853e55d29a 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>
> @@ -128,8 +129,11 @@ static int keyboard_led_probe(struct platform_device *pdev)
>         int error;
>
>         drvdata = acpi_device_get_match_data(&pdev->dev);
> -       if (!drvdata)
> -               return -EINVAL;
> +       if (!drvdata) {
> +               drvdata = of_device_get_match_data(&pdev->dev);
> +               if (!drvdata)
> +                       return -EINVAL;
> +       }

I'm not familiar with this driver, so can't do a full review, but
shouldn't device_get_match_data()
from property.h [1] be able to handle both DT and ACPI cases?

[1]: https://elixir.bootlin.com/linux/v5.17-rc4/source/include/linux/property.h

>
>         if (drvdata->init) {
>                 error = drvdata->init(pdev);
> @@ -161,10 +165,19 @@ static const struct acpi_device_id keyboard_led_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
>
> +static const struct of_device_id keyboard_led_of_match[] = {
> +       {
> +               .compatible = "google,cros-kbd-led-backlight",
> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, keyboard_led_of_match);
> +
>  static struct platform_driver keyboard_led_driver = {
>         .driver         = {
>                 .name   = "chromeos-keyboard-leds",
>                 .acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
> +               .of_match_table = of_match_ptr(keyboard_led_of_match),
>         },
>         .probe          = keyboard_led_probe,
>  };
> --
> 2.35.1.265.g69c8d7142f-goog
>
>
Tzung-Bi Shih Feb. 16, 2022, 2:31 a.m. UTC | #2 
On Tue, Feb 15, 2022 at 05:10:04PM -0800, Prashant Malani wrote:
> On Sun, Feb 13, 2022 at 9:37 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> > diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> > index 814f2b74c602..ba853e55d29a 100644
> > --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> > +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/leds.h>
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >
> > @@ -128,8 +129,11 @@ static int keyboard_led_probe(struct platform_device *pdev)
> >         int error;
> >
> >         drvdata = acpi_device_get_match_data(&pdev->dev);
> > -       if (!drvdata)
> > -               return -EINVAL;
> > +       if (!drvdata) {
> > +               drvdata = of_device_get_match_data(&pdev->dev);
> > +               if (!drvdata)
> > +                       return -EINVAL;
> > +       }
> 
> I'm not familiar with this driver, so can't do a full review, but
> shouldn't device_get_match_data()
> from property.h [1] be able to handle both DT and ACPI cases?
> 
> [1]: https://elixir.bootlin.com/linux/v5.17-rc4/source/include/linux/property.h

Yes, it does[2][3].  Thanks for the feedback, will fix it in next version.

[2]: https://elixir.bootlin.com/linux/v5.17-rc4/source/drivers/of/property.c#L1474
[3]: https://elixir.bootlin.com/linux/v5.17-rc4/source/drivers/acpi/property.c#L1386