Hi Hans,
Thank you for reviewing this work.
On Thu, Jan 9, 2025 at 9:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Kate,
>
> Thank you for your work on this.
>
> On 18-Dec-24 9:59 AM, Kate Hsuan wrote:
> > This work migrated the multicolor LED control to the standard multicolor
> > LED API. Moreover, the codes related to the "color" attribute used to
> > set up the color previously were removed.
>
> I think the commit message needs some work, I would write for example:
>
> """
> Replace the custom "color" sysfs attribute with the standard multicolor
> LED API.
>
> This also removes the code for the custom "color" sysfs attribute,
> the "color" sysfs attribute was never documented so hopefully it is not
> used by anyone.
>
> If we get complaints we can re-add the "color" sysfs attribute as
> a compatibility wrapper setting the subleds intensity.
> """
I'll update it to the commit message.
>
> >
> > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
> > ---
> > drivers/hid/hid-lg-g15.c | 145 ++++++++++++++++++---------------------
> > 1 file changed, 68 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> > index 53e7b90f9cc3..52159cecca27 100644
> > --- a/drivers/hid/hid-lg-g15.c
> > +++ b/drivers/hid/hid-lg-g15.c
> > @@ -8,11 +8,13 @@
> > #include <linux/device.h>
> > #include <linux/hid.h>
> > #include <linux/leds.h>
> > +#include <linux/led-class-multicolor.h>
> > #include <linux/module.h>
> > #include <linux/random.h>
> > #include <linux/sched.h>
> > #include <linux/usb.h>
> > #include <linux/wait.h>
> > +#include <dt-bindings/leds/common.h>
> >
> > #include "hid-ids.h"
> >
> > @@ -44,7 +46,10 @@ enum lg_g15_led_type {
> > };
> >
> > struct lg_g15_led {
> > - struct led_classdev cdev;
> > + union {
> > + struct led_classdev cdev;
> > + struct led_classdev_mc mcdev;
> > + };
> > enum led_brightness brightness;
> > enum lg_g15_led_type led;
>
> red, green and blue are now only used to store the initial color intensities
> I would add a comment for this:
>
> /* Used to store initial color intensities before subled_info is allocated */
Okay
> > u8 red, green, blue;
> > @@ -227,17 +232,18 @@ static int lg_g510_get_initial_led_brightness(struct lg_g15_data *g15, int i)
> > /* Must be called with g15->mutex locked */
> > static int lg_g510_kbd_led_write(struct lg_g15_data *g15,
> > struct lg_g15_led *g15_led,
> > + struct mc_subled *subleds,
>
> The g15_led already gives you a pointer to the subleds, so I would
> drop this argument, leaving the function signature unchanged.
>
> > enum led_brightness brightness)
> > {
>
> And then instead at a subleds helper variable here:
>
> struct mc_subled *subleds = g15_led->mcdev.subled_info;
>
> > int ret;
>
> I would do the led_mc_calc_color_components() call here instead of in
> lg_g510_kbd_led_set():
>
> led_mc_calc_color_components(&g15_led->mcdev, brightness);
>
Okay. I'll remove this argument.
> > g15->transfer_buf[0] = 5 + g15_led->led;
> > g15->transfer_buf[1] =
> > - DIV_ROUND_CLOSEST(g15_led->red * brightness, 255);
> > + DIV_ROUND_CLOSEST(subleds[0].intensity * brightness, 255);
>
> The reason to do this here, is because led_mc_calc_color_components()
> already does the scaling of intensity by brightness for you, see
> its code:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/led-class-multicolor.c#n14
>
> So instead of using subleds[x].intensity, you should directly use
> subleds[i].brightness and drop the calculations, resulting in:
>
> g15->transfer_buf[0] = 5 + g15_led->led;
> g15->transfer_buf[1] = subleds[0].brightness;
> g15->transfer_buf[2] = subleds[1].brightness;
> g15->transfer_buf[3] = subleds[2].brightness;
>
Thank you for the information. led API already scales the intensity. :)
> > ret = hid_hw_raw_request(g15->hdev,
> > LG_G510_FEATURE_BACKLIGHT_RGB + g15_led->led,
> > @@ -258,9 +264,11 @@ static int lg_g510_kbd_led_write(struct lg_g15_data *g15,
> > static int lg_g510_kbd_led_set(struct led_classdev *led_cdev,
> > enum led_brightness brightness)
> > {
> > + struct led_classdev_mc *mc = lcdev_to_mccdev(led_cdev);
> > struct lg_g15_led *g15_led =
> > container_of(led_cdev, struct lg_g15_led, cdev);
>
> This container_of() now is incorrect, this assumes that led_cdev points
> to the cdev part of the anonymous union in struct lg_g15_led, but for
> the g510_kbd_led handling the mcdev part of that union is now used.
>
> So the correct container_of() usage would be:
>
> struct lg_g15_led *g15_led =
> container_of(mc, struct lg_g15_led, mcdev);
>
> please fix this.
Got it.
>
> (the old code happens to work because the led_cdev member of
> struct led_classdev_mc happens to be the first member)
>
> > struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> > + struct mc_subled *subleds;
>
> With my proposal above to keep the lg_g510_kbd_led_write() function
> prototype unchanged the other changes to lg_g510_kbd_led_set() can
> be dropped.
>
> > int ret;
> >
> > /* Ignore LED off on unregister / keyboard unplug */
> > @@ -268,7 +276,11 @@ static int lg_g510_kbd_led_set(struct led_classdev *led_cdev,
> > return 0;
> >
> > mutex_lock(&g15->mutex);
> > - ret = lg_g510_kbd_led_write(g15, g15_led, brightness);
> > +
> > + led_mc_calc_color_components(mc, brightness);
> > + subleds = mc->subled_info;
> > +
> > + ret = lg_g510_kbd_led_write(g15, g15_led, subleds, brightness);
> > mutex_unlock(&g15->mutex);
> >
> > return ret;
> > @@ -282,76 +294,15 @@ static enum led_brightness lg_g510_kbd_led_get(struct led_classdev *led_cdev)
> > return g15_led->brightness;
> > }
>
> You also need to update the container_of() in lg_g510_kbd_led_get(),
> so you get:
>
> static enum led_brightness lg_g510_kbd_led_get(struct led_classdev *led_cdev)
> {
> struct led_classdev_mc *mc = lcdev_to_mccdev(led_cdev);
> struct lg_g15_led *g15_led =
> container_of(mc, struct lg_g15_led, mcdev);
>
> return g15_led->brightness;
> }
Okay
>
>
> > -static ssize_t color_store(struct device *dev, struct device_attribute *attr,
> > - const char *buf, size_t count)
> > -{
> > - struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > - struct lg_g15_led *g15_led =
> > - container_of(led_cdev, struct lg_g15_led, cdev);
> > - struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> > - unsigned long value;
> > - int ret;
> > -
> > - if (count < 7 || (count == 8 && buf[7] != '\n') || count > 8)
> > - return -EINVAL;
> > -
> > - if (buf[0] != '#')
> > - return -EINVAL;
> > -
> > - ret = kstrtoul(buf + 1, 16, &value);
> > - if (ret)
> > - return ret;
> > -
> > - mutex_lock(&g15->mutex);
> > - g15_led->red = (value & 0xff0000) >> 16;
> > - g15_led->green = (value & 0x00ff00) >> 8;
> > - g15_led->blue = (value & 0x0000ff);
> > - ret = lg_g510_kbd_led_write(g15, g15_led, g15_led->brightness);
> > - mutex_unlock(&g15->mutex);
> > -
> > - return (ret < 0) ? ret : count;
> > -}
> > -
> > -static ssize_t color_show(struct device *dev, struct device_attribute *attr,
> > - char *buf)
> > -{
> > - struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > - struct lg_g15_led *g15_led =
> > - container_of(led_cdev, struct lg_g15_led, cdev);
> > - struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> > - ssize_t ret;
> > -
> > - mutex_lock(&g15->mutex);
> > - ret = sprintf(buf, "#%02x%02x%02x\n",
> > - g15_led->red, g15_led->green, g15_led->blue);
> > - mutex_unlock(&g15->mutex);
> > -
> > - return ret;
> > -}
> > -
> > -static DEVICE_ATTR_RW(color);
> > -
> > -static struct attribute *lg_g510_kbd_led_attrs[] = {
> > - &dev_attr_color.attr,
> > - NULL,
> > -};
> > -
> > -static const struct attribute_group lg_g510_kbd_led_group = {
> > - .attrs = lg_g510_kbd_led_attrs,
> > -};
> > -
> > -static const struct attribute_group *lg_g510_kbd_led_groups[] = {
> > - &lg_g510_kbd_led_group,
> > - NULL,
> > -};
> > -
> > static void lg_g510_leds_sync_work(struct work_struct *work)
> > {
> > struct lg_g15_data *g15 = container_of(work, struct lg_g15_data, work);
> > + struct led_classdev_mc *mc = &g15->leds[LG_G15_KBD_BRIGHTNESS].mcdev;
> > + struct lg_g15_led *g15_led = &g15->leds[LG_G15_KBD_BRIGHTNESS];
> > + struct mc_subled *subleds = mc->subled_info;
> >
> > mutex_lock(&g15->mutex);
> > - lg_g510_kbd_led_write(g15, &g15->leds[LG_G15_KBD_BRIGHTNESS],
> > - g15->leds[LG_G15_KBD_BRIGHTNESS].brightness);
> > + lg_g510_kbd_led_write(g15, g15_led, subleds, g15_led->brightness);
> > mutex_unlock(&g15->mutex);
>
> With my proposal above to keep the lg_g510_kbd_led_write() function
> prototype unchanged all changes to lg_g510_leds_sync_work() can be dropped.
>
Got it and I'll drop it.
>
> > }
> >
> > @@ -667,8 +618,47 @@ static void lg_g15_input_close(struct input_dev *dev)
> > hid_hw_close(hdev);
> > }
> >
> > +static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
> > +{
> > + int i;
> > + struct mc_subled *subled_info;
> > +
> > + g15->leds[index].mcdev.led_cdev.brightness_set_blocking =
> > + lg_g510_kbd_led_set;
> > + g15->leds[index].mcdev.led_cdev.brightness_get =
> > + lg_g510_kbd_led_get;
> > + g15->leds[index].mcdev.led_cdev.max_brightness = g15->leds[index].brightness;
>
> max_brightness should be 255, not the current brightness:
>
> g15->leds[index].mcdev.led_cdev.max_brightness = 255;
Okay
>
> > + g15->leds[index].mcdev.num_colors = 3;
> > +
> > + subled_info = devm_kcalloc(&g15->hdev->dev, 3, sizeof(*subled_info), GFP_KERNEL);
> > + if (!subled_info)
> > + return;
> > +
> > + for (i = 0; i < 3; i++) {
> > + switch (i + 1) {
> > + case LED_COLOR_ID_RED:
> > + subled_info[i].color_index = LED_COLOR_ID_RED;
> > + subled_info[i].intensity = g15->leds[index].red;
> > + break;
> > + case LED_COLOR_ID_GREEN:
> > + subled_info[i].color_index = LED_COLOR_ID_GREEN;
> > + subled_info[i].intensity = g15->leds[index].green;
> > + break;
> > + case LED_COLOR_ID_BLUE:
> > + subled_info[i].color_index = LED_COLOR_ID_BLUE;
> > + subled_info[i].intensity = g15->leds[index].blue;
> > + break;
> > + }
> > + subled_info[i].channel = i;
> > + subled_info[i].intensity = 255;
>
> You are already setting subled_info[i].intensity to the correct value above,
> which you are overriding here, so this line should be dropped.
Okay
>
>
> > + }
> > + g15->leds[index].mcdev.subled_info = subled_info;
> > +}
> > +
> > static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
> > {
> > + int ret;
> > +
> > g15->leds[i].led = i;
> > g15->leds[i].cdev.name = name;
> >
> > @@ -685,6 +675,7 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
> > } else {
> > g15->leds[i].cdev.max_brightness = 1;
> > }
> > + ret = devm_led_classdev_register(&g15->hdev->dev, &g15->leds[i].cdev);
> > break;
> > case LG_G510:
> > case LG_G510_USB_AUDIO:
> > @@ -697,12 +688,11 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
> > g15->leds[i].cdev.name = "g15::power_on_backlight_val";
> > fallthrough;
> > case LG_G15_KBD_BRIGHTNESS:
> > - g15->leds[i].cdev.brightness_set_blocking =
> > - lg_g510_kbd_led_set;
> > - g15->leds[i].cdev.brightness_get =
> > - lg_g510_kbd_led_get;
> > - g15->leds[i].cdev.max_brightness = 255;
> > - g15->leds[i].cdev.groups = lg_g510_kbd_led_groups;
> > + /* register multicolor */
> > + lg_g15_setup_led_rgb(g15, i);
> > + ret = devm_led_classdev_multicolor_register_ext(&g15->hdev->dev,
> > + &g15->leds[i].mcdev,
> > + NULL);
> > break;
> > default:
> > g15->leds[i].cdev.brightness_set_blocking =
> > @@ -710,11 +700,12 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
> > g15->leds[i].cdev.brightness_get =
> > lg_g510_mkey_led_get;
> > g15->leds[i].cdev.max_brightness = 1;
> > + ret = devm_led_classdev_register(&g15->hdev->dev, &g15->leds[i].cdev);
> > }
> > break;
> > }
> >
> > - return devm_led_classdev_register(&g15->hdev->dev, &g15->leds[i].cdev);
> > + return ret;
> > }
> >
> > /* Common input device init code shared between keyboards and Z-10 speaker handling */
>
> Regards,
>
> Hans
>
>
>
I'll propose a v2 patch to include the fixes mentioned above.
--
BR,
Kate
