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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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