Message-ID: <CAFqHKT=YRoSsThEbqXLPHR_1M3=zRHw9f758JKm++7TfN8ZWKA@mail.gmail.com>
Date: Mon, 28 Jul 2025 20:25:24 -0700
From: Derek John Clark <derekjohn.clark@...il.com>
To: Armin Wolf <W_Armin@....de>
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

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?

> > +
> > +     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;

> > +             .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.

> > +
> > +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