Re: [PATCH v2] HID: playstation: Support DualSense player LED brightness control

[Roderick Colenbrander]

Hi Kateřina,

Thanks for sharing. I had an initial look and it could work (would
need to look in more detail). However, I do have some concerns as this
is a userspace change, though at first glance a minor one, but it has
major impact.

The problem is applications on Android use the LEDs. We did a lot of
work to expose these through the Android Lights framework and many
applications including major ones (e.g. PlayStation RemotePlay) use
it. So if we were to add this, applications would need to be changed
to deal with handling different values depending on the kernel
version, which wouldn't be fun and in practice is close to impossible
unfortunately.

Thanks,
Roderick

On Tue, Jul 14, 2026 at 11:45 AM k8ie <k8ie@xxxxxxx> wrote:
>
> From: Kateřina Medvědová <k8ie@xxxxxxx>
>
> The DualSense's 5 player indicator LEDs are currently only exposed as
> plain on/off LED classdevs (max_brightness = 1). The controller
> firmware also supports a global brightness level (high/medium/low)
> for whichever player LEDs are currently lit. The driver already
> defines the brightness byte but never uses it
> (dualsense_output_report_common::led_brightness).
>
> This patch wires up the controls for the brightness byte, allowing
> the player indicator brightness to be adjusted.
>
> Increase max_brightness of each player's LED classdev from 1 to 3.
>
> brightness = 0 still turns an LED off, writing brightness = 1..3
> turns the LED on and applies the brightness to all lit player LEDs.
> This is a hardware limitation and the driver reflects that by
> keeping the sysfs brightness of player LEDs in sync. The last
> brightness write wins.
>
> The firmware encoding is inverted (lower is brighter), so map LED
> core brightness values to the device-specific encoding before
> sending reports.
>
> Also set DS_OUTPUT_VALID_FLAG2_LED_BRIGHTNESS_CONTROL_ENABLE
> whenever player LED state is updated so firmware applies the
> brightness byte.
>
> Existing userspace that writes brightness=1 as a simple "on" value
> continues to work, but LEDs will now be lit at the lowest brightness
> instead of the highest brightness.
>
> Tested on a DualSense controller (product ID 0x0ce6) using Bluetooth
> and USB.
>
> Assisted-by: OpenCode:claude-sonnet-5
> Signed-off-by: Kateřina Medvědová <k8ie@xxxxxxx>
> ---
> First-time contributing a patch.
>
> v1 -> v2: Fixed a data race flagged by the list's automated review bot:
> dualsense_player_led_get_brightness() read player_leds_state and
> player_leds_brightness without holding ds->base.lock, while the setter
> updates both fields together under that lock. Now takes the same lock
> in the getter.
>
> The bot also flagged a pre-existing missing cancel_work_sync() in the
> dualsense_create()/dualshock4_create() error paths. That is unrelated
> to this change (present before this patch) and is intentionally left
> unfixed.
> ---
>  drivers/hid/hid-playstation.c | 50 +++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index f9dc9378cf77..58786117ec01 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -164,11 +164,20 @@ struct ps_led_info {
>  #define DS_OUTPUT_VALID_FLAG1_AUDIO_CONTROL2_ENABLE            BIT(7)
>  #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE    BIT(1)
>  #define DS_OUTPUT_VALID_FLAG2_COMPATIBLE_VIBRATION2            BIT(2)
> +#define DS_OUTPUT_VALID_FLAG2_LED_BRIGHTNESS_CONTROL_ENABLE    BIT(0)
>  #define DS_OUTPUT_AUDIO_FLAGS_OUTPUT_PATH_SEL                  GENMASK(5, 4)
>  #define DS_OUTPUT_AUDIO_FLAGS2_SP_PREAMP_GAIN                  GENMASK(2, 0)
>  #define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE                  BIT(4)
>  #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT                     BIT(1)
>
> +/*
> + * Player LED brightness levels. Lower values are brighter; this is inverted
> + * from the LED subsystem's convention where higher values mean brighter.
> + */
> +#define DS_OUTPUT_PLAYER_LED_BRIGHTNESS_HIGH                   0
> +#define DS_OUTPUT_PLAYER_LED_BRIGHTNESS_MEDIUM                 1
> +#define DS_OUTPUT_PLAYER_LED_BRIGHTNESS_LOW                    2
> +
>  /* DualSense hardware limits */
>  #define DS_ACC_RES_PER_G       8192
>  #define DS_ACC_RANGE           (4 * DS_ACC_RES_PER_G)
> @@ -225,6 +234,7 @@ struct dualsense {
>         /* Player leds */
>         bool update_player_leds;
>         u8 player_leds_state;
> +       u8 player_leds_brightness;
>         struct led_classdev player_leds[5];
>
>         struct work_struct output_worker;
> @@ -1219,12 +1229,24 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
>         return 0;
>  }
>
> +/*
> + * The DualSense's player LEDs only support a single, shared brightness level
> + * for all lit LEDs -- there is no per-LED brightness control. We still expose
> + * per-LED on/off state plus 3 brightness levels through each LED classdev's
> + * max_brightness of 3; the last value written by any player LED classdev sets
> + * the shared level for all of them.
> + */
>  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));
> +       guard(spinlock_irqsave)(&ds->base.lock);
> +
> +       if (!(ds->player_leds_state & BIT(led - ds->player_leds)))
> +               return LED_OFF;
> +
> +       return DS_OUTPUT_PLAYER_LED_BRIGHTNESS_LOW + 1 - ds->player_leds_brightness;
>  }
>
>  static int dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
> @@ -1235,10 +1257,18 @@ static int dualsense_player_led_set_brightness(struct led_classdev *led, enum le
>
>         scoped_guard(spinlock_irqsave, &ds->base.lock) {
>                 led_index = led - ds->player_leds;
> -               if (value == LED_OFF)
> +               if (value == LED_OFF) {
>                         ds->player_leds_state &= ~BIT(led_index);
> -               else
> +               } else {
>                         ds->player_leds_state |= BIT(led_index);
> +                       /* Convert Linux brightness (1=dimmest) to firmware
> +                        * scale (0=brightest), clamping to the valid range.
> +                        */
> +                       value = clamp_t(enum led_brightness, value, 1,
> +                                        DS_OUTPUT_PLAYER_LED_BRIGHTNESS_LOW + 1);
> +                       ds->player_leds_brightness =
> +                               DS_OUTPUT_PLAYER_LED_BRIGHTNESS_LOW + 1 - value;
> +               }
>
>                 ds->update_player_leds = true;
>         }
> @@ -1355,6 +1385,10 @@ static void dualsense_output_worker(struct work_struct *work)
>                                 DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
>                         common->player_leds = ds->player_leds_state;
>
> +                       common->valid_flag2 |=
> +                               DS_OUTPUT_VALID_FLAG2_LED_BRIGHTNESS_CONTROL_ENABLE;
> +                       common->led_brightness = ds->player_leds_brightness;
> +
>                         ds->update_player_leds = false;
>                 }
>
> @@ -1742,15 +1776,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         int i, ret;
>
>         static const struct ps_led_info player_leds_info[] = {
> -               { LED_FUNCTION_PLAYER1, "white", 1, dualsense_player_led_get_brightness,
> +               { LED_FUNCTION_PLAYER1, "white", 3, dualsense_player_led_get_brightness,
>                                 dualsense_player_led_set_brightness },
> -               { LED_FUNCTION_PLAYER2, "white", 1, dualsense_player_led_get_brightness,
> +               { LED_FUNCTION_PLAYER2, "white", 3, dualsense_player_led_get_brightness,
>                                 dualsense_player_led_set_brightness },
> -               { LED_FUNCTION_PLAYER3, "white", 1, dualsense_player_led_get_brightness,
> +               { LED_FUNCTION_PLAYER3, "white", 3, dualsense_player_led_get_brightness,
>                                 dualsense_player_led_set_brightness },
> -               { LED_FUNCTION_PLAYER4, "white", 1, dualsense_player_led_get_brightness,
> +               { LED_FUNCTION_PLAYER4, "white", 3, dualsense_player_led_get_brightness,
>                                 dualsense_player_led_set_brightness },
> -               { LED_FUNCTION_PLAYER5, "white", 1, dualsense_player_led_get_brightness,
> +               { LED_FUNCTION_PLAYER5, "white", 3, dualsense_player_led_get_brightness,
>                                 dualsense_player_led_set_brightness }
>         };
>
> --
> 2.55.0
>
>