[PATCH 00/14] MFD/extcon/ASoC: Add support for Intel Bay Trail boards with WM5102 codec
Hi All,
This patch series adds support for Intel Bay Trail based device which use a WM5102 codec for audio output/input. This was developed and tested on a Lenovo Yoga Tablet 1051L.
This series consists of 3 parts: 1. Arizona MFD drv patches for ACPI bindings, better jack-detect support and misc. fixes 2. extcon-arizona driver fixes and improved jack reporting (this depends on the MFD changes) 3. ASoC patches in the form of a quirk for BYTCR detecting, a new machine driver for BYT + WM5102 and jack-detect support for the new machine driver (which again depends on the MFD changes).
Given that 2. and 3. depend on the MFD changes I believe that it is best if all patches in this series are merged through the MFD tree (once reviewed and acked) and then Lee can provide a immutable branch for the ASoC and extcon maintainers to merge into their trees.
I have a patch with matching UCM profile changes available here: https://github.com/jwrdegoede/alsa-ucm-conf/commit/316109e7814926ba984322c1d...
This series + the UCM profile has been tested with both the SST and SOF ASoC drivers for BYT devices.
Regards,
Hans
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(+)
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];
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 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: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
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 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 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: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.
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
The (shared) probing code of the arizona-i2c and arizona-spi modules takes the following steps during init:
1. Call mfd_add_devices() for a set of early child-devices, this includes the arizona_ldo1 device which provides one of the core-regulators.
2. Bulk enable the core-regulators.
3. Read the device id.
4. Call mfd_add_devices() for the other child-devices.
This sequence depends on 1. leading to not only the child-device being created, but also the driver for the child-device binding to it and registering its regulator.
This requires the arizona_ldo1 driver to be loaded before the shared probing code runs. Add a doftdep for this to both modules to ensure that this requirement is met.
Note this mirrors the existing MODULE_SOFTDEP("pre: wm8994_regulator") in the wm8994 code, which has a similar init sequence.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mfd/arizona-i2c.c | 1 + drivers/mfd/arizona-spi.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/drivers/mfd/arizona-i2c.c b/drivers/mfd/arizona-i2c.c index 4b58e3ad6eb6..2a4a3a164d0a 100644 --- a/drivers/mfd/arizona-i2c.c +++ b/drivers/mfd/arizona-i2c.c @@ -115,6 +115,7 @@ static struct i2c_driver arizona_i2c_driver = {
module_i2c_driver(arizona_i2c_driver);
+MODULE_SOFTDEP("pre: arizona_ldo1"); MODULE_DESCRIPTION("Arizona I2C bus interface"); MODULE_AUTHOR("Mark Brown broonie@opensource.wolfsonmicro.com"); MODULE_LICENSE("GPL"); diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c index 2633e147b76c..704f214d2614 100644 --- a/drivers/mfd/arizona-spi.c +++ b/drivers/mfd/arizona-spi.c @@ -110,6 +110,7 @@ static struct spi_driver arizona_spi_driver = {
module_spi_driver(arizona_spi_driver);
+MODULE_SOFTDEP("pre: arizona_ldo1"); MODULE_DESCRIPTION("Arizona SPI bus interface"); MODULE_AUTHOR("Mark Brown broonie@opensource.wolfsonmicro.com"); MODULE_LICENSE("GPL");
On Sun, Dec 27, 2020 at 10:12:20PM +0100, Hans de Goede wrote:
The (shared) probing code of the arizona-i2c and arizona-spi modules takes the following steps during init:
- Call mfd_add_devices() for a set of early child-devices, this
includes the arizona_ldo1 device which provides one of the core-regulators.
Bulk enable the core-regulators.
Read the device id.
Call mfd_add_devices() for the other child-devices.
This sequence depends on 1. leading to not only the child-device being created, but also the driver for the child-device binding to it and registering its regulator.
This requires the arizona_ldo1 driver to be loaded before the shared probing code runs. Add a doftdep for this to both modules to ensure that this requirement is met.
Note this mirrors the existing MODULE_SOFTDEP("pre: wm8994_regulator") in the wm8994 code, which has a similar init sequence.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
The Intel Bay Trail (x86/ACPI) based Lenovo Yoga Tablet 2 series use a WM5102 codec connected over SPI.
Add support for ACPI enumeration to arizona-spi so that arizona-spi can bind to the codec on these tablets.
This is loosely based on an earlier attempt (for Android-x86) at this by Christian Hartmann, combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [1].
[1] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
Cc: Christian Hartmann cornogle@googlemail.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mfd/arizona-spi.c | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+)
diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c index 704f214d2614..bcdbd72fefb5 100644 --- a/drivers/mfd/arizona-spi.c +++ b/drivers/mfd/arizona-spi.c @@ -7,7 +7,10 @@ * Author: Mark Brown broonie@opensource.wolfsonmicro.com */
+#include <linux/acpi.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> @@ -15,11 +18,119 @@ #include <linux/slab.h> #include <linux/spi/spi.h> #include <linux/of.h> +#include <uapi/linux/input-event-codes.h>
#include <linux/mfd/arizona/core.h>
#include "arizona.h"
+#ifdef CONFIG_ACPI +const struct acpi_gpio_params reset_gpios = { 1, 0, false }; +const struct acpi_gpio_params ldoena_gpios = { 2, 0, false }; + +static const struct acpi_gpio_mapping arizona_acpi_gpios[] = { + { "reset-gpios", &reset_gpios, 1, }, + { "wlf,ldoena-gpios", &ldoena_gpios, 1 }, + { } +}; + +/* + * The ACPI resources for the device only describe external GPIO-s. They do + * not provide mappings for the GPIO-s coming from the Arizona codec itself. + */ +static const struct gpiod_lookup arizona_soc_gpios[] = { + { "arizona", 2, "wlf,spkvdd-ena", 0, GPIO_ACTIVE_HIGH }, + { "arizona", 4, "wlf,micd-pol", 0, GPIO_ACTIVE_LOW }, +}; + +/* + * The AOSP 3.5 mm Headset: Accessory Specification gives the following values: + * Function A Play/Pause: 0 ohm + * Function D Voice assistant: 135 ohm + * Function B Volume Up 240 ohm + * Function C Volume Down 470 ohm + * Minimum Mic DC resistance 1000 ohm + * Minimum Ear speaker impedance 16 ohm + * Note the first max value below must be less then the min. speaker impedance, + * to allow CTIA/OMTP detection to work. The other max values are the closest + * value from extcon-arizona.c:arizona_micd_levels halfway 2 button resistances. + */ +static const struct arizona_micd_range arizona_micd_aosp_ranges[] = { + { .max = 11, .key = KEY_PLAYPAUSE }, + { .max = 186, .key = KEY_VOICECOMMAND }, + { .max = 348, .key = KEY_VOLUMEUP }, + { .max = 752, .key = KEY_VOLUMEDOWN }, +}; + +static void arizona_spi_acpi_remove_lookup(void *lookup) +{ + gpiod_remove_lookup_table(lookup); +} + +static int arizona_spi_acpi_probe(struct arizona *arizona) +{ + struct gpiod_lookup_table *lookup; + int i, ret; + + /* Add mappings for the 2 ACPI declared GPIOs used for reset and ldo-ena */ + devm_acpi_dev_add_driver_gpios(arizona->dev, arizona_acpi_gpios); + + /* Add lookups for the SoCs own GPIOs used for micdet-polarity and spkVDD-enable */ + lookup = devm_kzalloc(arizona->dev, + struct_size(lookup, table, ARRAY_SIZE(arizona_soc_gpios) + 1), + GFP_KERNEL); + if (!lookup) + return -ENOMEM; + + lookup->dev_id = dev_name(arizona->dev); + for (i = 0; i < ARRAY_SIZE(arizona_soc_gpios); i++) + lookup->table[i] = arizona_soc_gpios[i]; + + gpiod_add_lookup_table(lookup); + ret = devm_add_action_or_reset(arizona->dev, arizona_spi_acpi_remove_lookup, lookup); + if (ret) + return ret; + + /* Enable 32KHz clock from SoC to codec for jack-detect */ + acpi_evaluate_object(ACPI_HANDLE(arizona->dev), "CLKE", NULL, NULL); + + /* + * Some DSDTs wrongly declare the IRQ trigger-type as IRQF_TRIGGER_FALLING + * The IRQ line will stay low when a new IRQ event happens between reading + * the IRQ status flags and acknowledging them. When the IRQ line stays + * low like this the IRQ will never trigger again when its type is set + * to IRQF_TRIGGER_FALLING. Correct the IRQ trigger-type to fix this. + */ + arizona->pdata.irq_flags = IRQF_TRIGGER_LOW; + + /* Wait 200 ms after jack insertion */ + arizona->pdata.micd_detect_debounce = 200; + + /* Use standard AOSP values for headset-button mappings */ + arizona->pdata.micd_ranges = arizona_micd_aosp_ranges; + arizona->pdata.num_micd_ranges = ARRAY_SIZE(arizona_micd_aosp_ranges); + + return 0; +} + +static const struct acpi_device_id arizona_acpi_match[] = { + { + .id = "WM510204", + .driver_data = WM5102, + }, + { + .id = "WM510205", + .driver_data = WM5102, + }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match); +#else +static void arizona_spi_acpi_probe(struct arizona *arizona) +{ +} +#endif + static int arizona_spi_probe(struct spi_device *spi) { const struct spi_device_id *id = spi_get_device_id(spi); @@ -30,6 +141,8 @@ static int arizona_spi_probe(struct spi_device *spi)
if (spi->dev.of_node) type = arizona_of_get_type(&spi->dev); + else if (ACPI_COMPANION(&spi->dev)) + type = (unsigned long)acpi_device_get_match_data(&spi->dev); else type = id->driver_data;
@@ -75,6 +188,12 @@ static int arizona_spi_probe(struct spi_device *spi) arizona->dev = &spi->dev; arizona->irq = spi->irq;
+ if (ACPI_COMPANION(&spi->dev)) { + ret = arizona_spi_acpi_probe(arizona); + if (ret) + return ret; + } + return arizona_dev_init(arizona); }
@@ -102,6 +221,7 @@ static struct spi_driver arizona_spi_driver = { .name = "arizona", .pm = &arizona_pm_ops, .of_match_table = of_match_ptr(arizona_of_match), + .acpi_match_table = ACPI_PTR(arizona_acpi_match), }, .probe = arizona_spi_probe, .remove = arizona_spi_remove,
Hi Hans,
I love your patch! Perhaps something to improve:
[auto build test WARNING on lee-mfd/for-mfd-next] [also build test WARNING on chanwoo-extcon/extcon-next asoc/for-next v5.10 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/MFD-extcon-ASoC-Add-s... base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next config: i386-randconfig-s002-20201227 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-184-g1b896707-dirty # https://github.com/0day-ci/linux/commit/bc64046ad088b340f127f9eefdb8f941c03d... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Hans-de-Goede/MFD-extcon-ASoC-Add-support-for-Intel-Bay-Trail-boards-with-WM5102-codec/20201228-051806 git checkout bc64046ad088b340f127f9eefdb8f941c03dcb53 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
"sparse warnings: (new ones prefixed by >>)"
drivers/mfd/arizona-spi.c:29:31: sparse: sparse: symbol 'ldoena_gpios' was not declared. Should it be static?
Please review and possibly fold the followup patch.
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Reported-by: kernel test robot lkp@intel.com Signed-off-by: kernel test robot lkp@intel.com --- arizona-spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c index bcdbd72fefb5ae..7453c8fdd3f6d1 100644 --- a/drivers/mfd/arizona-spi.c +++ b/drivers/mfd/arizona-spi.c @@ -26,7 +26,7 @@
#ifdef CONFIG_ACPI const struct acpi_gpio_params reset_gpios = { 1, 0, false }; -const struct acpi_gpio_params ldoena_gpios = { 2, 0, false }; +static const struct acpi_gpio_params ldoena_gpios = { 2, 0, false };
static const struct acpi_gpio_mapping arizona_acpi_gpios[] = { { "reset-gpios", &reset_gpios, 1, },
On Sun, Dec 27, 2020 at 11:16 PM Hans de Goede hdegoede@redhat.com wrote:
The Intel Bay Trail (x86/ACPI) based Lenovo Yoga Tablet 2 series use a WM5102 codec connected over SPI.
Add support for ACPI enumeration to arizona-spi so that arizona-spi can bind to the codec on these tablets.
This is loosely based on an earlier attempt (for Android-x86) at this by Christian Hartmann, combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [1].
Few nitpicks here and there, but the most important bit that hits me is device_get_match_data().
[1] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
Cc: Christian Hartmann cornogle@googlemail.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/mfd/arizona-spi.c | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+)
diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c index 704f214d2614..bcdbd72fefb5 100644 --- a/drivers/mfd/arizona-spi.c +++ b/drivers/mfd/arizona-spi.c @@ -7,7 +7,10 @@
- Author: Mark Brown broonie@opensource.wolfsonmicro.com
*/
+#include <linux/acpi.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> @@ -15,11 +18,119 @@ #include <linux/slab.h> #include <linux/spi/spi.h> #include <linux/of.h> +#include <uapi/linux/input-event-codes.h>
#include <linux/mfd/arizona/core.h>
#include "arizona.h"
+#ifdef CONFIG_ACPI +const struct acpi_gpio_params reset_gpios = { 1, 0, false }; +const struct acpi_gpio_params ldoena_gpios = { 2, 0, false };
+static const struct acpi_gpio_mapping arizona_acpi_gpios[] = {
{ "reset-gpios", &reset_gpios, 1, },{ "wlf,ldoena-gpios", &ldoena_gpios, 1 },{ }+};
+/*
- The ACPI resources for the device only describe external GPIO-s. They do
- not provide mappings for the GPIO-s coming from the Arizona codec itself.
- */
+static const struct gpiod_lookup arizona_soc_gpios[] = {
{ "arizona", 2, "wlf,spkvdd-ena", 0, GPIO_ACTIVE_HIGH },{ "arizona", 4, "wlf,micd-pol", 0, GPIO_ACTIVE_LOW },+};
+/*
- The AOSP 3.5 mm Headset: Accessory Specification gives the following values:
- Function A Play/Pause: 0 ohm
- Function D Voice assistant: 135 ohm
- Function B Volume Up 240 ohm
- Function C Volume Down 470 ohm
- Minimum Mic DC resistance 1000 ohm
- Minimum Ear speaker impedance 16 ohm
- Note the first max value below must be less then the min. speaker impedance,
- to allow CTIA/OMTP detection to work. The other max values are the closest
- value from extcon-arizona.c:arizona_micd_levels halfway 2 button resistances.
- */
+static const struct arizona_micd_range arizona_micd_aosp_ranges[] = {
{ .max = 11, .key = KEY_PLAYPAUSE },{ .max = 186, .key = KEY_VOICECOMMAND },{ .max = 348, .key = KEY_VOLUMEUP },{ .max = 752, .key = KEY_VOLUMEDOWN },+};
+static void arizona_spi_acpi_remove_lookup(void *lookup) +{
gpiod_remove_lookup_table(lookup);+}
+static int arizona_spi_acpi_probe(struct arizona *arizona) +{
struct gpiod_lookup_table *lookup;int i, ret;/* Add mappings for the 2 ACPI declared GPIOs used for reset and ldo-ena */devm_acpi_dev_add_driver_gpios(arizona->dev, arizona_acpi_gpios);/* Add lookups for the SoCs own GPIOs used for micdet-polarity and spkVDD-enable */lookup = devm_kzalloc(arizona->dev,struct_size(lookup, table, ARRAY_SIZE(arizona_soc_gpios) + 1),GFP_KERNEL);if (!lookup)return -ENOMEM;lookup->dev_id = dev_name(arizona->dev);
for (i = 0; i < ARRAY_SIZE(arizona_soc_gpios); i++)lookup->table[i] = arizona_soc_gpios[i];
Would memcpy() do the same at one pass?
gpiod_add_lookup_table(lookup);ret = devm_add_action_or_reset(arizona->dev, arizona_spi_acpi_remove_lookup, lookup);if (ret)return ret;
/* Enable 32KHz clock from SoC to codec for jack-detect */acpi_evaluate_object(ACPI_HANDLE(arizona->dev), "CLKE", NULL, NULL);
No error check?
/** Some DSDTs wrongly declare the IRQ trigger-type as IRQF_TRIGGER_FALLING* The IRQ line will stay low when a new IRQ event happens between reading* the IRQ status flags and acknowledging them. When the IRQ line stays* low like this the IRQ will never trigger again when its type is set* to IRQF_TRIGGER_FALLING. Correct the IRQ trigger-type to fix this.*/arizona->pdata.irq_flags = IRQF_TRIGGER_LOW;/* Wait 200 ms after jack insertion */arizona->pdata.micd_detect_debounce = 200;/* Use standard AOSP values for headset-button mappings */arizona->pdata.micd_ranges = arizona_micd_aosp_ranges;arizona->pdata.num_micd_ranges = ARRAY_SIZE(arizona_micd_aosp_ranges);return 0;+}
+static const struct acpi_device_id arizona_acpi_match[] = {
{.id = "WM510204",.driver_data = WM5102,},{.id = "WM510205",.driver_data = WM5102,},
{ },
No need for comma here.
+}; +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match); +#else
+static void arizona_spi_acpi_probe(struct arizona *arizona) +{ +}
Can be one line?
+#endif
static int arizona_spi_probe(struct spi_device *spi) { const struct spi_device_id *id = spi_get_device_id(spi); @@ -30,6 +141,8 @@ static int arizona_spi_probe(struct spi_device *spi)
if (spi->dev.of_node) type = arizona_of_get_type(&spi->dev);
else if (ACPI_COMPANION(&spi->dev))type = (unsigned long)acpi_device_get_match_data(&spi->dev);
Can we rather get rid of these and use device_get_match_data() directly?
else type = id->driver_data;@@ -75,6 +188,12 @@ static int arizona_spi_probe(struct spi_device *spi) arizona->dev = &spi->dev; arizona->irq = spi->irq;
if (ACPI_COMPANION(&spi->dev)) {
has_acpi_companion() ?
ret = arizona_spi_acpi_probe(arizona);if (ret)return ret;}return arizona_dev_init(arizona);}
@@ -102,6 +221,7 @@ static struct spi_driver arizona_spi_driver = { .name = "arizona", .pm = &arizona_pm_ops, .of_match_table = of_match_ptr(arizona_of_match),
.acpi_match_table = ACPI_PTR(arizona_acpi_match), }, .probe = arizona_spi_probe, .remove = arizona_spi_remove,-- 2.28.0
Hi,
Thank you for the review.
On 12/28/20 3:14 PM, Andy Shevchenko wrote:
On Sun, Dec 27, 2020 at 11:16 PM Hans de Goede hdegoede@redhat.com wrote:
The Intel Bay Trail (x86/ACPI) based Lenovo Yoga Tablet 2 series use a WM5102 codec connected over SPI.
Add support for ACPI enumeration to arizona-spi so that arizona-spi can bind to the codec on these tablets.
This is loosely based on an earlier attempt (for Android-x86) at this by Christian Hartmann, combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [1].
Few nitpicks here and there, but the most important bit that hits me is device_get_match_data().
[1] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
Cc: Christian Hartmann cornogle@googlemail.com Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/mfd/arizona-spi.c | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+)
diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c index 704f214d2614..bcdbd72fefb5 100644 --- a/drivers/mfd/arizona-spi.c +++ b/drivers/mfd/arizona-spi.c @@ -7,7 +7,10 @@
- Author: Mark Brown broonie@opensource.wolfsonmicro.com
*/
+#include <linux/acpi.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> @@ -15,11 +18,119 @@ #include <linux/slab.h> #include <linux/spi/spi.h> #include <linux/of.h> +#include <uapi/linux/input-event-codes.h>
#include <linux/mfd/arizona/core.h>
#include "arizona.h"
+#ifdef CONFIG_ACPI +const struct acpi_gpio_params reset_gpios = { 1, 0, false }; +const struct acpi_gpio_params ldoena_gpios = { 2, 0, false };
+static const struct acpi_gpio_mapping arizona_acpi_gpios[] = {
{ "reset-gpios", &reset_gpios, 1, },{ "wlf,ldoena-gpios", &ldoena_gpios, 1 },{ }+};
+/*
- The ACPI resources for the device only describe external GPIO-s. They do
- not provide mappings for the GPIO-s coming from the Arizona codec itself.
- */
+static const struct gpiod_lookup arizona_soc_gpios[] = {
{ "arizona", 2, "wlf,spkvdd-ena", 0, GPIO_ACTIVE_HIGH },{ "arizona", 4, "wlf,micd-pol", 0, GPIO_ACTIVE_LOW },+};
+/*
- The AOSP 3.5 mm Headset: Accessory Specification gives the following values:
- Function A Play/Pause: 0 ohm
- Function D Voice assistant: 135 ohm
- Function B Volume Up 240 ohm
- Function C Volume Down 470 ohm
- Minimum Mic DC resistance 1000 ohm
- Minimum Ear speaker impedance 16 ohm
- Note the first max value below must be less then the min. speaker impedance,
- to allow CTIA/OMTP detection to work. The other max values are the closest
- value from extcon-arizona.c:arizona_micd_levels halfway 2 button resistances.
- */
+static const struct arizona_micd_range arizona_micd_aosp_ranges[] = {
{ .max = 11, .key = KEY_PLAYPAUSE },{ .max = 186, .key = KEY_VOICECOMMAND },{ .max = 348, .key = KEY_VOLUMEUP },{ .max = 752, .key = KEY_VOLUMEDOWN },+};
+static void arizona_spi_acpi_remove_lookup(void *lookup) +{
gpiod_remove_lookup_table(lookup);+}
+static int arizona_spi_acpi_probe(struct arizona *arizona) +{
struct gpiod_lookup_table *lookup;int i, ret;/* Add mappings for the 2 ACPI declared GPIOs used for reset and ldo-ena */devm_acpi_dev_add_driver_gpios(arizona->dev, arizona_acpi_gpios);/* Add lookups for the SoCs own GPIOs used for micdet-polarity and spkVDD-enable */lookup = devm_kzalloc(arizona->dev,struct_size(lookup, table, ARRAY_SIZE(arizona_soc_gpios) + 1),GFP_KERNEL);if (!lookup)return -ENOMEM;lookup->dev_id = dev_name(arizona->dev);
for (i = 0; i < ARRAY_SIZE(arizona_soc_gpios); i++)lookup->table[i] = arizona_soc_gpios[i];Would memcpy() do the same at one pass?
True, fixed for v2.
gpiod_add_lookup_table(lookup);ret = devm_add_action_or_reset(arizona->dev, arizona_spi_acpi_remove_lookup, lookup);if (ret)return ret;
/* Enable 32KHz clock from SoC to codec for jack-detect */acpi_evaluate_object(ACPI_HANDLE(arizona->dev), "CLKE", NULL, NULL);No error check?
The codec will still work without the 32KHz clk, but we will loose jack-detect functionality. I expect the ACPI call to already print some error, but to make sure something is logged and to clarify what any previous logged ACPI errors are about I've added code to log a warning when this fails for v2.
/** Some DSDTs wrongly declare the IRQ trigger-type as IRQF_TRIGGER_FALLING* The IRQ line will stay low when a new IRQ event happens between reading* the IRQ status flags and acknowledging them. When the IRQ line stays* low like this the IRQ will never trigger again when its type is set* to IRQF_TRIGGER_FALLING. Correct the IRQ trigger-type to fix this.*/arizona->pdata.irq_flags = IRQF_TRIGGER_LOW;/* Wait 200 ms after jack insertion */arizona->pdata.micd_detect_debounce = 200;/* Use standard AOSP values for headset-button mappings */arizona->pdata.micd_ranges = arizona_micd_aosp_ranges;arizona->pdata.num_micd_ranges = ARRAY_SIZE(arizona_micd_aosp_ranges);return 0;+}
+static const struct acpi_device_id arizona_acpi_match[] = {
{.id = "WM510204",.driver_data = WM5102,},{.id = "WM510205",.driver_data = WM5102,},
{ },No need for comma here.
Fixed for v2.
+}; +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match); +#else
+static void arizona_spi_acpi_probe(struct arizona *arizona) +{ +}
Can be one line?
I find it more readable as is.
+#endif
static int arizona_spi_probe(struct spi_device *spi) { const struct spi_device_id *id = spi_get_device_id(spi); @@ -30,6 +141,8 @@ static int arizona_spi_probe(struct spi_device *spi)
if (spi->dev.of_node) type = arizona_of_get_type(&spi->dev);
else if (ACPI_COMPANION(&spi->dev))type = (unsigned long)acpi_device_get_match_data(&spi->dev);Can we rather get rid of these and use device_get_match_data() directly?
That is a good idea, I'll add a nw preparation patch to v2 replacing the custom arizona_of_get_type() helper with device_get_match_data().
else type = id->driver_data;@@ -75,6 +188,12 @@ static int arizona_spi_probe(struct spi_device *spi) arizona->dev = &spi->dev; arizona->irq = spi->irq;
if (ACPI_COMPANION(&spi->dev)) {has_acpi_companion() ?
Fixed for v2.
ret = arizona_spi_acpi_probe(arizona);if (ret)return ret;}return arizona_dev_init(arizona);}
@@ -102,6 +221,7 @@ static struct spi_driver arizona_spi_driver = { .name = "arizona", .pm = &arizona_pm_ops, .of_match_table = of_match_ptr(arizona_of_match),
.acpi_match_table = ACPI_PTR(arizona_acpi_match), }, .probe = arizona_spi_probe, .remove = arizona_spi_remove,-- 2.28.0
Regards,
Hans
Hi Hans,
I love your patch! Yet something to improve:
[auto build test ERROR on lee-mfd/for-mfd-next] [also build test ERROR on chanwoo-extcon/extcon-next asoc/for-next v5.11-rc1 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/MFD-extcon-ASoC-Add-s... base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next config: s390-randconfig-r034-20201228 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/0day-ci/linux/commit/bc64046ad088b340f127f9eefdb8f941c03d... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Hans-de-Goede/MFD-extcon-ASoC-Add-support-for-Intel-Bay-Trail-boards-with-WM5102-codec/20201228-051806 git checkout bc64046ad088b340f127f9eefdb8f941c03dcb53 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x000000ffUL) << 24) | \ ^ In file included from drivers/mfd/arizona-spi.c:16: In file included from include/linux/regmap.h:20: In file included from include/linux/iopoll.h:14: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:80: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x0000ff00UL) << 8) | \ ^ In file included from drivers/mfd/arizona-spi.c:16: In file included from include/linux/regmap.h:20: In file included from include/linux/iopoll.h:14: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:80: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x00ff0000UL) >> 8) | \ ^ In file included from drivers/mfd/arizona-spi.c:16: In file included from include/linux/regmap.h:20: In file included from include/linux/iopoll.h:14: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:80: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0xff000000UL) >> 24))) ^ In file included from drivers/mfd/arizona-spi.c:16: In file included from include/linux/regmap.h:20: In file included from include/linux/iopoll.h:14: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:80: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32' __fswab32(x)) ^ In file included from drivers/mfd/arizona-spi.c:16: In file included from include/linux/regmap.h:20: In file included from include/linux/iopoll.h:14: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:80: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] readsl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesb(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesw(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^ include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] writesl(PCI_IOBASE + addr, buffer, count); ~~~~~~~~~~ ^
drivers/mfd/arizona-spi.c:192:7: error: assigning to 'int' from incompatible type 'void'
ret = arizona_spi_acpi_probe(arizona); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 20 warnings and 1 error generated.
vim +192 drivers/mfd/arizona-spi.c
133 134 static int arizona_spi_probe(struct spi_device *spi) 135 { 136 const struct spi_device_id *id = spi_get_device_id(spi); 137 struct arizona *arizona; 138 const struct regmap_config *regmap_config = NULL; 139 unsigned long type; 140 int ret; 141 142 if (spi->dev.of_node) 143 type = arizona_of_get_type(&spi->dev); 144 else if (ACPI_COMPANION(&spi->dev)) 145 type = (unsigned long)acpi_device_get_match_data(&spi->dev); 146 else 147 type = id->driver_data; 148 149 switch (type) { 150 case WM5102: 151 if (IS_ENABLED(CONFIG_MFD_WM5102)) 152 regmap_config = &wm5102_spi_regmap; 153 break; 154 case WM5110: 155 case WM8280: 156 if (IS_ENABLED(CONFIG_MFD_WM5110)) 157 regmap_config = &wm5110_spi_regmap; 158 break; 159 case WM1831: 160 case CS47L24: 161 if (IS_ENABLED(CONFIG_MFD_CS47L24)) 162 regmap_config = &cs47l24_spi_regmap; 163 break; 164 default: 165 dev_err(&spi->dev, "Unknown device type %ld\n", type); 166 return -EINVAL; 167 } 168 169 if (!regmap_config) { 170 dev_err(&spi->dev, 171 "No kernel support for device type %ld\n", type); 172 return -EINVAL; 173 } 174 175 arizona = devm_kzalloc(&spi->dev, sizeof(*arizona), GFP_KERNEL); 176 if (arizona == NULL) 177 return -ENOMEM; 178 179 arizona->regmap = devm_regmap_init_spi(spi, regmap_config); 180 if (IS_ERR(arizona->regmap)) { 181 ret = PTR_ERR(arizona->regmap); 182 dev_err(&spi->dev, "Failed to allocate register map: %d\n", 183 ret); 184 return ret; 185 } 186 187 arizona->type = type; 188 arizona->dev = &spi->dev; 189 arizona->irq = spi->irq; 190 191 if (ACPI_COMPANION(&spi->dev)) {
192 ret = arizona_spi_acpi_probe(arizona);
193 if (ret) 194 return ret; 195 } 196 197 return arizona_dev_init(arizona); 198 } 199
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Hans,
I love your patch! Yet something to improve:
[auto build test ERROR on lee-mfd/for-mfd-next] [also build test ERROR on chanwoo-extcon/extcon-next asoc/for-next v5.11-rc1 next-20201223] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/MFD-extcon-ASoC-Add-s... base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next config: alpha-randconfig-r035-20201228 (attached as .config) compiler: alpha-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/bc64046ad088b340f127f9eefdb8f941c03d... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Hans-de-Goede/MFD-extcon-ASoC-Add-support-for-Intel-Bay-Trail-boards-with-WM5102-codec/20201228-051806 git checkout bc64046ad088b340f127f9eefdb8f941c03dcb53 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/mfd/arizona-spi.c: In function 'arizona_spi_probe':
drivers/mfd/arizona-spi.c:192:7: error: void value not ignored as it ought to be
192 | ret = arizona_spi_acpi_probe(arizona); | ^
vim +192 drivers/mfd/arizona-spi.c
133 134 static int arizona_spi_probe(struct spi_device *spi) 135 { 136 const struct spi_device_id *id = spi_get_device_id(spi); 137 struct arizona *arizona; 138 const struct regmap_config *regmap_config = NULL; 139 unsigned long type; 140 int ret; 141 142 if (spi->dev.of_node) 143 type = arizona_of_get_type(&spi->dev); 144 else if (ACPI_COMPANION(&spi->dev)) 145 type = (unsigned long)acpi_device_get_match_data(&spi->dev); 146 else 147 type = id->driver_data; 148 149 switch (type) { 150 case WM5102: 151 if (IS_ENABLED(CONFIG_MFD_WM5102)) 152 regmap_config = &wm5102_spi_regmap; 153 break; 154 case WM5110: 155 case WM8280: 156 if (IS_ENABLED(CONFIG_MFD_WM5110)) 157 regmap_config = &wm5110_spi_regmap; 158 break; 159 case WM1831: 160 case CS47L24: 161 if (IS_ENABLED(CONFIG_MFD_CS47L24)) 162 regmap_config = &cs47l24_spi_regmap; 163 break; 164 default: 165 dev_err(&spi->dev, "Unknown device type %ld\n", type); 166 return -EINVAL; 167 } 168 169 if (!regmap_config) { 170 dev_err(&spi->dev, 171 "No kernel support for device type %ld\n", type); 172 return -EINVAL; 173 } 174 175 arizona = devm_kzalloc(&spi->dev, sizeof(*arizona), GFP_KERNEL); 176 if (arizona == NULL) 177 return -ENOMEM; 178 179 arizona->regmap = devm_regmap_init_spi(spi, regmap_config); 180 if (IS_ERR(arizona->regmap)) { 181 ret = PTR_ERR(arizona->regmap); 182 dev_err(&spi->dev, "Failed to allocate register map: %d\n", 183 ret); 184 return ret; 185 } 186 187 arizona->type = type; 188 arizona->dev = &spi->dev; 189 arizona->irq = spi->irq; 190 191 if (ACPI_COMPANION(&spi->dev)) {
192 ret = arizona_spi_acpi_probe(arizona);
193 if (ret) 194 return ret; 195 } 196 197 return arizona_dev_init(arizona); 198 } 199
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
There is no reason why the arizona core,irq and codec model specific regmap bits cannot be build as a module. All they do is export symbols which are used by the arizona-spi and/or arizona-i2c modules, which themselves can be built as module.
Change the Kconfig and Makefile arizona bits so that the arizona MFD-core can be built as a module.
This is especially useful on x86 platforms with a WM5102 codec, this allows the arizona MFD driver necessary for the WM5102 codec to be enabled in generic distro-kernels without growing the base kernel-image size.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mfd/Kconfig | 2 +- drivers/mfd/Makefile | 14 +++++++------- drivers/mfd/arizona-core.c | 2 ++ 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index cc0b73280c68..8fe9e10eaf51 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1831,7 +1831,7 @@ config MFD_ARIZONA select REGMAP select REGMAP_IRQ select MFD_CORE - bool + tristate
config MFD_ARIZONA_I2C tristate "Cirrus Logic/Wolfson Microelectronics Arizona platform with I2C" diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 14fdb188af02..3f208af1664f 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -41,24 +41,24 @@ obj-$(CONFIG_MFD_TQMX86) += tqmx86.o
obj-$(CONFIG_MFD_LOCHNAGAR) += lochnagar-i2c.o
-obj-$(CONFIG_MFD_ARIZONA) += arizona-core.o -obj-$(CONFIG_MFD_ARIZONA) += arizona-irq.o +arizona-objs := arizona-core.o arizona-irq.o +obj-$(CONFIG_MFD_ARIZONA) += arizona.o obj-$(CONFIG_MFD_ARIZONA_I2C) += arizona-i2c.o obj-$(CONFIG_MFD_ARIZONA_SPI) += arizona-spi.o ifeq ($(CONFIG_MFD_WM5102),y) -obj-$(CONFIG_MFD_ARIZONA) += wm5102-tables.o +arizona-objs += wm5102-tables.o endif ifeq ($(CONFIG_MFD_WM5110),y) -obj-$(CONFIG_MFD_ARIZONA) += wm5110-tables.o +arizona-objs += wm5110-tables.o endif ifeq ($(CONFIG_MFD_WM8997),y) -obj-$(CONFIG_MFD_ARIZONA) += wm8997-tables.o +arizona-objs += wm8997-tables.o endif ifeq ($(CONFIG_MFD_WM8998),y) -obj-$(CONFIG_MFD_ARIZONA) += wm8998-tables.o +arizona-objs += wm8998-tables.o endif ifeq ($(CONFIG_MFD_CS47L24),y) -obj-$(CONFIG_MFD_ARIZONA) += cs47l24-tables.o +arizona-objs += cs47l24-tables.o endif obj-$(CONFIG_MFD_WCD934X) += wcd934x.o obj-$(CONFIG_MFD_WM8400) += wm8400-core.o diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c index 000cb82023e3..a9ba1c865abf 100644 --- a/drivers/mfd/arizona-core.c +++ b/drivers/mfd/arizona-core.c @@ -1478,3 +1478,5 @@ int arizona_dev_exit(struct arizona *arizona) return 0; } EXPORT_SYMBOL_GPL(arizona_dev_exit); + +MODULE_LICENSE("GPL v2");
On Sun, Dec 27, 2020 at 10:12:22PM +0100, Hans de Goede wrote:
There is no reason why the arizona core,irq and codec model specific regmap bits cannot be build as a module. All they do is export symbols which are used by the arizona-spi and/or arizona-i2c modules, which themselves can be built as module.
Change the Kconfig and Makefile arizona bits so that the arizona MFD-core can be built as a module.
This is especially useful on x86 platforms with a WM5102 codec, this allows the arizona MFD driver necessary for the WM5102 codec to be enabled in generic distro-kernels without growing the base kernel-image size.
Signed-off-by: Hans de Goede hdegoede@redhat.com
I think this patch might still cause some issues. ASoC has an idiom where the machine driver does a select on the necessary CODEC drivers. Select doesn't take care of dependencies etc. So I believe if you build the machine driver as built in, it then selects the CODEC as built in. If you have the MFD as a module the build then fails due to the the CODEC calling some Arizona functions.
arizona_request_irq, arizona_free_irq, arizona_set_irq_wake
On Madera we made the equivalents inline functions to avoid the issue, the same should work here.
include/linux/irqchip/irq-madera.h
Thanks, Charles
Hi,
First of all thank you for the review and for all your comments (also in the other part of the thread).
On 12/29/20 1:00 PM, Charles Keepax wrote:
On Sun, Dec 27, 2020 at 10:12:22PM +0100, Hans de Goede wrote:
There is no reason why the arizona core,irq and codec model specific regmap bits cannot be build as a module. All they do is export symbols which are used by the arizona-spi and/or arizona-i2c modules, which themselves can be built as module.
Change the Kconfig and Makefile arizona bits so that the arizona MFD-core can be built as a module.
This is especially useful on x86 platforms with a WM5102 codec, this allows the arizona MFD driver necessary for the WM5102 codec to be enabled in generic distro-kernels without growing the base kernel-image size.
Signed-off-by: Hans de Goede hdegoede@redhat.com
I think this patch might still cause some issues. ASoC has an idiom where the machine driver does a select on the necessary CODEC drivers. Select doesn't take care of dependencies etc. So I believe if you build the machine driver as built in, it then selects the CODEC as built in. If you have the MFD as a module the build then fails due to the the CODEC calling some Arizona functions.
The rules for using select in Kconfig say that the Kconfig symbol doing the selecting must either have a "requires on or a "select" on any dependencies of the Kconfig symbol which it is selecting. Looking at the new machine-driver which this series adds:
config SND_SOC_INTEL_BYTCR_WM5102_MACH tristate "Baytrail and Baytrail-CR with WM5102 codec" depends on SPI && ACPI depends on X86_INTEL_LPSS || COMPILE_TEST select SND_SOC_ACPI select SND_SOC_WM5102 help This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR platforms with WM5102 audio codec. Say Y if you have such a device. If unsure select "N".
I now see that I'm breaking that rule myself and this is missing a "depends on MFD_WM5102" I do see a "depends on MFD_WM5102" here:
config SND_SOC_WM5102 tristate depends on MFD_WM5102
So I would expect some sort of error if I unselect MFD_WM5102 and indeed the following happens if I unselect MFD_WM5102:
[hans@x1 linux]$ make oldconfig
WARNING: unmet direct dependencies detected for SND_SOC_WM5102 Depends on [n]: SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && MFD_WM5102 [=n] Selected by [m]: - SND_SOC_INTEL_BYTCR_WM5102_MACH [=m] && SOUND [=m] && !UML && SND [=m] && SND_SOC [=m] && SND_SOC_INTEL_MACH [=y] && (SND_SST_ATOM_HIFI2_PLATFORM [=m] || SND_SOC_SOF_BAYTRAIL [=m]) && SPI [=y] && ACPI [=y] && (X86_INTEL_LPSS [=y] || COMPILE_TEST [=n])
Which of course is not supposed to happen and is caused by the config SND_SOC_INTEL_BYTCR_WM5102_MACH missing "depends on MFD_WM5102", which is my bad.
But typing this (rubber ducky effect) has made me realized what the problem is, the codec Kconfig symbols depend on the: MFD_WM5102, MFD_WM5110, etc. Kconfig symbols. But those are booleans which enable/disable support for those codecs inside arizona-core.c. So you are right that this ("mfd: arizona: Allow building arizona MFD-core as module") could cause a scenario where the core is built as a module and the codec driver is builtin.
But I do not think that the solution is to inline all the functions used from the codec drivers. The solution is to extend:
config SND_SOC_WM5102 tristate depends on MFD_WM5102
to:
config SND_SOC_WM5102 tristate depends on MFD_WM5102 depends on MFD_ARIZONA
And to update machine drivers to still match the: "The Kconfig symbol doing the selecting must either have a "requires on" or a "select" on any dependencies of the Kconfig symbol which it is selecting." rule which I formulated above.
(and idem for all the other codecs which use symbols from MFD_ARIZONA)
arizona_request_irq, arizona_free_irq, arizona_set_irq_wake
On Madera we made the equivalents inline functions to avoid the issue, the same should work here.
include/linux/irqchip/irq-madera.h
Yes that will work too, but I would prefer to solve what is a Kconfig issue with Kconfig changes.
Note I will simply drop this patch for the next version of this series (*) since the whole jack rework thing we discussed is really orthogonal to this and that one will be interesting enough the review all by itself :)
Regards,
Hans
*) I will resubmit a fixed version later after the other stuff has landed
When the jack is partially inserted and then removed again it may be removed while the hpdet code is running. In this case the following may happen:
1. The "JACKDET rise" or ""JACKDET fall" IRQ triggers 2. arizona_jackdet runs and takes info->lock 3. The "HPDET" IRQ triggers 4. arizona_hpdet_irq runs, blocks on info->lock 5. arizona_jackdet calls arizona_stop_mic() and clears info->hpdet_done 6. arizona_jackdet releases info->lock 7. arizona_hpdet_irq now can continue running and: 7.1 Calls arizona_start_mic() (if a mic was detected) 7.2 sets info->hpdet_done
Step 7 is undeseriable / a bug: 7.1 causes the device to stay in a high power-state (with MICVDD enabled) 7.2 causes hpdet to not run on the next jack insertion, which in turn causes the EXTCON_JACK_HEADPHONE state to never get set
This fixes both issues by skipping these 2 steps when arizona_hpdet_irq runs after the jack has been unplugged.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index aae82db542a5..f7ef247de46a 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -601,7 +601,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data) struct arizona *arizona = info->arizona; int id_gpio = arizona->pdata.hpdet_id_gpio; unsigned int report = EXTCON_JACK_HEADPHONE; - int ret, reading; + int ret, reading, state; bool mic = false;
mutex_lock(&info->lock); @@ -614,12 +614,11 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data) }
/* If the cable was removed while measuring ignore the result */ - ret = extcon_get_state(info->edev, EXTCON_MECHANICAL); - if (ret < 0) { - dev_err(arizona->dev, "Failed to check cable state: %d\n", - ret); + state = extcon_get_state(info->edev, EXTCON_MECHANICAL); + if (state < 0) { + dev_err(arizona->dev, "Failed to check cable state: %d\n", state); goto out; - } else if (!ret) { + } else if (!state) { dev_dbg(arizona->dev, "Ignoring HPDET for removed cable\n"); goto done; } @@ -667,7 +666,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data) gpio_set_value_cansleep(id_gpio, 0);
/* If we have a mic then reenable MICDET */ - if (mic || info->mic) + if (state && (mic || info->mic)) arizona_start_mic(info);
if (info->hpdet_active) { @@ -675,7 +674,9 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data) info->hpdet_active = false; }
- info->hpdet_done = true; + /* Do not set hp_det done when the cable has been unplugged */ + if (state) + info->hpdet_done = true;
out: mutex_unlock(&info->lock);
We must free/disable all interrupts and cancel all pending works before doing further cleanup.
Before this commit arizona_extcon_remove() was doing several register writes to shut things down before disabling the IRQs and it was cancelling only 1 of the 3 different works used.
Move all the register-writes shutting things down to after the disabling of the IRQs and add the 2 missing cancel_delayed_work_sync() calls.
This fixes various possible races on driver unbind. One of which would always trigger on devices using the mic-clamp feature for jack detection. The ARIZONA_MICD_CLAMP_MODE_MASK update was done before disabling the IRQs, causing: 1. arizona_jackdet() to run 2. detect a jack being inserted (clamp disabled means jack inserted) 3. call arizona_start_mic() which: 3.1 Enables the MICVDD regulator 3.2 takes a pm_runtime_reference
And this was all happening after the ARIZONA_MICD_ENA bit clearing, which would undo 3.1 and 3.2 because the ARIZONA_MICD_CLAMP_MODE_MASK update was being done after the ARIZONA_MICD_ENA bit clearing.
So this means that arizona_extcon_remove() would exit with 1. MICVDD enabled and 2. The pm_runtime_reference being unbalanced.
MICVDD still being enabled caused the following oops when the regulator is released by the devm framework:
[ 2850.745757] ------------[ cut here ]------------ [ 2850.745827] WARNING: CPU: 2 PID: 2098 at drivers/regulator/core.c:2123 _regulator_put.part.0+0x19f/0x1b0 [ 2850.745835] Modules linked in: extcon_arizona ... ... [ 2850.746909] Call Trace: [ 2850.746932] regulator_put+0x2d/0x40 [ 2850.746946] release_nodes+0x22a/0x260 [ 2850.746984] __device_release_driver+0x190/0x240 [ 2850.747002] driver_detach+0xd4/0x120 ... [ 2850.747337] ---[ end trace f455dfd7abd9781f ]---
Note this oops is just one of various theoretically possible races caused by the wrong ordering inside arizona_extcon_remove(), this fixes the ordering fixing all possible races, including the reported oops.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 40 +++++++++++++++++---------------- 1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index f7ef247de46a..76aacbac5869 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -1760,25 +1760,6 @@ static int arizona_extcon_remove(struct platform_device *pdev) bool change; int ret;
- ret = regmap_update_bits_check(arizona->regmap, ARIZONA_MIC_DETECT_1, - ARIZONA_MICD_ENA, 0, - &change); - if (ret < 0) { - dev_err(&pdev->dev, "Failed to disable micd on remove: %d\n", - ret); - } else if (change) { - regulator_disable(info->micvdd); - pm_runtime_put(info->dev); - } - - gpiod_put(info->micd_pol_gpio); - - pm_runtime_disable(&pdev->dev); - - regmap_update_bits(arizona->regmap, - ARIZONA_MICD_CLAMP_CONTROL, - ARIZONA_MICD_CLAMP_MODE_MASK, 0); - if (info->micd_clamp) { jack_irq_rise = ARIZONA_IRQ_MICD_CLAMP_RISE; jack_irq_fall = ARIZONA_IRQ_MICD_CLAMP_FALL; @@ -1794,10 +1775,31 @@ static int arizona_extcon_remove(struct platform_device *pdev) arizona_free_irq(arizona, jack_irq_rise, info); arizona_free_irq(arizona, jack_irq_fall, info); cancel_delayed_work_sync(&info->hpdet_work); + cancel_delayed_work_sync(&info->micd_detect_work); + cancel_delayed_work_sync(&info->micd_timeout_work); + + ret = regmap_update_bits_check(arizona->regmap, ARIZONA_MIC_DETECT_1, + ARIZONA_MICD_ENA, 0, + &change); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to disable micd on remove: %d\n", + ret); + } else if (change) { + regulator_disable(info->micvdd); + pm_runtime_put(info->dev); + } + + regmap_update_bits(arizona->regmap, + ARIZONA_MICD_CLAMP_CONTROL, + ARIZONA_MICD_CLAMP_MODE_MASK, 0); regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE, ARIZONA_JD1_ENA, 0); arizona_clk32k_disable(arizona);
+ gpiod_put(info->micd_pol_gpio); + + pm_runtime_disable(&pdev->dev); + return 0; }
On Sun, Dec 27, 2020 at 10:12:24PM +0100, Hans de Goede wrote:
We must free/disable all interrupts and cancel all pending works before doing further cleanup.
Before this commit arizona_extcon_remove() was doing several register writes to shut things down before disabling the IRQs and it was cancelling only 1 of the 3 different works used.
Move all the register-writes shutting things down to after the disabling of the IRQs and add the 2 missing cancel_delayed_work_sync() calls.
This fixes various possible races on driver unbind. One of which would always trigger on devices using the mic-clamp feature for jack detection. The ARIZONA_MICD_CLAMP_MODE_MASK update was done before disabling the IRQs, causing:
- arizona_jackdet() to run
- detect a jack being inserted (clamp disabled means jack inserted)
- call arizona_start_mic() which:
3.1 Enables the MICVDD regulator 3.2 takes a pm_runtime_reference
And this was all happening after the ARIZONA_MICD_ENA bit clearing, which would undo 3.1 and 3.2 because the ARIZONA_MICD_CLAMP_MODE_MASK update was being done after the ARIZONA_MICD_ENA bit clearing.
So this means that arizona_extcon_remove() would exit with
- MICVDD enabled and 2. The pm_runtime_reference being unbalanced.
MICVDD still being enabled caused the following oops when the regulator is released by the devm framework:
[ 2850.745757] ------------[ cut here ]------------ [ 2850.745827] WARNING: CPU: 2 PID: 2098 at drivers/regulator/core.c:2123 _regulator_put.part.0+0x19f/0x1b0 [ 2850.745835] Modules linked in: extcon_arizona ... ... [ 2850.746909] Call Trace: [ 2850.746932] regulator_put+0x2d/0x40 [ 2850.746946] release_nodes+0x22a/0x260 [ 2850.746984] __device_release_driver+0x190/0x240 [ 2850.747002] driver_detach+0xd4/0x120 ... [ 2850.747337] ---[ end trace f455dfd7abd9781f ]---
Note this oops is just one of various theoretically possible races caused by the wrong ordering inside arizona_extcon_remove(), this fixes the ordering fixing all possible races, including the reported oops.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Sorry yes there are a few rough corners on the extcon stuff, I have clean up series I have been working on as part of upstreaming the Madera extcon hopefully I will get that sent out one day.
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
Fix the modalias so that the driver will be loaded automatically. The module's name is "extcon-arizona", following other extcon module-names.
But the driver's and platform-device's name is "arizona-extcon" and the modalias must match that.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index 76aacbac5869..95acfe7620fd 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -1816,4 +1816,4 @@ module_platform_driver(arizona_extcon_driver); MODULE_DESCRIPTION("Arizona Extcon driver"); MODULE_AUTHOR("Mark Brown broonie@opensource.wolfsonmicro.com"); MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:extcon-arizona"); +MODULE_ALIAS("platform:arizona-extcon");
On Sun, Dec 27, 2020 at 10:12:25PM +0100, Hans de Goede wrote:
Fix the modalias so that the driver will be loaded automatically. The module's name is "extcon-arizona", following other extcon module-names.
But the driver's and platform-device's name is "arizona-extcon" and the modalias must match that.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
The initial value of the GPIO should match the info->micd_modes[0].gpio value. arizona_extcon_probe() already stores the necessary flag in a mode variable, but instead of passing mode as flags to the gpiod_get() it was using a hardcoded GPIOD_OUT_LOW.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index 95acfe7620fd..145ca5cd40d5 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -1510,7 +1510,7 @@ static int arizona_extcon_probe(struct platform_device *pdev) */ info->micd_pol_gpio = gpiod_get_optional(arizona->dev, "wlf,micd-pol", - GPIOD_OUT_LOW); + mode); if (IS_ERR(info->micd_pol_gpio)) { ret = PTR_ERR(info->micd_pol_gpio); dev_err(arizona->dev,
On Sun, Dec 27, 2020 at 10:12:26PM +0100, Hans de Goede wrote:
The initial value of the GPIO should match the info->micd_modes[0].gpio value. arizona_extcon_probe() already stores the necessary flag in a mode variable, but instead of passing mode as flags to the gpiod_get() it was using a hardcoded GPIOD_OUT_LOW.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
All the callers of extcon_set_state_sync() log an error on failure, without doing any further error-handling (as there is nothing they can do about the error).
Factor this out into a helper to remove some duplicate code.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 47 ++++++++++++--------------------- 1 file changed, 17 insertions(+), 30 deletions(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index 145ca5cd40d5..d5b3231744f9 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -595,6 +595,16 @@ static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading, return 0; }
+static void arizona_set_extcon_state(struct arizona_extcon_info *info, + unsigned int id, bool state) +{ + int ret; + + ret = extcon_set_state_sync(info->edev, id, state); + if (ret) + dev_err(info->arizona->dev, "Failed to set extcon state: %d\n", ret); +} + static irqreturn_t arizona_hpdet_irq(int irq, void *data) { struct arizona_extcon_info *info = data; @@ -648,11 +658,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data) else report = EXTCON_JACK_HEADPHONE;
- ret = extcon_set_state_sync(info->edev, report, true); - if (ret != 0) - dev_err(arizona->dev, "Failed to report HP/line: %d\n", - ret); - + arizona_set_extcon_state(info, report, true); done: /* Reset back to starting range */ regmap_update_bits(arizona->regmap, @@ -727,9 +733,7 @@ static void arizona_identify_headphone(struct arizona_extcon_info *info) pm_runtime_put_autosuspend(info->dev);
/* Just report headphone */ - ret = extcon_set_state_sync(info->edev, EXTCON_JACK_HEADPHONE, true); - if (ret != 0) - dev_err(arizona->dev, "Failed to report headphone: %d\n", ret); + arizona_set_extcon_state(info, EXTCON_JACK_HEADPHONE, true);
if (info->mic) arizona_start_mic(info); @@ -781,10 +785,7 @@ static void arizona_start_hpdet_acc_id(struct arizona_extcon_info *info)
err: /* Just report headphone */ - ret = extcon_set_state_sync(info->edev, EXTCON_JACK_HEADPHONE, true); - if (ret != 0) - dev_err(arizona->dev, "Failed to report headphone: %d\n", ret); - + arizona_set_extcon_state(info, EXTCON_JACK_HEADPHONE, true); info->hpdet_active = false; }
@@ -904,11 +905,7 @@ static int arizona_micdet_reading(void *priv)
arizona_identify_headphone(info);
- ret = extcon_set_state_sync(info->edev, - EXTCON_JACK_MICROPHONE, true); - if (ret != 0) - dev_err(arizona->dev, "Headset report failed: %d\n", - ret); + arizona_set_extcon_state(info, EXTCON_JACK_MICROPHONE, true);
/* Don't need to regulate for button detection */ ret = regulator_allow_bypass(info->micvdd, true); @@ -1175,12 +1172,7 @@ static irqreturn_t arizona_jackdet(int irq, void *data)
if (info->last_jackdet == present) { dev_dbg(arizona->dev, "Detected jack\n"); - ret = extcon_set_state_sync(info->edev, - EXTCON_MECHANICAL, true); - - if (ret != 0) - dev_err(arizona->dev, "Mechanical report failed: %d\n", - ret); + arizona_set_extcon_state(info, EXTCON_MECHANICAL, true);
info->detecting = true; info->mic = false; @@ -1216,13 +1208,8 @@ static irqreturn_t arizona_jackdet(int irq, void *data) info->micd_ranges[i].key, 0); input_sync(info->input);
- for (i = 0; i < ARRAY_SIZE(arizona_cable) - 1; i++) { - ret = extcon_set_state_sync(info->edev, - arizona_cable[i], false); - if (ret != 0) - dev_err(arizona->dev, - "Removal report failed: %d\n", ret); - } + for (i = 0; i < ARRAY_SIZE(arizona_cable) - 1; i++) + arizona_set_extcon_state(info, arizona_cable[i], false);
/* * If the jack was removed during a headphone detection we
On Sun, Dec 27, 2020 at 10:12:27PM +0100, Hans de Goede wrote:
All the callers of extcon_set_state_sync() log an error on failure, without doing any further error-handling (as there is nothing they can do about the error).
Factor this out into a helper to remove some duplicate code.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/extcon/extcon-arizona.c | 47 ++++++++++++--------------------- 1 file changed, 17 insertions(+), 30 deletions(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index 145ca5cd40d5..d5b3231744f9 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -595,6 +595,16 @@ static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading, return 0; }
+static void arizona_set_extcon_state(struct arizona_extcon_info *info,
unsigned int id, bool state)+{
- int ret;
- ret = extcon_set_state_sync(info->edev, id, state);
- if (ret)
dev_err(info->arizona->dev, "Failed to set extcon state: %d\n", ret);+}
Would be nice to also print which ID it is that is failing, would help to narrow things down since we lose the customer error messages for each case.
Thanks, Charles
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 extcon-arizona driver handles jack-detect support and exports info about the jack state to userspace through the standard extcon sysfs class interface.
Standard Linux userspace does not monitor/use the extcon sysfs interface for jack-detection, resulting in the jack-state not being taken into account by userspace.
The ASoC machine-driver may have created a standard ASoC jack when registering the card. In this case also report the jack-state through the ASoC jack so that jack-detection works with standard Linux userspace.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index d5b3231744f9..931a7d239aea 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -20,6 +20,7 @@ #include <linux/regulator/consumer.h> #include <linux/extcon-provider.h>
+#include <sound/jack.h> #include <sound/soc.h>
#include <linux/mfd/arizona/core.h> @@ -598,11 +599,19 @@ static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading, static void arizona_set_extcon_state(struct arizona_extcon_info *info, unsigned int id, bool state) { - int ret; + int ret, mask = 0;
ret = extcon_set_state_sync(info->edev, id, state); if (ret) dev_err(info->arizona->dev, "Failed to set extcon state: %d\n", ret); + + switch (id) { + case EXTCON_JACK_HEADPHONE: mask = SND_JACK_HEADPHONE; break; + case EXTCON_JACK_MICROPHONE: mask = SND_JACK_MICROPHONE; break; + } + + if (info->arizona->jack && mask) + snd_soc_jack_report(info->arizona->jack, state ? mask : 0, mask); }
static irqreturn_t arizona_hpdet_irq(int irq, void *data)
On Sun, Dec 27, 2020 at 11:16 PM Hans de Goede hdegoede@redhat.com 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 extcon-arizona driver handles jack-detect support and exports info about the jack state to userspace through the standard extcon sysfs class interface.
Standard Linux userspace does not monitor/use the extcon sysfs interface for jack-detection, resulting in the jack-state not being taken into account by userspace.
The ASoC machine-driver may have created a standard ASoC jack when registering the card. In this case also report the jack-state through the ASoC jack so that jack-detection works with standard Linux userspace.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/extcon/extcon-arizona.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index d5b3231744f9..931a7d239aea 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -20,6 +20,7 @@ #include <linux/regulator/consumer.h> #include <linux/extcon-provider.h>
+#include <sound/jack.h> #include <sound/soc.h>
#include <linux/mfd/arizona/core.h> @@ -598,11 +599,19 @@ static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int *reading, static void arizona_set_extcon_state(struct arizona_extcon_info *info, unsigned int id, bool state) {
int ret;
int ret, mask = 0;
I would rather prefer... drop assignment here...
ret = extcon_set_state_sync(info->edev, id, state); if (ret) dev_err(info->arizona->dev, "Failed to set extcon state: %d\n", ret);
switch (id) {case EXTCON_JACK_HEADPHONE: mask = SND_JACK_HEADPHONE; break;case EXTCON_JACK_MICROPHONE: mask = SND_JACK_MICROPHONE; break;
...introduce default here, which immediately bails out (return)...
}if (info->arizona->jack && mask)
...and drop mask check here.
snd_soc_jack_report(info->arizona->jack, state ? mask : 0, mask);}
static irqreturn_t arizona_hpdet_irq(int irq, void *data)
2.28.0
When the machine driver creates an ASoC jack for jack-detect and CONFIG_SND_JACK_INPUT_DEV is enabled then this will also create an input-device. In this case use the already created input-device to report button presses instead of creating a second "Headset" input-device for the same headset.
This relies on the machine-driver setting up the jack-input-device to report the EV_KEY key-codes configured in arizona_pdata.micd_ranges, before registering it.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/extcon/extcon-arizona.c | 46 +++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index 931a7d239aea..292ca4088418 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -1376,6 +1376,7 @@ static int arizona_extcon_probe(struct platform_device *pdev) { struct arizona *arizona = dev_get_drvdata(pdev->dev.parent); struct arizona_pdata *pdata = &arizona->pdata; + bool using_jack_input_dev = false; struct arizona_extcon_info *info; unsigned int val; unsigned int clamp_mode; @@ -1453,15 +1454,28 @@ static int arizona_extcon_probe(struct platform_device *pdev) return ret; }
- info->input = devm_input_allocate_device(&pdev->dev); - if (!info->input) { - dev_err(arizona->dev, "Can't allocate input dev\n"); - ret = -ENOMEM; - return ret; - } +#ifdef CONFIG_SND_JACK_INPUT_DEV + if (arizona->jack) { + info->input = input_get_device(arizona->jack->jack->input_dev); + using_jack_input_dev = true; + } else +#endif + { + info->input = devm_input_allocate_device(&pdev->dev); + if (!info->input) { + dev_err(arizona->dev, "Can't allocate input dev\n"); + ret = -ENOMEM; + return ret; + }
- info->input->name = "Headset"; - info->input->phys = "arizona/extcon"; + /* + * balance the put in arizona_extcon_remove, which is necessary + * when re-using the jack-device's input-device. + */ + input_get_device(info->input); + info->input->name = "Headset"; + info->input->phys = "arizona/extcon"; + }
if (!pdata->micd_timeout) pdata->micd_timeout = DEFAULT_MICD_TIMEOUT; @@ -1603,8 +1617,9 @@ static int arizona_extcon_probe(struct platform_device *pdev) arizona_micd_levels[j], i);
arizona_micd_set_level(arizona, i, j); - input_set_capability(info->input, EV_KEY, - info->micd_ranges[i].key); + if (!using_jack_input_dev) + input_set_capability(info->input, EV_KEY, + info->micd_ranges[i].key);
/* Enable reporting of that range */ regmap_update_bits(arizona->regmap, ARIZONA_MIC_DETECT_2, @@ -1718,10 +1733,12 @@ static int arizona_extcon_probe(struct platform_device *pdev) dev_warn(arizona->dev, "Failed to set MICVDD to bypass: %d\n", ret);
- ret = input_register_device(info->input); - if (ret) { - dev_err(&pdev->dev, "Can't register input device: %d\n", ret); - goto err_hpdet; + if (!using_jack_input_dev) { + ret = input_register_device(info->input); + if (ret) { + dev_err(&pdev->dev, "Can't register input device: %d\n", ret); + goto err_hpdet; + } }
pm_runtime_put(&pdev->dev); @@ -1792,6 +1809,7 @@ static int arizona_extcon_remove(struct platform_device *pdev) ARIZONA_JD1_ENA, 0); arizona_clk32k_disable(arizona);
+ input_put_device(info->input); gpiod_put(info->micd_pol_gpio);
pm_runtime_disable(&pdev->dev);
Some Bay Trail systems: 1. Use a non CR version of the Bay Trail SoC 2. Contain at least 6 interrupt resources so that the platform_get_resource(pdev, IORESOURCE_IRQ, 5) check to workaround non CR systems which list their IPC IRQ at index 0 despite being non CR does not work 3. Despite 1. and 2. still have their IPC IRQ at index 0 rather then 5
Add a DMI quirk table to check for the few known models with this issue, so that the right IPC IRQ index is used on these systems.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/common/soc-intel-quirks.h | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/sound/soc/intel/common/soc-intel-quirks.h b/sound/soc/intel/common/soc-intel-quirks.h index b07df3059926..a93987ab7f4d 100644 --- a/sound/soc/intel/common/soc-intel-quirks.h +++ b/sound/soc/intel/common/soc-intel-quirks.h @@ -11,6 +11,7 @@
#if IS_ENABLED(CONFIG_X86)
+#include <linux/dmi.h> #include <asm/cpu_device_id.h> #include <asm/intel-family.h> #include <asm/iosf_mbi.h> @@ -38,12 +39,36 @@ SOC_INTEL_IS_CPU(cml, KABYLAKE_L);
static inline bool soc_intel_is_byt_cr(struct platform_device *pdev) { + /* + * List of systems which: + * 1. Use a non CR version of the Bay Trail SoC + * 2. Contain at least 6 interrupt resources so that the + * platform_get_resource(pdev, IORESOURCE_IRQ, 5) check below + * succeeds + * 3. Despite 1. and 2. still have their IPC IRQ at index 0 rather then 5 + * + * This needs to be here so that it can be shared between the SST and + * SOF drivers. We rely on the compiler to optimize this out in files + * where soc_intel_is_byt_cr is not used. + */ + static const struct dmi_system_id force_bytcr_table[] = { + { /* Lenovo Yoga Tablet 2 series */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_FAMILY, "YOGATablet2"), + }, + }, + {} + }; struct device *dev = &pdev->dev; int status = 0;
if (!soc_intel_is_byt()) return false;
+ if (dmi_check_system(force_bytcr_table)) + return true; + if (iosf_mbi_available()) { u32 bios_status;
On 12/27/20 3:12 PM, Hans de Goede wrote:
Some Bay Trail systems:
- Use a non CR version of the Bay Trail SoC
- Contain at least 6 interrupt resources so that the platform_get_resource(pdev, IORESOURCE_IRQ, 5) check to workaround non CR systems which list their IPC IRQ at index 0 despite being non CR does not work
- Despite 1. and 2. still have their IPC IRQ at index 0 rather then 5
Add a DMI quirk table to check for the few known models with this issue, so that the right IPC IRQ index is used on these systems.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Thanks Hans!
sound/soc/intel/common/soc-intel-quirks.h | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/sound/soc/intel/common/soc-intel-quirks.h b/sound/soc/intel/common/soc-intel-quirks.h index b07df3059926..a93987ab7f4d 100644 --- a/sound/soc/intel/common/soc-intel-quirks.h +++ b/sound/soc/intel/common/soc-intel-quirks.h @@ -11,6 +11,7 @@
#if IS_ENABLED(CONFIG_X86)
+#include <linux/dmi.h> #include <asm/cpu_device_id.h> #include <asm/intel-family.h> #include <asm/iosf_mbi.h> @@ -38,12 +39,36 @@ SOC_INTEL_IS_CPU(cml, KABYLAKE_L);
static inline bool soc_intel_is_byt_cr(struct platform_device *pdev) {
/*
* List of systems which:* 1. Use a non CR version of the Bay Trail SoC* 2. Contain at least 6 interrupt resources so that the* platform_get_resource(pdev, IORESOURCE_IRQ, 5) check below* succeeds* 3. Despite 1. and 2. still have their IPC IRQ at index 0 rather then 5** This needs to be here so that it can be shared between the SST and* SOF drivers. We rely on the compiler to optimize this out in files* where soc_intel_is_byt_cr is not used.*/static const struct dmi_system_id force_bytcr_table[] = {
{ /* Lenovo Yoga Tablet 2 series */.matches = {DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),DMI_MATCH(DMI_PRODUCT_FAMILY, "YOGATablet2"),},},{}}; struct device *dev = &pdev->dev; int status = 0;
if (!soc_intel_is_byt()) return false;
if (dmi_check_system(force_bytcr_table))
return true;if (iosf_mbi_available()) { u32 bios_status;
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add a new ASoc Machine driver for Intel Baytrail platforms with a Wolfson Microelectronics WM5102 codec.
This is based on a past contributions [1] from Paulo Sergio Travaglia pstglia@gmail.com based on the Levono kernel [2] combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [3].
[1] https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e0... [2] https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.... [3] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
The original machine driver from the Android ports was a crude modified copy of bytcr_rt5640.c adjusted to work with the WM5102 codec. This version has been extensively reworked to:
1. Remove all rt5640 related quirk handling. to the best of my knowledge this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13 inch models) which all use the same setup. So there is no need to deal with all the variations with which we need to deal on rt5640 boards.
2. Rework clock handling, properly turn off the FLL and the platform-clock when they are no longer necessary and don't reconfigure the FLL unnecessarily when it is already running. This fixes a number of: "Timed out waiting for lock" warnings being logged.
3. Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY
This only adds the machine driver and ACPI hooks, the BYT-CR detection quirk which these devices need will be added in a separate patch.
Co-authored-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/Kconfig | 12 + sound/soc/intel/boards/Makefile | 2 + sound/soc/intel/boards/bytcr_wm5102.c | 472 ++++++++++++++++++ .../intel/common/soc-acpi-intel-byt-match.c | 16 + 4 files changed, 502 insertions(+) create mode 100644 sound/soc/intel/boards/bytcr_wm5102.c
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig index c10c37803c67..4088e19eb6b4 100644 --- a/sound/soc/intel/boards/Kconfig +++ b/sound/soc/intel/boards/Kconfig @@ -111,6 +111,18 @@ config SND_SOC_INTEL_BYTCR_RT5651_MACH Say Y or m if you have such a device. This is a recommended option. If unsure select "N".
+config SND_SOC_INTEL_BYTCR_WM5102_MACH + tristate "Baytrail and Baytrail-CR with WM5102 codec" + depends on SPI && ACPI + depends on X86_INTEL_LPSS || COMPILE_TEST + select SND_SOC_ACPI + select SND_SOC_WM5102 + help + This adds support for ASoC machine driver for Intel(R) Baytrail and Baytrail-CR + platforms with WM5102 audio codec. + Say Y if you have such a device. + If unsure select "N". + config SND_SOC_INTEL_CHT_BSW_RT5672_MACH tristate "Cherrytrail & Braswell with RT5672 codec" depends on I2C && ACPI diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile index a58e4d22e9c8..86661357bd22 100644 --- a/sound/soc/intel/boards/Makefile +++ b/sound/soc/intel/boards/Makefile @@ -10,6 +10,7 @@ snd-soc-sst-sof-wm8804-objs := sof_wm8804.o snd-soc-sst-glk-rt5682_max98357a-objs := glk_rt5682_max98357a.o hda_dsp_common.o snd-soc-sst-bytcr-rt5640-objs := bytcr_rt5640.o snd-soc-sst-bytcr-rt5651-objs := bytcr_rt5651.o +snd-soc-sst-bytcr-wm5102-objs := bytcr_wm5102.o snd-soc-sst-cht-bsw-rt5672-objs := cht_bsw_rt5672.o snd-soc-sst-cht-bsw-rt5645-objs := cht_bsw_rt5645.o snd-soc-sst-cht-bsw-max98090_ti-objs := cht_bsw_max98090_ti.o @@ -51,6 +52,7 @@ obj-$(CONFIG_SND_SOC_INTEL_BDW_RT5650_MACH) += snd-soc-sst-bdw-rt5650-mach.o obj-$(CONFIG_SND_SOC_INTEL_BDW_RT5677_MACH) += snd-soc-sst-bdw-rt5677-mach.o obj-$(CONFIG_SND_SOC_INTEL_BYTCR_RT5640_MACH) += snd-soc-sst-bytcr-rt5640.o obj-$(CONFIG_SND_SOC_INTEL_BYTCR_RT5651_MACH) += snd-soc-sst-bytcr-rt5651.o +obj-$(CONFIG_SND_SOC_INTEL_BYTCR_WM5102_MACH) += snd-soc-sst-bytcr-wm5102.o obj-$(CONFIG_SND_SOC_INTEL_CHT_BSW_RT5672_MACH) += snd-soc-sst-cht-bsw-rt5672.o obj-$(CONFIG_SND_SOC_INTEL_CHT_BSW_RT5645_MACH) += snd-soc-sst-cht-bsw-rt5645.o obj-$(CONFIG_SND_SOC_INTEL_CHT_BSW_MAX98090_TI_MACH) += snd-soc-sst-cht-bsw-max98090_ti.o diff --git a/sound/soc/intel/boards/bytcr_wm5102.c b/sound/soc/intel/boards/bytcr_wm5102.c new file mode 100644 index 000000000000..7de09cb5c50e --- /dev/null +++ b/sound/soc/intel/boards/bytcr_wm5102.c @@ -0,0 +1,472 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * bytcr_wm5102.c - ASoc Machine driver for Intel Baytrail platforms with a + * Wolfson Microelectronics WM5102 codec + * + * Copyright (C) 2020 Hans de Goede hdegoede@redhat.com + * Loosely based on bytcr_rt5640.c which is: + * Copyright (C) 2014-2020 Intel Corp + * Author: Subhransu S. Prusty subhransu.s.prusty@intel.com + */ + +#include <linux/acpi.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/spi/spi.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-acpi.h> +#include "../../codecs/wm5102.h" +#include "../atom/sst-atom-controls.h" + +#define MCLK_FREQ 25000000 + +#define WM5102_MAX_SYSCLK_4K 49152000 /* max sysclk for 4K family */ +#define WM5102_MAX_SYSCLK_11025 45158400 /* max sysclk for 11.025K family */ + +struct byt_wm5102_private { + struct clk *mclk; + struct gpio_desc *spkvdd_en_gpio; +}; + +static int byt_wm5102_spkvdd_power_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_card *card = w->dapm->card; + struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card); + + gpiod_set_value_cansleep(priv->spkvdd_en_gpio, + !!SND_SOC_DAPM_EVENT_ON(event)); + + return 0; +} + +static int byt_wm5102_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) +{ + struct snd_soc_component *codec_component = codec_dai->component; + int sr_mult = ((rate % 4000) == 0) ? + (WM5102_MAX_SYSCLK_4K / rate) : + (WM5102_MAX_SYSCLK_11025 / rate); + int ret; + + /* Reset FLL1 */ + snd_soc_dai_set_pll(codec_dai, WM5102_FLL1_REFCLK, ARIZONA_FLL_SRC_NONE, 0, 0); + snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0); + + /* Configure the FLL1 PLL before selecting it */ + ret = snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_CLK_SRC_MCLK1, + MCLK_FREQ, rate * sr_mult); + if (ret) { + dev_err(codec_component->dev, "Error setting PLL: %d\n", ret); + return ret; + } + + ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_SYSCLK, + ARIZONA_CLK_SRC_FLL1, rate * sr_mult, + SND_SOC_CLOCK_IN); + if (ret) { + dev_err(codec_component->dev, "Error setting ASYNCCLK: %d\n", ret); + return ret; + } + + ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK, 0, + rate * sr_mult, SND_SOC_CLOCK_OUT); + if (ret) { + dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK, + rate * 512, SND_SOC_CLOCK_IN); + if (ret) { + dev_err(codec_component->dev, "Error setting clock: %d\n", ret); + return ret; + } + + return 0; +} + +static int platform_clock_control(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct snd_soc_card *card = dapm->card; + struct snd_soc_dai *codec_dai; + struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card); + int ret; + + codec_dai = snd_soc_card_get_codec_dai(card, "wm5102-aif1"); + if (!codec_dai) { + dev_err(card->dev, "Error codec DAI not found\n"); + return -EIO; + } + + if (SND_SOC_DAPM_EVENT_ON(event)) { + ret = clk_prepare_enable(priv->mclk); + if (ret) { + dev_err(card->dev, "Error enabling MCLK: %d\n", ret); + return ret; + } + ret = byt_wm5102_prepare_and_enable_pll1(codec_dai, 48000); + if (ret) { + dev_err(card->dev, "Error setting codec sysclk: %d\n", ret); + return ret; + } + } else { + /* + * The WM5102 has a separate 32KHz clock for jack-detect + * so we can disable the PLL, followed by disabling the + * platform clock which is the source-clock for the PLL. + */ + snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0); + clk_disable_unprepare(priv->mclk); + } + + return 0; +} + +static const struct snd_soc_dapm_widget byt_wm5102_widgets[] = { + SND_SOC_DAPM_HP("Headphone", NULL), + SND_SOC_DAPM_MIC("Headset Mic", NULL), + SND_SOC_DAPM_MIC("Internal Mic", NULL), + SND_SOC_DAPM_SPK("Speaker", NULL), + SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0, + platform_clock_control, SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_SUPPLY("Speaker VDD", SND_SOC_NOPM, 0, 0, + byt_wm5102_spkvdd_power_event, + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU), +}; + +static const struct snd_soc_dapm_route byt_wm5102_audio_map[] = { + {"Headphone", NULL, "Platform Clock"}, + {"Headset Mic", NULL, "Platform Clock"}, + {"Internal Mic", NULL, "Platform Clock"}, + {"Speaker", NULL, "Platform Clock"}, + + {"Speaker", NULL, "SPKOUTLP"}, + {"Speaker", NULL, "SPKOUTLN"}, + {"Speaker", NULL, "SPKOUTRP"}, + {"Speaker", NULL, "SPKOUTRN"}, + {"Speaker", NULL, "Speaker VDD"}, + + {"Headphone", NULL, "HPOUT1L"}, + {"Headphone", NULL, "HPOUT1R"}, + + {"Internal Mic", NULL, "MICBIAS3"}, + {"IN3L", NULL, "Internal Mic"}, + + /* + * The Headset Mix uses MICBIAS1 or 2 depending on if a CTIA/OMTP Headset + * is connected, as the MICBIAS is applied after the CTIA/OMTP cross-switch. + */ + {"Headset Mic", NULL, "MICBIAS1"}, + {"Headset Mic", NULL, "MICBIAS2"}, + {"IN1L", NULL, "Headset Mic"}, + + {"AIF1 Playback", NULL, "ssp0 Tx"}, + {"ssp0 Tx", NULL, "modem_out"}, + + {"modem_in", NULL, "ssp0 Rx"}, + {"ssp0 Rx", NULL, "AIF1 Capture"}, +}; + +static const struct snd_kcontrol_new byt_wm5102_controls[] = { + SOC_DAPM_PIN_SWITCH("Headphone"), + SOC_DAPM_PIN_SWITCH("Headset Mic"), + SOC_DAPM_PIN_SWITCH("Internal Mic"), + SOC_DAPM_PIN_SWITCH("Speaker"), +}; + +static int byt_wm5102_init(struct snd_soc_pcm_runtime *runtime) +{ + struct snd_soc_card *card = runtime->card; + struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card); + int ret; + + card->dapm.idle_bias_off = true; + + ret = snd_soc_add_card_controls(card, byt_wm5102_controls, + ARRAY_SIZE(byt_wm5102_controls)); + if (ret) { + dev_err(card->dev, "Error adding card controls: %d\n", ret); + return ret; + } + + /* + * The firmware might enable the clock at boot (this information + * may or may not be reflected in the enable clock register). + * To change the rate we must disable the clock first to cover these + * cases. Due to common clock framework restrictions that do not allow + * to disable a clock that has not been enabled, we need to enable + * the clock first. + */ + ret = clk_prepare_enable(priv->mclk); + if (!ret) + clk_disable_unprepare(priv->mclk); + + ret = clk_set_rate(priv->mclk, MCLK_FREQ); + if (ret) { + dev_err(card->dev, "Error setting MCLK rate: %d\n", ret); + return ret; + } + + return 0; +} + +static const struct snd_soc_pcm_stream byt_wm5102_dai_params = { + .formats = SNDRV_PCM_FMTBIT_S16_LE, + .rate_min = 48000, + .rate_max = 48000, + .channels_min = 2, + .channels_max = 2, +}; + +static int byt_wm5102_codec_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + struct snd_interval *rate = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_RATE); + struct snd_interval *channels = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_CHANNELS); + int ret; + + /* The DSP will covert the FE rate to 48k, stereo */ + rate->min = 48000; + rate->max = 48000; + channels->min = 2; + channels->max = 2; + + /* set SSP0 to 16-bit */ + params_set_format(params, SNDRV_PCM_FORMAT_S16_LE); + + /* + * Default mode for SSP configuration is TDM 4 slot, override config + * with explicit setting to I2S 2ch 16-bit. The word length is set with + * dai_set_tdm_slot() since there is no other API exposed + */ + ret = snd_soc_dai_set_fmt(asoc_rtd_to_cpu(rtd, 0), + SND_SOC_DAIFMT_I2S | + SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBS_CFS); + if (ret) { + dev_err(rtd->dev, "Error setting format to I2S: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_tdm_slot(asoc_rtd_to_cpu(rtd, 0), 0x3, 0x3, 2, 16); + if (ret) { + dev_err(rtd->dev, "Error setting I2S config: %d\n", ret); + return ret; + } + + return 0; +} + +static int byt_wm5102_aif1_startup(struct snd_pcm_substream *substream) +{ + return snd_pcm_hw_constraint_single(substream->runtime, + SNDRV_PCM_HW_PARAM_RATE, 48000); +} + +static const struct snd_soc_ops byt_wm5102_aif1_ops = { + .startup = byt_wm5102_aif1_startup, +}; + +SND_SOC_DAILINK_DEF(dummy, + DAILINK_COMP_ARRAY(COMP_DUMMY())); + +SND_SOC_DAILINK_DEF(media, + DAILINK_COMP_ARRAY(COMP_CPU("media-cpu-dai"))); + +SND_SOC_DAILINK_DEF(deepbuffer, + DAILINK_COMP_ARRAY(COMP_CPU("deepbuffer-cpu-dai"))); + +SND_SOC_DAILINK_DEF(ssp0_port, + DAILINK_COMP_ARRAY(COMP_CPU("ssp0-port"))); + +SND_SOC_DAILINK_DEF(ssp0_codec, + DAILINK_COMP_ARRAY(COMP_CODEC( + /* + * Note there is no need to overwrite the codec-name as is done in + * other bytcr machine drivers, because the codec is a MFD child-dev. + */ + "wm5102-codec", + "wm5102-aif1"))); + +SND_SOC_DAILINK_DEF(platform, + DAILINK_COMP_ARRAY(COMP_PLATFORM("sst-mfld-platform"))); + +static struct snd_soc_dai_link byt_wm5102_dais[] = { + [MERR_DPCM_AUDIO] = { + .name = "Baytrail Audio Port", + .stream_name = "Baytrail Audio", + .nonatomic = true, + .dynamic = 1, + .dpcm_playback = 1, + .dpcm_capture = 1, + .ops = &byt_wm5102_aif1_ops, + SND_SOC_DAILINK_REG(media, dummy, platform), + + }, + [MERR_DPCM_DEEP_BUFFER] = { + .name = "Deep-Buffer Audio Port", + .stream_name = "Deep-Buffer Audio", + .nonatomic = true, + .dynamic = 1, + .dpcm_playback = 1, + .ops = &byt_wm5102_aif1_ops, + SND_SOC_DAILINK_REG(deepbuffer, dummy, platform), + }, + /* back ends */ + { + /* + * This must be named SSP2-Codec even though this machine driver + * always uses SSP0. Most machine drivers support both and dynamically + * update the dailink to point to SSP0 or SSP2, while keeping the name + * as "SSP2-Codec". The SOF tplg files hardcode the "SSP2-Codec" even + * in the byt-foo-ssp0.tplg versions because the other machine-drivers + * use "SSP2-Codec" even when SSP0 is used. + */ + .name = "SSP2-Codec", + .id = 0, + .no_pcm = 1, + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBS_CFS, + .be_hw_params_fixup = byt_wm5102_codec_fixup, + .nonatomic = true, + .dpcm_playback = 1, + .dpcm_capture = 1, + .init = byt_wm5102_init, + SND_SOC_DAILINK_REG(ssp0_port, ssp0_codec, platform), + }, +}; + +/* use space before codec name to simplify card ID, and simplify driver name */ +#define SOF_CARD_NAME "bytcht wm5102" /* card name will be 'sof-bytcht wm5102' */ +#define SOF_DRIVER_NAME "SOF" + +#define CARD_NAME "bytcr-wm5102" +#define DRIVER_NAME NULL /* card name will be used for driver name */ + +/* SoC card */ +static struct snd_soc_card byt_wm5102_card = { + .owner = THIS_MODULE, + .dai_link = byt_wm5102_dais, + .num_links = ARRAY_SIZE(byt_wm5102_dais), + .dapm_widgets = byt_wm5102_widgets, + .num_dapm_widgets = ARRAY_SIZE(byt_wm5102_widgets), + .dapm_routes = byt_wm5102_audio_map, + .num_dapm_routes = ARRAY_SIZE(byt_wm5102_audio_map), + .fully_routed = true, +}; + +static int snd_byt_wm5102_mc_probe(struct platform_device *pdev) +{ + char codec_name[SND_ACPI_I2C_ID_LEN]; + struct device *dev = &pdev->dev; + struct byt_wm5102_private *priv; + struct snd_soc_acpi_mach *mach; + const char *platform_name; + struct acpi_device *adev; + struct device *codec_dev; + bool sof_parent; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_ATOMIC); + if (!priv) + return -ENOMEM; + + /* Get MCLK */ + priv->mclk = devm_clk_get(dev, "pmc_plt_clk_3"); + if (IS_ERR(priv->mclk)) + return dev_err_probe(dev, PTR_ERR(priv->mclk), "getting pmc_plt_clk_3\n"); + + /* + * Get speaker VDD enable GPIO: + * 1. Get codec-device-name + * 2. Get codec-device + * 3. Get GPIO from codec-device + */ + mach = dev->platform_data; + adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1); + if (!adev) { + dev_err(dev, "Error cannot find acpi-dev for codec\n"); + return -ENOENT; + } + snprintf(codec_name, sizeof(codec_name), "spi-%s", acpi_dev_name(adev)); + put_device(&adev->dev); + + codec_dev = bus_find_device_by_name(&spi_bus_type, NULL, codec_name); + if (!codec_dev) + return -EPROBE_DEFER; + + /* Note no devm_ here since we call gpiod_get on codec_dev rather then dev */ + priv->spkvdd_en_gpio = gpiod_get(codec_dev, "wlf,spkvdd-ena", GPIOD_OUT_LOW); + put_device(codec_dev); + + if (IS_ERR(priv->spkvdd_en_gpio)) + return dev_err_probe(dev, PTR_ERR(priv->spkvdd_en_gpio), "getting spkvdd-GPIO\n"); + + /* override platform name, if required */ + byt_wm5102_card.dev = dev; + platform_name = mach->mach_params.platform; + ret = snd_soc_fixup_dai_links_platform_name(&byt_wm5102_card, platform_name); + if (ret) + goto out_put_gpio; + + /* set card and driver name and pm-ops */ + sof_parent = snd_soc_acpi_sof_parent(dev); + if (sof_parent) { + byt_wm5102_card.name = SOF_CARD_NAME; + byt_wm5102_card.driver_name = SOF_DRIVER_NAME; + dev->driver->pm = &snd_soc_pm_ops; + } else { + byt_wm5102_card.name = CARD_NAME; + byt_wm5102_card.driver_name = DRIVER_NAME; + } + + snd_soc_card_set_drvdata(&byt_wm5102_card, priv); + ret = devm_snd_soc_register_card(dev, &byt_wm5102_card); + if (ret) { + dev_err_probe(dev, ret, "registering card\n"); + goto out_put_gpio; + } + + platform_set_drvdata(pdev, &byt_wm5102_card); + return 0; + +out_put_gpio: + gpiod_put(priv->spkvdd_en_gpio); + return ret; +} + +static int snd_byt_wm5102_mc_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card); + + gpiod_put(priv->spkvdd_en_gpio); + return 0; +} + +static struct platform_driver snd_byt_wm5102_mc_driver = { + .driver = { + .name = "bytcr_wm5102", + }, + .probe = snd_byt_wm5102_mc_probe, + .remove = snd_byt_wm5102_mc_remove, +}; + +module_platform_driver(snd_byt_wm5102_mc_driver); + +MODULE_DESCRIPTION("ASoC Baytrail with WM5102 codec machine driver"); +MODULE_AUTHOR("Hans de Goede hdegoede@redhat.com"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:bytcr_wm5102"); diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c index c348607b49a5..ec7932549655 100644 --- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c @@ -154,6 +154,22 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_machines[] = { .sof_fw_filename = "sof-byt.ri", .sof_tplg_filename = "sof-byt-rt5651.tplg", }, + { + .id = "WM510204", + .drv_name = "bytcr_wm5102", + .fw_filename = "intel/fw_sst_0f28.bin", + .board = "bytcr_wm5102", + .sof_fw_filename = "sof-byt.ri", + .sof_tplg_filename = "sof-byt-wm5102.tplg", + }, + { + .id = "WM510205", + .drv_name = "bytcr_wm5102", + .fw_filename = "intel/fw_sst_0f28.bin", + .board = "bytcr_wm5102", + .sof_fw_filename = "sof-byt.ri", + .sof_tplg_filename = "sof-byt-wm5102.tplg", + }, { .id = "DLGS7212", .drv_name = "bytcht_da7213",
On Sun, Dec 27, 2020 at 10:12:31PM +0100, Hans de Goede wrote:
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add a new ASoc Machine driver for Intel Baytrail platforms with a Wolfson Microelectronics WM5102 codec.
This is based on a past contributions [1] from Paulo Sergio Travaglia pstglia@gmail.com based on the Levono kernel [2] combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [3].
[1] https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e0... [2] https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.... [3] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
The original machine driver from the Android ports was a crude modified copy of bytcr_rt5640.c adjusted to work with the WM5102 codec. This version has been extensively reworked to:
- Remove all rt5640 related quirk handling. to the best of my knowledge
this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13 inch models) which all use the same setup. So there is no need to deal with all the variations with which we need to deal on rt5640 boards.
- Rework clock handling, properly turn off the FLL and the platform-clock
when they are no longer necessary and don't reconfigure the FLL unnecessarily when it is already running. This fixes a number of: "Timed out waiting for lock" warnings being logged.
- Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY
This only adds the machine driver and ACPI hooks, the BYT-CR detection quirk which these devices need will be added in a separate patch.
Co-authored-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Just a couple really minor comments.
+static int byt_wm5102_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) +{
- struct snd_soc_component *codec_component = codec_dai->component;
- int sr_mult = ((rate % 4000) == 0) ?
(WM5102_MAX_SYSCLK_4K / rate) :(WM5102_MAX_SYSCLK_11025 / rate);- int ret;
- /* Reset FLL1 */
- snd_soc_dai_set_pll(codec_dai, WM5102_FLL1_REFCLK, ARIZONA_FLL_SRC_NONE, 0, 0);
- snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0);
- /* Configure the FLL1 PLL before selecting it */
- ret = snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_CLK_SRC_MCLK1,
MCLK_FREQ, rate * sr_mult);- if (ret) {
dev_err(codec_component->dev, "Error setting PLL: %d\n", ret);return ret;- }
- ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_SYSCLK,
ARIZONA_CLK_SRC_FLL1, rate * sr_mult,SND_SOC_CLOCK_IN);- if (ret) {
dev_err(codec_component->dev, "Error setting ASYNCCLK: %d\n", ret);
Error message should say SYSCLK not ASYNCCLK.
return ret;- }
- ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK, 0,
rate * sr_mult, SND_SOC_CLOCK_OUT);- if (ret) {
dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret);return ret;- }
OPCLK is a clock that can be outputted on the CODECs GPIOs. Is that being used to clock some external component? If so it should be added to the DAPM graph, if not you might as well remove this call.
- ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK,
rate * 512, SND_SOC_CLOCK_IN);- if (ret) {
dev_err(codec_component->dev, "Error setting clock: %d\n", ret);return ret;- }
The rate you set here doesn't actually matter, on wm5102 this just links the DAI to a specific clock domain and as they all default to SYSCLK you can omit this call if you want. Although no harm is caused by leaving it in.
Thanks, Charles
return ret;- }
- ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK, 0,
rate * sr_mult, SND_SOC_CLOCK_OUT);- if (ret) {
dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret);return ret;- }
OPCLK is a clock that can be outputted on the CODECs GPIOs. Is that being used to clock some external component? If so it should be added to the DAPM graph, if not you might as well remove this call.
Thanks Charles for the feedback.
I have a vague recollection that the routing was supposed to be something like:
AP-----V WM5102----> BT HFP Modem--^
with processing on WM5102.
But when I look at the initial Android code there are references to the HFP path being handled by the AP.
Probably better to remove this clock configuration if the need for it is unclear, the goal is only to enable speaker/headset for now.
Hi,
Thank you for the review.
On 12/29/20 2:58 PM, Charles Keepax wrote:
On Sun, Dec 27, 2020 at 10:12:31PM +0100, Hans de Goede wrote:
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Add a new ASoc Machine driver for Intel Baytrail platforms with a Wolfson Microelectronics WM5102 codec.
This is based on a past contributions [1] from Paulo Sergio Travaglia pstglia@gmail.com based on the Levono kernel [2] combined with insights in things like the speaker GPIO from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [3].
[1] https://patchwork.kernel.org/project/alsa-devel/patch/593313f5.3636c80a.50e0... [2] https://github.com/lenovo-yt2-dev/android_kernel_lenovo_baytrail/blob/cm-12.... [3] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
The original machine driver from the Android ports was a crude modified copy of bytcr_rt5640.c adjusted to work with the WM5102 codec. This version has been extensively reworked to:
- Remove all rt5640 related quirk handling. to the best of my knowledge
this setup is only used on the Lenovo Yoga Tablet 2 series (8, 10 and 13 inch models) which all use the same setup. So there is no need to deal with all the variations with which we need to deal on rt5640 boards.
- Rework clock handling, properly turn off the FLL and the platform-clock
when they are no longer necessary and don't reconfigure the FLL unnecessarily when it is already running. This fixes a number of: "Timed out waiting for lock" warnings being logged.
- Add the GPIO controlled Speaker-VDD regulator as a DAPM_SUPPLY
This only adds the machine driver and ACPI hooks, the BYT-CR detection quirk which these devices need will be added in a separate patch.
Co-authored-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Hans de Goede hdegoede@redhat.com
Just a couple really minor comments.
+static int byt_wm5102_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) +{
- struct snd_soc_component *codec_component = codec_dai->component;
- int sr_mult = ((rate % 4000) == 0) ?
(WM5102_MAX_SYSCLK_4K / rate) :(WM5102_MAX_SYSCLK_11025 / rate);- int ret;
- /* Reset FLL1 */
- snd_soc_dai_set_pll(codec_dai, WM5102_FLL1_REFCLK, ARIZONA_FLL_SRC_NONE, 0, 0);
- snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_FLL_SRC_NONE, 0, 0);
- /* Configure the FLL1 PLL before selecting it */
- ret = snd_soc_dai_set_pll(codec_dai, WM5102_FLL1, ARIZONA_CLK_SRC_MCLK1,
MCLK_FREQ, rate * sr_mult);- if (ret) {
dev_err(codec_component->dev, "Error setting PLL: %d\n", ret);return ret;- }
- ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_SYSCLK,
ARIZONA_CLK_SRC_FLL1, rate * sr_mult,SND_SOC_CLOCK_IN);- if (ret) {
dev_err(codec_component->dev, "Error setting ASYNCCLK: %d\n", ret);Error message should say SYSCLK not ASYNCCLK.
Fixed for v2.
return ret;- }
- ret = snd_soc_component_set_sysclk(codec_component, ARIZONA_CLK_OPCLK, 0,
rate * sr_mult, SND_SOC_CLOCK_OUT);- if (ret) {
dev_err(codec_component->dev, "Error setting OPCLK: %d\n", ret);return ret;- }
OPCLK is a clock that can be outputted on the CODECs GPIOs. Is that being used to clock some external component? If so it should be added to the DAPM graph, if not you might as well remove this call.
I copy pasted this from the work done for Android X86 to get sound to work on the Lenovo Tablet 2 series: https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
I believe when you say it is unnecessary, so I will remove it for v2 (and test without this present to make sure it is really unnecessary).
- ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK,
rate * 512, SND_SOC_CLOCK_IN);- if (ret) {
dev_err(codec_component->dev, "Error setting clock: %d\n", ret);return ret;- }
The rate you set here doesn't actually matter, on wm5102 this just links the DAI to a specific clock domain and as they all default to SYSCLK you can omit this call if you want. Although no harm is caused by leaving it in.
I'm going to leave this in as I prefer to be explicit about things like this, rather then relying on defaults.
Regards,
Hans
The Linux Arizona/WM5102 driver uses the MFD framework to create several sub-devices for the WM5102 codec and then uses a driver per function.
The jack-detect support for the WM5102 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.
The extcon-arizona driver can also report jack-detect state through the standard ASoC jack interface. For this we need to create a snd_soc_jack and pass this to the extcon-arizona driver through the shared arizona data struct.
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 --- sound/soc/intel/boards/bytcr_wm5102.c | 36 ++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/boards/bytcr_wm5102.c b/sound/soc/intel/boards/bytcr_wm5102.c index 7de09cb5c50e..25c7a6cacaad 100644 --- a/sound/soc/intel/boards/bytcr_wm5102.c +++ b/sound/soc/intel/boards/bytcr_wm5102.c @@ -13,11 +13,13 @@ #include <linux/clk.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/input.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/spi/spi.h> +#include <sound/jack.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -31,6 +33,8 @@ #define WM5102_MAX_SYSCLK_11025 45158400 /* max sysclk for 11.025K family */
struct byt_wm5102_private { + struct snd_soc_jack jack; + struct arizona *arizona; struct clk *mclk; struct gpio_desc *spkvdd_en_gpio; }; @@ -184,11 +188,22 @@ static const struct snd_kcontrol_new byt_wm5102_controls[] = { SOC_DAPM_PIN_SWITCH("Speaker"), };
+static struct snd_soc_jack_pin byt_wm5102_pins[] = { + { + .pin = "Headphone", + .mask = SND_JACK_HEADPHONE, + }, + { + .pin = "Headset Mic", + .mask = SND_JACK_MICROPHONE, + }, +}; + static int byt_wm5102_init(struct snd_soc_pcm_runtime *runtime) { struct snd_soc_card *card = runtime->card; struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card); - int ret; + int ret, jack_type;
card->dapm.idle_bias_off = true;
@@ -217,6 +232,22 @@ static int byt_wm5102_init(struct snd_soc_pcm_runtime *runtime) return ret; }
+ jack_type = SND_JACK_HEADSET | SND_JACK_BTN_0 | SND_JACK_BTN_1 | + SND_JACK_BTN_2 | SND_JACK_BTN_3; + ret = snd_soc_card_jack_new(card, "Headset", jack_type, + &priv->jack, byt_wm5102_pins, + ARRAY_SIZE(byt_wm5102_pins)); + if (ret) { + dev_err(card->dev, "Error creating jack: %d\n", ret); + return ret; + } + snd_jack_set_key(priv->jack.jack, SND_JACK_BTN_0, KEY_PLAYPAUSE); + snd_jack_set_key(priv->jack.jack, SND_JACK_BTN_1, KEY_VOICECOMMAND); + snd_jack_set_key(priv->jack.jack, SND_JACK_BTN_2, KEY_VOLUMEUP); + snd_jack_set_key(priv->jack.jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN); + + priv->arizona->jack = &priv->jack; + return 0; }
@@ -409,6 +440,8 @@ static int snd_byt_wm5102_mc_probe(struct platform_device *pdev)
/* Note no devm_ here since we call gpiod_get on codec_dev rather then dev */ priv->spkvdd_en_gpio = gpiod_get(codec_dev, "wlf,spkvdd-ena", GPIOD_OUT_LOW); + + priv->arizona = dev_get_drvdata(codec_dev); put_device(codec_dev);
if (IS_ERR(priv->spkvdd_en_gpio)) @@ -452,6 +485,7 @@ static int snd_byt_wm5102_mc_remove(struct platform_device *pdev) struct snd_soc_card *card = platform_get_drvdata(pdev); struct byt_wm5102_private *priv = snd_soc_card_get_drvdata(card);
+ priv->arizona->jack = NULL; gpiod_put(priv->spkvdd_en_gpio); return 0; }
On Sun, Dec 27, 2020 at 11:15 PM Hans de Goede hdegoede@redhat.com wrote:
Hi All,
This patch series adds support for Intel Bay Trail based device which use a WM5102 codec for audio output/input. This was developed and tested on a Lenovo Yoga Tablet 1051L.
This series consists of 3 parts:
- Arizona MFD drv patches for ACPI bindings, better jack-detect support and misc. fixes
- extcon-arizona driver fixes and improved jack reporting (this depends on the MFD changes)
- ASoC patches in the form of a quirk for BYTCR detecting, a new machine driver for BYT + WM5102 and jack-detect support for the new machine driver (which again depends on the MFD changes).
Given that 2. and 3. depend on the MFD changes I believe that it is best if all patches in this series are merged through the MFD tree (once reviewed and acked) and then Lee can provide a immutable branch for the ASoC and extcon maintainers to merge into their trees.
I have a patch with matching UCM profile changes available here: https://github.com/jwrdegoede/alsa-ucm-conf/commit/316109e7814926ba984322c1d...
This series + the UCM profile has been tested with both the SST and SOF ASoC drivers for BYT devices.
Thanks for fixing this! I found the series pretty much in a good shape (only two patches I think need a bit of work), FWIW Reviewed-by: Andy Shevchenko andy.shevchenko@gmail.com after addressing comments.
Shouldn't this be somewhere in the "main" fix? (Yes, I understand that it may sound silly and should be copied to almost half of the series, but if there is a good place it would be nice to have in the Git history)
Hi,
On 12/28/20 3:19 PM, Andy Shevchenko wrote:
On Sun, Dec 27, 2020 at 11:15 PM Hans de Goede hdegoede@redhat.com wrote:
Hi All,
This patch series adds support for Intel Bay Trail based device which use a WM5102 codec for audio output/input. This was developed and tested on a Lenovo Yoga Tablet 1051L.
This series consists of 3 parts:
- Arizona MFD drv patches for ACPI bindings, better jack-detect support and misc. fixes
- extcon-arizona driver fixes and improved jack reporting (this depends on the MFD changes)
- ASoC patches in the form of a quirk for BYTCR detecting, a new machine driver for BYT + WM5102 and jack-detect support for the new machine driver (which again depends on the MFD changes).
Given that 2. and 3. depend on the MFD changes I believe that it is best if all patches in this series are merged through the MFD tree (once reviewed and acked) and then Lee can provide a immutable branch for the ASoC and extcon maintainers to merge into their trees.
I have a patch with matching UCM profile changes available here: https://github.com/jwrdegoede/alsa-ucm-conf/commit/316109e7814926ba984322c1d...
This series + the UCM profile has been tested with both the SST and SOF ASoC drivers for BYT devices.
Thanks for fixing this! I found the series pretty much in a good shape (only two patches I think need a bit of work), FWIW Reviewed-by: Andy Shevchenko andy.shevchenko@gmail.com after addressing comments.
Thank you, because of the jack-handling discussion I've extensively reworked this series (and I'm going to split it into 2 series for v2). I've kept your Reviewed-by: for the patches which are >= 99% unchanged from v1. I've not added it to a whole bunch of new patches (and some other patches were dropped).
Shouldn't this be somewhere in the "main" fix? (Yes, I understand that it may sound silly and should be copied to almost half of the series, but if there is a good place it would be nice to have in the Git history)
Ack. I've added this to the: "ASoC: Intel: bytcr_wm5102: Add machine driver for BYT/WM5102" patches' commit msg now.
Regards,
Hans
participants (7)
-
Andy Shevchenko -
Charles Keepax -
Hans de Goede -
kernel test robot -
Mark Brown -
Pierre-Louis Bossart -
Richard Fitzgerald