linux-kernel - Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <121c2aa1-c89b-49bc-c71e-73009b4e04fe@linux.intel.com>
Date: Mon, 12 Aug 2024 18:57:31 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Carlos Ferreira <carlosmiguelferreira.2003@...il.com>
cc: mustafa.eskieksi@...il.com, Hans de Goede <hdegoede@...hat.com>, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb

On Mon, 12 Aug 2024, Hans de Goede wrote:

> Hi,
> 
> Thank you for the new version, much better, almost there I would say.
> 
> On 7/19/24 11:59 AM, Carlos Ferreira wrote:
> > This driver adds supports for 4 zone keyboard rgb on omen laptops
> > using the multicolor led api.
> > 
> > Tested on the HP Omen 15-en1001np.
> > 
> > Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@...il.com>

> > +static int __init fourzone_leds_init(struct platform_device *device)
> > +{
> > +	enum led_brightness hw_brightness;
> > +	u32 colors[KBD_ZONE_COUNT * 3];
> > +	int ret, i, j;
> > +
> > +	ret = fourzone_get_hw_colors(colors);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	hw_brightness = fourzone_get_hw_brightness();
> > +
> > +	for (i = 0; i < KBD_ZONE_COUNT; i++) {
> > +		for (j = 0; j < 3; j++)
> > +			fourzone_leds[i].subleds[j] = (struct mc_subled) {
> > +				.color_index = j + 1,
> > +				.brightness = hw_brightness ? colors[i * 3 + j] : 0,
> 
> I think it would be cleaner to drop setting subled brightness here and instead
> call led_mc_calc_color_components() below ... :
> 
> > +				.intensity = colors[i * 3 + j],
> > +			};
> > +
> > +		fourzone_leds[i].mc_led = (struct led_classdev_mc) {
> > +			.led_cdev = {
> > +				.name = fourzone_zone_names[i],
> > +				.brightness = hw_brightness ? 255 : 0,
> > +				.max_brightness = 255,
> > +				.brightness_set = fourzone_set_brightness,
> > +				.color = LED_COLOR_ID_RGB,
> > +				.flags = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN,
> > +			},
> > +			.num_colors = 3,
> > +			.subled_info = fourzone_leds[i].subleds;
> > +		};
> > +
> 
> With this all setup, you can now call:
> 
> 		led_mc_calc_color_components(&fourzone_leds[i].mc_led, fourzone_leds[i].mc_led.led_cdev.brightness);

One additional thing, having a temporary variable for fourzone_leds[i] 
would be advicable to reduce the line lengths / complexity of the 
expressions.

> here, this makes how the subled brightness is set here (on init) identical with
> how it is done on set_brightness calls which is more consistent.
> 
> > +		fourzone_leds[i].brightness = 255;
> > +
> > +		ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds[i].mc_led);
> > +		if (ret)
> > +			return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}

-- 
 i.