Message-ID: <c39ac349-b1f3-4ab3-bbd7-98450fe4f7e9@redhat.com>
Date: Thu, 9 Jan 2025 14:31:44 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Kate Hsuan <hpa@...hat.com>
Cc: Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hid: hid-lg-g15: Use standard multicolor LED API

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

> 
> Signed-off-by: Kate Hsuan <hpa@...hat.com>
> ---
>  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 */
>  	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);

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

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

(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;
}


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


>  }
>  
> @@ -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;

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


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

Powered by blists - more mailing lists