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
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Nov 14, 2015 at 08:44:00AM +0100, Pavel Machek wrote:
> Obviously I'll need to use the existing interfaces, or extend them as
> needed. I'd expect subsystem maintainer to know if the existing
> interfaces are ok or what needs to be fixed, but it seems you either
> don't know how your subsystem works, or are not willing to tell me.
I *am* trying to tell you but you appear to not be responding to the
bits where I'm asking you to look at what's going on on your system. To
repeat what you cut from the e-mail you're replying to:
| Look at how we resolve supplies when we do lookups, then look at how we
| create aliases for the MFD cells to map supplies into the function
| devices and figure out why those mappings aren't being found. The NULL
| you're seeing above seems like a bit of a warning sign here - where did
| that come from?
especially the bit about the NULL which looks likely to be the source of
your problem.
> Is there someone else I should talk to with respect to regulators-ALSA
> interface?
To restate one of my previous questions could you please tell me what
this "regulators-ALSA" interface you keep talking about is? In order to
help you I really need you to at least be looking at the code and
talking in specifics about it and the concepts it implements. We need
to be on something like the same page here, at the very least I need you
to talk about what code you're looking at and what you don't understand
so I can try to help you follow it but right now I'm just not sure where
to start, it feels like you're trying to treat a lot of the code as a
black box without following the abstractions it provides which makes
things very hard.
If you're asking about the regulator API or embedded ALSA both of those
are me but there are other things in here - the driver you're working
with and the MFD core at least. At the minute I'm not convinced that
the problem here isn't just that the MFD and/or MFD core hasn't set up
the mappings to the child devices properly.
Hi!
> > Obviously I'll need to use the existing interfaces, or extend them as
> > needed. I'd expect subsystem maintainer to know if the existing
> > interfaces are ok or what needs to be fixed, but it seems you either
> > don't know how your subsystem works, or are not willing to tell me.
>
> I *am* trying to tell you but you appear to not be responding to the
> bits where I'm asking you to look at what's going on on your system. To
> repeat what you cut from the e-mail you're replying to:
>
> | Look at how we resolve supplies when we do lookups, then look at how we
> | create aliases for the MFD cells to map supplies into the function
> | devices and figure out why those mappings aren't being found. The NULL
> | you're seeing above seems like a bit of a warning sign here - where did
> | that come from?
>
> especially the bit about the NULL which looks likely to be the source of
> your problem.
Well, mfd_core.c seems to call regulator_bulk_register_supply_alias()
with device that does not have dev_name initialized.
I can do this (patch below), but I'm not quite sure it is the right
thing. (This is v4.2 based).
The debug output is then this:
[ 1.424740] Initializing arizona-micsupp for codec 2
[ 1.429970] Registering expected codec
[ 1.434446] mfd_add_device ... arizona-gpio, (null), arizona-gpio
[ 1.441004] mfd_add_device ... arizona-gpio,
spi32766.2-arizona-gpio, arizona-gpio
[ 1.449449] mfd_add_device ... arizona-haptics, (null),
arizona-haptics
[ 1.456594] mfd_add_device ... arizona-haptics,
spi32766.2-arizona-haptics, arizona-haptics
[ 1.465629] mfd_add_device ... arizona-pwm, (null), arizona-pwm
[ 1.471970] mfd_add_device ... arizona-pwm, spi32766.2-arizona-pwm,
arizona-pwm
[ 1.479879] mfd_add_device ... wm5102-codec, (null), wm5102-codec
[ 1.486402] mfd_add_device ... wm5102-codec,
spi32766.2-wm5102-codec, wm5102-codec
[ 1.494403] Adding alias for supply MICVDD,spi32766.2-wm5102-codec
-> MICVDD,spi32766.2
[ 1.502848] Adding alias for supply DBVDD2,spi32766.2-wm5102-codec
-> DBVDD2,spi32766.2
[ 1.511279] Adding alias for supply DBVDD3,spi32766.2-wm5102-codec
-> DBVDD3,spi32766.2
[ 1.519711] Adding alias for supply CPVDD,spi32766.2-wm5102-codec
-> CPVDD,spi32766.2
[ 1.527957] Adding alias for supply SPKVDDL,spi32766.2-wm5102-codec
-> SPKVDDL,spi32766.2
[ 1.536583] Adding alias for supply SPKVDDR,spi32766.2-wm5102-codec
-> SPKVDDR,spi32766.2
[ 1.546882] vcan: Virtual CAN interface driver
> > Is there someone else I should talk to with respect to regulators-ALSA
> > interface?
>
> To restate one of my previous questions could you please tell me what
> this "regulators-ALSA" interface you keep talking about is? In order to
> help you I really need you to at least be looking at the code and
> talking in specifics about it and the concepts it implements. We
> need
It seems there are more "MICVDD"s then I assumed.
regulator_bulk_register_supply_alias() results in "Adding alias"
stuff, and then drivers/regulator/arizona-micsupp.c tries to register
another "MICVDD".
And now we have
sound/soc/codecs/wm5102.c, around line 1093:
@@ -1092,7 +1094,6 @@ SND_SOC_DAPM_SUPPLY("ASYNCOPCLK",
ARIZONA_OUTPUT_ASYNC_CLOCK,
SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0),
SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0),
SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0),
-SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0),
SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
That is the regulator<->alsa interface I'm talking about. But as you
may recall, I have 2 arizona chips here, so two wm5102.c instances,
and I believe this means that "MICVDD" is not suitable here, and we
want something like "MICVDD,spi32766.2" here.
But a) code does not seem to be quite ready for that, and b) you said
you disliked that approach.
> to be on something like the same page here, at the very least I need you
> to talk about what code you're looking at and what you don't understand
> so I can try to help you follow it but right now I'm just not sure where
> to start, it feels like you're trying to treat a lot of the code as a
> black box without following the abstractions it provides which makes
> things very hard.
Well, the code is pretty close to the black box for me :-(.
I have hardware with two arizona chips, apparently code is not quite
ready for that. I got it to somehow work with some rather ugly hacks,
and I don't see a clear way to non-hacky version.
(For the reference, patch is attached, but it is rather ugly at places.)
> If you're asking about the regulator API or embedded ALSA both of those
> are me but there are other things in here - the driver you're working
> with and the MFD core at least. At the minute I'm not convinced that
> the problem here isn't just that the MFD and/or MFD core hasn't set up
> the mappings to the child devices properly.
Ok, good. I don't understand how the things are expected to fit
together. See above. I believe SND_SOC_ macros should have another
argument "device", or maybe regulator names should have "device" name
embedded in them.
Best regards,
Pavel
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 14fd5cb..c05feb4 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -147,6 +147,18 @@ static int mfd_add_device(struct device *parent, int id,
pdev->dev.dma_parms = parent->dma_parms;
pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
+
+ printk("mfd_add_device ... %s, %s, %s\n", cell->name, dev_name(&pdev->dev), pdev->name);
+ {
+ char buf[128];
+ sprintf(buf, "%s-%s", dev_name(parent), cell->name);
+ pdev->dev.kobj.name = kstrdup(buf, GFP_KERNEL);
+ }
+
+
+ printk("mfd_add_device ... %s, %s, %s\n", cell->name, dev_name(&pdev->dev), pdev->name);
+ /* &pdev->dev is not initialized correctly? */
+
ret = regulator_bulk_register_supply_alias(
&pdev->dev, cell->parent_supplies,
parent, cell->parent_supplies,
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 34f3856..0bdf435 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1670,6 +1670,8 @@ int regulator_register_supply_alias(struct device *dev, const char *id,
pr_info("Adding alias for supply %s,%s -> %s,%s\n",
id, dev_name(dev), alias_id, dev_name(alias_dev));
+ WARN_ON(!dev);
+
return 0;
}
EXPORT_SYMBOL_GPL(regulator_register_supply_alias);
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
> Well, mfd_core.c seems to call regulator_bulk_register_supply_alias()
> with device that does not have dev_name initialized.
OK, that'll be the problem then - we're not mapping the supply into the
individual child device but rather system wide, probably because that
mapping is being done too early, before we've actually created the
device.
> regulator_bulk_register_supply_alias() results in "Adding alias"
> stuff, and then drivers/regulator/arizona-micsupp.c tries to register
> another "MICVDD".
That's fine because all supplies should be namespaced with a device.
The goal is to say "Supply X on device Y" (we do support exceptions for
the few cases where there are not yet any devices involved but this
clearly isn't one of them).
> And now we have
> sound/soc/codecs/wm5102.c, around line 1093:
> @@ -1092,7 +1094,6 @@ SND_SOC_DAPM_SUPPLY("ASYNCOPCLK",
> ARIZONA_OUTPUT_ASYNC_CLOCK,
> SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD2", 0, 0),
> SND_SOC_DAPM_REGULATOR_SUPPLY("DBVDD3", 0, 0),
> SND_SOC_DAPM_REGULATOR_SUPPLY("CPVDD", 20, 0),
> -SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
> SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDL", 0, 0),
> SND_SOC_DAPM_REGULATOR_SUPPLY("SPKVDDR", 0, 0),
> That is the regulator<->alsa interface I'm talking about. But as you
So if you look at this just templates out some boilerplate regulator API
client code which calls regulator_get() like any other client and then
hooks that regulator into the audio power management.
> may recall, I have 2 arizona chips here, so two wm5102.c instances,
> and I believe this means that "MICVDD" is not suitable here, and we
> want something like "MICVDD,spi32766.2" here.
> But a) code does not seem to be quite ready for that, and b) you said
> you disliked that approach.
Please go and look at how regulator clients request their supplies and
how those get resolved into actual supplies - it's exactly the same
struct device based namespacing that we use for clocks, PWMs and other
resources. It's not that I dislike this approach, it's that this
approach does not make sense in the model we use for requesting supplies
and is not supported in any way by the code.
I'm not sure how I can be any clearer that supply names are namespaced
by client device and that as a result fiddling around with the supply
name is not going to help anything.
> > to be on something like the same page here, at the very least I need you
> > to talk about what code you're looking at and what you don't understand
> > so I can try to help you follow it but right now I'm just not sure where
> > to start, it feels like you're trying to treat a lot of the code as a
> > black box without following the abstractions it provides which makes
> > things very hard.
> Well, the code is pretty close to the black box for me :-(.
How far have you got in trying to follow the code, what specific areas
are confusing you?
> Ok, good. I don't understand how the things are expected to fit
> together. See above. I believe SND_SOC_ macros should have another
> argument "device", or maybe regulator names should have "device" name
> embedded in them.
Regulator names *do* have a device. This is the whole point with
namespacing by client device.
HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
> On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
>
> > Well, mfd_core.c seems to call regulator_bulk_register_supply_alias()
> > with device that does not have dev_name initialized.
>
> OK, that'll be the problem then - we're not mapping the supply into the
> individual child device but rather system wide, probably because that
> mapping is being done too early, before we've actually created the
> device.
Take a look at mfd_add_device(). Yes, we do
regulator_bulk_register_supply_alias() before doing
platform_device_add().
> > regulator_bulk_register_supply_alias() results in "Adding alias"
> > stuff, and then drivers/regulator/arizona-micsupp.c tries to register
> > another "MICVDD".
>
> That's fine because all supplies should be namespaced with a device.
> The goal is to say "Supply X on device Y" (we do support exceptions for
> the few cases where there are not yet any devices involved but this
> clearly isn't one of them).
Well, clearly someone should tell that to wm5102
maintainers. Hmm. It looks like driver is marked supported but there
are no names attached?
WOLFSON MICROELECTRONICS DRIVERS
L: [email protected]
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
On Sat, Nov 14, 2015 at 10:16:33PM +0100, Pavel Machek wrote:
> HiOn Sat 2015-11-14 18:49:40, Mark Brown wrote:
> > On Sat, Nov 14, 2015 at 06:59:16PM +0100, Pavel Machek wrote:
> > > Well, mfd_core.c seems to call regulator_bulk_register_supply_alias()
> > > with device that does not have dev_name initialized.
> > OK, that'll be the problem then - we're not mapping the supply into the
> > individual child device but rather system wide, probably because that
> > mapping is being done too early, before we've actually created the
> > device.
> Take a look at mfd_add_device(). Yes, we do
> regulator_bulk_register_supply_alias() before doing
> platform_device_add().
Looking at this I suspect we need to re-add the code for matching
regulators on the actual struct device and do that since this is going
to be very error prone.
> I guess Charles Keepax should be listed there?
Possibly, up to them.
> > So if you look at this just templates out some boilerplate regulator API
> > client code which calls regulator_get() like any other client and then
> > hooks that regulator into the audio power management.
> Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have
> two regulators, right? Is there similar macro I can use?
No? What would make you say that?
> Do you have an example (filename, linenumber) of sound driver that
> gets this right?
I'm not aware of any drivers that get this wrong.
> > I'm not sure how I can be any clearer that supply names are namespaced
> > by client device and that as a result fiddling around with the supply
> > name is not going to help anything.
> Well, you are saying that pretty clearly, but sound driver I seen
> assumes names are global, and /sys interface assumed the names are
> global. Give me an example I can look at and I should be able to
> figure it out... You are clear, but the kernel code seems to disagree
> with you.
Every single sound driver gets this right, none of them assume the name
is global. What makes you say that they assume names are global?
Hi!
> > > > Well, mfd_core.c seems to call regulator_bulk_register_supply_alias()
> > > > with device that does not have dev_name initialized.
>
> > > OK, that'll be the problem then - we're not mapping the supply into the
> > > individual child device but rather system wide, probably because that
> > > mapping is being done too early, before we've actually created the
> > > device.
>
> > Take a look at mfd_add_device(). Yes, we do
> > regulator_bulk_register_supply_alias() before doing
> > platform_device_add().
>
> Looking at this I suspect we need to re-add the code for matching
> regulators on the actual struct device and do that since this is going
> to be very error prone.
Well, I guess I can test the patches :-).
> > > So if you look at this just templates out some boilerplate regulator API
> > > client code which calls regulator_get() like any other client and then
> > > hooks that regulator into the audio power management.
>
> > Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have
> > two regulators, right? Is there similar macro I can use?
>
> No? What would make you say that?
Well.. the fact that in my setup regulators are global (by mistake as
you say?) and it still picks them up without warning?
Plus, I'd expect to see some kind of "struct device" argument to
SND_SOC_DAPM_REGULATOR_SUPPLY(). I'm selecting supply name, but I'm
not selecting on which device...
> Every single sound driver gets this right, none of them assume the name
> is global. What makes you say that they assume names are global?
Ok, so you are saying that if I fix mfd initialization, sound will
automagically switch from global regulators to device-specific
regulators and things will start working?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Nov 16, 2015 at 08:45:34AM +0100, Pavel Machek wrote:
> > > Ok, so SND_SOC_DAPM_REGULATOR_SUPPLY() does not work, because I have
> > > two regulators, right? Is there similar macro I can use?
> > No? What would make you say that?
> Plus, I'd expect to see some kind of "struct device" argument to
> SND_SOC_DAPM_REGULATOR_SUPPLY(). I'm selecting supply name, but I'm
> not selecting on which device...
I've already pointed you at the implementation at least once and
hopefully the regulator API interfaces are quite clear here too that
it's not possible to request a regulator without providing a device.
> > Every single sound driver gets this right, none of them assume the name
> > is global. What makes you say that they assume names are global?
> Ok, so you are saying that if I fix mfd initialization, sound will
> automagically switch from global regulators to device-specific
> regulators and things will start working?
Yes.
Hi!
> > > Every single sound driver gets this right, none of them assume the name
> > > is global. What makes you say that they assume names are global?
>
> > Ok, so you are saying that if I fix mfd initialization, sound will
> > automagically switch from global regulators to device-specific
> > regulators and things will start working?
>
> Yes.
Ok, so something like this should be applied?
(I'm not sure how to test it, as audio works before and after the
patch.)
Thanks,
Pavel
Signed-off-by: Pavel Machek <[email protected]>
commit d6005263acb94343645e719ee90b8940cb2545df
Author: Pavel <[email protected]>
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
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 <[email protected]>
>
> commit d6005263acb94343645e719ee90b8940cb2545df
> Author: Pavel <[email protected]>
> Date: Mon Nov 16 13:19:21 2015 +0100
>
> regulator_bulk_register() needs device to be already
> registered. Reorganize the code to make it so.
>
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 14fd5cb..e891f10 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -147,12 +147,6 @@ static int mfd_add_device(struct device *parent, int id,
> pdev->dev.dma_parms = parent->dma_parms;
> pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
>
> - ret = regulator_bulk_register_supply_alias(
> - &pdev->dev, cell->parent_supplies,
> - parent, cell->parent_supplies,
> - cell->num_parent_supplies);
> - if (ret < 0)
> - goto fail_res;
This does look like it is too early, but really all we are doing
it adding the device pointer and the supply name into a lookup
table, neither of those things change across the stuff below. The
printout for the mapping does indeed appear to be broken (because
you can't dev_name on the device yet, until after it has been
added), but that will be all that should get fixed by this move.
>
> if (parent->of_node && cell->of_compatible) {
> for_each_child_of_node(parent->of_node, np) {
> @@ -169,12 +176,12 @@ static int mfd_add_device(struct device *parent, int id,
> ret = platform_device_add_data(pdev,
> cell->platform_data, cell->pdata_size);
> if (ret)
> - goto fail_alias;
> + goto fail_res;
> }
>
> ret = mfd_platform_add_cell(pdev, cell, usage_count);
> if (ret)
> - goto fail_alias;
> + goto fail_res;
>
> for (r = 0; r < cell->num_resources; r++) {
> res[r].name = cell->resources[r].name;
> @@ -210,22 +217,29 @@ static int mfd_add_device(struct device *parent, int id,
> if (has_acpi_companion(&pdev->dev)) {
> ret = acpi_check_resource_conflict(&res[r]);
> if (ret)
> - goto fail_alias;
> + goto fail_res;
> }
> }
> }
>
> ret = platform_device_add_resources(pdev, res, cell->num_resources);
> if (ret)
> - goto fail_alias;
> + goto fail_res;
>
> ret = platform_device_add(pdev);
> if (ret)
> - goto fail_alias;
> + goto fail_res;
>
> if (cell->pm_runtime_no_callbacks)
> pm_runtime_no_callbacks(&pdev->dev);
>
> + ret = regulator_bulk_register_supply_alias(
> + &pdev->dev, cell->parent_supplies,
> + parent, cell->parent_supplies,
> + cell->num_parent_supplies);
> + if (ret < 0)
> + goto fail_alias;
> +
Furthermore, the trouble is that you can't move this to here. The
platform_device_add will usually cause the device to probe and
most devices request their supplies in their probe. Thus if you
only register the alias here then it will not be available when
the device needs it.
Thanks,
Charles
On 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
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: [email protected]
> 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
On Mon, Nov 16, 2015 at 01:29:47PM +0100, Pavel Machek wrote:
> Hi!
>
> > > > Every single sound driver gets this right, none of them assume the name
> > > > is global. What makes you say that they assume names are global?
> >
> > > Ok, so you are saying that if I fix mfd initialization, sound will
> > > automagically switch from global regulators to device-specific
> > > regulators and things will start working?
> >
> > Yes.
>
> Ok, so something like this should be applied?
>
> (I'm not sure how to test it, as audio works before and after the
> patch.)
Well I guess the first test is do these error messages you where
getting disappear:
> [ 2.734198] wm5102-codec.1 supply MICVDD,spi32766.1 not found, using dummy regulator
> [ 3.170912] wm5102-codec.2 supply MICVDD,spi32766.2 not found, using dummy regulator
I think what we are missing here is looking at why the lookup is
actually failing. I have had a look at the code, but I don't see
an obvious reason why having a second CODEC would cause the
supply lookup to fail.
Can you please apply this diff and provide a log so we can have a
look at what is going on with the supply aliasing:
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7181ffd..f1c8897 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1345,9 +1345,11 @@ static struct regulator_supply_alias *regulator_find_supply_alias(
{
struct regulator_supply_alias *map;
- list_for_each_entry(map, ®ulator_supply_alias_list, list)
+ list_for_each_entry(map, ®ulator_supply_alias_list, list) {
+ dev_err(dev, "Checking %s,%p\n", map->src_supply, map->src_dev);
if (map->src_dev == dev && strcmp(map->src_supply, supply) == 0)
return map;
+ }
return NULL;
}
@@ -1356,11 +1358,12 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
{
struct regulator_supply_alias *map;
+ dev_err(*dev, "Looking for %s,%p\n", *supply, *dev);
map = regulator_find_supply_alias(*dev, *supply);
if (map) {
- dev_dbg(*dev, "Mapping supply %s to %s,%s\n",
+ dev_err(*dev, "Mapping supply %s to %s,%p\n",
*supply, map->alias_supply,
- dev_name(map->alias_dev));
+ map->alias_dev);
*dev = map->alias_dev;
*supply = map->alias_supply;
}
@@ -1796,8 +1799,8 @@ int regulator_register_supply_alias(struct device *dev, const char *id,
list_add(&map->list, ®ulator_supply_alias_list);
- pr_info("Adding alias for supply %s,%s -> %s,%s\n",
- id, dev_name(dev), alias_id, dev_name(alias_dev));
+ pr_info("Adding alias for supply %s,%p -> %s,%p\n",
+ id, dev, alias_id, alias_dev);
return 0;
}
Thanks,
Charles
On Mon, Nov 16, 2015 at 01:29:47PM +0100, Pavel Machek wrote:
> > > Ok, so you are saying that if I fix mfd initialization, sound will
> > > automagically switch from global regulators to device-specific
> > > regulators and things will start working?
> > Yes.
> Ok, so something like this should be applied?
No, moving the mapping of the aliases after the device has been
instantiated won't help here since it means that we will try to probe
the device without the mappings set up at all which guarantees that
things will go wrong.
On 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: [email protected]
> > 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
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: [email protected]
> > > 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
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: [email protected]
> > > > 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
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: [email protected]
> > > > > 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.
On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
> On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
> > That's what I'm saying. It is good to know who is the person of
> > authority, as you can't tell from the From: address.
> It's unreasonable to expect that one member of the Cirrus software team
> has the time to answer every inquiry about our drivers and never goes on
> vacation, or that there's only one person who is an authority about the
> drivers. There's a mailing list for a reason.
It's perfectly OK to list multiple people - look at how other companies
handle this, there's usually two or three people listed.
On Mon, Nov 23, 2015 at 11:30:41AM +0000, Mark Brown wrote:
> On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
> > On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
>
> > > That's what I'm saying. It is good to know who is the person of
> > > authority, as you can't tell from the From: address.
>
> > It's unreasonable to expect that one member of the Cirrus software team
> > has the time to answer every inquiry about our drivers and never goes on
> > vacation, or that there's only one person who is an authority about the
> > drivers. There's a mailing list for a reason.
>
> It's perfectly OK to list multiple people - look at how other companies
> handle this, there's usually two or three people listed.
I don't really object to sticking a few people in here if the
general feeling is that would be better, but personally I didn't
see any problem with the current setup.
Shall I do a patch to add a few of us in here?
Thanks,
Charles
On Mon, 23 Nov 2015, Charles Keepax wrote:
> On Mon, Nov 23, 2015 at 11:30:41AM +0000, Mark Brown wrote:
> > On Mon, Nov 23, 2015 at 10:25:22AM +0000, Richard Fitzgerald wrote:
> > > On Mon, 2015-11-23 at 11:11 +0100, Pavel Machek wrote:
> >
> > > > That's what I'm saying. It is good to know who is the person of
> > > > authority, as you can't tell from the From: address.
> >
> > > It's unreasonable to expect that one member of the Cirrus software team
> > > has the time to answer every inquiry about our drivers and never goes on
> > > vacation, or that there's only one person who is an authority about the
> > > drivers. There's a mailing list for a reason.
> >
> > It's perfectly OK to list multiple people - look at how other companies
> > handle this, there's usually two or three people listed.
>
> I don't really object to sticking a few people in here if the
> general feeling is that would be better, but personally I didn't
> see any problem with the current setup.
Me either.
> Shall I do a patch to add a few of us in here?
Up to you. Happy either way.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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