Message-ID: <86888be2-3920-4232-9335-d411625bd8fd@gmx.de>
Date: Tue, 29 Jul 2025 06:18:24 +0200
From: Armin Wolf <W_Armin@....de>
To: Derek John Clark <derekjohn.clark@...il.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Hans de Goede <hansg@...nel.org>, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Alok Tiwari <alok.a.tiwari@...cle.com>,
 David Box <david.e.box@...ux.intel.com>,
 platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
 Lee Jones <lee@...nel.org>, pavel@...nel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v3 3/4] platform/x86: (ayn-ec) Add RGB Interface

Am 29.07.25 um 05:25 schrieb Derek John Clark:

> On Sat, Jul 26, 2025 at 4:59 PM Armin Wolf <W_Armin@....de> wrote:
>> Am 26.07.25 um 22:40 schrieb Derek J. Clark:
>>
>>> Adds an EC controlled LED Multicolor Class Device for controlling the
>>> RGB rings around the joysticks.
>>>
>>> The EC provides a single register for each of the colors red, green, and
>>> blue, as well as a mode switching register. The EC accepts values
>>> [0-255] for all colors. There are two available effects: breathe, which is
>>> the default when the device is started, and monocolor. When resuming from
>>> sleep the user selected effect will be overwritten by the EC, so the
>>> driver retains the last setting and resets on resume. When setting a
>>> color, each color register is set before a final "write" code is sent to
>>> the device. The EC may briefly reflect the "write" code when writing, but
>>> quickly changes to the "monocolor" value once complete. The driver
>>> interprets both of these values as "monocolor" in _show to simplify the
>>> sysfs exposed to the user.
>>>
>>> Two custom attributes are added to the standard LED parent device:
>>> effect, a RW file descriptor used to set the effect, and effect_index,
>>> which enumerates the available valid options.
>>>
>>> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
>>> ---
>>>    drivers/platform/x86/Kconfig  |   3 +
>>>    drivers/platform/x86/ayn-ec.c | 285 ++++++++++++++++++++++++++++++++++
>>>    2 files changed, 288 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 4819bfcffb6b..85dfb88cca6f 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -308,6 +308,9 @@ config AYN_EC
>>>        tristate "AYN x86 devices EC platform control"
>>>        depends on ACPI
>>>        depends on HWMON
>>> +     depends on NEW_LEDS
>>> +     select LEDS_CLASS
>>> +     select LEDS_CLASS_MULTICOLOR
>>>        help
>>>          This is a driver for AYN and Tectoy x86 handheld devices. It provides
>>>          temperature monitoring, manual fan speed control, fan curve control,
>>> diff --git a/drivers/platform/x86/ayn-ec.c b/drivers/platform/x86/ayn-ec.c
>>> index 466cc33adcb0..25f748d7db18 100644
>>> --- a/drivers/platform/x86/ayn-ec.c
>>> +++ b/drivers/platform/x86/ayn-ec.c
>>> @@ -28,6 +28,8 @@
>>>    #include <linux/hwmon.h>
>>>    #include <linux/init.h>
>>>    #include <linux/kernel.h>
>>> +#include <linux/led-class-multicolor.h>
>>> +#include <linux/leds.h>
>>>    #include <linux/module.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/sysfs.h>
>>> @@ -68,6 +70,16 @@
>>>    #define AYN_SENSOR_PROC_TEMP_REG    0x09 /* CPU Core */
>>>    #define AYN_SENSOR_VCORE_TEMP_REG   0x08 /* vCore */
>>>
>>> +/* EC Controlled RGB registers */
>>> +#define AYN_LED_MC_RED_REG   0xB0 /* Range 0x00-0xFF */
>>> +#define AYN_LED_MC_GREEN_REG 0xB1 /* Range 0x00-0xFF */
>>> +#define AYN_LED_MC_BLUE_REG  0xB2 /* Range 0x00-0xFF */
>>> +#define AYN_RGB_EFFECT_REG   0xB3
>>> +
>>> +/* RGB effect modes */
>>> +#define AYN_RGB_EFFECT_BREATHE               0x00
>>> +#define AYN_RGB_EFFECT_MONOCOLOR     0x55
>>> +#define AYN_RGB_EFFECT_WRITE         0xAA
>>>
>>>    /* Handle ACPI lock mechanism */
>>>    #define ACPI_LOCK_DELAY_MS 500
>>> @@ -86,7 +98,9 @@ int ayn_pwm_curve_registers[10] = {
>>>    };
>>>
>>>    struct ayn_device {
>>> +     struct led_classdev *led_cdev;
>>>        u32 ayn_lock; /* ACPI EC Lock */
>>> +     u8 rgb_effect;
>>>    } drvdata;
>>>
>>>    struct thermal_sensor {
>>> @@ -103,6 +117,33 @@ static struct thermal_sensor thermal_sensors[] = {
>>>        {}
>>>    };
>>>
>>> +#define DEVICE_ATTR_RW_NAMED(_name, _attrname)               \
>>> +     struct device_attribute dev_attr_##_name = {         \
>>> +             .attr = { .name = _attrname, .mode = 0644 }, \
>>> +             .show = _name##_show,                        \
>>> +             .store = _name##_store,                      \
>>> +     }
>>> +
>>> +#define DEVICE_ATTR_RO_NAMED(_name, _attrname)               \
>>> +     struct device_attribute dev_attr_##_name = {         \
>>> +             .attr = { .name = _attrname, .mode = 0444 }, \
>>> +             .show = _name##_show,                        \
>>> +     }
>> Please use DEVICE_ATTR_RW()/DEVICE_ATTR_RO() directly.
>>
>>> +
>>> +/* Handle ACPI lock mechanism */
>>> +#define ACPI_LOCK_DELAY_MS 500
>> You already defined ACPI_LOCK_DELAY_MS earlier, please remove.
>>
>>> +
>>> +/* RGB effect values */
>>> +enum RGB_EFFECT_OPTION {
>>> +     BREATHE,
>>> +     MONOCOLOR,
>>> +};
>>> +
>>> +static const char *const RGB_EFFECT_TEXT[] = {
>>> +     [BREATHE] = "breathe",
>>> +     [MONOCOLOR] = "monocolor",
>>> +};
>> No capslock for variables please.
>>
>>> +
>>>    static bool lock_global_acpi_lock(void)
>>>    {
>>>        return ACPI_SUCCESS(acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
>>> @@ -528,10 +569,253 @@ static struct attribute *ayn_sensors_attrs[] = {
>>>
>>>    ATTRIBUTE_GROUPS(ayn_sensors);
>>>
>>> +/**
>>> + * rgb_effect_write() - Set the RGB effect stored in drvdata.rgb_effect.
>>> + */
>>> +static int rgb_effect_write(void)
>>> +{
>>> +     return write_to_ec(AYN_RGB_EFFECT_REG, drvdata.rgb_effect);
>>> +};
>>> +
>>> +/**
>>> + * rgb_effect_read() - Read the RGB effect and store it in drvdata.rgb_effect.
>>> + */
>>> +static int rgb_effect_read(void)
>>> +{
>>> +     int ret;
>>> +     long effect;
>>> +
>>> +     ret = read_from_ec(AYN_RGB_EFFECT_REG, 1, &effect);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     switch (effect) {
>>> +     case AYN_RGB_EFFECT_WRITE:
>>> +     case AYN_RGB_EFFECT_MONOCOLOR:
>>> +             drvdata.rgb_effect = AYN_RGB_EFFECT_WRITE;
>>> +             break;
>>> +     default:
>>> +             drvdata.rgb_effect = AYN_RGB_EFFECT_BREATHE;
>> You will need some locking around rgb_effect.
>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/**
>>> + * rgb_effect_store() - Store the given RGB effect and set it.
>>> + *
>>> + * @dev: parent device of the given attribute.
>>> + * @attr: The attribute to write to.
>>> + * @buf: Input value string from sysfs write.
>>> + * @count: The number of bytes written.
>>> + *
>>> + * Return: The number of bytes written, or an error.
>>> + */
>>> +static ssize_t rgb_effect_store(struct device *dev,
>>> +                             struct device_attribute *attr, const char *buf,
>>> +                             size_t count)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = sysfs_match_string(RGB_EFFECT_TEXT, buf);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     if (ret)
>>> +             drvdata.rgb_effect = AYN_RGB_EFFECT_WRITE;
>>> +     else
>>> +             drvdata.rgb_effect = AYN_RGB_EFFECT_BREATHE;
>>> +
>>> +     ret = rgb_effect_write();
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return count;
>>> +};
>>> +
>>> +/**
>>> + * rgb_effect_show() - Read the current RGB effect.
>>> + *
>>> + * @dev: parent device of the given attribute.
>>> + * @attr: The attribute to read.
>>> + * @buf: Buffer to read to.
>>> + *
>>> + * Return: The number of bytes read, or an error.
>>> + */
>>> +static ssize_t rgb_effect_show(struct device *dev,
>>> +                            struct device_attribute *attr, char *buf)
>>> +{
>>> +     int ret, i;
>>> +
>>> +     ret = rgb_effect_read();
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     switch (drvdata.rgb_effect) {
>>> +     case AYN_RGB_EFFECT_WRITE:
>>> +     case AYN_RGB_EFFECT_MONOCOLOR:
>>> +             i = MONOCOLOR;
>>> +             break;
>>> +     default:
>>> +             i = BREATHE;
>>> +             break;
>>> +     }
>>> +
>>> +     return sysfs_emit(buf, "%s\n", RGB_EFFECT_TEXT[i]);
>>> +};
>>> +
>>> +static DEVICE_ATTR_RW_NAMED(rgb_effect, "effect");
>>> +
>>> +/**
>>> + * rgb_effect_show() - Display the RGB effects available.
>>> + *
>>> + * @dev: parent device of the given attribute.
>>> + * @attr: The attribute to read.
>>> + * @buf: Buffer to read to.
>>> + *
>>> + * Return: The number of bytes read, or an error.
>>> + */
>>> +static ssize_t rgb_effect_index_show(struct device *dev,
>>> +                                  struct device_attribute *attr, char *buf)
>>> +{
>>> +     size_t count = 0;
>>> +     unsigned int i;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(RGB_EFFECT_TEXT); i++)
>>> +             count += sysfs_emit_at(buf, count, "%s ", RGB_EFFECT_TEXT[i]);
>>> +
>>> +     buf[count - 1] = '\n';
>>> +
>>> +     return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO_NAMED(rgb_effect_index, "effect_index");
>> We might need to coordinate this with the LED subsystem maintainer. I CCed him so that he can
>> voice his opinion about those sysfs attributes. Personally i would move those attributes below
>> the platform device.
>>
> The main reason I added them to the LED interface is to make writing
> udev rules more intuitive. Quite a few folks using the DKMS version of
> this driver just want to set a specific color on boot (usually off).
> IMO it makes logical sense that all the settings related to the LEDs
> would be on the LED device. I'll wait for the response from your CC
> before sending a v4.
>
>>> +
>>> +/**
>>> + * ayn_led_mc_brightness_set() - Write the brightness for the RGB LED.
>>> + *
>>> + * @led_cdev: Parent LED device for the led_classdev_mc.
>>> + * @brightness: Brightness value to write [0-255].
>>> + */
>>> +static void ayn_led_mc_brightness_set(struct led_classdev *led_cdev,
>>> +                                   enum led_brightness brightness)
>>> +{
>>> +     struct led_classdev_mc *led_cdev_mc = lcdev_to_mccdev(led_cdev);
>>> +     struct mc_subled s_led;
>>> +     int i, ret, val;
>>> +
>>> +     switch (drvdata.rgb_effect) {
>>> +     case AYN_RGB_EFFECT_WRITE:
>>> +     case AYN_RGB_EFFECT_MONOCOLOR:
>>> +             break;
>>> +     case AYN_RGB_EFFECT_BREATHE:
>>> +             return;
>>> +     }
>> This might confuse uses when they switch back to monocolor mode. I suggest that
>> you write the RGB values regardless of the currently selected effect.
>>
> I'll test if this interferes with breathe mode. I wrote this driver a
> couple years ago as a DKMS module so I don't remember immediately if I
> had to add this mitigation to prevent switching to monocolor if the
> multi_index or brightness was written to. If that does turn out to be
> the case, should I cache the latest write until monocolor is set?

In such a case i think led_sysfs_disable()/led_sysfs_enable() should be called
to signal userspace applications that the LED cannot currently be accessed.

>>> +
>>> +     led_cdev->brightness = brightness;
>>> +     for (i = 0; i < led_cdev_mc->num_colors; i++) {
>>> +             s_led = led_cdev_mc->subled_info[i];
>>> +             val = brightness * s_led.intensity / led_cdev->max_brightness;
>> Please check if you can use led_mc_calc_color_components() instead.
>>
>>> +             ret = write_to_ec(s_led.channel, val);
>>> +             if (ret) {
>>> +                     dev_err(led_cdev->dev,
>>> +                             "Error setting brightness:  %d\n", ret);
>>> +                     return;
>>> +             }
>>> +     }
>>> +
>>> +     /* Must write mode again to change to set color */
>>> +     write_to_ec(AYN_RGB_EFFECT_REG, AYN_RGB_EFFECT_WRITE);
>>> +};
>>> +
>>> +/**
>>> + * ayn_led_mc_brightness_get() - Get the brightness for the RGB LED.
>>> + *
>>> + * @led_cdev: Parent LED device for the led_classdev_mc.
>>> + *
>>> + * Return: Current brightness.
>>> + */
>>> +static enum led_brightness ayn_led_mc_brightness_get(struct led_classdev *led_cdev)
>>> +{
>>> +     return led_cdev->brightness;
>>> +};
>> This looks strange, are you sure that you have to provide this callback?
> Hmm, maybe not.
>
>>> +
>>> +static struct attribute *ayn_led_mc_attrs[] = {
>>> +     &dev_attr_rgb_effect.attr,
>>> +     &dev_attr_rgb_effect_index.attr,
>>> +     NULL,
>>> +};
>>> +
>>> +static struct attribute_group ayn_led_mc_group = {
>>> +     .attrs = ayn_led_mc_attrs,
>>> +};
>>> +
>>> +struct mc_subled ayn_led_mc_subled_info[] = {
>>> +     {
>>> +             .color_index = LED_COLOR_ID_RED,
>>> +             .brightness = 0,
>>> +             .intensity = 0,
>>> +             .channel = AYN_LED_MC_RED_REG,
>>> +     },
>>> +     {
>>> +             .color_index = LED_COLOR_ID_GREEN,
>>> +             .brightness = 0,
>>> +             .intensity = 0,
>>> +             .channel = AYN_LED_MC_GREEN_REG,
>>> +     },
>>> +     {
>>> +             .color_index = LED_COLOR_ID_BLUE,
>>> +             .brightness = 0,
>>> +             .intensity = 0,
>>> +             .channel = AYN_LED_MC_BLUE_REG,
>>> +     },
>>> +};
>> Please initialize the intensity fields with the current RGB register values
>> during probe. Also please declare the array as static.
>>
> Good idea, thanks.
>
>>> +
>>> +struct led_classdev_mc ayn_led_mc = {
>>> +     .led_cdev = {
>>> +             .name = "ayn:rgb:joystick_rings",
>>> +             .brightness = 0,
>> Does the EC support some kind of "RGB off" state? If not then please initialize the brightness field
>> with 0 if the RGB value during probe is not 0x000000 and 255 otherwise. Also please declare the LED device
>> as static.
>>
> Off is done by setting all color registers to 0. Simple enough to add.
> I'm thinking something like:
>
> if (red || green || blue)
>          led_cdev.brightness = 255;
> else
>          led_cdev.brightness = 0;

Looks good.

>>> +             .max_brightness = 255,
>>> +             .brightness_set = ayn_led_mc_brightness_set,
>>> +             .brightness_get = ayn_led_mc_brightness_get,
>>> +             .color = LED_COLOR_ID_RGB,
>>> +     },
>>> +     .num_colors = ARRAY_SIZE(ayn_led_mc_subled_info),
>>> +     .subled_info = ayn_led_mc_subled_info,
>>> +};
>> Should the LED be disabled during suspend? If yes then please set the LED_CORE_SUSPENDRESUME flag on the LED device.
>>
> The EC takes over during suspend and switches back to breathe mode.
> Resume exists to return to user settings.

I see, in this case the LED core should indeed not perform any suspend/resume handling by itself.

Thanks,
Armin Wolf

>>> +
>>> +static int ayn_ec_resume(struct platform_device *pdev)
>>> +{
>>> +     struct led_classdev *led_cdev = drvdata.led_cdev;
>>> +     int ret;
>>> +
>>> +     ret = rgb_effect_write();
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ayn_led_mc_brightness_set(led_cdev, led_cdev->brightness);
>>> +
>>> +     return 0;
>>> +}
>> Using regmap would make this much easier.
>>
>>> +
>>>    static int ayn_ec_probe(struct platform_device *pdev)
>>>    {
>>>        struct device *dev = &pdev->dev;
>>>        struct device *hwdev;
>>> +     int ret;
>>> +
>>> +     ret = devm_led_classdev_multicolor_register(dev, &ayn_led_mc);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = devm_device_add_group(ayn_led_mc.led_cdev.dev, &ayn_led_mc_group);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     drvdata.led_cdev = &ayn_led_mc.led_cdev;
>>> +     ret = rgb_effect_read();
>>> +     if (ret)
>>> +             return ret;
>>>
>>>        hwdev = devm_hwmon_device_register_with_info(dev, "aynec", NULL,
>>>                                                     &ayn_ec_chip_info,
>>> @@ -544,6 +828,7 @@ static struct platform_driver ayn_ec_driver = {
>>>                .name = "ayn-ec",
>>>        },
>>>        .probe = ayn_ec_probe,
>>> +     .resume = ayn_ec_resume,
>> Please do not use the legacy PM callback, instead use DEFINE_SIMPLE_DEV_PM_OPS() and the driver.pm field.
>>
> Okay, I wasn't aware of the newer callbacks. I'll look them up.
>
> Thanks,
> Derek
>
>> Thanks,
>> Armin Wolf
>>
>>>    };
>>>
>>>    static struct platform_device *ayn_ec_device;

Powered by blists - more mailing lists