The Linux Arizona driver uses the MFD framework to create several
sub-devices for the Arizona codec and then uses a driver per function.
The jack-detect support for the Arizona codec is handled by the
extcon-arizona driver. This driver exports info about the jack state
to userspace through the standard extcon sysfs class interface.
But standard Linux userspace does not monitor/use the extcon sysfs
interface for jack-detection.
Add a jack pointer to the shared arizona data struct, this allows
the ASoC machine driver to create a snd_soc_jack and then pass this
to the extcon-arizona driver to report jack-detect state, so that
jack-detection works with standard Linux userspace.
The extcon-arizona code already depends on (waits for with -EPROBE_DEFER)
the snd_card being registered by the machine driver, so this does not
cause any ordering issues.
Signed-off-by: Hans de Goede <[email protected]>
---
include/linux/mfd/arizona/core.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
index 6d6f96b2b29f..5eb269bdbfcb 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -115,6 +115,7 @@ enum arizona_type {
#define ARIZONA_NUM_IRQ 75
struct snd_soc_dapm_context;
+struct snd_soc_jack;
struct arizona {
struct regmap *regmap;
@@ -148,6 +149,7 @@ struct arizona {
bool ctrlif_error;
struct snd_soc_dapm_context *dapm;
+ struct snd_soc_jack *jack;
int tdm_width[ARIZONA_MAX_AIF];
int tdm_slots[ARIZONA_MAX_AIF];
--
2.28.0
On Sun, Dec 27, 2020 at 10:12:19PM +0100, Hans de Goede wrote:
> The Linux Arizona driver uses the MFD framework to create several
> sub-devices for the Arizona codec and then uses a driver per function.
>
> The jack-detect support for the Arizona codec is handled by the
> extcon-arizona driver. This driver exports info about the jack state
> to userspace through the standard extcon sysfs class interface.
>
> But standard Linux userspace does not monitor/use the extcon sysfs
> interface for jack-detection.
This seems like the wrong layer to fix this problem at, this issue will
apply to all extcon devices that can detect audio.
Hi,
On 12/28/20 1:21 PM, Mark Brown wrote:
> On Sun, Dec 27, 2020 at 10:12:19PM +0100, Hans de Goede wrote:
>> The Linux Arizona driver uses the MFD framework to create several
>> sub-devices for the Arizona codec and then uses a driver per function.
>>
>> The jack-detect support for the Arizona codec is handled by the
>> extcon-arizona driver. This driver exports info about the jack state
>> to userspace through the standard extcon sysfs class interface.
>>
>> But standard Linux userspace does not monitor/use the extcon sysfs
>> interface for jack-detection.
>
> This seems like the wrong layer to fix this problem at, this issue will
> apply to all extcon devices that can detect audio.
Well, the problem really is that using extcon to report jack-state is
rather unusual to do, extcon-arizona.c is the only extcon driver which
deals with jack-state (typically extcon is used for things like determining
the type of charger connected to an USB charging port):
[hans@x1 linux]$ grep -lr EXTCON_JACK_HEADPHONE drivers/extcon/
drivers/extcon/extcon-arizona.c
drivers/extcon/extcon.c
And more in general AFAIK extcon is sort of deprecated and it is
not advised to use it for new code. I would esp. not expect it to
be used for new jack-detection code since we already have standard
uAPI support for that through sound/core/jack.c .
So extcon-arizona really is the odd duck here and writing some
generic extcon to sound/core/jack.c glue seems unnecessary since
we are just trying dealing with one special case here.
Also at first I tried to use extcon-glue like code in
sound/soc/intel/boards/bytcr_wm5102.c making it listen for
extcon events and have sound/soc/intel/boards/bytcr_wm5102.c
report jack events instead of sharing the jack with extcon-arizona.c
through the shared MFD data struct. But that did not work, because
the extcon-arizona.c probe function already (before this patch-set)
has this:
if (!arizona->dapm || !arizona->dapm->card)
return -EPROBE_DEFER;
Which means that the sound/soc/intel/boards/bytcr_wm5102.c machine
driver must first complete calling devm_snd_soc_register_card() before
the extcon driver will bind and register the extcon device.
But listening to extcon events requires the machine driver to do an:
extcon_get_extcon_dev("arizona-extcon") and as long as that returns
NULL, return -EPROBE_DEFER.
So now we have the machine-driver's probe returning with -EPROBE_DEFER
until the extcon driver shows up and the other-way around, so neither
ever binds.
I could have fixed this by making the machine driver bind without the
extcon driver being bound and then poll every second for the extcon device
to show up, and once it has shown up stop polling and register the jack,
once it has the extcon device.
But that seems quite ugly, so I did not even try to implement that
coming up with this solution instead which is much more KISS really.
Also note that sharing the jack is necessary to avoid creating 2
separate input_device-s for the headset, which also looks weird / ugly.
Besides being ugly, there also is another potential problem with
polling to wait for the extcon device to show up: the jack must be
registered before the card registration completes otherwise
snd_jack_dev_register will not run, since we are post registration.
But I guess that the sound core might be smart enough to call
the dev_register op immediately if the card has already been
registered ?
TL;DR: writing a generic solution for what is a special case used
in just driver seems like overkill and also writing such a
generic solution is not easily possible because of probe ordering
issues. So instead I've gone with this approach which is a much
simpler solution and as such seems a better way to deal with this
special case.
Regards,
Hans
On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
> And more in general AFAIK extcon is sort of deprecated and it is
> not advised to use it for new code. I would esp. not expect it to
> be used for new jack-detection code since we already have standard
> uAPI support for that through sound/core/jack.c .
Has Android been fixed to use the ALSA/input layer interfaces? That's
why that code is there, long term the goal was to have ALSA generate
extcon events too so userspace could fall over to using that. The basic
thing at the time was that nobody liked any of the existing interfaces
(the input layer thing is a total bodge stemming from it having been
easy to hack in a key for GPIO detection and using ALSA controls means
having to link against alsa-lib which is an awful faff for system level
UI stuff) and there were three separate userspace interfaces used by
different software stacks which needed to be joined together, extcon was
felt to be a bit more designed and is a superset so that was the
direction we were heading in.
On Mon, Dec 28, 2020 at 04:28:07PM +0000, Mark Brown wrote:
> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
>
> > And more in general AFAIK extcon is sort of deprecated and it is
> > not advised to use it for new code. I would esp. not expect it to
> > be used for new jack-detection code since we already have standard
> > uAPI support for that through sound/core/jack.c .
>
> Has Android been fixed to use the ALSA/input layer interfaces? That's
> why that code is there, long term the goal was to have ALSA generate
> extcon events too so userspace could fall over to using that. The basic
> thing at the time was that nobody liked any of the existing interfaces
> (the input layer thing is a total bodge stemming from it having been
> easy to hack in a key for GPIO detection and using ALSA controls means
> having to link against alsa-lib which is an awful faff for system level
> UI stuff) and there were three separate userspace interfaces used by
> different software stacks which needed to be joined together, extcon was
> felt to be a bit more designed and is a superset so that was the
> direction we were heading in.
Android has been updated to have the option to catch input events
for jack detection now.
I have always been slightly confused between extcon and the ALSA
jack reporting and have been unsure as to what is the longer term
plan here. I vaguely thought there was a gentle plan to move to
extcon, it is interesting to see Hans basically saying the
opposite that extcon is intended to be paritially deprecated. I
assume you just mean with respect to audio jacks, not other
connector types?
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.
Thanks,
Charles
Hi,
On 12/29/20 2:06 PM, Charles Keepax wrote:
> On Mon, Dec 28, 2020 at 04:28:07PM +0000, Mark Brown wrote:
>> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
>>
>>> And more in general AFAIK extcon is sort of deprecated and it is
>>> not advised to use it for new code. I would esp. not expect it to
>>> be used for new jack-detection code since we already have standard
>>> uAPI support for that through sound/core/jack.c .
>>
>> Has Android been fixed to use the ALSA/input layer interfaces? That's
>> why that code is there, long term the goal was to have ALSA generate
>> extcon events too so userspace could fall over to using that. The basic
>> thing at the time was that nobody liked any of the existing interfaces
>> (the input layer thing is a total bodge stemming from it having been
>> easy to hack in a key for GPIO detection and using ALSA controls means
>> having to link against alsa-lib which is an awful faff for system level
>> UI stuff) and there were three separate userspace interfaces used by
>> different software stacks which needed to be joined together, extcon was
>> felt to be a bit more designed and is a superset so that was the
>> direction we were heading in.
>
> Android has been updated to have the option to catch input events
> for jack detection now.
>
> I have always been slightly confused between extcon and the ALSA
> jack reporting and have been unsure as to what is the longer term
> plan here. I vaguely thought there was a gentle plan to move to
> extcon, it is interesting to see Hans basically saying the
> opposite that extcon is intended to be paritially deprecated. I
> assume you just mean with respect to audio jacks, not other
> connector types?
No I mean that afaik extcon is being deprecated in general. Extcon
is mostly meant for kernel internal use, to allow things like
charger-type-detection done by e.g. a fsa micro-usb mux or a
Type-C PD controller to be hooked up to the actual charger chip
and set the input-current-limit based on this.
But there is no clean way to express these relation-ships
in extcon terms in devicetree. At least for new device with
Type-C functionality this is all being moved to OF graph bindings
combined with using the power_supply class to provide info like
negotiated voltage and current to the charger-IC.
Note this is mostly my 2 cents / my impression about where extcon
is heading. I have actually tried to use extcon for Type-C stuff
and this was frowned upon by both the devicetree bindings people and
the USB Type-C code controller people.
Also despite its age, the extcon code is still not really in good
shape. E.g. a driver can do extcon_get_extcon_dev, but that does
not return a device-reference, that just does a lookup and returns
the requested extcon-device, but there are 0 guarantees that the
extcon device will not go away and dereferencing the returned
pointer after doing a rmmod of the extcon-driver will result in
a nasty crash.
So all this suggests to me that extcon should not be used for
new code / projects.
> 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).
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.
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).
Regards,
Hans
On Tue, Dec 29, 2020 at 02:57:38PM +0100, Hans de Goede wrote:
> On 12/29/20 2:06 PM, Charles Keepax wrote:
> > On Mon, Dec 28, 2020 at 04:28:07PM +0000, Mark Brown wrote:
> >> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
> >>
> >>> And more in general AFAIK extcon is sort of deprecated and it is
> >>> not advised to use it for new code. I would esp. not expect it to
> >>> be used for new jack-detection code since we already have standard
> >>> uAPI support for that through sound/core/jack.c .
> >>
> >> Has Android been fixed to use the ALSA/input layer interfaces? That's
> >> why that code is there, long term the goal was to have ALSA generate
> >> extcon events too so userspace could fall over to using that. The basic
> >> thing at the time was that nobody liked any of the existing interfaces
> >> (the input layer thing is a total bodge stemming from it having been
> >> easy to hack in a key for GPIO detection and using ALSA controls means
> >> having to link against alsa-lib which is an awful faff for system level
> >> UI stuff) and there were three separate userspace interfaces used by
> >> different software stacks which needed to be joined together, extcon was
> >> felt to be a bit more designed and is a superset so that was the
> >> direction we were heading in.
> >
> > Android has been updated to have the option to catch input events
> > for jack detection now.
> >
> > I have always been slightly confused between extcon and the ALSA
> > jack reporting and have been unsure as to what is the longer term
> > plan here. I vaguely thought there was a gentle plan to move to
> > extcon, it is interesting to see Hans basically saying the
> > opposite that extcon is intended to be paritially deprecated. I
> > assume you just mean with respect to audio jacks, not other
> > connector types?
>
> No I mean that afaik extcon is being deprecated in general. Extcon
> is mostly meant for kernel internal use, to allow things like
> charger-type-detection done by e.g. a fsa micro-usb mux or a
> Type-C PD controller to be hooked up to the actual charger chip
> and set the input-current-limit based on this.
>
Fascinating thanks for taking the time to write such detailed
answers. I thought it was mostly intended for user-space usage,
but I guess I never really thought through that most of this
stuff you don't really need to know from user-space.
> > 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).
>
> 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.
>
> 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).
>
All reasonable arguments, with Android now supporting input
events for jacks I guess there would be no need for us to use
extcon for future devices.
There is maybe more argument for porting the Arizona code across
anyways, since for a long time Android didn't properly support extcon
either. It supported the earlier out of tree switch stuff, extcon
had a switch compatibility mode, but that didn't actually work I
think due to android hard coding some sysfs naming or something
(memory is a little fuzzy on the details was a while ago now).
I think extcon support was fixed in Android at about the same time
the support for input events was added. So it might be harmless but
someone probably needs to go and check the timeline before we go
changing stuff.
Thanks,
Charles
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. 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.
> 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.
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.
> 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.
On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
> There is maybe more argument for porting the Arizona code across
> anyways, since for a long time Android didn't properly support extcon
> either. It supported the earlier out of tree switch stuff, extcon
Completely moving the driver doesn't cause the same problems as the
current proposal (unless it drops functionality I guess, there were
issues with adding new detection types into the input layer but I can't
remember if this hardware was impacted by that or not).
> had a switch compatibility mode, but that didn't actually work I
> think due to android hard coding some sysfs naming or something
> (memory is a little fuzzy on the details was a while ago now).
Yeah, it was a trivial edit to make it work which given everyone was
patching Android anyway wasn't a huge issue for shipping things (or you
could futz it in the kernel too if you felt like it).
Hi,
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
capability.
> 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 /
SW_MICROPHONE_INSERT / SW_LINEOUT_INSERT too. That patch did work,
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 ?
Regards,
Hans
*) 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.
Hi,
On 12/29/20 4:15 PM, Mark Brown wrote:
> On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
>
>> There is maybe more argument for porting the Arizona code across
>> anyways, since for a long time Android didn't properly support extcon
>> either. It supported the earlier out of tree switch stuff, extcon
>
> Completely moving the driver doesn't cause the same problems as the
> current proposal (unless it drops functionality I guess, there were
> issues with adding new detection types into the input layer but I can't
> remember if this hardware was impacted by that or not).
The input-layer supports the following switches:
SW_HEADPHONE_INSERT
SW_MICROPHONE_INSERT
SW_LINEOUT_INSERT
SW_JACK_PHYSICAL_INSERT
Which is a 1:1 mapping with the cable-types currently exported by
extcon-arizona.c .
I'm fine with fully moving extcon-arizona.c over to only using
sound/core/jack.c functionality and it no longer exporting an
extcon device.
I guess we should move it out of drivers/extcon then though.
I suggest using: sound/soc/cirrus/arizona-jack-detect.c
Note that sound/soc/cirrus is a new dir here. Would that work
for you ?
And I guess we probably also want to change the MFD instantiated
platform-dev's name to which it binds then?
I suggest using: "arizona-jack-detect" as new pdev name.
It will take me some time before I can make time to implement this,
but this is a plan which I can get behind.
Regards,
Hans
On 29/12/2020 15:06, Charles Keepax 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:
>>> On Mon, Dec 28, 2020 at 04:28:07PM +0000, Mark Brown wrote:
>>>> On Mon, Dec 28, 2020 at 02:16:04PM +0100, Hans de Goede wrote:
>>>>
>>>>> And more in general AFAIK extcon is sort of deprecated and it is
>>>>> not advised to use it for new code. I would esp. not expect it to
>>>>> be used for new jack-detection code since we already have standard
>>>>> uAPI support for that through sound/core/jack.c .
>>>>
>>>> Has Android been fixed to use the ALSA/input layer interfaces? That's
>>>> why that code is there, long term the goal was to have ALSA generate
>>>> extcon events too so userspace could fall over to using that. The basic
>>>> thing at the time was that nobody liked any of the existing interfaces
>>>> (the input layer thing is a total bodge stemming from it having been
>>>> easy to hack in a key for GPIO detection and using ALSA controls means
>>>> having to link against alsa-lib which is an awful faff for system level
>>>> UI stuff) and there were three separate userspace interfaces used by
>>>> different software stacks which needed to be joined together, extcon was
>>>> felt to be a bit more designed and is a superset so that was the
>>>> direction we were heading in.
>>>
>>> Android has been updated to have the option to catch input events
>>> for jack detection now.
>>>
>>> I have always been slightly confused between extcon and the ALSA
>>> jack reporting and have been unsure as to what is the longer term
>>> plan here. I vaguely thought there was a gentle plan to move to
>>> extcon, it is interesting to see Hans basically saying the
>>> opposite that extcon is intended to be paritially deprecated. I
>>> assume you just mean with respect to audio jacks, not other
>>> connector types?
>>
>> No I mean that afaik extcon is being deprecated in general. Extcon
>> is mostly meant for kernel internal use, to allow things like
>> charger-type-detection done by e.g. a fsa micro-usb mux or a
>> Type-C PD controller to be hooked up to the actual charger chip
>> and set the input-current-limit based on this.
>>
>
> Fascinating thanks for taking the time to write such detailed
> answers. I thought it was mostly intended for user-space usage,
> but I guess I never really thought through that most of this
> stuff you don't really need to know from user-space.
>
>>> 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).
>>
>> 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 majority of customers for these codecs + Linux were building Android
devices. So compatibility with Android was the most important goal. At
the time this code was written Android used a custom kernel subsystem
called "switch" to report jack events. extcon is basically a cleaner
version of "switch" and the advantage is that they are almost identical
so it was easy to port a mainline extcon driver back into an Android
"switch" driver. Android didn't use input events or ALSA jack events so
one of those drivers would be no use to the majority of users of this
codec at that time. And as Android's "switch" was so similar to extcon,
surely Android would just convert to supporting extcon...? The arizona
extcon driver is a legacy of the way Android did jack detection.
>> 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).
>>
>
> All reasonable arguments, with Android now supporting input
> events for jacks I guess there would be no need for us to use
> extcon for future devices.
>
> There is maybe more argument for porting the Arizona code across
> anyways, since for a long time Android didn't properly support extcon
> either. It supported the earlier out of tree switch stuff, extcon
> had a switch compatibility mode, but that didn't actually work I
> think due to android hard coding some sysfs naming or something
> (memory is a little fuzzy on the details was a while ago now).
>
> I think extcon support was fixed in Android at about the same time
> the support for input events was added. So it might be harmless but
> someone probably needs to go and check the timeline before we go
> changing stuff.
>
> Thanks,
> Charles
>
On 29/12/2020 15:40, Hans de Goede wrote:
> Hi,
>
> On 12/29/20 4:15 PM, Mark Brown wrote:
>> On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
>>
>>> There is maybe more argument for porting the Arizona code across
>>> anyways, since for a long time Android didn't properly support extcon
>>> either. It supported the earlier out of tree switch stuff, extcon
>>
>> Completely moving the driver doesn't cause the same problems as the
>> current proposal (unless it drops functionality I guess, there were
>> issues with adding new detection types into the input layer but I can't
>> remember if this hardware was impacted by that or not).
>
> The input-layer supports the following switches:
>
> SW_HEADPHONE_INSERT
> SW_MICROPHONE_INSERT
> SW_LINEOUT_INSERT
> SW_JACK_PHYSICAL_INSERT
>
> Which is a 1:1 mapping with the cable-types currently exported by
> extcon-arizona.c .
>
> I'm fine with fully moving extcon-arizona.c over to only using
> sound/core/jack.c functionality and it no longer exporting an
> extcon device.
>
> I guess we should move it out of drivers/extcon then though.
> I suggest using: sound/soc/cirrus/arizona-jack-detect.c
> Note that sound/soc/cirrus is a new dir here. Would that work
> for you ?
Shouldn't it be sound/soc/codecs/arizona-jack.c so that it is with all
the other code for those codecs?
>
> And I guess we probably also want to change the MFD instantiated
> platform-dev's name to which it binds then?
>
> I suggest using: "arizona-jack-detect" as new pdev name.
>
> It will take me some time before I can make time to implement this,
> but this is a plan which I can get behind.
>
> Regards,
>
> Hans
>
Hi,
On 12/29/20 5:51 PM, Richard Fitzgerald wrote:
>
>
> On 29/12/2020 15:40, Hans de Goede wrote:
>> Hi,
>>
>> On 12/29/20 4:15 PM, Mark Brown wrote:
>>> On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
>>>
>>>> There is maybe more argument for porting the Arizona code across
>>>> anyways, since for a long time Android didn't properly support extcon
>>>> either. It supported the earlier out of tree switch stuff, extcon
>>>
>>> Completely moving the driver doesn't cause the same problems as the
>>> current proposal (unless it drops functionality I guess, there were
>>> issues with adding new detection types into the input layer but I can't
>>> remember if this hardware was impacted by that or not).
>>
>> The input-layer supports the following switches:
>>
>> SW_HEADPHONE_INSERT
>> SW_MICROPHONE_INSERT
>> SW_LINEOUT_INSERT
>> SW_JACK_PHYSICAL_INSERT
>>
>> Which is a 1:1 mapping with the cable-types currently exported by
>> extcon-arizona.c .
>>
>> I'm fine with fully moving extcon-arizona.c over to only using
>> sound/core/jack.c functionality and it no longer exporting an
>> extcon device.
>>
>> I guess we should move it out of drivers/extcon then though.
>> I suggest using: sound/soc/cirrus/arizona-jack-detect.c
>> Note that sound/soc/cirrus is a new dir here. Would that work
>> for you ?
>
> Shouldn't it be sound/soc/codecs/arizona-jack.c so that it is with all
> the other code for those codecs?
The arizona codecs use the MFD framework and there is a separate
platform-device instantiated for the jack-detect functionality, so this
(mostly) a standalone platform-driver which has very little interaction
with the rest of the codec code.
It is not a codec driver, or code shared between the codec drivers,
so putting it under sound/soc/codecs would be a bit weird.
With that said I have no strong preference for putting it under
a new sound/soc/cirrus dir, if everyone is ok with putting it under
sound/soc/codecs then that works for me.
Regards,
Hans
On 30/12/2020 11:04, Hans de Goede wrote:
> Hi,
>
> On 12/29/20 5:51 PM, Richard Fitzgerald wrote:
>>
>>
>> On 29/12/2020 15:40, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12/29/20 4:15 PM, Mark Brown wrote:
>>>> On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
>>>>
>>>>> There is maybe more argument for porting the Arizona code across
>>>>> anyways, since for a long time Android didn't properly support extcon
>>>>> either. It supported the earlier out of tree switch stuff, extcon
>>>>
>>>> Completely moving the driver doesn't cause the same problems as the
>>>> current proposal (unless it drops functionality I guess, there were
>>>> issues with adding new detection types into the input layer but I can't
>>>> remember if this hardware was impacted by that or not).
>>>
>>> The input-layer supports the following switches:
>>>
>>> SW_HEADPHONE_INSERT
>>> SW_MICROPHONE_INSERT
>>> SW_LINEOUT_INSERT
>>> SW_JACK_PHYSICAL_INSERT
>>>
>>> Which is a 1:1 mapping with the cable-types currently exported by
>>> extcon-arizona.c .
>>>
>>> I'm fine with fully moving extcon-arizona.c over to only using
>>> sound/core/jack.c functionality and it no longer exporting an
>>> extcon device.
>>>
>>> I guess we should move it out of drivers/extcon then though.
>>> I suggest using: sound/soc/cirrus/arizona-jack-detect.c
>>> Note that sound/soc/cirrus is a new dir here. Would that work
>>> for you ?
>>
>> Shouldn't it be sound/soc/codecs/arizona-jack.c so that it is with all
>> the other code for those codecs?
>
> The arizona codecs use the MFD framework and there is a separate
> platform-device instantiated for the jack-detect functionality, so this
That is because it is an extcon driver. It is a different subsystem to
the other child drivers so has to be a separate child.
> (mostly) a standalone platform-driver which has very little interaction
> with the rest of the codec code.
>
> It is not a codec driver, or code shared between the codec drivers,
> so putting it under sound/soc/codecs would be a bit weird.
>
In fact it is tied into the codec driver. The code in arizona.c that
handles HP OUT has to synchronize with the jack detection to avoid one
driver trashing the state of the other. But because they are currently
separate drivers they have to communicate through hp_ena and
hp_clamp in the parent mfd data. See arizona_hp_ev().
> With that said I have no strong preference for putting it under
> a new sound/soc/cirrus dir, if everyone is ok with putting it under
> sound/soc/codecs then that works for me.
>
> Regards,
>
> Hans
>
Hi,
On 12/30/20 12:23 PM, Richard Fitzgerald wrote:
> On 30/12/2020 11:04, Hans de Goede wrote:
>> Hi,
>>
>> On 12/29/20 5:51 PM, Richard Fitzgerald wrote:
>>>
>>>
>>> On 29/12/2020 15:40, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12/29/20 4:15 PM, Mark Brown wrote:
>>>>> On Tue, Dec 29, 2020 at 03:06:35PM +0000, Charles Keepax wrote:
>>>>>
>>>>>> There is maybe more argument for porting the Arizona code across
>>>>>> anyways, since for a long time Android didn't properly support extcon
>>>>>> either. It supported the earlier out of tree switch stuff, extcon
>>>>>
>>>>> Completely moving the driver doesn't cause the same problems as the
>>>>> current proposal (unless it drops functionality I guess, there were
>>>>> issues with adding new detection types into the input layer but I can't
>>>>> remember if this hardware was impacted by that or not).
>>>>
>>>> The input-layer supports the following switches:
>>>>
>>>> SW_HEADPHONE_INSERT
>>>> SW_MICROPHONE_INSERT
>>>> SW_LINEOUT_INSERT
>>>> SW_JACK_PHYSICAL_INSERT
>>>>
>>>> Which is a 1:1 mapping with the cable-types currently exported by
>>>> extcon-arizona.c .
>>>>
>>>> I'm fine with fully moving extcon-arizona.c over to only using
>>>> sound/core/jack.c functionality and it no longer exporting an
>>>> extcon device.
>>>>
>>>> I guess we should move it out of drivers/extcon then though.
>>>> I suggest using: sound/soc/cirrus/arizona-jack-detect.c
>>>> Note that sound/soc/cirrus is a new dir here. Would that work
>>>> for you ?
>>>
>>> Shouldn't it be sound/soc/codecs/arizona-jack.c so that it is with all
>>> the other code for those codecs?
>>
>> The arizona codecs use the MFD framework and there is a separate
>> platform-device instantiated for the jack-detect functionality, so this
>
> That is because it is an extcon driver. It is a different subsystem to
> the other child drivers so has to be a separate child.
>
>> (mostly) a standalone platform-driver which has very little interaction
>> with the rest of the codec code.
>>
>> It is not a codec driver, or code shared between the codec drivers,
>> so putting it under sound/soc/codecs would be a bit weird.
>>
>
> In fact it is tied into the codec driver. The code in arizona.c that
> handles HP OUT has to synchronize? with the jack detection to avoid one
> driver trashing the state of the other. But because they are currently
> separate drivers they have to communicate through hp_ena and
> hp_clamp in the parent mfd data. See arizona_hp_ev().
So what you are suggesting is to do something along these lines ? :
1. Drop the MFD instantiated arizona-extcon device
2. Move the extcon code to something more like a library
3. Have the various codec drivers call into the library
at various points
4. Have the library call snd_soc_card_jack_new,
snd_soc_jack_report, etc. ?
That works for me, but I would like to make sure we are all on
the same page here before spending time on coding this solution.
Regards,
Hans
On Tue, Dec 29, 2020 at 04:51:57PM +0000, Richard Fitzgerald wrote:
> On 29/12/2020 15:40, Hans de Goede wrote:
> > I guess we should move it out of drivers/extcon then though.
> > I suggest using: sound/soc/cirrus/arizona-jack-detect.c
> > Note that sound/soc/cirrus is a new dir here. Would that work
> > for you ?
> Shouldn't it be sound/soc/codecs/arizona-jack.c so that it is with all
> the other code for those codecs?
That is what I would expect too.
On Tue, Dec 29, 2020 at 04:33:09PM +0100, Hans de Goede wrote:
> On 12/29/20 4:08 PM, Mark Brown wrote:
> > 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...
It was extended as part of getting it merged into mainline, there was
some thought about what people could need to do with jacks at the time.
> > 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.
Yes.
> 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.
Quite apart from anything else the reason the switch API was created was
AIUI that the Android people couldn't figure out how to do jack
detection on Linux - the current APIs are not terribly discoverable.
There's also issues with extensibility and applicability to non-audio
use csaes with the existing APIs.
> 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.
Bodging it at the driver level gets in the way of improving the generic
code.
> 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
> capability.
That *is* the plan, though clearly it's not exactly been backed by work.
extcon already exists and supports reporting jacks.
> > 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 ?
Like I say it's discoverabiity and extensibility in a range of axes.
Other examples of thing that'd be good to have that we don't really have
with the input API are things like saying things like "the red jack on
the front panel".
Hi,
On 12/30/20 2:38 PM, Mark Brown wrote:
> On Tue, Dec 29, 2020 at 04:33:09PM +0100, Hans de Goede wrote:
>> On 12/29/20 4:08 PM, Mark Brown wrote:
<snip>
>>> 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.
>
> Yes.
Ok, so this seems to be the same solution as which the opensource.cirrus.com
folks want in that both you and the opensource.cirrus.com people want to
change the arizona-extcon.c driver to be changed to stop reporting extcon
info directly itself and instead do all the reporting through sound/core/jack.c.
Where the thoughts on this seem to differ is that the opensource.cirrus.com
folks seem to be fine with dropping extcon support, where as you suggest
to extend sound/core/jack.c to register an extcon device and have it report
extcon events.
I'm with the opensource.cirrus.com folks here that ATM it seems best to just
drop extcon support since the only user is Android, which also supports the
input_dev API.
If the need arises we can always add extcon-support to sound/core/jack.c
later.
So I'll start on reworking this patch-series to change the arizona-extcon.c
driver to stop it reporting extcon info directly itself and instead do all
the reporting through sound/core/jack.c.
Regards,
Hans