Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Wed, 4 Oct 2023 21:08:10 +0200 (CEST)
From:   Jiri Kosina <jikos@kernel.org>
To:     Martino Fontana <tinozzo123@gmail.com>
cc:     djogorchock@gmail.com, benjamin.tissoires@redhat.com,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 RESEND] HID: nintendo: cleanup LED code
In-Reply-To: <20230924141547.11597-1-tinozzo123@gmail.com>
Message-ID: <nycvar.YFH.7.76.2310042107570.3534@cbobk.fhfr.pm>
References: <20230924141547.11597-1-tinozzo123@gmail.com>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 24 Sep 2023, Martino Fontana wrote:

> - Support player LED patterns up to 8 players.
>   (Note that the behavior still consinsts in increasing the player number
>   every time a controller is connected, never decreasing it. It should be
>   as is described in https://bugzilla.kernel.org/show_bug.cgi?id=216225.
>   However, any implementation here would stop making sense as soon as a
>   non-Nintendo controller is connected, which is why I'm not bothering.)
> 
> - Split part of `joycon_home_led_brightness_set` (which is called by hid)
>   into `joycon_set_home_led` (which is what actually sets the LEDs), for
>   consistency with player LEDs.
> 
> - `joycon_player_led_brightness_set` won't try it to "determine which
>   player led this is" anymore: it's already looking at every LED
>   brightness value.
> 
> - Instead of first registering the `led_classdev`, then attempting to set
>   the LED and unregistering the `led_classdev` if it fails, first attempt
>   to set the LED, then register the `led_classdev` only if it succeeds
>   (the class is still filled up in either case).
> 
> - If setting the player LEDs fails, still attempt setting the home LED.
>   (I don't know there's a third party controller where this may actually
>   happen, but who knows...)
> 
> - Use `JC_NUM_LEDS` where appropriate instead of 4.
> 
> - Print return codes in more places.
> 
> - Use spinlock instead of mutex for `input_num`. Copy its value to a local
>   variable, so that it can be unlocked immediately.
> 
> - `input_num` starts counting from 0
> 
> - Less holding of mutexes in general.
> 
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> ---
> Changes for v2:
> 
> Applied suggestions, commit message explains more stuff, restored `return ret`
> when `devm_led_classdev_register` fails (since all other hid drivers do this).
> If setting LED fails, `hid_warn` now explicitly says "skipping registration".
> 
> Changes for v3 and v4:
> 
> Fixed errors and warnings from test robot.
> 
> Changes for v5:
> 
> I thought that when connecting the controller on an actual Nintendo Switch,
> only the nth player LED would turn on, which is true... on Wii and Wii U.
> So I reverted that, and to compensate, now this supports the LED patterns
> up to 8 players.
> 
>  drivers/hid/hid-nintendo.c | 133 +++++++++++++++++++++----------------
>  1 file changed, 76 insertions(+), 57 deletions(-)

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs