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

["Barnabás Pőcze" <pobrn@protonmail.com>]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ad8eedd3d2bf..384449e3095d 100644
> [...]
> +static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
> +		if (&ds->player_leds[i] == led)
> +			return !!(ds->player_leds_state & BIT(i));
> +	}

Is there any reason why

```
  !!(ds->player_leds_state & BIT( led - ds->player_leds ))
```

or something similar is not used? Or am I missing something that prevents this
from working?

Furthermore, I don't quite see the purpose of this function. The LED core
can handle if no brightness_get() callback is provided. And since this
function returns just a cached value, I fail to see how it is different from
the default behaviour of the LED core, which is returning the last brightness
value. Am I missing something?


> +
> +	return LED_OFF;
> +}
> +
> +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);
> +

I believe this could be simplified as well using the fact that
`led - ds->player_leds` gives the index of the LED.


> +	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);
> +}
> [...]


Regards,
Barnabás Pőcze