| Message ID | 20151012090045.GA7448@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote: > Does this look like a step in right direction? > static const struct regulator_desc arizona_ldo1_hc = { > - .name = "LDO1", No, you definitely shouldn't be doing this - the regulator names should reflect the names the device has in the datasheet to aid people in going from software to the hardware and back again. They shouldn't be dynamically generated at runtime. If you need to namespace by device provide an interface which explicitly namespaces by device rather than hacking it into another interface, the usual thing is to use the struct device as the context.
Hi! On Mon 2015-10-12 16:47:15, Mark Brown wrote: > On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote: > > > Does this look like a step in right direction? > > > static const struct regulator_desc arizona_ldo1_hc = { > > - .name = "LDO1", > > No, you definitely shouldn't be doing this - the regulator names should > reflect the names the device has in the datasheet to aid people in going > from software to the hardware and back again. They shouldn't be > dynamically generated at runtime. If you need to namespace by device They already are, see wm831x-ldo.c . > provide an interface which explicitly namespaces by device rather than > hacking it into another interface, the usual thing is to use the struct > device as the context. I'll need some more help here. I need to use it from ALSA, so I don't think I can influence that interface easily. What is currently in tree _does not work_, as there are two arizona chips, and two "LDO1" regulators. (Doable) suggestions how to fix that are welcome. Pavel
On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote: > On Mon 2015-10-12 16:47:15, Mark Brown wrote: > > On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote: > > > static const struct regulator_desc arizona_ldo1_hc = { > > > - .name = "LDO1", > > No, you definitely shouldn't be doing this - the regulator names should > > reflect the names the device has in the datasheet to aid people in going > > from software to the hardware and back again. They shouldn't be > > dynamically generated at runtime. If you need to namespace by > device > They already are, see wm831x-ldo.c . No, that's a different case where we actually have a repeatable IP we can enumerate multiple instances of on a single piece of silicon which has multiple variants available. This is a single device with a single regulator on it. > > provide an interface which explicitly namespaces by device rather than > > hacking it into another interface, the usual thing is to use the struct > > device as the context. > I'll need some more help here. I need to use it from ALSA, so I don't > think I can influence that interface easily. Sorry? If this is going into the userspace ABI there's something seriously wrong... > What is currently in tree _does not work_, as there are two arizona > chips, and two "LDO1" regulators. (Doable) suggestions how to fix that > are welcome. To repeat what I said above, provide an interface which namespaces by device (as we normally do when we need to distinguish between multiple instances of the same device). Given that everything is part of the same device it's very easy to discover which device so it's clearly no problem when mapping the supplies.
On Tue 2015-10-13 12:53:55, Mark Brown wrote: > On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote: > > On Mon 2015-10-12 16:47:15, Mark Brown wrote: > > > On Mon, Oct 12, 2015 at 11:00:45AM +0200, Pavel Machek wrote: > > > > > static const struct regulator_desc arizona_ldo1_hc = { > > > > - .name = "LDO1", > > > > No, you definitely shouldn't be doing this - the regulator names should > > > reflect the names the device has in the datasheet to aid people in going > > > from software to the hardware and back again. They shouldn't be > > > dynamically generated at runtime. If you need to namespace by > > device > > > They already are, see wm831x-ldo.c . > > No, that's a different case where we actually have a repeatable IP we > can enumerate multiple instances of on a single piece of silicon which > has multiple variants available. This is a single device with a single > regulator on it. Ok. But I'd still like to get it working. Now... I got up-to v4.2 kernel, and it seems that it has support for multiple sources with same name (but on different chips): [ 1.125485] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.1 [ 1.285470] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.2 ...but it does not look like I can use those aliases from the ALSA side: [ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator I tried to do this: SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD,spi32766.1", 0, SND_SOC_DAPM_REGULATOR_BYPASS), Any idea what I did wrong, or what needs to be fixed? > > > provide an interface which explicitly namespaces by device rather than > > > hacking it into another interface, the usual thing is to use the struct > > > device as the context. > > > I'll need some more help here. I need to use it from ALSA, so I don't > > think I can influence that interface easily. > > Sorry? If this is going into the userspace ABI there's something > seriously wrong... It is exposed to the ALSA. If ALSA exposes it to userspace, I'm not sure. > > What is currently in tree _does not work_, as there are two arizona > > chips, and two "LDO1" regulators. (Doable) suggestions how to fix that > > are welcome. > > To repeat what I said above, provide an interface which namespaces by > device (as we normally do when we need to distinguish between multiple > instances of the same device). Given that everything is part of the > same device it's very easy to discover which device so it's clearly no > problem when mapping the supplies. I'm afraid I don't know how to do this. See above. Best regards, Pavel
On Fri, Nov 13, 2015 at 10:58:12PM +0100, Pavel Machek wrote: > On Tue 2015-10-13 12:53:55, Mark Brown wrote: > > On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote: > > > > No, you definitely shouldn't be doing this - the regulator names should > > > > reflect the names the device has in the datasheet to aid people in going > > > > from software to the hardware and back again. They shouldn't be > > > > dynamically generated at runtime. If you need to namespace by > > > device > Ok. But I'd still like to get it working. So as I've been saying use the existing interfaces, or extend them as needed. > Now... I got up-to v4.2 kernel, and it seems that it has support for > multiple sources with same name (but on different chips): > [ 1.125485] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.1 > [ 1.285470] Adding alias for supply MICVDD,(null) -> MICVDD,spi32766.2 > ...but it does not look like I can use those aliases from the ALSA side: > [ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator > [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator > I tried to do this: > SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD,spi32766.1", 0, SND_SOC_DAPM_REGULATOR_BYPASS), You're attempting to put a system specific string into a generic driver, this will break all other users which is clearly not OK. > Any idea what I did wrong, or what needs to be fixed? Well, if we look at the code that prints the alias message you pasted above: pr_info("Adding alias for supply %s,%s -> %s,%s\n", id, dev_name(dev), alias_id, dev_name(alias_dev)); we can see that it's not just rewriting a string here but is rather mapping one supply, device tuple to another. You shouldn't find any places where the device and supply are concatenated into a single strong, including the interface used to request regulators, so attempting to rewrite the name of the supply is not going to get anywhere. > > > > provide an interface which explicitly namespaces by device rather than > > > > hacking it into another interface, the usual thing is to use the struct > > > > device as the context. > > > I'll need some more help here. I need to use it from ALSA, so I don't > > > think I can influence that interface easily. > > Sorry? If this is going into the userspace ABI there's something > > seriously wrong... > It is exposed to the ALSA. If ALSA exposes it to userspace, I'm not sure. So if it's not exposed to userspace (and it *really* shouldn't be) why would it not be possible to influence whatever interface you're thinking of here? I'm really confused by what you're saying here. > > > What is currently in tree _does not work_, as there are two arizona > > > chips, and two "LDO1" regulators. (Doable) suggestions how to fix that > > > are welcome. > > To repeat what I said above, provide an interface which namespaces by > > device (as we normally do when we need to distinguish between multiple > > instances of the same device). Given that everything is part of the > > same device it's very easy to discover which device so it's clearly no > > problem when mapping the supplies. > I'm afraid I don't know how to do this. See above. Look at how we resolve supplies when we do lookups, then look at how we create aliases for the MFD cells to map supplies into the function devices and figure out why those mappings aren't being found. The NULL you're seeing above seems like a bit of a warning sign here - where did that come from?
On Fri 2015-11-13 22:53:55, Mark Brown wrote: > On Fri, Nov 13, 2015 at 10:58:12PM +0100, Pavel Machek wrote: > > On Tue 2015-10-13 12:53:55, Mark Brown wrote: > > > On Mon, Oct 12, 2015 at 10:11:38PM +0200, Pavel Machek wrote: > > > > > > No, you definitely shouldn't be doing this - the regulator names should > > > > > reflect the names the device has in the datasheet to aid people in going > > > > > from software to the hardware and back again. They shouldn't be > > > > > dynamically generated at runtime. If you need to namespace by > > > > device > > > Ok. But I'd still like to get it working. > > So as I've been saying use the existing interfaces, or extend them as > needed. Obviously I'll need to use the existing interfaces, or extend them as needed. I'd expect subsystem maintainer to know if the existing interfaces are ok or what needs to be fixed, but it seems you either don't know how your subsystem works, or are not willing to tell me. Is there someone else I should talk to with respect to regulators-ALSA interface? Thanks, Pavel
On Sat, Nov 14, 2015 at 08:44:00AM +0100, Pavel Machek wrote: > Obviously I'll need to use the existing interfaces, or extend them as > needed. I'd expect subsystem maintainer to know if the existing > interfaces are ok or what needs to be fixed, but it seems you either > don't know how your subsystem works, or are not willing to tell me. I *am* trying to tell you but you appear to not be responding to the bits where I'm asking you to look at what's going on on your system. To repeat what you cut from the e-mail you're replying to: | Look at how we resolve supplies when we do lookups, then look at how we | create aliases for the MFD cells to map supplies into the function | devices and figure out why those mappings aren't being found. The NULL | you're seeing above seems like a bit of a warning sign here - where did | that come from? especially the bit about the NULL which looks likely to be the source of your problem. > Is there someone else I should talk to with respect to regulators-ALSA > interface? To restate one of my previous questions could you please tell me what this "regulators-ALSA" interface you keep talking about is? In order to help you I really need you to at least be looking at the code and talking in specifics about it and the concepts it implements. We need to be on something like the same page here, at the very least I need you to talk about what code you're looking at and what you don't understand so I can try to help you follow it but right now I'm just not sure where to start, it feels like you're trying to treat a lot of the code as a black box without following the abstractions it provides which makes things very hard. If you're asking about the regulator API or embedded ALSA both of those are me but there are other things in here - the driver you're working with and the MFD core at least. At the minute I'm not convinced that the problem here isn't just that the MFD and/or MFD core hasn't set up the mappings to the child devices properly.
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",