| Message ID | 20201227211232.117801-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
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
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(+)