Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244509 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  multi-codec support for arizona-ldo1 was Re: System with multiple  arizona (wm5102) codecs Pavel Machek <pavel@ucw.cz> - 2015-10-12 11:10 +0200
    Re: multi-codec support for arizona-ldo1 was Re: System with  multiple arizona (wm5102) codecs Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2015-10-12 14:00 +0200
    Re: multi-codec support for arizona-ldo1 was Re: System with  multiple arizona (wm5102) codecs Mark Brown <broonie@kernel.org> - 2015-10-12 17:50 +0200
      Re: multi-codec support for arizona-ldo1 was Re: System with  multiple arizona (wm5102) codecs Pavel Machek <pavel@ucw.cz> - 2015-10-12 22:20 +0200
        Re: multi-codec support for arizona-ldo1 was Re: System with  multiple arizona (wm5102) codecs Mark Brown <broonie@kernel.org> - 2015-10-13 14:00 +0200

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web