Hi Samuel,

Thanks for your review!

On Sat, Dec 26, 2020 at 11:27 AM Samuel Čavoj [off-list ref] wrote:

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:

quoted

+
+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.

See comments below. Normally yes the brightness is already updated,
but as mentioned below it might still be best to change the code.

quoted

+
+     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.

Thanks for your deep analysis. While writing the code I already found
it strange there were 2 layers of bookkeeping. As you point out this
is likely due to the power management logic.

The LEDs are created by 'ps_led_register' and it is setting:
    led->flags = LED_CORE_SUSPENDRESUME;

So those code paths would indeed be triggered provided
'CONFIG_PM_SLEEP' is set. So it is probably safest for me to change
that for-loop anyway. I had hoped to skip the 'find the LED logic' (it
just feels ugly somehow, but not a big deal). Let me make this quick
change. I will just wait a few more days on potential other feedback
from the list before submitting a 'v2'. So far nothing major, but just
these few small things :)

Regards,
Samuel

Thanks,
Roderick


-- 
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com