Re: [PATCH 5/5] leds: add driver for AAEON devices

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Tue, 25 May 2021 at 01:42, <aaeon.asus@xxxxxxxxx> wrote:
>
> From: Kunyang_Fan <kunyang_fan@xxxxxxxx>
>
> This patch adds support for the led devices which can
> be controlled from sysfs through ASUS WMI interface.
>
> Signed-off-by: Kunyang_Fan <kunyang_fan@xxxxxxxx>
> ---
>  drivers/leds/Kconfig      |  11 +++
>  drivers/leds/Makefile     |   1 +
>  drivers/leds/leds-aaeon.c | 142 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
>  create mode 100644 drivers/leds/leds-aaeon.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index be4536eef1fe..34d1b80855af 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -49,6 +49,17 @@ config LEDS_88PM860X
>           This option enables support for on-chip LED drivers found on Marvell
>           Semiconductor 88PM8606 PMIC.
>
> +config LEDS_AAEON
> +       tristate "AAEON LED driver"
> +       depends on X86
> +       select MFD_AAEON
> +       help
> +         This led driver adds support for LED brightness control on Single
> +         Board Computers produced by AAEON.
> +
> +         This driver leverages the ASUS WMI interface to access device
> +         resources.
> +
>  config LEDS_AAT1290
>         tristate "LED support for the AAT1290"
>         depends on LEDS_CLASS_FLASH
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d6b8a792c936..a8a77acc5e11 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)             += led-triggers.o
>
>  # LED Platform Drivers (keep this sorted, M-| sort)
>  obj-$(CONFIG_LEDS_88PM860X)            += leds-88pm860x.o
> +obj-$(CONFIG_LEDS_AAEON)               += leds-aaeon.o
>  obj-$(CONFIG_LEDS_AAT1290)             += leds-aat1290.o
>  obj-$(CONFIG_LEDS_ADP5520)             += leds-adp5520.o
>  obj-$(CONFIG_LEDS_AN30259A)            += leds-an30259a.o
> diff --git a/drivers/leds/leds-aaeon.c b/drivers/leds/leds-aaeon.c
> new file mode 100644
> index 000000000000..10090a4bff65
> --- /dev/null
> +++ b/drivers/leds/leds-aaeon.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AAEON LED driver
> + *
> + * Copyright (c) 2021, AAEON Ltd.
> + *
> + * Author: Kunyang Fan <kunyang_fan@xxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/x86/asus-wmi.h>
> +#include <linux/platform_device.h>
> +
> +#define DRVNAME "led_aaeon"
> +#define ASUS_NB_WMI_EVENT_GUID   "0B3CBB35-E3C2-45ED-91C2-4C5A6D195D1C"
> +#define AAEON_WMI_MGMT_GUID      "97845ED0-4E6D-11DE-8A39-0800200C9A66"
> +
> +#define GET_LED_NUMBER_ID        0x00060000
> +#define GET_LED_METHOD_ID        0x00060001
> +#define SET_LED_METHOD_ID        0x00060002
> +#define GET_LED_NUMBER_METHOD_ID 0x10
> +
> +

Previous patches apply here as well (no double blank lines, missing
SoB, missing CC to maintainers, not needed license text).

> +struct aaeon_led_data {
> +       int id;
> +       struct led_classdev cdev;
> +};
> +
> +static int aaeon_led_get_number(void)
> +{
> +       int err, retval;
> +
> +       err = asus_wmi_evaluate_method(GET_LED_NUMBER_ID,
> +                                      GET_LED_NUMBER_METHOD_ID,
> +                                      0, &retval);
> +       if (err)
> +               return err;
> +
> +       return retval;
> +}
> +
> +static enum led_brightness aaeon_led_brightness_get(struct led_classdev
> +                                                     *cdev)
> +{
> +       int err, brightness;
> +       struct aaeon_led_data *led =
> +                       container_of(cdev, struct aaeon_led_data, cdev);
> +       u32 arg0;
> +
> +       arg0 = (u32)(led->id & 0xF);

Hm, why do you need a cast here?

> +       err = asus_wmi_evaluate_method(GET_LED_METHOD_ID, arg0, 0, &brightness);
> +       if (err)
> +               return err;
> +
> +       return brightness;
> +};
> +
> +static void aaeon_led_brightness_set(struct led_classdev *cdev,
> +                                      enum led_brightness brightness)
> +{
> +       int err, retval;
> +       struct aaeon_led_data *led =
> +                       container_of(cdev, struct aaeon_led_data, cdev);
> +       u32 arg0;
> +
> +       arg0 = (u32)(led->id & 0xF);
> +       if (brightness != LED_OFF)
> +               arg0 |= BIT(16);
> +
> +       err = asus_wmi_evaluate_method(SET_LED_METHOD_ID, arg0, 0, &retval);
> +};
> +
> +static int __init aaeon_add_led_device(struct platform_device *pdev,
> +                                          int id)
> +{
> +       struct aaeon_led_data *led;
> +
> +       led = devm_kzalloc(&pdev->dev, sizeof(struct aaeon_led_data), GFP_KERNEL);

sizeof(*led)

> +       if (!led)
> +               return -ENOMEM;
> +
> +       led->id = id;
> +       led->cdev.brightness_get = aaeon_led_brightness_get;
> +       led->cdev.brightness_set = aaeon_led_brightness_set;
> +       led->cdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "led:%d:", id);
> +
> +       if (!led->cdev.name)
> +               return -ENOMEM;
> +
> +       return devm_led_classdev_register(&pdev->dev, &led->cdev);
> +}
> +
> +static int aaeon_led_probe(struct platform_device *pdev)
> +{
> +       int err = -ENODEV, i;

Split declaration of initialized and uninitialized variables.

> +       int led_number = 0;
> +
> +       pr_debug("aaeon led device probe!\n");

No printks for simple probes. This pollutes the dmesg without any benefit.

Best regards,
Krzysztof