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
