linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Samuel Čavoj" <samuel@cavoj.net>
To: Roderick Colenbrander <roderick@gaikai.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	linux-input@vger.kernel.org, Chris Ye <lzye@google.com>,
	Roderick Colenbrander <roderick.colenbrander@sony.com>
Subject: Re: [PATCH 11/13] HID: playstation: add DualSense player LEDs support.
Date: Sat, 26 Dec 2020 20:27:12 +0100	[thread overview]
Message-ID: <20201226192712.ky56nhdbh6n43bp6@fastboi.localdomain> (raw)
In-Reply-To: <20201219062336.72568-12-roderick@gaikai.com>

Hi,

I noticed that the `value` argument is not at all used in the
player_led_set_brightness function.

On 18.12.2020 22:23, Roderick Colenbrander wrote:
> +
> +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);

Is it guaranteed at this point, that the led->brightness is set to the
new value? I'm unfamiliar with the led subsystem, but skimming other
drivers I found that they update the device based on the value of the
`value` argument.

> +
> +	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);
> +}

Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
set the brightness attribute, but the _nopm one does not and it is
exported, although it is not used anywhere other than led-core.c and
led-class.c.

However, I find the usage in led_classdev_suspend and _resume
interesting. In suspend, set_brightness_nopm is called with a value of
0, which should turn off the LED while retaining the value of the
brightness attribute, which is later recalled in _resume. I assume the
intended behaviour is the LED to turn off when suspending and return to
its original state on resume, without overwriting the attribute.

Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
can be different from led->brightness *on purpose* and should be used
instead.

I would write something along these lines:

for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
	if (&ds->player_leds[i] == led) {
		if (value == LED_OFF)
			player_leds_state &= ~(1 << i);
		else
			player_leds_state |= (1 << i);
		break;
	}
}

This is just me hypothesizing though, could anyone clear this up for
me? Thank you very much.

Regards,
Samuel

  reply	other threads:[~2020-12-26 19:28 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-19  6:23 [PATCH 00/13] HID: new driver for PS5 'DualSense' controller Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 01/13] HID: playstation: initial DualSense USB support Roderick Colenbrander
2020-12-27 16:23   ` Barnabás Pőcze
2020-12-27 23:04     ` Roderick Colenbrander
2020-12-29 19:12       ` Barnabás Pőcze
2020-12-31  0:08   ` Barnabás Pőcze
2020-12-31  1:08     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 02/13] HID: playstation: use DualSense MAC address as unique identifier Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 03/13] HID: playstation: add DualSense battery support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 04/13] HID: playstation: add DualSense touchpad support Roderick Colenbrander
2020-12-26  2:14   ` Samuel Čavoj
2020-12-26 22:27     ` Roderick Colenbrander
2020-12-29 19:49   ` Barnabás Pőcze
2020-12-29 21:44     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 05/13] HID: playstation: add DualSense accelerometer and gyroscope support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 06/13] HID: playstation: track devices in list Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 07/13] HID: playstation: add DualSense Bluetooth support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 08/13] HID: playstation: add DualSense classic rumble support Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 09/13] HID: playstation: add DualSense lightbar support Roderick Colenbrander
2020-12-27 14:41   ` Barnabás Pőcze
2020-12-28 21:26     ` Roderick Colenbrander
2020-12-29 18:59       ` Barnabás Pőcze
2020-12-29 19:54         ` Roderick Colenbrander
2020-12-29 20:22           ` Barnabás Pőcze
2020-12-19  6:23 ` [PATCH 10/13] HID: playstation: add microphone mute support for DualSense Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 11/13] HID: playstation: add DualSense player LEDs support Roderick Colenbrander
2020-12-26 19:27   ` Samuel Čavoj [this message]
2020-12-26 23:07     ` Roderick Colenbrander
2020-12-27 14:27   ` Barnabás Pőcze
2020-12-28 22:02     ` Roderick Colenbrander
2020-12-29 18:49       ` Barnabás Pőcze
2020-12-19  6:23 ` [PATCH 12/13] HID: playstation: DualSense set LEDs to default player id Roderick Colenbrander
2020-12-27  0:08   ` Samuel Čavoj
2020-12-27 23:07     ` Roderick Colenbrander
2020-12-19  6:23 ` [PATCH 13/13] HID: playstation: report DualSense hardware and firmware version Roderick Colenbrander
2020-12-27 17:06   ` Barnabás Pőcze
2020-12-27 22:21     ` Roderick Colenbrander
2020-12-27 22:27       ` Roderick Colenbrander
2020-12-27 22:37         ` Barnabás Pőcze
2020-12-28 22:45           ` Roderick Colenbrander
2020-12-29 15:10             ` Barnabás Pőcze
2020-12-19  8:38 ` [PATCH 00/13] HID: new driver for PS5 'DualSense' controller Bastien Nocera
2020-12-19 22:39   ` Roderick.Colenbrander

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201226192712.ky56nhdbh6n43bp6@fastboi.localdomain \
    --to=samuel@cavoj.net \
    --cc=benjamin.tissoires@redhat.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=lzye@google.com \
    --cc=roderick.colenbrander@sony.com \
    --cc=roderick@gaikai.com \
    --subject='Re: [PATCH 11/13] HID: playstation: add DualSense player LEDs support.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox