Re: [PATCH 11/13] HID: playstation: add DualSense player LEDs support.

["Samuel Čavoj" <samuel@cavoj.net>]

Hi,

I noticed that the `value` argument is not at all used in the
player_led_set_brightness function.

On 18.12.2020 22:23, Roderick Colenbrander wrote:
> +
> +static void dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);
> +	uint8_t player_leds_state = 0;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> +		player_leds_state |= (ds->player_leds[i].brightness << i);

Is it guaranteed at this point, that the led->brightness is set to the
new value? I'm unfamiliar with the led subsystem, but skimming other
drivers I found that they update the device based on the value of the
`value` argument.

> +
> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->player_leds_state = player_leds_state;
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}

Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
set the brightness attribute, but the _nopm one does not and it is
exported, although it is not used anywhere other than led-core.c and
led-class.c.

However, I find the usage in led_classdev_suspend and _resume
interesting. In suspend, set_brightness_nopm is called with a value of
0, which should turn off the LED while retaining the value of the
brightness attribute, which is later recalled in _resume. I assume the
intended behaviour is the LED to turn off when suspending and return to
its original state on resume, without overwriting the attribute.

Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
can be different from led->brightness *on purpose* and should be used
instead.

I would write something along these lines:

for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
	if (&ds->player_leds[i] == led) {
		if (value == LED_OFF)
			player_leds_state &= ~(1 << i);
		else
			player_leds_state |= (1 << i);
		break;
	}
}

This is just me hypothesizing though, could anyone clear this up for
me? Thank you very much.

Regards,
Samuel