Message-ID: <CAEc3jaD8tUNW6hkPHDp=iGmdwD5m3uKg0vNtyZr-u1mmPSAkVQ@mail.gmail.com>
Date: Mon, 21 Jul 2025 23:18:15 -0700
From: Roderick Colenbrander <thunderbird2k@...il.com>
To: Cristian Ciocaltea <cristian.ciocaltea@...labora.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 v2 00/11] HID: playstation: Add support for audio jack
 handling on DualSense

Hi Cristian and Jiri,

One thing I forgot to bring up is whether it is best to have the audio
plug logic have its separate input device or have it be part of an
existing (e.g. main gamepad). The patch currently creates a separate
input device. Originally we added multiple input devices (gamepad,
touchpad and sensors) due to axes and button collisions really.

For this feature there is no collision. There are not many devices in
the kernel, which support these audio EV_SW. I see for example the
Switch 2 controller has a mini jack port as well. Some xbox
controllers too (though audio not supported in the kernel from a quick
glance or at least no HID or xpad driver features for them).

I don't have a strong opinion yet. Initial feeling was perhaps have it
on the 'main' input device. But on the other hand, I'm not sure what
software is normally listening for these kinds of EV_SW events. What
would be listening for this like a pipewire?

Thanks,
Roderick

On Mon, Jul 21, 2025 at 10:47 PM Roderick Colenbrander
<thunderbird2k@...il.com> wrote:
>
> Hi Christian,
>
> I just got back from Japan (trip was a bit extended). In the meantime
> I had some of employees had a look as well.
>
> The audio patches towards the end seem to be okay. We tried to dig for
> the official volume numbers, but they were too hard to find (too many
> layers, too many repositories). When we use a PS5, the default volume
> for the headset and speaker are both close to 70% (just eyeballing).
> At the hardware level the volume is quite non-linear and internally we
> use a mapping table (not sure what the curve is based on). For the
> speaker this starts at 0x3d as you found out already. The 70% volume
> for the speaker seems to correspond to a value of 93 and headphones
> 83.
> The set pre-amp gain of 0x2 is a common value we seem to set and means
> +6dB, so change comment around to mean that I guess.
>
> As for the other patches I'm not entirely sure yet. I know they were
> well intended, but let me just say, they rubbed some of my team
> members quite the wrong way resulting in some heavy discussion. I have
> somewhat similar feelings about the ultra strict checkpatch toggle as
> well.
>
> We had to move mountains to be allowed to even upstream controller
> code among our limited time (it is closer to a hobby thing, even
> though many products nowadays use it as well). So that's a factor
> which adds up a bit as well.
>
> I think some of the patches we could live with if it came to it. There
> is no real agreed up full kernel standard (as it is contentious). So
> for example we tend to prefer more uint8_t family, where older kernel
> style was more u8 and the kernel allows for both. I think we would
> probably lean towards keeping it at the modern form.
>
> Some of the macros also felt a little too magical. Our feeling tends
> to be if you have to go many layers deep to understand what a macro or
> line of code does (and it is easier to then printk the value),
> something feels off...
>
> Thanks,
>
> Roderick Colenbrander
> Sr. Director - Hardware & Systems Engineering
> Sony Interactive Entertainment, LLC
>
> On Thu, Jul 10, 2025 at 2:31 PM Roderick Colenbrander
> <thunderbird2k@...il.com> wrote:
> >
> > Hi Cristian,
> >
> > I'm on a business trip to Japan, I started looking during my flight
> > last weekend. But due to meetings haven't had a lot of opportunity. I
> > may have a little bit of time in the coming days in between meetings.
> >
> > Thanks,
> > Roderick
> >
> > On Wed, Jul 9, 2025 at 10:24 PM Cristian Ciocaltea
> > <cristian.ciocaltea@...labora.com> wrote:
> > >
> > > Hi Roderick,
> > >
> > > On 7/3/25 10:48 AM, Jiri Kosina wrote:
> > > > On Wed, 25 Jun 2025, Cristian Ciocaltea wrote:
> > > >
> > > >> The Sony DualSense wireless controller (PS5) provides an internal mono
> > > >> speaker, in addition to the 3.5mm jack socket for headphone output and
> > > >> headset microphone input.  However, the default audio output path is set
> > > >> to headphones, regardless of whether they are actually inserted or not.
> > > >>
> > > >> This patch series aims to improve the audio support when operating in
> > > >> USB mode, by implementing the following changes:
> > > >>
> > > >> * Detect when the plugged state of the 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.
> > > >>
> > > >> * Adjust the speaker volume since its default level is too low and,
> > > >>   therefore, cannot generate any audible sound.
> > > >>
> > > >> * Register a dedicated input device for the audio jack and use it to
> > > >>   report all headphone and headset mic insert events.
> > > >>
> > > >> It's worth noting the latter is necessary since the controller complies
> > > >> with v1.0 of the USB Audio Class spec (UAC1) and, therefore, cannot
> > > >> advertise any jack detection capability.
> > > >>
> > > >> However, this feature can be implemented in the generic USB audio driver
> > > >> via quirks, i.e. by configuring an input handler to receive hotplug
> > > >> events from the HID driver.  That's exactly what has been accomplished
> > > >> via the "ALSA: usb-audio: Support jack detection on Sony DualSense"
> > > >> patchset [1], which has been already merged and should be available in
> > > >> v6.17.
> > > >>
> > > >> Unrelated to the above, also provide a few driver cleanup patches, e.g.
> > > >> make use of bitfields macros, simplify locking, fix coding style.
> > > >>
> > > >> [1] https://lore.kernel.org/all/20250526-dualsense-alsa-jack-v1-0-1a821463b632@collabora.com/
> > > >>
> > > >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
> > > >> ---
> > > >> Changes in v2:
> > > >> - Updated cover letter including a reference to the usb-audio patch series
> > > >> - Updated 'HID: playstation: Make use of bitfield macros' patch to drop
> > > >>   DS_STATUS_CHARGING_SHIFT and use FIELD_GET() for battery status ops
> > > >> - Replaced 'HID: playstation: Rename DualSense input report status
> > > >>   field' with 'HID: playstation: Redefine DualSense input report status
> > > >>   field' changing data type to a 3-byte array instead of renaming the
> > > >>   struct member (Roderick)
> > > >> - Updated 'HID: playstation: Support DualSense audio jack hotplug
> > > >>   detection' according to Roderick's feedback:
> > > >>  * Used DS_STATUS1_ prefixes for the plugged status register and rename
> > > >>    its bits to match the datasheet
> > > >>  * Defined MIC_VOLUME_ENABLE bit of DS_OUTPUT_VALID_FLAG0 register
> > > >>  * Renamed the newly introduced audio controls members in struct
> > > >>    dualsense_output_report_common: headphone_volume, speaker_volume,
> > > >>    mic_volume, audio_control, audio_control2
> > > >> - Restricted audio jack hotplug detection and event reporting to USB
> > > >>   operation mode only, since Bluetooth audio is currently not supported
> > > >>   and it might have a negative impact on the battery life (Roderick)
> > > >> - Rebased series onto next-20250624
> > > >> - Link to v1: https://lore.kernel.org/r/20250526-dualsense-hid-jack-v1-0-a65fee4a60cc@collabora.com
> > > >
> > > > Just for the record -- I like the v2, and am inclined to merge it, but
> > > > would prefer doing that with Roderick's Ack, so I am waiting for a bit
> > > > here.
> > >
> > > Could you please confirm you are fine with the latest changes so that Jiri
> > > is able to merge the series?
> > >
> > > If you cannot find the time to look into every detail right now, we can
> > > still take care of any non-essential matters afterwards.
> > >
> > > Thanks,
> > > Cristian

Powered by blists - more mailing lists