[alsa-devel] System with multiple arizona (wm5102) codecs
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards, Pavel
On Mon, Sep 14, 2015 at 01:54:39PM +0200, Pavel Machek wrote:
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards, Pavel
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
Thanks, Charles
On Mon, Sep 14, 2015 at 12:52:55PM +0100, Charles Keepax wrote:
On Mon, Sep 14, 2015 at 01:54:39PM +0200, Pavel Machek wrote:
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards, Pavel
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
Oh and one more all the calls to mfd_add_devices in arizona-core.c use PLATFORM_DEVID_NONE at the moment, which I suspect will also cause problems at some point.
Thanks, Charles
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards,
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Thanks a lot for pointers. I did split wm5102_dapm_widgets into two structures, and modified them by hand (but probably did not get all the right fields)... and thought that there must be better way.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
I guess I'll have to undo my horrible hacks, first.
Best regards, Pavel
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards, Pavel
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
It seems that davinci-evm takes data from device tree, but then uses statically-allocated evm_soc_card, which would lead to problems in dual-codec config....?
Thanks, Pavel
Signed-off-by: Pavel Machek pavel@ucw.cz
commit 977baecbcdb362bdc92096e7c454c379af319f8a Author: Pavel pavel@ucw.cz Date: Tue Sep 15 08:16:02 2015 +0200
Split card allocation in davinci-evm.c
diff --git a/sound/soc/davinci/davinci-evm.c b/sound/soc/davinci/davinci-evm.c index 3296116..de277ca 100644 --- a/sound/soc/davinci/davinci-evm.c +++ b/sound/soc/davinci/davinci-evm.c @@ -885,8 +899,12 @@ static int davinci_evm_probe(struct platform_device *pdev) struct snd_soc_card_drvdata_davinci *drvdata = NULL; struct clk *mclk; int ret = 0; + struct snd_soc_card *card = devm_kzalloc(&pdev->dev, sizeof(struct snd_soc_card), GFP_KERNEL); + + *card = evm_soc_card;
- evm_soc_card.dai_link = dai; + printk("bluebox / davinci_evm_probe: probing!\n"); + card->dai_link = dai;
dai->codec_of_node = of_parse_phandle(np, "ti,audio-codec", 0); if (!dai->codec_of_node) @@ -900,8 +918,9 @@ static int davinci_evm_probe(struct platform_device *pdev) if (!dai->platform_name) dai->platform_of_node = dai->cpu_of_node;
- evm_soc_card.dev = &pdev->dev; - ret = snd_soc_of_parse_card_name(&evm_soc_card, "ti,model"); + card->codec_conf = rx51_codec_conf_2; + card->dev = &pdev->dev; + ret = snd_soc_of_parse_card_name(card, "ti,model"); if (ret) return ret;
@@ -938,8 +957,8 @@ static int davinci_evm_probe(struct platform_device *pdev) requestd_rate, drvdata->sysclk); }
- snd_soc_card_set_drvdata(&evm_soc_card, drvdata); - ret = snd_soc_register_card(&evm_soc_card); + snd_soc_card_set_drvdata(card, drvdata); + ret = snd_soc_register_card(card);
if (ret) dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
On Tue, Sep 15, 2015 at 08:18:32AM +0200, Pavel Machek wrote:
Hi!
I've got an embedded system with two arizona / wm5102 codecs.
Unfortunately, kernel does not seem to be ready for that configuration.
In particular, drivers/regulator/arizona-ldo1.c and drivers/regulator/arizona-micsupp.c register system-wide "MICVDD" and "LDO1" regulators, but with two codecs in the system, we really have wm5102-codec.1.MICVDD, wm5102-codec.2.MICVDD, wm5102-codec.1.LDO1 and wm5102-codec.2.LDO1.
That got me second codec working in two-codec configuration, but first one still stops working as soon as two codecs are enabled.
If you have idea what else needs fixing, let me know.
Best regards, Pavel
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
It seems that davinci-evm takes data from device tree, but then uses statically-allocated evm_soc_card, which would lead to problems in dual-codec config....?
That somewhat depends on how you plan on doing things. I had assumed you would be having a single machine driver with both CODECs connected to it, in which case the statically allocated snd_soc_card wouldn't be a problem. However, if you wanted to have two seperate machine drivers with a single CODEC connected to each then you would have an issue.
I guess either approach is reasonable and probably just depends on what your end goal is.
Thanks, Charles
Hi!
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
It seems that davinci-evm takes data from device tree, but then uses statically-allocated evm_soc_card, which would lead to problems in dual-codec config....?
That somewhat depends on how you plan on doing things. I had assumed you would be having a single machine driver with both CODECs connected to it, in which case the statically allocated snd_soc_card wouldn't be a problem. However, if you wanted to have two seperate machine drivers with a single CODEC connected to each then you would have an issue.
I guess either approach is reasonable and probably just depends on what your end goal is.
The way dts is set up in my case, I ended up with two snd_soc_cards. It seems to work for me now (on old kernel and with some rather extreme hacks).
I'll most likely clean it up and get into mainline-ready form, but it will take some time. If you want to see the ugly patches, let me know.
Thanks and best regards, Pavel
Hi Pavel, I'd love to see the patches :-) I've been trying to figure out the *right* way to add multiple codecs to a single card (and single CPU DAI) for some days now. Any help would be greatly appreciated.
Thanks, -Caleb
On Tue, Sep 15, 2015 at 1:35 AM, Pavel Machek pavel@ucw.cz wrote:
Hi!
I must confess I haven't ever tested a system with two Arizona CODECs connected. Yes it seems you would get clashes on the regulator names, I guess that would need to be fixed up. If you were doing so wm831x-ldo.c would probably make a reasonable example.
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
Those are the only two things that spring to mind at the moment but keep me informed on how you are getting on and I will let you know if I can come up with any other traps.
It seems that davinci-evm takes data from device tree, but then uses statically-allocated evm_soc_card, which would lead to problems in dual-codec config....?
That somewhat depends on how you plan on doing things. I had assumed you would be having a single machine driver with both CODECs connected to it, in which case the statically allocated snd_soc_card wouldn't be a problem. However, if you wanted to have two seperate machine drivers with a single CODEC connected to each then you would have an issue.
I guess either approach is reasonable and probably just depends on what your end goal is.
The way dts is set up in my case, I ended up with two snd_soc_cards. It seems to work for me now (on old kernel and with some rather extreme hacks).
I'll most likely clean it up and get into mainline-ready form, but it will take some time. If you want to see the ugly patches, let me know.
Thanks and best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Sep 15, 2015 at 06:56:39AM -0700, Caleb Crome wrote:
I'd love to see the patches :-) I've been trying to figure out the *right* way to add multiple codecs to a single card (and single CPU DAI) for some days now. Any help would be greatly appreciated.
Like Charles said earlier the Bells machine in mainline has multiple CODECs hooked up. Speyside too. To hook up multiple CODECs to a single DAI link see 88bd870f02dff5c94 (ASoC: core: Add initial support for DAI multicodec), sadly I don't think Benoit ever got round to submitting a machine.
Like Charles said earlier the Bells machine in mainline has multiple CODECs hooked up. Speyside too. To hook up multiple CODECs to a single DAI link see 88bd870f02dff5c94 (ASoC: core: Add initial support for DAI multicodec), sadly I don't think Benoit ever got round to submitting a machine.
Thanks Mark.
I've been staring at that diff for a a day or two, and I still can't quite figure out how to use it.
I think I'm getting close: all codecs are registered, the DAPM stuff seems to be connected (all with prefixed names), but the card won't open more than a 2 channel interface.
For example, when I do aplay -l, I get this: **** List of PLAYBACK Hardware Devices **** card 0: PUPPYAUDIO [PUPPY-AUDIO], device 0: AIC3X tlv320aic3x-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 1: AIC3X tlv320aic3x-hifi-1 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 2: AIC3X tlv320aic3x-hifi-2 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 3: AIC3X tlv320aic3x-hifi-3 [] Subdevices: 1/1 Subdevice #0: subdevice #0
Each device is a 2 channel codec, so I thought I should get 8 channels. but when I try to run jackd with 8 channels, I get the following: # jackd -d alsa -D -i 8 -o 8 -S -r16000 ... ALSA: cannot set channel count to 8 for capture ALSA: cannot configure capture channel ...
So, here are the relevent bits of my patch. Any chance you could point out the error in my ways?
Basically, what I did was add a snd_soc_dai_link and a snd_soc_codec_conf for each codec, and set num_links and num_configs to the number of codecs.
Thanks
-Caleb
diff --git a/sound/soc/davinci/davinci-evm.c b/sound/soc/davinci/davinci-evm.c index 731fb0d..d2e7049 100644 --- a/sound/soc/davinci/davinci-evm.c +++ b/sound/soc/davinci/davinci-evm.c @@ -23,10 +23,11 @@
#include <asm/dma.h> #include <asm/mach-types.h> - struct snd_soc_card_drvdata_davinci { struct clk *mclk; unsigned sysclk; + int controls_added_already; };
@@ -118,11 +122,18 @@ static int evm_aic3x_init(struct snd_soc_pcm_runtime *rtd) { struct snd_soc_card *card = rtd->card; struct device_node *np = card->dev->of_node; + + struct snd_soc_card_drvdata_davinci *drvdata = + snd_soc_card_get_drvdata(card); int ret;
/* Add davinci-evm specific widgets */ - snd_soc_dapm_new_controls(&card->dapm, aic3x_dapm_widgets, - ARRAY_SIZE(aic3x_dapm_widgets)); + if (!drvdata->controls_added_already) { + snd_soc_dapm_new_controls(&card->dapm, aic3x_dapm_widgets, + ARRAY_SIZE(aic3x_dapm_widgets)); + drvdata->controls_added_already = 1; + }
if (np) { ret = snd_soc_of_parse_audio_routing(card, "ti,audio-routing"); @@ -330,14 +342,71 @@ static struct snd_soc_card da850_snd_soc_card = { * The struct is used as place holder. It will be completely * filled with data from dt node. */ -static struct snd_soc_dai_link evm_dai_tlv320aic3x = { - .name = "TLV320AIC3X", +static struct snd_soc_dai_link evm_dai_tlv320aic3x[] = { + { + .name = "TLV320AIC3X a", .stream_name = "AIC3X", .codec_dai_name = "tlv320aic3x-hifi", .ops = &evm_ops, .init = evm_aic3x_init, - .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | - SND_SOC_DAIFMT_IB_NF, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X b", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X c", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X d", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X e", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X f", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X g", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, + { + .name = "TLV320AIC3X h", + .stream_name = "AIC3X", + .codec_dai_name = "tlv320aic3x-hifi", + .ops = &evm_ops, + .init = evm_aic3x_init, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_IB_NF, + }, };
static const struct of_device_id davinci_evm_dt_ids[] = { @@ -355,6 +424,8 @@ static struct snd_soc_card evm_soc_card = { .num_links = 1, };
+static struct snd_soc_codec_conf evm_codec_confs[16]; + static int davinci_evm_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -364,18 +435,36 @@ static int davinci_evm_probe(struct platform_device *pdev) struct snd_soc_card_drvdata_davinci *drvdata = NULL; struct clk *mclk; int ret = 0; + int i;
evm_soc_card.dai_link = dai; - - dai->codec_of_node = of_parse_phandle(np, "ti,audio-codec", 0); - if (!dai->codec_of_node) + + evm_soc_card.codec_conf = evm_codec_confs; + + for (i = 0; + (of_parse_phandle(np, "ti,audio-codec", i) != NULL) && + (i < ARRAY_SIZE(evm_dai_tlv320aic3x)-1); + i++) { + char *name_prefix = kzalloc(4, GFP_KERNEL); + + dai[i].codec_of_node = of_parse_phandle(np, "ti,audio-codec", i); + + if (!dai[i].codec_of_node) return -EINVAL;
- dai->cpu_of_node = of_parse_phandle(np, "ti,mcasp-controller", 0); - if (!dai->cpu_of_node) + evm_codec_confs[i].of_node = dai[i].codec_of_node; + snprintf(name_prefix, 4, "%c", 'a'+i); + evm_codec_confs[i].name_prefix = name_prefix; + + dai[i].cpu_of_node = of_parse_phandle(np, "ti,mcasp-controller", 0); + if (!dai[i].cpu_of_node) return -EINVAL;
- dai->platform_of_node = dai->cpu_of_node; + dai[i].platform_of_node = dai[i].cpu_of_node; + } + evm_soc_card.num_configs=i; + evm_soc_card.num_links =i; +
evm_soc_card.dev = &pdev->dev; ret = snd_soc_of_parse_card_name(&evm_soc_card, "ti,model"); diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts index 6335072..19af41f 100644 --- a/arch/arm/boot/dts/am335x-boneblack.dts +++ b/arch/arm/boot/dts/am335x-boneblack.dts +&i2c1 { + clock-frequency = <100000>; + status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&i2c1_pins_default>; + status="okay"; + + tlv320aic3x_a: tlv320aic3x@18 { + compatible = "ti,tlv320aic3x"; + reg = <0x18>; + tdm-offset = <0>; + status = "okay"; + }; + + tlv320aic3x_b: tlv320aic3x@19 { + compatible = "ti,tlv320aic3x"; + reg = <0x19>; + tdm-offset = <32>; + status = "okay"; + }; + + tlv320aic3x_c: tlv320aic3x@1a { + compatible = "ti,tlv320aic3x"; + reg = <0x1a>; + tdm-offset = <64>; + status = "okay"; + }; + + tlv320aic3x_d: tlv320aic3x@1b { + compatible = "ti,tlv320aic3x"; + reg = <0x1b>; + tdm-offset = <96>; + status = "okay"; + }; + +}; + +&mcasp0 { + pinctrl-names = "default"; + pinctrl-0 = <&mcasp_0_pins_default>; + status = "okay"; + + op-mode = <0>; /* MCASP_IIS_MODE */ + tdm-slots = <16>; + num-serializer = <16>; + serial-dir = < /* 0: INACTIVE, 1: TX, 2: RX */ + 0 0 1 2 + 0 0 0 0 + 0 0 0 0 + 0 0 0 0 + >; + tx-num-evt = <1>; + rx-num-evt = <1>; };
+ / { + sound { + compatible = "ti,da830-evm-audio"; + ti,model = "PUPPY-AUDIO"; + ti,audio-codec = < + &tlv320aic3x_a + &tlv320aic3x_b + &tlv320aic3x_c + &tlv320aic3x_d + >; + ti,mcasp-controller = <&mcasp0>; + ti,codec-clock-rate = <12288000>; + ti,audio-routing = + "Headphone Jack", "a HPLOUT", + "Headphone Jack", "a HPROUT", + "a LINE1L", "Line In", + "a LINE1R", "Line In"; + status="okay"; + }; };
&rtc {
On Tue, Sep 15, 2015 at 08:26:36AM -0700, Caleb Crome wrote:
Like Charles said earlier the Bells machine in mainline has multiple CODECs hooked up. Speyside too. To hook up multiple CODECs to a single DAI link see 88bd870f02dff5c94 (ASoC: core: Add initial support for DAI multicodec), sadly I don't think Benoit ever got round to submitting a machine.
Thanks Mark.
I've been staring at that diff for a a day or two, and I still can't quite figure out how to use it.
I think I'm getting close: all codecs are registered, the DAPM stuff seems to be connected (all with prefixed names), but the card won't open more than a 2 channel interface.
For example, when I do aplay -l, I get this: **** List of PLAYBACK Hardware Devices **** card 0: PUPPYAUDIO [PUPPY-AUDIO], device 0: AIC3X tlv320aic3x-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 1: AIC3X tlv320aic3x-hifi-1 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 2: AIC3X tlv320aic3x-hifi-2 [] Subdevices: 1/1 Subdevice #0: subdevice #0 card 0: PUPPYAUDIO [PUPPY-AUDIO], device 3: AIC3X tlv320aic3x-hifi-3 [] Subdevices: 1/1 Subdevice #0: subdevice #0
That doesn't look entirely like what I'd expect... I'd epect to see one DAI presented to userspace. Indeed looking through your diff I don't see any usage of struct snd_soc_dai_link_component as described in the changelog for the change I pointed you at. I'd expect to see one DAI link with a bunch of those hanging off it giving a single DAI to aplay.
OTOH I'm not sure it's going to work as I'm not immediately seeing how we handle the ability to have more capabilities than an individual device (based on the changelog I suspect the original use case may have been two mono I2S devices which have stereo interfaces even if they only pay attention to one channel on themm). But let's at least get everything appearing as one DAI first before we move on to worrying about that, I didn't check thoroughly yet.
Hi!
I'd love to see the patches :-) I've been trying to figure out the *right* way to add multiple codecs to a single card (and single CPU DAI) for some days now. Any help would be greatly appreciated.
I eventually got it to work, but I'm really sure that what I've done can't be considered "right" :-). Pavel
Hi!
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
wm9081 is indeed useful example.
Does this look like a step in right direction?
Thanks, Pavel
diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c index 81d8681..2be9513 100644 --- a/drivers/regulator/arizona-ldo1.c +++ b/drivers/regulator/arizona-ldo1.c @@ -27,13 +27,17 @@ #include <linux/mfd/arizona/registers.h>
struct arizona_ldo1 { + char name[99]; struct regulator_dev *regulator; struct arizona *arizona; + struct regulator_desc desc;
struct regulator_consumer_supply supply; struct regulator_init_data init_data; };
+ + static int arizona_ldo1_hc_list_voltage(struct regulator_dev *rdev, unsigned int selector) { @@ -121,7 +125,6 @@ static struct regulator_ops arizona_ldo1_hc_ops = { };
static const struct regulator_desc arizona_ldo1_hc = { - .name = "LDO1", .supply_name = "LDOVDD", .type = REGULATOR_VOLTAGE, .ops = &arizona_ldo1_hc_ops, @@ -146,7 +149,6 @@ static struct regulator_ops arizona_ldo1_ops = { };
static const struct regulator_desc arizona_ldo1 = { - .name = "LDO1", .supply_name = "LDOVDD", .type = REGULATOR_VOLTAGE, .ops = &arizona_ldo1_ops, @@ -183,8 +185,8 @@ static const struct regulator_init_data arizona_ldo1_default = { static int arizona_ldo1_probe(struct platform_device *pdev) { struct arizona *arizona = dev_get_drvdata(pdev->dev.parent); - const struct regulator_desc *desc; struct regulator_config config = { }; + int id = 0; struct arizona_ldo1 *ldo1; int ret;
@@ -194,8 +196,10 @@ static int arizona_ldo1_probe(struct platform_device *pdev) return -ENOMEM; }
+ + + printk("Initializing arizona-ldo1 for codec %d\n", id); ldo1->arizona = arizona; - /* * Since the chip usually supplies itself we provide some * default init_data for it. This will be overridden with @@ -203,15 +207,18 @@ static int arizona_ldo1_probe(struct platform_device *pdev) */ switch (arizona->type) { case WM5102: - desc = &arizona_ldo1_hc; ldo1->init_data = arizona_ldo1_dvfs; + ldo1->desc = arizona_ldo1_hc; break; default: - desc = &arizona_ldo1; ldo1->init_data = arizona_ldo1_default; + ldo1->desc = arizona_ldo1; break; }
+ ldo1->desc.name = ldo1->name; + sprintf(ldo1->name, "LDO1_%d", id); + ldo1->init_data.consumer_supplies = &ldo1->supply; ldo1->supply.supply = "DCVDD"; ldo1->supply.dev_name = dev_name(arizona->dev); @@ -226,7 +233,7 @@ static int arizona_ldo1_probe(struct platform_device *pdev) else config.init_data = &ldo1->init_data;
- ldo1->regulator = regulator_register(desc, &config); + ldo1->regulator = regulator_register(&ldo1->desc, &config); if (IS_ERR(ldo1->regulator)) { ret = PTR_ERR(ldo1->regulator); dev_err(arizona->dev, "Failed to register LDO1 supply: %d\n",
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote:
Hi!
I guess you would need to be careful with the machine driver as well, you will need to use a snd_soc_codec_conf structure for at least one (although I would do both) of the CODECs to give a prefix for all the widget/control names, otherwise those will clash and everything will probably behave very strangely. See sound/soc/samsung/bells.c for an example doing this for wm9081.
wm9081 is indeed useful example.
Does this look like a step in right direction?
Yeah looks reasonable a few comments added.
Thanks, Pavel
diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c index 81d8681..2be9513 100644 --- a/drivers/regulator/arizona-ldo1.c +++ b/drivers/regulator/arizona-ldo1.c @@ -27,13 +27,17 @@ #include <linux/mfd/arizona/registers.h>
struct arizona_ldo1 {
- char name[99];
Can probably use a much smaller buffer here only really need a couple of characters room on it.
struct regulator_dev *regulator; struct arizona *arizona;
struct regulator_desc desc;
struct regulator_consumer_supply supply; struct regulator_init_data init_data;
};
static int arizona_ldo1_hc_list_voltage(struct regulator_dev *rdev, unsigned int selector) { @@ -121,7 +125,6 @@ static struct regulator_ops arizona_ldo1_hc_ops = { };
static const struct regulator_desc arizona_ldo1_hc = {
- .name = "LDO1", .supply_name = "LDOVDD", .type = REGULATOR_VOLTAGE, .ops = &arizona_ldo1_hc_ops,
@@ -146,7 +149,6 @@ static struct regulator_ops arizona_ldo1_ops = { };
static const struct regulator_desc arizona_ldo1 = {
- .name = "LDO1", .supply_name = "LDOVDD", .type = REGULATOR_VOLTAGE, .ops = &arizona_ldo1_ops,
@@ -183,8 +185,8 @@ static const struct regulator_init_data arizona_ldo1_default = { static int arizona_ldo1_probe(struct platform_device *pdev) { struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
- const struct regulator_desc *desc; struct regulator_config config = { };
- int id = 0;
Should the id not be coming from the pdev?
struct arizona_ldo1 *ldo1; int ret;
@@ -194,8 +196,10 @@ static int arizona_ldo1_probe(struct platform_device *pdev) return -ENOMEM; }
- printk("Initializing arizona-ldo1 for codec %d\n", id); ldo1->arizona = arizona;
- /*
- Since the chip usually supplies itself we provide some
- default init_data for it. This will be overridden with
@@ -203,15 +207,18 @@ static int arizona_ldo1_probe(struct platform_device *pdev) */ switch (arizona->type) { case WM5102:
desc = &arizona_ldo1_hc;
ldo1->desc = arizona_ldo1_hc;
desc = &arizona_ldo1;
ldo1->desc = arizona_ldo1;break; }
ldo1->desc.name = ldo1->name;
sprintf(ldo1->name, "LDO1_%d", id);
Would be nice to use an snprintf here.
Thanks, Charles
Hi!
If I undestand things correctly... when there's more than one wm5102, we need to dynamically allocate struct snd_soc_card. So something like this?
(Patch from v4.2)
Signed-off-by: Pavel Machek pavel@ucw.cz
diff --git a/sound/soc/davinci/davinci-evm.c b/sound/soc/davinci/davinci-evm.c index 731fb0d..718631a 100644 --- a/sound/soc/davinci/davinci-evm.c +++ b/sound/soc/davinci/davinci-evm.c @@ -364,8 +516,11 @@ static int davinci_evm_probe(struct platform_device *pdev) struct snd_soc_card_drvdata_davinci *drvdata = NULL; struct clk *mclk; int ret = 0; + struct snd_soc_card *card = devm_kzalloc(&pdev->dev, sizeof(struct snd_soc_card), GFP_KERNEL); + + *card = evm_soc_card;
- evm_soc_card.dai_link = dai; + card->dai_link = dai;
dai->codec_of_node = of_parse_phandle(np, "ti,audio-codec", 0); if (!dai->codec_of_node) @@ -377,8 +533,8 @@ static int davinci_evm_probe(struct platform_device *pdev)
dai->platform_of_node = dai->cpu_of_node;
- evm_soc_card.dev = &pdev->dev; - ret = snd_soc_of_parse_card_name(&evm_soc_card, "ti,model"); + card->dev = &pdev->dev; + ret = snd_soc_of_parse_card_name(card, "ti,model"); if (ret) return ret;
@@ -415,8 +588,8 @@ static int davinci_evm_probe(struct platform_device *pdev) requestd_rate, drvdata->sysclk); }
- snd_soc_card_set_drvdata(&evm_soc_card, drvdata); - ret = devm_snd_soc_register_card(&pdev->dev, &evm_soc_card); + snd_soc_card_set_drvdata(card, drvdata); + ret = devm_snd_soc_register_card(&pdev->dev, card);
if (ret) dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
Hi!
Add support for than one arizona. We introduce codec_num and fill it based on device tree. If there's better way, let me know. (For v4.2, but I believe nothing changed there. And yes, I'd need to document the dt binding. Will do if the rest of patch is ok.)
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c index a72ddb2..4f9dbd8 100644 --- a/drivers/mfd/arizona-core.c +++ b/drivers/mfd/arizona-core.c @@ -755,7 +755,15 @@ static int arizona_of_get_core_pdata(struct arizona *arizona) int ret, i; int count = 0;
+ ret = of_property_read_u32(arizona->dev->of_node, + "wlf,codec-number", &arizona->pdata.codec_num); + if (ret < 0) { + dev_info(arizona->dev, "No codec-number property in DT => single-codec mode\n"); + arizona->pdata.codec_num = -1; + } else { + dev_info(arizona->dev, "codec-number from DT: %d\n", arizona->pdata.codec_num); + }
ret = of_property_read_u32_array(arizona->dev->of_node, "wlf,gpio-defaults", arizona->pdata.gpio_defaults, @@ -925,7 +952,7 @@ int arizona_dev_init(struct arizona *arizona) /* Mark DCVDD as external, LDO1 driver will clear if internal */ arizona->external_dcvdd = true;
- ret = mfd_add_devices(arizona->dev, -1, early_devs, + ret = mfd_add_devices(arizona->dev, arizona->pdata.codec_num, early_devs, ARRAY_SIZE(early_devs), NULL, 0, NULL); if (ret != 0) { dev_err(dev, "Failed to add early children: %d\n", ret); @@ -1259,9 +1286,12 @@ int arizona_dev_init(struct arizona *arizona) arizona_request_irq(arizona, ARIZONA_IRQ_UNDERCLOCKED, "Underclocked", arizona_underclocked, arizona);
+ // FIXME: multiple codec handling only added for WM5102 switch (arizona->type) { case WM5102: - ret = mfd_add_devices(arizona->dev, -1, wm5102_devs, + printk("arizona: wm5102, working with device %d\n", arizona->pdata.codec_num); + + ret = mfd_add_devices(arizona->dev, arizona->pdata.codec_num, wm5102_devs, ARRAY_SIZE(wm5102_devs), NULL, 0, NULL); break; case WM5110: diff --git a/include/linux/mfd/arizona/pdata.h b/include/linux/mfd/arizona/pdata.h index 43db4fa..b36a750 100644 --- a/include/linux/mfd/arizona/pdata.h +++ b/include/linux/mfd/arizona/pdata.h @@ -75,6 +75,8 @@ struct arizona_micd_range { };
struct arizona_pdata { + int codec_num; // -1: n/a (=> single codec only) ; 1 or 2: codec #1 or #2 (=> two codecs) + int reset; /** GPIO controlling /RESET, if any */ int ldoena; /** GPIO controlling LODENA, if any */
On Mon, Nov 30, 2015 at 12:37:17PM +0100, Pavel Machek wrote:
Hi!
Please submit patches in the format covered in SubmittingPatches.
- ret = of_property_read_u32(arizona->dev->of_node,
"wlf,codec-number", &arizona->pdata.codec_num);
Why does this make sense - what information is this adding?
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote:
Does this look like a step in right direction?
static const struct regulator_desc arizona_ldo1_hc = {
- .name = "LDO1",
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by device provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
Hi!
On Mon 2015-10-12 16:47:15, Mark Brown wrote:
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote:
Does this look like a step in right direction?
static const struct regulator_desc arizona_ldo1_hc = {
- .name = "LDO1",
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by
device
They already are, see wm831x-ldo.c .
provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
I'll need some more help here. I need to use it from ALSA, so I don't think I can influence that interface easily.
What is currently in tree _does not work_, as there are two arizona chips, and two "LDO1" regulators. (Doable) suggestions how to fix that are welcome. Pavel
On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote:
On Mon 2015-10-12 16:47:15, Mark Brown wrote:
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote:
static const struct regulator_desc arizona_ldo1_hc = {
- .name = "LDO1",
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by
device
They already are, see wm831x-ldo.c .
No, that's a different case where we actually have a repeatable IP we can enumerate multiple instances of on a single piece of silicon which has multiple variants available. This is a single device with a single regulator on it.
provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
I'll need some more help here. I need to use it from ALSA, so I don't think I can influence that interface easily.
Sorry? If this is going into the userspace ABI there's something seriously wrong...
What is currently in tree _does not work_, as there are two arizona chips, and two "LDO1" regulators. (Doable) suggestions how to fix that are welcome.
To repeat what I said above, provide an interface which namespaces by device (as we normally do when we need to distinguish between multiple instances of the same device). Given that everything is part of the same device it's very easy to discover which device so it's clearly no problem when mapping the supplies.
On Tue 2015-10-13 12:53:55, Mark Brown wrote:
On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote:
On Mon 2015-10-12 16:47:15, Mark Brown wrote:
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote:
static const struct regulator_desc arizona_ldo1_hc = {
- .name = "LDO1",
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by
device
They already are, see wm831x-ldo.c .
No, that's a different case where we actually have a repeatable IP we can enumerate multiple instances of on a single piece of silicon which has multiple variants available. This is a single device with a single regulator on it.
Ok. But I'd still like to get it working.
Now... I got up-to v4.2 kernel, and it seems that it has support for multiple sources with same name (but on different chips):
[ 1.125485] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.1 [ 1.285470] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.2
...but it does not look like I can use those aliases from the ALSA side:
[ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator
I tried to do this:
SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD,spi32766.1", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
Any idea what I did wrong, or what needs to be fixed?
provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
I'll need some more help here. I need to use it from ALSA, so I don't think I can influence that interface easily.
Sorry? If this is going into the userspace ABI there's something seriously wrong...
It is exposed to the ALSA. If ALSA exposes it to userspace, I'm not sure.
What is currently in tree _does not work_, as there are two arizona chips, and two "LDO1" regulators. (Doable) suggestions how to fix that are welcome.
To repeat what I said above, provide an interface which namespaces by device (as we normally do when we need to distinguish between multiple instances of the same device). Given that everything is part of the same device it's very easy to discover which device so it's clearly no problem when mapping the supplies.
I'm afraid I don't know how to do this. See above.
Best regards, Pavel
On Fri, Nov 13, 2015 at 10:58:12PM +0100, Pavel Machek wrote:
On Tue 2015-10-13 12:53:55, Mark Brown wrote:
On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote:
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by
device
Ok. But I'd still like to get it working.
So as I've been saying use the existing interfaces, or extend them as needed.
Now... I got up-to v4.2 kernel, and it seems that it has support for multiple sources with same name (but on different chips):
[ 1.125485] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.1 [ 1.285470] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.2
...but it does not look like I can use those aliases from the ALSA side:
[ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator
I tried to do this:
SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD,spi32766.1", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
You're attempting to put a system specific string into a generic driver, this will break all other users which is clearly not OK.
Any idea what I did wrong, or what needs to be fixed?
Well, if we look at the code that prints the alias message you pasted above:
pr_info("Adding alias for supply %s,%s -> %s,%s\n", id, dev_name(dev), alias_id, dev_name(alias_dev));
we can see that it's not just rewriting a string here but is rather mapping one supply, device tuple to another. You shouldn't find any places where the device and supply are concatenated into a single strong, including the interface used to request regulators, so attempting to rewrite the name of the supply is not going to get anywhere.
provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
I'll need some more help here. I need to use it from ALSA, so I don't think I can influence that interface easily.
Sorry? If this is going into the userspace ABI there's something seriously wrong...
It is exposed to the ALSA. If ALSA exposes it to userspace, I'm not sure.
So if it's not exposed to userspace (and it *really* shouldn't be) why would it not be possible to influence whatever interface you're thinking of here? I'm really confused by what you're saying here.
What is currently in tree _does not work_, as there are two arizona chips, and two "LDO1" regulators. (Doable) suggestions how to fix that are welcome.
To repeat what I said above, provide an interface which namespaces by device (as we normally do when we need to distinguish between multiple instances of the same device). Given that everything is part of the same device it's very easy to discover which device so it's clearly no problem when mapping the supplies.
I'm afraid I don't know how to do this. See above.
Look at how we resolve supplies when we do lookups, then look at how we create aliases for the MFD cells to map supplies into the function devices and figure out why those mappings aren't being found. The NULL you're seeing above seems like a bit of a warning sign here - where did that come from?
On Fri 2015-11-13 22:53:55, Mark Brown wrote:
On Fri, Nov 13, 2015 at 10:58:12PM +0100, Pavel Machek wrote:
On Tue 2015-10-13 12:53:55, Mark Brown wrote:
On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote:
No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by
device
Ok. But I'd still like to get it working.
So as I've been saying use the existing interfaces, or extend them as needed.
Obviously I'll need to use the existing interfaces, or extend them as needed. I'd expect subsystem maintainer to know if the existing interfaces are ok or what needs to be fixed, but it seems you either don't know how your subsystem works, or are not willing to tell me.
Is there someone else I should talk to with respect to regulators-ALSA interface?
Thanks, Pavel
On Sat, Nov 14, 2015 at 08:44:00AM +0100, Pavel Machek wrote:
Obviously I'll need to use the existing interfaces, or extend them as needed. I'd expect subsystem maintainer to know if the existing interfaces are ok or what needs to be fixed, but it seems you either don't know how your subsystem works, or are not willing to tell me.
I *am* trying to tell you but you appear to not be responding to the bits where I'm asking you to look at what's going on on your system. To repeat what you cut from the e-mail you're replying to:
| Look at how we resolve supplies when we do lookups, then look at how we | create aliases for the MFD cells to map supplies into the function | devices and figure out why those mappings aren't being found. The NULL | you're seeing above seems like a bit of a warning sign here - where did | that come from?
especially the bit about the NULL which looks likely to be the source of your problem.
Is there someone else I should talk to with respect to regulators-ALSA interface?
To restate one of my previous questions could you please tell me what this "regulators-ALSA" interface you keep talking about is? In order to help you I really need you to at least be looking at the code and talking in specifics about it and the concepts it implements. We need to be on something like the same page here, at the very least I need you to talk about what code you're looking at and what you don't understand so I can try to help you follow it but right now I'm just not sure where to start, it feels like you're trying to treat a lot of the code as a black box without following the abstractions it provides which makes things very hard.
If you're asking about the regulator API or embedded ALSA both of those are me but there are other things in here - the driver you're working with and the MFD core at least. At the minute I'm not convinced that the problem here isn't just that the MFD and/or MFD core hasn't set up the mappings to the child devices properly.
Hi!
Obviously I'll need to use the existing interfaces, or extend them as needed. I'd expect subsystem maintainer to know if the existing interfaces are ok or what needs to be fixed, but it seems you either don't know how your subsystem works, or are not willing to tell me.
I *am* trying to tell you but you appear to not be responding to the bits where I'm asking you to look at what's going on on your system. To repeat what you cut from the e-mail you're replying to:
| Look at how we resolve supplies when we do lookups, then look at how we | create aliases for the MFD cells to map supplies into the function | devices and figure out why those mappings aren't being found. The NULL | you're seeing above seems like a bit of a warning sign here - where did | that come from?
especially the bit about the NULL which looks likely to be the source of your problem.
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
I can do this (patch below), but I'm not quite sure it is the right thing. (This is v4.2 based).
The debug output is then this:
[ 1.424740] Initializing arizona-micsupp for codec 2 [ 1.429970] Registering expected codec [ 1.434446] mfd_add_device ... arizona-gpio, (null), arizona-gpio [ 1.441004] mfd_add_device ... arizona-gpio, spi32766.2-arizona-gpio, arizona-gpio [ 1.449449] mfd_add_device ... arizona-haptics, (null), arizona-haptics [ 1.456594] mfd_add_device ... arizona-haptics, spi32766.2-arizona-haptics, arizona-haptics [ 1.465629] mfd_add_device ... arizona-pwm, (null), arizona-pwm [ 1.471970] mfd_add_device ... arizona-pwm, spi32766.2-arizona-pwm, arizona-pwm [ 1.479879] mfd_add_device ... wm5102-codec, (null), wm5102-codec [ 1.486402] mfd_add_device ... wm5102-codec, spi32766.2-wm5102-codec, wm5102-codec [ 1.494403] Adding alias for supply MICVDD,spi32766.2-wm5102-codec -> MICVDD,spi32766.2 [ 1.502848] Adding alias for supply DBVDD2,spi32766.2-wm5102-codec -> DBVDD2,spi32766.2 [ 1.511279] Adding alias for supply DBVDD3,spi32766.2-wm5102-codec -> DBVDD3,spi32766.2 [ 1.519711] Adding alias for supply CPVDD,spi32766.2-wm5102-codec -> CPVDD,spi32766.2 [ 1.527957] Adding alias for supply SPKVDDL,spi32766.2-wm5102-codec -> SPKVDDL,spi32766.2 [ 1.536583] Adding alias for supply SPKVDDR,spi32766.2-wm5102-codec -> SPKVDDR,spi32766.2 [ 1.546882] vcan: Virtual CAN interface driver
Is there someone else I should talk to with respect to regulators-ALSA interface?
To restate one of my previous questions could you please tell me what this "regulators-ALSA" interface you keep talking about is? In order to help you I really need you to at least be looking at the code and talking in specifics about it and the concepts it implements. We need
It seems there are more "MICVDD"s then I assumed.
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
And now we have
sound/soc/codecs/wm5102.c, around line 1093:
@@ -1092,7 +1094,6 @@ SND_SOC_DAPM_SUPPLY("ASYNCOPCLK", ARIZONA_OUTPUT_ASYNC_CLOCK, SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0), -SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
That is the regulator<->alsa interface I'm talking about. But as you may recall, I have 2 arizona chips here, so two wm5102.c instances, and I believe this means that "MICVDD" is not suitable here, and we want something like "MICVDD,spi32766.2" here.
But a) code does not seem to be quite ready for that, and b) you said you disliked that approach.
to be on something like the same page here, at the very least I need you to talk about what code you're looking at and what you don't understand so I can try to help you follow it but right now I'm just not sure where to start, it feels like you're trying to treat a lot of the code as a black box without following the abstractions it provides which makes things very hard.
Well, the code is pretty close to the black box for me :-(.
I have hardware with two arizona chips, apparently code is not quite ready for that. I got it to somehow work with some rather ugly hacks, and I don't see a clear way to non-hacky version.
(For the reference, patch is attached, but it is rather ugly at places.)
If you're asking about the regulator API or embedded ALSA both of those are me but there are other things in here - the driver you're working with and the MFD core at least. At the minute I'm not convinced that the problem here isn't just that the MFD and/or MFD core hasn't set up the mappings to the child devices properly.
Ok, good. I don't understand how the things are expected to fit together. See above. I believe SND_SOC_ macros should have another argument "device", or maybe regulator names should have "device" name embedded in them.
Best regards, Pavel
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index 14fd5cb..c05feb4 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -147,6 +147,18 @@ static int mfd_add_device(struct device *parent, int id, pdev->dev.dma_parms = parent->dma_parms; pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
+ + printk("mfd_add_device ... %s, %s, %s\n", cell->name, dev_name(&pdev->dev), pdev->name); + { + char buf[128]; + sprintf(buf, "%s-%s", dev_name(parent), cell->name); + pdev->dev.kobj.name = kstrdup(buf, GFP_KERNEL); + } + + + printk("mfd_add_device ... %s, %s, %s\n", cell->name, dev_name(&pdev->dev), pdev->name); + /* &pdev->dev is not initialized correctly? */ + ret = regulator_bulk_register_supply_alias( &pdev->dev, cell->parent_supplies, parent, cell->parent_supplies, diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 34f3856..0bdf435 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1670,6 +1670,8 @@ int regulator_register_supply_alias(struct device *dev, const char *id, pr_info("Adding alias for supply %s,%s -> %s,%s\n", id, dev_name(dev), alias_id, dev_name(alias_dev));
+ WARN_ON(!dev); + return 0; } EXPORT_SYMBOL_GPL(regulator_register_supply_alias);
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
And now we have
sound/soc/codecs/wm5102.c, around line 1093:
@@ -1092,7 +1094,6 @@ SND_SOC_DAPM_SUPPLY("ASYNCOPCLK", ARIZONA_OUTPUT_ASYNC_CLOCK, SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0), -SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
That is the regulator<->alsa interface I'm talking about. But as you
So if you look at this just templates out some boilerplate regulator API client code which calls regulator_get() like any other client and then hooks that regulator into the audio power management.
may recall, I have 2 arizona chips here, so two wm5102.c instances, and I believe this means that "MICVDD" is not suitable here, and we want something like "MICVDD,spi32766.2" here.
But a) code does not seem to be quite ready for that, and b) you said you disliked that approach.
Please go and look at how regulator clients request their supplies and how those get resolved into actual supplies - it's exactly the same struct device based namespacing that we use for clocks, PWMs and other resources. It's not that I dislike this approach, it's that this approach does not make sense in the model we use for requesting supplies and is not supported in any way by the code.
I'm not sure how I can be any clearer that supply names are namespaced by client device and that as a result fiddling around with the supply name is not going to help anything.
to be on something like the same page here, at the very least I need you to talk about what code you're looking at and what you don't understand so I can try to help you follow it but right now I'm just not sure where to start, it feels like you're trying to treat a lot of the code as a black box without following the abstractions it provides which makes things very hard.
Well, the code is pretty close to the black box for me :-(.
How far have you got in trying to follow the code, what specific areas are confusing you?
Ok, good. I don't understand how the things are expected to fit together. See above. I believe SND_SOC_ macros should have another argument "device", or maybe regulator names should have "device" name embedded in them.
Regulator names *do* have a device. This is the whole point with namespacing by client device.
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
And now we have
sound/soc/codecs/wm5102.c, around line 1093:
@@ -1092,7 +1094,6 @@ SND_SOC_DAPM_SUPPLY("ASYNCOPCLK", ARIZONA_OUTPUT_ASYNC_CLOCK, SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0), -SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0), SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
That is the regulator<->alsa interface I'm talking about. But as you
So if you look at this just templates out some boilerplate regulator API client code which calls regulator_get() like any other client and then hooks that regulator into the audio power management.
Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have two regulators, right? Is there similar macro I can use?
Do you have an example (filename, linenumber) of sound driver that gets this right?
may recall, I have 2 arizona chips here, so two wm5102.c instances, and I believe this means that "MICVDD" is not suitable here, and we want something like "MICVDD,spi32766.2" here.
But a) code does not seem to be quite ready for that, and b) you said you disliked that approach.
Please go and look at how regulator clients request their supplies and how those get resolved into actual supplies - it's exactly the same struct device based namespacing that we use for clocks, PWMs and other resources. It's not that I dislike this approach, it's that this approach does not make sense in the model we use for requesting supplies and is not supported in any way by the code.
I'm not sure how I can be any clearer that supply names are namespaced by client device and that as a result fiddling around with the supply name is not going to help anything.
Well, you are saying that pretty clearly, but sound driver I seen assumes names are global, and /sys interface assumed the names are global. Give me an example I can look at and I should be able to figure it out... You are clear, but the kernel code seems to disagree with you.
Best regards, Pavel
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
Looking at this I suspect we need to re-add the code for matching regulators on the actual struct device and do that since this is going to be very error prone.
I guess Charles Keepax should be listed there?
Possibly, up to them.
So if you look at this just templates out some boilerplate regulator API client code which calls regulator_get() like any other client and then hooks that regulator into the audio power management.
Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have two regulators, right? Is there similar macro I can use?
No? What would make you say that?
Do you have an example (filename, linenumber) of sound driver that gets this right?
I'm not aware of any drivers that get this wrong.
I'm not sure how I can be any clearer that supply names are namespaced by client device and that as a result fiddling around with the supply name is not going to help anything.
Well, you are saying that pretty clearly, but sound driver I seen assumes names are global, and /sys interface assumed the names are global. Give me an example I can look at and I should be able to figure it out... You are clear, but the kernel code seems to disagree with you.
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Hi!
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
Looking at this I suspect we need to re-add the code for matching regulators on the actual struct device and do that since this is going to be very error prone.
Well, I guess I can test the patches :-).
So if you look at this just templates out some boilerplate regulator API client code which calls regulator_get() like any other client and then hooks that regulator into the audio power management.
Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have two regulators, right? Is there similar macro I can use?
No? What would make you say that?
Well.. the fact that in my setup regulators are global (by mistake as you say?) and it still picks them up without warning?
Plus, I'd expect to see some kind of "struct device" argument to SND_SOC_DAPM_REGULATOR_SUPPLY(). I'm selecting supply name, but I'm not selecting on which device...
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Thanks, Pavel
On Mon, Nov 16, 2015 at 08:45:34AM +0100, Pavel Machek wrote:
Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have two regulators, right? Is there similar macro I can use?
No? What would make you say that?
Plus, I'd expect to see some kind of "struct device" argument to SND_SOC_DAPM_REGULATOR_SUPPLY(). I'm selecting supply name, but I'm not selecting on which device...
I've already pointed you at the implementation at least once and hopefully the regulator API interfaces are quite clear here too that it's not possible to request a regulator without providing a device.
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Yes.
Hi!
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Yes.
Ok, so something like this should be applied?
(I'm not sure how to test it, as audio works before and after the patch.)
Thanks, Pavel
Signed-off-by: Pavel Machek pavel@denx.de
commit d6005263acb94343645e719ee90b8940cb2545df Author: Pavel pavel@ucw.cz Date: Mon Nov 16 13:19:21 2015 +0100
regulator_bulk_register() needs device to be already registered. Reorganize the code to make it so.
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index 14fd5cb..e891f10 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -147,12 +147,6 @@ static int mfd_add_device(struct device *parent, int id, pdev->dev.dma_parms = parent->dma_parms; pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
- ret = regulator_bulk_register_supply_alias( - &pdev->dev, cell->parent_supplies, - parent, cell->parent_supplies, - cell->num_parent_supplies); - if (ret < 0) - goto fail_res;
if (parent->of_node && cell->of_compatible) { for_each_child_of_node(parent->of_node, np) { @@ -169,12 +176,12 @@ static int mfd_add_device(struct device *parent, int id, ret = platform_device_add_data(pdev, cell->platform_data, cell->pdata_size); if (ret) - goto fail_alias; + goto fail_res; }
ret = mfd_platform_add_cell(pdev, cell, usage_count); if (ret) - goto fail_alias; + goto fail_res;
for (r = 0; r < cell->num_resources; r++) { res[r].name = cell->resources[r].name; @@ -210,22 +217,29 @@ static int mfd_add_device(struct device *parent, int id, if (has_acpi_companion(&pdev->dev)) { ret = acpi_check_resource_conflict(&res[r]); if (ret) - goto fail_alias; + goto fail_res; } } }
ret = platform_device_add_resources(pdev, res, cell->num_resources); if (ret) - goto fail_alias; + goto fail_res;
ret = platform_device_add(pdev); if (ret) - goto fail_alias; + goto fail_res;
if (cell->pm_runtime_no_callbacks) pm_runtime_no_callbacks(&pdev->dev);
+ ret = regulator_bulk_register_supply_alias( + &pdev->dev, cell->parent_supplies, + parent, cell->parent_supplies, + cell->num_parent_supplies); + if (ret < 0) + goto fail_alias; + kfree(res);
return 0;
On Mon, Nov 16, 2015 at 01:29:47PM +0100, Pavel Machek wrote:
Hi!
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Yes.
Ok, so something like this should be applied?
(I'm not sure how to test it, as audio works before and after the patch.)
Apologies this all going down over the weekend I have only just caught up. I will probably send a few other inline replies to the thread.
Thanks, Pavel
Signed-off-by: Pavel Machek pavel@denx.de
commit d6005263acb94343645e719ee90b8940cb2545df Author: Pavel pavel@ucw.cz Date: Mon Nov 16 13:19:21 2015 +0100
regulator_bulk_register() needs device to be already registered. Reorganize the code to make it so.diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index 14fd5cb..e891f10 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -147,12 +147,6 @@ static int mfd_add_device(struct device *parent, int id, pdev->dev.dma_parms = parent->dma_parms; pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
- ret = regulator_bulk_register_supply_alias(
&pdev->dev, cell->parent_supplies,parent, cell->parent_supplies,cell->num_parent_supplies);- if (ret < 0)
goto fail_res;
This does look like it is too early, but really all we are doing it adding the device pointer and the supply name into a lookup table, neither of those things change across the stuff below. The printout for the mapping does indeed appear to be broken (because you can't dev_name on the device yet, until after it has been added), but that will be all that should get fixed by this move.
if (parent->of_node && cell->of_compatible) { for_each_child_of_node(parent->of_node, np) { @@ -169,12 +176,12 @@ static int mfd_add_device(struct device *parent, int id, ret = platform_device_add_data(pdev, cell->platform_data, cell->pdata_size); if (ret)
goto fail_alias;
goto fail_res;}
ret = mfd_platform_add_cell(pdev, cell, usage_count); if (ret)
goto fail_alias;
goto fail_res;for (r = 0; r < cell->num_resources; r++) { res[r].name = cell->resources[r].name;
@@ -210,22 +217,29 @@ static int mfd_add_device(struct device *parent, int id, if (has_acpi_companion(&pdev->dev)) { ret = acpi_check_resource_conflict(&res[r]); if (ret)
goto fail_alias;
goto fail_res; }} }
ret = platform_device_add_resources(pdev, res, cell->num_resources); if (ret)
goto fail_alias;
goto fail_res;ret = platform_device_add(pdev); if (ret)
goto fail_alias;
goto fail_res;if (cell->pm_runtime_no_callbacks) pm_runtime_no_callbacks(&pdev->dev);
ret = regulator_bulk_register_supply_alias(
&pdev->dev, cell->parent_supplies,parent, cell->parent_supplies,cell->num_parent_supplies);if (ret < 0)
goto fail_alias;
Furthermore, the trouble is that you can't move this to here. The platform_device_add will usually cause the device to probe and most devices request their supplies in their probe. Thus if you only register the alias here then it will not be available when the device needs it.
Thanks, Charles
On Mon, Nov 16, 2015 at 01:29:47PM +0100, Pavel Machek wrote:
Hi!
Every single sound driver gets this right, none of them assume the name is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Yes.
Ok, so something like this should be applied?
(I'm not sure how to test it, as audio works before and after the patch.)
Well I guess the first test is do these error messages you where getting disappear:
[ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator
I think what we are missing here is looking at why the lookup is actually failing. I have had a look at the code, but I don't see an obvious reason why having a second CODEC would cause the supply lookup to fail.
Can you please apply this diff and provide a log so we can have a look at what is going on with the supply aliasing:
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 7181ffd..f1c8897 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1345,9 +1345,11 @@ static struct regulator_supply_alias *regulator_find_supply_alias( { struct regulator_supply_alias *map;
- list_for_each_entry(map, ®ulator_supply_alias_list, list) + list_for_each_entry(map, ®ulator_supply_alias_list, list) { + dev_err(dev, "Checking %s,%p\n", map->src_supply, map->src_dev); if (map->src_dev == dev && strcmp(map->src_supply, supply) == 0) return map; + }
return NULL; } @@ -1356,11 +1358,12 @@ static void regulator_supply_alias(struct device **dev, const char **supply) { struct regulator_supply_alias *map;
+ dev_err(*dev, "Looking for %s,%p\n", *supply, *dev); map = regulator_find_supply_alias(*dev, *supply); if (map) { - dev_dbg(*dev, "Mapping supply %s to %s,%s\n", + dev_err(*dev, "Mapping supply %s to %s,%p\n", *supply, map->alias_supply, - dev_name(map->alias_dev)); + map->alias_dev); *dev = map->alias_dev; *supply = map->alias_supply; } @@ -1796,8 +1799,8 @@ int regulator_register_supply_alias(struct device *dev, const char *id,
list_add(&map->list, ®ulator_supply_alias_list);
- pr_info("Adding alias for supply %s,%s -> %s,%s\n", - id, dev_name(dev), alias_id, dev_name(alias_dev)); + pr_info("Adding alias for supply %s,%p -> %s,%p\n", + id, dev, alias_id, alias_dev);
return 0; }
Thanks, Charles
On Mon, Nov 16, 2015 at 01:29:47PM +0100, Pavel Machek wrote:
Ok, so you are saying that if I fix mfd initialization, sound will automagically switch from global regulators to device-specific regulators and things will start working?
Yes.
Ok, so something like this should be applied?
No, moving the mapping of the aliases after the device has been instantiated won't help here since it means that we will try to probe the device without the mappings set up at all which guarantees that things will go wrong.
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Indeed that should be the case.
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
The patches mailing list functions as maintainer here. That way anyone who works on upstream Linux stuff here will see the email so you have more chance of someone replying. Also convient for when people leave the company etc. makes for an easy transition. I am afraid though that as in this case you are not always going to get a reply over the weekend.
Thanks, Charles
On Mon 2015-11-16 14:11:42, Charles Keepax wrote:
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Indeed that should be the case.
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
The patches mailing list functions as maintainer here. That way anyone who works on upstream Linux stuff here will see the email so you have more chance of someone replying. Also convient for when people leave the company etc. makes for an easy transition. I am afraid though that as in this case you are not always going to get a reply over the weekend.
Well.. maintainer is a responsible person. Yes, you can list a mailing list in maintainers file.. but mailing list is not going to be responsible for that.
Having a real name of person helps; if your name was listed there, I'd know you (are supposed to be) authoritative person for this.
I see it will need to be updated from time to time, but having name really helps.
Thanks, Pavel
On Sun, 22 Nov 2015, Pavel Machek wrote:
On Mon 2015-11-16 14:11:42, Charles Keepax wrote:
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Indeed that should be the case.
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
The patches mailing list functions as maintainer here. That way anyone who works on upstream Linux stuff here will see the email so you have more chance of someone replying. Also convient for when people leave the company etc. makes for an easy transition. I am afraid though that as in this case you are not always going to get a reply over the weekend.
Well.. maintainer is a responsible person. Yes, you can list a mailing list in maintainers file.. but mailing list is not going to be responsible for that.
Having a real name of person helps; if your name was listed there, I'd know you (are supposed to be) authoritative person for this.
I see it will need to be updated from time to time, but having name really helps.
A Mailing List is perfectly adequate for drivers which reside in a maintained subsystem. No requirement to submit names, particularly if there are lots of authorities personnel or if the personalities change regularly.
On Mon 2015-11-23 08:18:57, Lee Jones wrote:
On Sun, 22 Nov 2015, Pavel Machek wrote:
On Mon 2015-11-16 14:11:42, Charles Keepax wrote:
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
regulator_bulk_register_supply_alias() results in "Adding alias" stuff, and then drivers/regulator/arizona-micsupp.c tries to register another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Indeed that should be the case.
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
The patches mailing list functions as maintainer here. That way anyone who works on upstream Linux stuff here will see the email so you have more chance of someone replying. Also convient for when people leave the company etc. makes for an easy transition. I am afraid though that as in this case you are not always going to get a reply over the weekend.
Well.. maintainer is a responsible person. Yes, you can list a mailing list in maintainers file.. but mailing list is not going to be responsible for that.
Having a real name of person helps; if your name was listed there, I'd know you (are supposed to be) authoritative person for this.
I see it will need to be updated from time to time, but having name really helps.
A Mailing List is perfectly adequate for drivers which reside in a maintained subsystem. No requirement to submit names, particularly if there are lots of authorities personnel or if the personalities change regularly.
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address. Pavel
On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
On Mon 2015-11-23 08:18:57, Lee Jones wrote:
On Sun, 22 Nov 2015, Pavel Machek wrote:
On Mon 2015-11-16 14:11:42, Charles Keepax wrote:
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
> Well, mfd_core.c seems to call regulator_bulk_register_supply_alias() > with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the individual child device but rather system wide, probably because that mapping is being done too early, before we've actually created the device.
Take a look at mfd_add_device(). Yes, we do regulator_bulk_register_supply_alias() before doing platform_device_add().
> regulator_bulk_register_supply_alias() results in "Adding alias" > stuff, and then drivers/regulator/arizona-micsupp.c tries to register > another "MICVDD".
That's fine because all supplies should be namespaced with a device. The goal is to say "Supply X on device Y" (we do support exceptions for the few cases where there are not yet any devices involved but this clearly isn't one of them).
Indeed that should be the case.
Well, clearly someone should tell that to wm5102 maintainers. Hmm. It looks like driver is marked supported but there are no names attached?
WOLFSON MICROELECTRONICS DRIVERS L: patches@opensource.wolfsonmicro.com T: git git://opensource.wolfsonmicro.com/linux-2.6-asoc T: git git://opensource.wolfsonmicro.com/linux-2.6-audioplus W: http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-device%5C s S: Supported
I guess Charles Keepax should be listed there?
The patches mailing list functions as maintainer here. That way anyone who works on upstream Linux stuff here will see the email so you have more chance of someone replying. Also convient for when people leave the company etc. makes for an easy transition. I am afraid though that as in this case you are not always going to get a reply over the weekend.
Well.. maintainer is a responsible person. Yes, you can list a mailing list in maintainers file.. but mailing list is not going to be responsible for that.
Having a real name of person helps; if your name was listed there, I'd know you (are supposed to be) authoritative person for this.
I see it will need to be updated from time to time, but having name really helps.
A Mailing List is perfectly adequate for drivers which reside in a maintained subsystem. No requirement to submit names, particularly if there are lots of authorities personnel or if the personalities change regularly.
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address. Pavel
It's unreasonable to expect that one member of the Cirrus software team has the time to answer every inquiry about our drivers and never goes on vacation, or that there's only one person who is an authority about the drivers. There's a mailing list for a reason.
On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address.
It's unreasonable to expect that one member of the Cirrus software team has the time to answer every inquiry about our drivers and never goes on vacation, or that there's only one person who is an authority about the drivers. There's a mailing list for a reason.
It's perfectly OK to list multiple people - look at how other companies handle this, there's usually two or three people listed.
On Mon, Nov 23, 2015 at 11:30:41AM +0000, Mark Brown wrote:
On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address.
It's unreasonable to expect that one member of the Cirrus software team has the time to answer every inquiry about our drivers and never goes on vacation, or that there's only one person who is an authority about the drivers. There's a mailing list for a reason.
It's perfectly OK to list multiple people - look at how other companies handle this, there's usually two or three people listed.
I don't really object to sticking a few people in here if the general feeling is that would be better, but personally I didn't see any problem with the current setup.
Shall I do a patch to add a few of us in here?
Thanks, Charles
On Mon, 23 Nov 2015, Charles Keepax wrote:
On Mon, Nov 23, 2015 at 11:30:41AM +0000, Mark Brown wrote:
On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address.
It's unreasonable to expect that one member of the Cirrus software team has the time to answer every inquiry about our drivers and never goes on vacation, or that there's only one person who is an authority about the drivers. There's a mailing list for a reason.
It's perfectly OK to list multiple people - look at how other companies handle this, there's usually two or three people listed.
I don't really object to sticking a few people in here if the general feeling is that would be better, but personally I didn't see any problem with the current setup.
Me either.
Shall I do a patch to add a few of us in here?
Up to you. Happy either way.
On Mon 2015-11-23 11:46:37, Charles Keepax wrote:
On Mon, Nov 23, 2015 at 11:30:41AM +0000, Mark Brown wrote:
On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
That's what I'm saying. It is good to know who is the person of authority, as you can't tell from the From: address.
It's unreasonable to expect that one member of the Cirrus software team has the time to answer every inquiry about our drivers and never goes on vacation, or that there's only one person who is an authority about the drivers. There's a mailing list for a reason.
It's perfectly OK to list multiple people - look at how other companies handle this, there's usually two or three people listed.
I don't really object to sticking a few people in here if the general feeling is that would be better, but personally I didn't see any problem with the current setup.
Shall I do a patch to add a few of us in here?
Yes, please.
Pavel
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
Hi!
If you're asking about the regulator API or embedded ALSA both of those are me but there are other things in here - the driver you're working with and the MFD core at least. At the minute I'm not convinced that the problem here isn't just that the MFD and/or MFD core hasn't set up the mappings to the child devices properly.
Ok, good. I don't understand how the things are expected to fit together. See above. I believe SND_SOC_ macros should have another argument "device", or maybe regulator names should have "device" name embedded in them.
Effectively the device is passed it is just implicit. If you look where the regulator is actually registered in soc-dapm.c
case snd_soc_dapm_regulator_supply: w->regulator = devm_regulator_get(dapm->dev, w->name); if (IS_ERR(w->regulator)) {
You see we are requesting the regulator with the dapm device, which will correspond to the CODEC.
Thanks, Charles
participants (6)
-
Caleb Crome -
Charles Keepax -
Lee Jones -
Mark Brown -
Pavel Machek -
Richard Fitzgerald