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

[Roderick Colenbrander <roderick@gaikai.com>]

Hi Barnabás,

On Sun, Dec 27, 2020 at 6:27 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 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?

I think this pointer math would work and need to give it a try. Hadn't
considered it

> 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?

Not all values may get set through sysfs. For example in the next
patch (12/13) the driver sets a default player LED mask value directly
and may set e.g. 0x1f or so. This could use the LED APIs, but the LED
framework doesn't have any group LED support (besides the new
multicolor class) and as such would get scheduled across multiple
output reports.

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

I will give this a try as well, thanks.

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

Thanks,
Roderick