| Message ID | 20210901223037.2964665-4-roderick.colenbrander@sony.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: playstation: add LED support | expand |
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... > +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. > +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. Best regards, Pavel
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
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index ff2fc315a89d..9b96239bba5f 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c @@ -56,6 +56,13 @@ struct ps_calibration_data { int sens_denom; }; +struct ps_led_info { + const char *name; + const char *color; + enum led_brightness (*brightness_get)(struct led_classdev *cdev); + void (*brightness_set)(struct led_classdev *cdev, enum led_brightness); +}; + /* Seed values for DualShock4 / DualSense CRC32 for different report types. */ #define PS_INPUT_CRC32_SEED 0xA1 #define PS_OUTPUT_CRC32_SEED 0xA2 @@ -531,6 +538,32 @@ static int ps_get_report(struct hid_device *hdev, uint8_t report_id, uint8_t *bu return 0; } +static int ps_led_register(struct ps_device *ps_dev, struct led_classdev *led, + const struct ps_led_info *led_info) +{ + int ret; + + led->name = devm_kasprintf(&ps_dev->hdev->dev, GFP_KERNEL, + "%s:%s:%s", ps_dev->input_dev_name, led_info->color, led_info->name); + + if (!led->name) + return -ENOMEM; + + led->brightness = 0; + led->max_brightness = 1; + led->flags = LED_CORE_SUSPENDRESUME; + led->brightness_get = led_info->brightness_get; + led->brightness_set = led_info->brightness_set; + + ret = devm_led_classdev_register(&ps_dev->hdev->dev, led); + if (ret) { + hid_err(ps_dev->hdev, "Failed to register LED %s: %d\n", led_info->name, ret); + return ret; + } + + return 0; +} + /* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */ static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev, int (*brightness_set)(struct led_classdev *, enum led_brightness)) @@ -822,6 +855,35 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev, return 0; } +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)); +} + +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); +} + static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp, void *buf) { @@ -1207,7 +1269,20 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) struct dualsense *ds; struct ps_device *ps_dev; uint8_t max_output_report_size; - int ret; + int i, ret; + + static const struct ps_led_info player_leds_info[] = { + { LED_FUNCTION_PLAYER "-1", "white", dualsense_player_led_get_brightness, + dualsense_player_led_set_brightness }, + { LED_FUNCTION_PLAYER "-2", "white", dualsense_player_led_get_brightness, + dualsense_player_led_set_brightness }, + { LED_FUNCTION_PLAYER "-3", "white", dualsense_player_led_get_brightness, + dualsense_player_led_set_brightness }, + { LED_FUNCTION_PLAYER "-4", "white", dualsense_player_led_get_brightness, + dualsense_player_led_set_brightness }, + { LED_FUNCTION_PLAYER "-5", "white", dualsense_player_led_get_brightness, + dualsense_player_led_set_brightness } + }; ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL); if (!ds) @@ -1297,6 +1372,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) /* Set default lightbar color. */ dualsense_set_lightbar(ds, 0, 0, 128); /* blue */ + for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) { + const struct ps_led_info *led_info = &player_leds_info[i]; + + ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info); + if (ret < 0) + goto err; + } + ret = ps_device_set_player_id(ps_dev); if (ret) { hid_err(hdev, "Failed to assign player id for DualSense: %d\n", ret);
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. Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com> --- drivers/hid/hid-playstation.c | 85 ++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-)