| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8ad71a05-db44-4d84-bb07-317bf6b31855@collabora.com>
Date: Thu, 12 Jun 2025 12:14:14 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Roderick Colenbrander <thunderbird2k@...il.com>
Cc: Roderick Colenbrander <roderick.colenbrander@...y.com>,
Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>,
Henrik Rydberg <rydberg@...math.org>, kernel@...labora.com,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/11] HID: playstation: Support DualSense audio jack
hotplug detection
Hi Roderick,
On 6/10/25 8:04 AM, Roderick Colenbrander wrote:
> Hi Christian,
>
> Thanks for sharing I need to have a closer look to see what we do on
> the console as well (we want to keep behavior where possible in sync
> also due to official PlayStation use cases on embedded devices).
>
> At least use this logic only for USB, we should not register these
> nodes or apply such settings for Bluetooth. Just setting these on
> bluetooth without audio I could see affecting battery life a bit. But
> in addition, I honestly don't really see a path to supporting
> Bluetooth audio for the DualSense. It would add confusion for users.
> The reason, I don't think there is a good way for adding audio is that
> the DualSense is very, very complex. In Bluetooth mode we have our own
> ways of doing audio (non-standard), which is already tricky. Above all
> else the audio is compressed and not PCM. The only way to do it is
> probably in userspace in something PulseAudio like. So before rushing
> adding this also for Bluetooth, I want to limit this to USB.
Thanks for shedding some light on the Bluetooth side of things - I'll
make sure the introduced changes will be further restricted to USB only.
> I'm sharing some other feedback in line. It is small stuff, but in
> particular I like to keep datasheets and code in sync, so of it is
> around naming. It allows us internally for easier review as well.
Yes, please highlight any DS inconsistencies you noticed and we'll
get them fixed in the next revision.
> There are some areas I need to study some more and get internal
> documentation, so I don't have thorough feedback on the audio code
> itself yet.
No worries, please let me know when I can start preparing v2.
> Thanks,
> Roderick
>
> On Mon, May 26, 2025 at 6:04 AM Cristian Ciocaltea
> <cristian.ciocaltea@...labora.com> wrote:
>>
>> The default audio output path on DualSense controller hardware is set to
>> headphones, regardless of whether they are actually inserted or not.
>>
>> Detect when the plugged state of the 3.5mm audio jack changes and toggle
>> audio output between headphones and internal speaker, as required. The
>> latter is achieved by essentially routing the right channel of the audio
>> source to the mono speaker.
>>
>> Additionally, adjust the speaker volume since its default level is too
>> low and, therefore, cannot generate any audible sound.
>>
>> It's worth noting the audio functionality is currently not supported for
>> Bluetooth, hence it's limited to USB connectivity.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>> drivers/hid/hid-playstation.c | 86 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 83 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
>> index b7777d3230b2fe277a4a2217ef6b1eff9cfad182..ed8e67c5bf5d3a9e9721da31a6bd84f0b6802b14 100644
>> --- a/drivers/hid/hid-playstation.c
>> +++ b/drivers/hid/hid-playstation.c
>> @@ -117,6 +117,12 @@ struct ps_led_info {
>> #define DS_STATUS_BATTERY_CHARGING GENMASK(7, 4)
>> #define DS_STATUS_BATTERY_CHARGING_SHIFT 4
>>
>> +/* Plugged status field of DualSense input report. */
>> +#define DS_STATUS_PLUGGED_HEADPHONES BIT(0)
>> +#define DS_STATUS_PLUGGED_MIC BIT(1)
>> +#define DS_STATUS_PLUGGED_JACK (DS_STATUS_PLUGGED_HEADPHONES | \
>> + DS_STATUS_PLUGGED_MIC)
>
> Like I mentioned in patch 10, there are 3 status bytes. So let's call
> this DS_STATUS1_. The other 5 bits in this one are unrelated to audio.
>
> Official datasheets call these 'HP_DETECT', 'MIC_DETECT' and
> 'MIC_MUTE'. I guess we should use those names.
Yep, will do.
>> +
>> /* Feature version from DualSense Firmware Info report. */
>> #define DS_FEATURE_VERSION_MINOR GENMASK(7, 0)
>> #define DS_FEATURE_VERSION_MAJOR GENMASK(15, 8)
>> @@ -144,13 +150,18 @@ struct ps_led_info {
>> /* Flags for DualSense output report. */
>> #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
>> #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
>> +#define DS_OUTPUT_VALID_FLAG0_SPEAKER_VOLUME_ENABLE BIT(5)
> Just for completeness also add BIT(6) which is MIC_VOLUME_ENABLE
>> +#define DS_OUTPUT_VALID_FLAG0_AUDIO_CONTROL_ENABLE BIT(7)
>> #define DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE BIT(0)
>> #define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
>> #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
>> #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
>> #define DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE BIT(4)
>> +#define DS_OUTPUT_VALID_FLAG1_AUDIO_CONTROL2_ENABLE BIT(7)
>> #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
>> #define DS_OUTPUT_VALID_FLAG2_COMPATIBLE_VIBRATION2 BIT(2)
>> +#define DS_OUTPUT_AUDIO_FLAGS_OUTPUT_PATH_SEL GENMASK(5, 4)
>> +#define DS_OUTPUT_AUDIO_FLAGS2_SP_PREAMP_GAIN GENMASK(2, 0)
>> #define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
>> #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
>>
>> @@ -193,6 +204,11 @@ struct dualsense {
>> u8 lightbar_green;
>> u8 lightbar_blue;
>>
>> + /* Audio Jack plugged state */
>> + u8 plugged_state;
>> + u8 prev_plugged_state;
>> + bool prev_plugged_state_valid;
>> +
>> /* Microphone */
>> bool update_mic_mute;
>> bool mic_muted;
>> @@ -237,7 +253,8 @@ struct dualsense_input_report {
>>
>> u8 reserved3[12];
>> u8 status_battery;
>> - u8 reserved4[10];
>> + u8 status_plugged;
>> + u8 reserved4[9];
>> } __packed;
>> /* Common input report size shared equals the size of the USB report minus 1 byte for ReportID. */
>> static_assert(sizeof(struct dualsense_input_report) == DS_INPUT_REPORT_USB_SIZE - 1);
>> @@ -252,11 +269,15 @@ struct dualsense_output_report_common {
>> u8 motor_left;
>>
>> /* Audio controls */
>> - u8 reserved[4];
>> + u8 headphone_audio_volume; /* 0-0x7f */
> Just call this headphone_volume
Ack.
>> + u8 speaker_audio_volume; /* 0-0xff */
> Just call this speaker_volume
Ack.
>> + u8 internal_microphone_volume; /* 0-0x40 */
> We call this one mic_volume
Ack.
>> + u8 audio_flags;
> We call this one audio_control.
Ack.
>
>> u8 mute_button_led;
>>
>> u8 power_save_control;
>> - u8 reserved2[28];
>> + u8 reserved2[27];
>> + u8 audio_flags2;
> This one we call audio_control2
Ack.
>>
>> /* LEDs and lightbar */
>> u8 valid_flag2;
[...]
Thanks for the review,
Cristian
Powered by blists - more mailing lists