From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4648C433E0 for ; Tue, 16 Feb 2021 17:23:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9E71264E07 for ; Tue, 16 Feb 2021 17:23:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231133AbhBPRWq (ORCPT ); Tue, 16 Feb 2021 12:22:46 -0500 Received: from lists.nic.cz ([217.31.204.67]:60470 "EHLO mail.nic.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230518AbhBPRWi (ORCPT ); Tue, 16 Feb 2021 12:22:38 -0500 Received: from localhost (unknown [IPv6:2a0e:b107:ae1:0:3e97:eff:fe61:c680]) by mail.nic.cz (Postfix) with ESMTPSA id AC049140BC8; Tue, 16 Feb 2021 18:21:55 +0100 (CET) Date: Tue, 16 Feb 2021 18:21:55 +0100 From: Marek Behun To: Benjamin Tissoires Cc: Roderick Colenbrander , Jiri Kosina , Pavel Machek , Dmitry Torokhov , pobm@protonmail.com, "open list:HID CORE LAYER" , linux-leds@vger.kernel.org, Roderick Colenbrander Subject: Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense. Message-ID: <20210216182155.4caba859@nic.cz> In-Reply-To: References: <20210215004549.135251-1-roderick@gaikai.com> <20210215004549.135251-3-roderick@gaikai.com> <20210215154045.4ac27ec0@nic.cz> <20210215191757.58992f44@nic.cz> <20210216174129.78b2e9ab@nic.cz> X-Mailer: Claws Mail 3.17.7 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Virus-Scanned: clamav-milter 0.102.2 at mail X-Virus-Status: Clean Precedence: bulk List-ID: X-Mailing-List: linux-leds@vger.kernel.org On Tue, 16 Feb 2021 18:12:39 +0100 Benjamin Tissoires wrote: > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun wrote: > > > > On Tue, 16 Feb 2021 00:33:24 -0800 > > Roderick Colenbrander wrote: > > > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun wrote: > > > > > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > > > Roderick Colenbrander wrote: > > > > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun wrote: > > > > > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > > > Roderick Colenbrander wrote: > > > > > > > > > > > > > From: Roderick Colenbrander > > > > > > > > > > > > > > The DualSense controller has a built-in microphone exposed as an > > > > > > > audio device over USB (or HID using Bluetooth). A dedicated > > > > > > > button on the controller handles mute, but software has to configure > > > > > > > the device to mute the audio stream. > > > > > > > > > > > > > > This patch captures the mute button and schedules an output report > > > > > > > to mute/unmute the audio stream as well as toggle the mute LED. > > > > > > > > > > > > > > Signed-off-by: Roderick Colenbrander > > > > > > > > > > > > Is the microphone supported via Linux? I.e. is there an audio driver > > > > > > for it? > > > > > > > > > > Yes and no. The microphone is supported using USB, not yet using > > > > > Bluetooth (uses a custom protocol). Actually there are various other > > > > > audio features in the DualSense (headphone jack, speaker, volume > > > > > controls,..) and they all work using custom protocols. We were > > > > > planning to defer this work through future patches as the features are > > > > > very complicated and need a deep analysis on how to realize them. For > > > > > example audio controls work through HID, but for USB the audio driver > > > > > is a generic hda audio device I think. Bluetooth is a custom protocol > > > > > and will be yet a different audio driver somewhere. > > > > > > > > > > > If it is, look at the audio-micmute LED trigger. > > > > > > > > > > > > > > > > I'm not sure if the expected behavior for the DualSense is similar to > > > > > the standard audio mute use cases. My understanding of these triggers > > > > > (please correct me if I'm wrong) is for e.g. an audio driver or user > > > > > space to send a signal to anything registering for a particular > > > > > trigger. In this case a global micmute. Is that, right? > > > > > > > > > > In our case for PlayStation games, there are often multiple > > > > > controllers connected and each user has their own microphone in their > > > > > controller. All can function at the same time (different from a > > > > > standard PC use case). That's why I'm wondering if this makes sense.I > > > > > know we are on Linux, but for Sony we want to properly support such > > > > > use cases. > > > > > > > > If there aren't audio drivers yet for this, simply have this driver > > > > also register a private LED trigger (with name "joystick-audiomute" > > > > or something similar), and when registering the LED, set the > > > > trigger_type member. Look at trigger_type in include/linux/leds.h, and > > > > in LED Documentation. > > > > > > Sorry for some more questions. I have been trying to understand > > > triggers all night. The concept is just so strange and foreign to me. > > > I understand it is in the end just a string and one use case is > > > in-kernel IPC and you can configure them from user space as well, but > > > I just don't get it. I understand you can use a trigger to in the end > > > program your LED in a automatic manner. I just don't understand how > > > the concepts fit together and how to implement it (maybe I will update > > > the docs later on... they are a bit sparse for if you don't know this > > > area). > > > > > > Regarding registering a private trigger. I see include/linux/leds.h > > > have a comment about trigger_type and how it should be set for private > > > triggers on led_classdev. I haven't been able to find any example > > > usages of this within the kernel. It doesn't seem to be used in the > > > kernel, maybe it is just around for future use? I also seem to need to > > > implement my own activate/deactive callbacks for the trigger. These I > > > would use to program the LED brightness I guess. Though I see various > > > trigger drivers (drivers/leds/triggers), but not all of them have > > > activate/deactivate callbacks. Mostly simple drivers, but not sure why > > > they don't need them. What else is the point of a trigger? > > > > > > > When this trigger is enabled for your LED, have your code switch LED > > > > state like it does now. When there is no trigger enabled, the userspace > > > > will be able to set brightness of this LED via sysfs. > > > > > > Right now I manage the button mute state directly from the input > > > handler (dualsense_parse_report) when the button is pressed and then > > > schedule an output report to toggle the LED and program the DualSense > > > to mute its audio (the PlayStation works very similar). I would need > > > to use led_trigger_event then here? > > > > > > If I then understand it right, I need to modify my "brightness_set" > > > handler and check if there is a trigger (based on > > > led_classdev->activated??). If there is none, then userspace can > > > change the LED state. Internally when I change the LED state, I will > > > also program the hardware to mute as well. (they are tied together) > > > > > > I am tempted to wait with the trigger code as I really don't understand it. > > > > Simple triggers are just normal triggers but with some simplifying code > > to avoid code repetition. Ignore them for now. > > > > When a trigger is set to a LED via sysfs, the trigger .activate() > > method is called and the led_cdev.trigger is set to point to that > > trigger. > > > > It is then up to the code inside the trigger's .activate() method to > > initialize mechanisms that will control the LED. > > > > For netdev trigger a delayed_work is scheduled periodically, and in each > > execution of that work's callback the netdevice's stats are compared to > > the last ones. If the new stats are greater, the trigger code blinks the > > LED. > > > > So in your case it is pretty simple to implement, because you already > > have the necessary code to manipulate the LED brightness automatically > > according to whether button was pressed. You are setting > > ds->update_mic_mute = true; > > in dualsense_parse_report() and then manipulate the LED in > > dualsense_output_worker(). > > > > Just add another boolean member into struct dualsense: > > bool control_mute_led; > > and change the code in dualsense_output_worker() to only change the > > mute_led brightness is this new member is true. > > > > Add this code to your driver: > > > > static struct led_hw_trigger_type ps_micmute_trigger_type; > > > > When registering the LED in ps_led_register(), also set > > led->trigger_type = &ps_micmute_trigger_type; > > > > Add this functions: > > static int ps_micmute_trig_activate(struct led_classdev *led_cdev) > > { > > struct dualsense *ds = container_of(...); > > > > /* make the worker control mute LED according to mute button */ > > ds->control_mute_led = true; > > > > /* make sure the mute LED shows the current mute button state */ > > ds->update_mic_mute = true; > > schedule_work(&ds->output_worker); > > > > return 0; > > } > > > > static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev) > > { > > struct dualsense *ds = container_of(...); > > > > ds->control_mute_led = false; > > } > > > > static struct led_trigger ps_micmute_trigger = { > > .name = "playstation-micmute", > > .activate = ps_micmute_trig_activate, > > .deactivate = ps_micmute_trig_deactivate, > > .trigger_type = &ps_micmute_trigger_type, > > }; > > > > Add this code to ps_init(): > > int ret; > > > > ret = led_trigger_register(&ps_micmute_trigger); > > if (ret) > > return ret; > > > > And to ps_exit(): > > led_trigger_unregister(&ps_micmute_trigger); > > > > All this will make sure that the driver will manipulate the mute > > LED state only when the playstation-micmute trigger is active on the > > LED. > > > > Moreover if you want this driver to be active on the LED by default, > > set this prior to registering the LED > > led->default_trigger = "playstation-micmute"; > > > > Finally add code to dualsense_mute_led_set_brightness() to make > > userspace/[other LED triggers] able to set mute LED brightness. > > > > The purpose of the .trigger_type member of struct led_classdev and > > struct led_trigger is that if this member is set for a trigger, this > > trigger will only be available for LEDs that have the same trigger_type. > > > > Thanks Marek for the in-depth 101 on LED triggers :) > > However, I am not sure we want to enable LED triggers for the micmute > on the controller itself. In the early discussions with Roderick, I > already suggested the use of the LED triggers, and the problem was > that they are shared system-wide. This is good for many use cases, but > in that particular case, the user expects the mic *of the controller* > to be muted, not everyone's controller's mics.This is a behavior > inherited from the Playstation 5 which would be hard to sell to owners > of the controllers. They are not system wide if private LED trigger API is used, as I explained and as the example code does it in my previous reply. This trigger will only be available for PlayStation micmute LEDs. > So if read-only LEDs are not an option, how about we simply ditch the > LED for micmute in the current code, and have a simple callback > executed by the driver to light up or not the LED when the player > presses the key. Or just revert entirely this commit in the currently > staged series. We can then figure out a better way in the next future > to handle that part. This is irrelevant if you take into account the information I wrote above. > BTW, AFAICT, the only problem we have left (if we put that micmute > issue aside) is about the naming convention. If we fix the naming > shortly, would you have any concerns if we still push that code to > Linus in 5.12-rc0? Yes, if naming is corrected I have no issue with this. LED triggers can be sent to 5.13. Marek