linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Roderick Colenbrander <roderick@gaikai.com>
To: "Barnabás Pőcze" <pobrn@protonmail.com>
Cc: Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	"linux-input@vger.kernel.org" <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: Mon, 28 Dec 2020 14:02:44 -0800	[thread overview]
Message-ID: <CANndSKkjhEDU0eB=J_F0CCZn2F=Jy3UMzBKOgJaNvecqRtAfnQ@mail.gmail.com> (raw)
In-Reply-To: <8ROOgrBVvXav7SOnZgBVtvZ4QKubyktYJ2WsauoAH22lewKj2-Rp9a-uEZyxhItvF3Q7pp8hKZ4etl0jgiD-G3XkbL1aM9z8cRNsHHmGN9A=@protonmail.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

  reply	other threads:[~2020-12-28 23:12 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
2020-12-26 23:07     ` Roderick Colenbrander
2020-12-27 14:27   ` Barnabás Pőcze
2020-12-28 22:02     ` Roderick Colenbrander [this message]
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='CANndSKkjhEDU0eB=J_F0CCZn2F=Jy3UMzBKOgJaNvecqRtAfnQ@mail.gmail.com' \
    --to=roderick@gaikai.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=lzye@google.com \
    --cc=pobrn@protonmail.com \
    --cc=roderick.colenbrander@sony.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