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

[Pavel Machek <pavel@ucw.cz>]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html