| Message ID | 20201227211232.117801-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | MFD/extcon/ASoC: Add support for Intel Bay Trail boards with WM5102 codec | expand |
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
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.
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: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 >
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".
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];
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 <hdegoede@redhat.com> --- include/linux/mfd/arizona/core.h | 2 ++ 1 file changed, 2 insertions(+)