System with multiple arizona (wm5102) codecs

System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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

Re: [alsa-devel] System with multiple arizona (wm5102) codecs

[Caleb Crome]

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

Re: System with multiple arizona (wm5102) codecs

[Caleb Crome]

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

Re: [alsa-devel] System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 566 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] System with multiple arizona (wm5102) codecs

[Caleb Crome]

>
> 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 {

Re: [alsa-devel] System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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",

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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->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);

Would be nice to use an snprintf here.

Thanks,
Charles

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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->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);

Would be nice to use an snprintf here.

Thanks,
Charles

multi-card support for davinci-evm

[Pavel Machek]

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);



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

multi-card support for arizona-core

[Pavel Machek]

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 */
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

multi-card support for arizona-core

[Pavel Machek]

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 */
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-card support for arizona-core

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1694 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3646 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2010 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3309 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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\
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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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\
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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2096 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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, &regulator_supply_alias_list, list)
+	list_for_each_entry(map, &regulator_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, &regulator_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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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, &regulator_supply_alias_list, list)
+	list_for_each_entry(map, &regulator_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, &regulator_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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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\
> 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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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\
> 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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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\
> > 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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\
> > 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Lee Jones]

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\
> > > 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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Lee Jones]

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\
> > > 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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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\
> > > > 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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Richard Fitzgerald]

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\
> > > > > 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.

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Richard Fitzgerald]

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\
> > > > > 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.

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Lee Jones]

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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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

Re: multi-codec support for arizona-ldo1 was Re: System with multiple arizona (wm5102) codecs

[Charles Keepax]

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