[lkml]   [2020]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 01/14] mfd: arizona: Add jack pointer to struct arizona

On 12/29/20 4:08 PM, Mark Brown wrote:
> On Tue, Dec 29, 2020 at 02:57:38PM +0100, Hans de Goede wrote:
>> On 12/29/20 2:06 PM, Charles Keepax wrote:
>>> I would agree with Mark though that if extcon exists for external
>>> connectors it seems odd that audio jacks would have their own
>>> special way rather than just using the connector stuff.
>> Well as I said above in me experience the extcon code is (was) mostly
>> meant for kernel internal use. The sysfs API is more of a debugging
>> tool then anything else (IMHO).
> No, that's not the case. extcon is a port of an Android custom API that
> looks very similar to what ended up in mainline, it was also a
> combination of sysfs and uevents but a bit less generic. It mainly
> existed to provide information to userspace about what was plugged into
> the various ports on devices, things like headphone jacks and the
> pre-USB C multifunction connectors you used to get on phones.

Interesting, I have close to 0 experience with Android userspace
(something which I would like to change one of these days). But I have
encountered a bunch of in-kernel use of extcon on Intel's Android X86
port for Bay and Cherry Trail devices (which I've ported to the mainline)
where extcon was e.g. used with a generic extcon-gpio like driver monitoring
the ID pin and then used by the USB code to detect (through monitoring the
extcon) if the USB port was in host or device/gadget mode.

So it seems that extcon has many different uses by different people...

> In kernel
> use wasn't really a thing for that as far as I can remember. It's
> become a bit less of a pressing concern for Android with the move to USB
> C and the deprecation of headphone jacks in favour of a combination of
> USB C and Bluetooth but the use case is still there and it's clear that
> none of the audio stuff is currently exactly designed.
> The issues I'm seeing are more to do with nobody working on things, I
> guess mainly due to the change in priorities for Android systems and in
> my case a job change.

Yes extcon definitely could use some TLC, although I do honestly wonder
if just deprecating it would not be better...

>> Also the kernel has support for a lot of sound devices, including
>> many with jack-detection support. Yet a grep for EXTCON_JACK_HEADPHONE
>> over the entire mainline kernel tree shows that only extcon-arizona.c
>> is using it. So given that we have dozens of drivers providing jack
>> functionality through the sound/core/jack.c core and only 1 driver
>> using the extcon interface I believe that the ship on how to export
>> this to userspace has long sailed, since most userspace code will
>> clearly expect the sound/core/jack.c way of doing things to be used.
> The whole purpose of creating sound/core/jack.c is to abstract away the
> userspace interfaces from the drivers, most audio devices shouldn't be
> working with extcon directly just as they shouldn't be creating input
> devices or ALSA controls directly. The missing bit there is to add
> extcon into jack.c.

So what you are suggesting is making sound/core/jack.c register the
extcon device and then have arizona-extcon.c talk to sound/core/jack.c
and no longer do any extcon stuff itself.

That would make the userspace API experience offered by arizona-extcon
consistent with other drivers with jack-detect capability.

But I do honestly wonder if we really want to use extcon for jack-detect
information reporting. At least in the mainline tree there is only 1
driver supporting this ATM and we are pretty much stuck with 1.
ALSA controls for jack detection as well as 2. input_device which
we already export, there is enough of a userspace dependency on
those that we cannot get rid of those.

So we already export 2 userspace-APIs for this IMHO adding a 3th one is not
really a good idea unless it offers significant benifits which I don't
really see. The input_dev API is simple enough to use that extcon does
not really offer any significant benefits.

But this has turned into a very generic discussion, while all I was
trying to do is make arizona-extcon export the jack-detect info through
the ALSA controls for jack detection.

My current solution to have the machine-driver register a jack and
then share that with the extcon-arizona.c code still seems like the
best/simplest solution to me here.

Unless we go with your plan to make sound/core/jack.c export
an extcon device for all sound-devs registering a jack, and then move
extcon-arizona.c over to using sound/core/jack.c without it doing any
extcon stuff itself. But as discussed I'm really not a fan of exporting
a 3th uAPI for jack-detection for all sound-cards with jack-detect

> BTW note that the existing arizona extcon driver does provide an input
> device so I'm guesing that whatever userspace you're concerned about is
> one that uses only the ALSA controls for jack detection.

The current extcon-arizona.c code only reports button presses to the
input-device, but I did try also making it report SW_HEADPHONE_INSERT /
but pulseaudio did not respond to this, so it seems that pulse indeed
is reliant on the ALSA controls for jack detection (I did not debug
this further, instead I went with the route of registering a jack
from the machinedriver).

I've tested with
>> Arguably we should/could maybe even drop the extcon part of extcon-arizona.c
>> but I did not do that as I did not want to regress existing userspace
>> code which may depend on this (on specific embedded/android devices).
> I do think pushing things over to extcon is a useful goal, the existing
> interfaces have fairly clear issues that it does actually address.

I don't really see any "fairly clear issues" with the input-device uAPI,
I agree that the ALSA controls can be hard to use, but that is not the
case for the input-device uAPI (*). What are your objections against using
the input-device uAPI ?



*) I agree that it is a bit weird to use the input_device API for this,
but that ship has sailed. We will need to support the input_device API
going forward anyways and given that I see little advantage in adding
the extcon API *on top* of that.

 \ /
  Last update: 2020-12-29 16:37    [W:0.067 / U:9.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site