Re: [PATCH v2 3/3] HID: playstation: expose DualSense player LEDs through LED class.

[Roderick Colenbrander <thunderbird2k@gmail.com>]

Hi Pavel,

On Fri, Sep 3, 2021 at 9:25 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > The DualSense player LEDs were so far not adjustable from user-space.
> > This patch exposes each LED individually through the LED class. Each
> > LED uses the new 'player' function resulting in a name like:
> > 'inputX:white:player-1' for the first LED.
> >
> >
> > +struct ps_led_info {
> ...
> > +     enum led_brightness (*brightness_get)(struct led_classdev *cdev);
> > +     void (*brightness_set)(struct led_classdev *cdev, enum led_brightness);
> > +};
>
> Do you need these fields? They are constant in the driver...

Currently they are constant, but over time support for some other
devices (e.g. DualShock 4) will be moved into this driver as well and
then these are needed.

>
> > +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);
> > +
> > +     return !!(ds->player_leds_state & BIT(led - ds->player_leds));
> > +}
>
> You should not need to implement get if all it does is returning cached state.

The player LEDs are configured at driver loading time with an initial
value by calling 'dualsense_set_player_leds'. This is done behind the
back of the LED framework, so it is driver internal state. The problem
is that we need to set multiple LEDs at once at driver startup and
they share a common HID output report. The LED framework has no 'group
LED support' (outside the new multicolor class), so there is no way to
really synchronize multiple LEDs right now.

> > +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);
> > +     unsigned long flags;
> > +     unsigned int led_index;
> > +
> > +     spin_lock_irqsave(&ds->base.lock, flags);
> > +
> > +     led_index = led - ds->player_leds;
> > +     if (value == LED_OFF)
> > +             ds->player_leds_state &= ~BIT(led_index);
> > +     else
> > +             ds->player_leds_state |= BIT(led_index);
> > +
> > +     ds->update_player_leds = true;
> > +     spin_unlock_irqrestore(&ds->base.lock, flags);
> > +
> > +     schedule_work(&ds->output_worker);
> > +}
>
> LED core can handle scheduling the work for you. You should use it, in similar
> way you handle the RGB led.

I'm not entirely sure what you meant. I guess you mean I should use
'cdev->brightness_set_blocking' instead of 'cdev->brightness_set' when
setting up the LED, so LED core can do more things itself? That's the
main difference I saw between my RGB and regular LED code. Both rely
on updating the internal dualsense structure and calling schedule_work
to schedule a HID output report (the output report contains data for
many things outside of just the LEDs). Is that indeed what you meant
or did I miss something (it is getting late here)?

>
> Best regards,
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Regards,
Roderick