| Message ID | 1344527635-6163-6-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <lee.jones@linaro.org> wrote: > +static struct irq_domain *db8500_irq_domain; So this is a good idea. > +int db8500_irq_get_virq(int irq); And I'm sceptic about this business. Why isn't this physical-to virtual mapping business confined to the core MFD driver? But enlighten me. Yours, Linus Walleij
On Tuesday 14 August 2012, Linus Walleij wrote: > > On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > +static struct irq_domain *db8500_irq_domain; > > So this is a good idea. > > > +int db8500_irq_get_virq(int irq); > > And I'm sceptic about this business. Why isn't this physical-to virtual > mapping business confined to the core MFD driver? But enlighten me. > I believe MFD does not (yet) know about IRQ domains. The wm8994 and 88pm80x drivers do this slightly different by exporting a foo_request_irq() function that performs the mapping. Traditionally, an MFD would add an offset to the local IRQ number to put the VIRQ into the IRQ resource, but this doesn't work when you have domains other than legacy. Arnd
On Tue, Aug 14, 2012 at 11:42 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 14 August 2012, Linus Walleij wrote: >> On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <lee.jones@linaro.org> wrote: >> >> > +int db8500_irq_get_virq(int irq); >> >> And I'm sceptic about this business. Why isn't this physical-to virtual >> mapping business confined to the core MFD driver? But enlighten me. > > Traditionally, an MFD would add an offset to the local IRQ number > to put the VIRQ into the IRQ resource, but this doesn't work when > you have domains other than legacy. Yes but I think I saw this other patch set from Lee, hitting irqdomain, OF and MFD to actually fix this ... or did I get it wrong? Yours, Linus Walleij
On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote: > On Tue, Aug 14, 2012 at 11:42 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 14 August 2012, Linus Walleij wrote: > >> On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <lee.jones@linaro.org> wrote: > >> > >> > +int db8500_irq_get_virq(int irq); > >> > >> And I'm sceptic about this business. Why isn't this physical-to virtual > >> mapping business confined to the core MFD driver? But enlighten me. > > > > Traditionally, an MFD would add an offset to the local IRQ number > > to put the VIRQ into the IRQ resource, but this doesn't work when > > you have domains other than legacy. > > Yes but I think I saw this other patch set from Lee, hitting > irqdomain, OF and MFD to actually fix this ... or did I get > it wrong? No, you're not wrong. Historically (in my patches) xb8500_irq_get_virq() was used by drivers to obtain a VIRQ when not using Device Tree. Now the MFD core handles conversion there is little requirement for it. In fact there are no more users for db8500_irq_get_virq() and only one user for ab8500_irq_get_virq() and that's itself. I guess we can rid them and call irq_get_mapping() directly instead.
On Tue, Aug 14, 2012 at 10:29:14AM +0200, Linus Walleij wrote: > On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > +static struct irq_domain *db8500_irq_domain; > > So this is a good idea. Did you mean this, or did you mean that it's _not_ a good idea? If the latter is true, where would you prefer to see it? > > +int db8500_irq_get_virq(int irq); > > And I'm sceptic about this business. Why isn't this physical-to virtual > mapping business confined to the core MFD driver? But enlighten me. > > Yours, > Linus Walleij
On Mon, Aug 20, 2012 at 09:36:43AM +0100, Lee Jones wrote: > On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote: > > Yes but I think I saw this other patch set from Lee, hitting > > irqdomain, OF and MFD to actually fix this ... or did I get > > it wrong? > No, you're not wrong. > Historically (in my patches) xb8500_irq_get_virq() was used by drivers > to obtain a VIRQ when not using Device Tree. Now the MFD core handles > conversion there is little requirement for it. In fact there are no > more users for db8500_irq_get_virq() and only one user for > ab8500_irq_get_virq() and that's itself. I guess we can rid them and > call irq_get_mapping() directly instead. Oh dear. Unfortunately whoever added this support to the MFD core did so in such a manner that it's only supported for device tree systems and only for devices which express the MFD cells as device tree nodes which means that most devices can't it - db8500 has got a reasonably unusual combination there.
On Mon, Aug 20, 2012 at 01:10:55PM +0100, Mark Brown wrote: > On Mon, Aug 20, 2012 at 09:36:43AM +0100, Lee Jones wrote: > > On Tue, Aug 14, 2012 at 12:44:37PM +0200, Linus Walleij wrote: > > > > Yes but I think I saw this other patch set from Lee, hitting > > > irqdomain, OF and MFD to actually fix this ... or did I get > > > it wrong? > > > No, you're not wrong. > > > Historically (in my patches) xb8500_irq_get_virq() was used by drivers > > to obtain a VIRQ when not using Device Tree. Now the MFD core handles > > conversion there is little requirement for it. In fact there are no > > more users for db8500_irq_get_virq() and only one user for > > ab8500_irq_get_virq() and that's itself. I guess we can rid them and > > call irq_get_mapping() directly instead. > > Oh dear. Unfortunately whoever added this support to the MFD core did > so in such a manner that it's only supported for device tree systems > and only for devices which express the MFD cells as device tree nodes > which means that most devices can't it - db8500 has got a reasonably > unusual combination there. Right, that was the initial intention. It would be a trivial semantic change if drivers without DT support wished to use the functionality though. However, the only examples I found of a non-DT enabled driver that could make good use of it in order to strip out some cruft would be the Arizona and one of the Samsung drivers, and they each have their own hand-rolled methods of hwirq -> virq conversion now, so any change to support them would result in multiple invocations of irq_create_mapping which would likely cause breakage.
On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote: > Right, that was the initial intention. It would be a trivial semantic > change if drivers without DT support wished to use the functionality > though. However, the only examples I found of a non-DT enabled driver > that could make good use of it in order to strip out some cruft would > be the Arizona and one of the Samsung drivers, and they each have All of the regmap devices could use this. > their own hand-rolled methods of hwirq -> virq conversion now, so any > change to support them would result in multiple invocations of > irq_create_mapping which would likely cause breakage. Multiple calls to irq_create_mapping() are totally fine.
On Mon, Aug 20, 2012 at 05:29:23PM +0100, Mark Brown wrote: > On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote: > > > Right, that was the initial intention. It would be a trivial semantic > > change if drivers without DT support wished to use the functionality > > though. However, the only examples I found of a non-DT enabled driver > > that could make good use of it in order to strip out some cruft would > > be the Arizona and one of the Samsung drivers, and they each have > > All of the regmap devices could use this. Only if they have linear domains and don't support DT. If they don't have linear domains there's no point, if they support DT then they can use it as it is. > > their own hand-rolled methods of hwirq -> virq conversion now, so any > > change to support them would result in multiple invocations of > > irq_create_mapping which would likely cause breakage. > > Multiple calls to irq_create_mapping() are totally fine. Sorry, perhaps I wasn't clear. <mfd_parent_device>_probe() | +-> mfd_add_device() | +-> res->start = res->end = irq_create_mapping(res->start); // Now a VIRQ <mfd_child_device>_probe() | +-> <mfd_parent_device>_irq_get_virq(res->start) // Ahhh, double VIRQ conversion To stop being DT dependent we'd need to remove all hand-rolled stuff that these drivers are currently using, or else they will be attempting to convert and already converted VIRQ value.
On Mon, Aug 20, 2012 at 05:49:50PM +0100, Lee Jones wrote: > On Mon, Aug 20, 2012 at 05:29:23PM +0100, Mark Brown wrote: > > All of the regmap devices could use this. > Only if they have linear domains and don't support DT. Neither of those restrictions really apply... > If they don't have linear domains there's no point, if they support DT > then they can use it as it is. All this stuff just works for any IRQ domain type, there's no requirement for a particular one. It's not urgently exciting for legacy domains but it's not harmful either and pushes all the handling of this stuff out of the MFD core and into the irqdomain code which is definitely an abstraction win. As far as DT goes supporting DT isn't enough, the current code will only work on systems that actively use DT and do so using a particular style of binding. Since the majority of Linux architectures don't support DT at all and it's not universally deployed on those that do this means that generic drivers can't rely on it, and even drivers that use DT might not be using the binding pattern which the framework code now uses. Indeed now that I think about the above isn't this going to be actively harmful for generic drivers if they use this pattern for their bindings? On DT systems the framework will unconditionally do the mapping but on others no mapping will be done. The function drivers don't know if the mapping has been performed or not. Unless I'm missing something here we need to fix this before the merge window... > To stop being DT dependent we'd need to remove all hand-rolled stuff > that these drivers are currently using, or else they will be attempting > to convert and already converted VIRQ value. Well, yes - using framework code would be the goal here...
> > If they don't have linear domains there's no point, if they support DT > > then they can use it as it is. > > All this stuff just works for any IRQ domain type, there's no > requirement for a particular one. It's not urgently exciting for legacy > domains but it's not harmful either and pushes all the handling of this > stuff out of the MFD core and into the irqdomain code which is > definitely an abstraction win. Wherever we do this from to be able to obtain the IRQ domain pointer, which is where I'm currently struggling. Our options are: - Call into a helper function based in the IRQ controller from each child device. In turn the IRQ controller will be responsible for creating the mapping necessary to obtain a virq. Using this method the child device doesn't need to know if we're using an IRQ domain or not, or whether we're using Device Tree or not. The drawback is that each child device will be required to call the helper function prior to requesting an IRQ. - If we're only talking MFD here, we can handle this stuff in the MFD core, but we need more information. The IRQ domain subsystem only allows domain look-up via a Device Tree node, so we need to get our hands on the domain another way in the case of non-DT enabled devices. Either we add another parameter to mfd_add_device(irq_domain, ...), or we standardise the 'irq_domain' variable name and use: irq_domain = container_of(parent, struct irq_domain, irq_domain); - I know that you have interest in pushing the functionality into the IRQ domain subsystem, but I'm struggling to see how. It's calling into the IRQ domain where we're seeing issues in the first place, specifically irq_create_mapping(). How about if we passed 'irq_domain' as a parameter when requesting the IRQ? That way we can pass the correct IRQ without worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem can do the necessary conversions. If 'irq_domain' is NULL it continues to use the requested IRQ as a virq.
On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote: > Wherever we do this from to be able to obtain the IRQ domain pointer, > which is where I'm currently struggling. Our options are: > - If we're only talking MFD here, we can handle this stuff in the MFD > core, but we need more information. The IRQ domain subsystem only allows > domain look-up via a Device Tree node, so we need to get our hands on What makes you say this? This is just a convenience for finding a domain, irqdomains are *completely* indepentant of device tree. > the domain another way in the case of non-DT enabled devices. Either we > add another parameter to mfd_add_device(irq_domain, ...), or we > standardise the 'irq_domain' variable name and use: > irq_domain = container_of(parent, struct irq_domain, irq_domain); Passing the domain into mfd seems like the obvious solution here - it's exactly what we currently do for legacy stuff (where we pass in the irq base), ideally what we'd end up with over time is that we could just remove the irq_base parameter entirely as everything would in time be moved over to using irqdomains. > - I know that you have interest in pushing the functionality into the > IRQ domain subsystem, but I'm struggling to see how. It's calling into > the IRQ domain where we're seeing issues in the first place, specifically > irq_create_mapping(). How about if we passed 'irq_domain' as a parameter > when requesting the IRQ? That way we can pass the correct IRQ without > worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem > can do the necessary conversions. If 'irq_domain' is NULL it continues to > use the requested IRQ as a virq. This is totally orthogonal to doing the mapping in the MFD subsystem which is the issue here.
On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote: > On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote: > > > Wherever we do this from to be able to obtain the IRQ domain pointer, > > which is where I'm currently struggling. Our options are: > > > - If we're only talking MFD here, we can handle this stuff in the MFD > > core, but we need more information. The IRQ domain subsystem only allows > > domain look-up via a Device Tree node, so we need to get our hands on > > What makes you say this? This is just a convenience for finding a > domain, irqdomains are *completely* indepentant of device tree. How can you say that? I think you mean _can_ be independent of DT. If that's what you mean then yes, that's true. All I'm saying is we need another way to get hold of the domain, because the only way to obtain it without having direct access is via a device node. > > the domain another way in the case of non-DT enabled devices. Either we > > add another parameter to mfd_add_device(irq_domain, ...), or we > > standardise the 'irq_domain' variable name and use: > > irq_domain = container_of(parent, struct irq_domain, irq_domain); > > Passing the domain into mfd seems like the obvious solution here - it's > exactly what we currently do for legacy stuff (where we pass in the irq > base), ideally what we'd end up with over time is that we could just > remove the irq_base parameter entirely as everything would in time be > moved over to using irqdomains. Right. That's fine (and easy) then. You threw me when you said you wanted it handled higher-up in the framework, as I didn't see how we could do this in the irqdomain itself. > > - I know that you have interest in pushing the functionality into the > > IRQ domain subsystem, but I'm struggling to see how. It's calling into > > the IRQ domain where we're seeing issues in the first place, specifically > > irq_create_mapping(). How about if we passed 'irq_domain' as a parameter > > when requesting the IRQ? That way we can pass the correct IRQ without > > worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem > > can do the necessary conversions. If 'irq_domain' is NULL it continues to > > use the requested IRQ as a virq. > > This is totally orthogonal to doing the mapping in the MFD subsystem > which is the issue here. Again, I only mentioned this because you said you wanted it to be handled by the irqdomain. I'll code up the second suggestion now.
On Tue, Aug 21, 2012 at 11:54:14AM +0100, Lee Jones wrote: > On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote: > > What makes you say this? This is just a convenience for finding a > > domain, irqdomains are *completely* indepentant of device tree. > How can you say that? I think you mean _can_ be independent of DT. If > that's what you mean then yes, that's true. All I'm saying is we need No, I really mean what I'm saying. Device tree builds on irqdomains, not the other way around. > another way to get hold of the domain, because the only way to obtain > it without having direct access is via a device node. This doesn't actually hold. > > > - I know that you have interest in pushing the functionality into the > > > IRQ domain subsystem, but I'm struggling to see how. It's calling into > > > the IRQ domain where we're seeing issues in the first place, specifically > > > irq_create_mapping(). How about if we passed 'irq_domain' as a parameter > > > when requesting the IRQ? That way we can pass the correct IRQ without > > > worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem > > > can do the necessary conversions. If 'irq_domain' is NULL it continues to > > > use the requested IRQ as a virq. > > This is totally orthogonal to doing the mapping in the MFD subsystem > > which is the issue here. > Again, I only mentioned this because you said you wanted it to be handled > by the irqdomain. The *mapping* should be being handled in irqdomain. > I'll code up the second suggestion now. I've already done this.
On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote: > On Tue, Aug 21, 2012 at 11:54:14AM +0100, Lee Jones wrote: > > On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote: > > > > What makes you say this? This is just a convenience for finding a > > > domain, irqdomains are *completely* indepentant of device tree. > > > How can you say that? I think you mean _can_ be independent of DT. If > > that's what you mean then yes, that's true. All I'm saying is we need > > No, I really mean what I'm saying. Device tree builds on irqdomains, > not the other way around. This is just semantics. > > another way to get hold of the domain, because the only way to obtain > > it without having direct access is via a device node. > > This doesn't actually hold. Okay, besides irq_find_host(struct device_node *np), how else can you fetch a domain from the irqdomain? > > > > - I know that you have interest in pushing the functionality into the > > > > IRQ domain subsystem, but I'm struggling to see how. It's calling into > > > > the IRQ domain where we're seeing issues in the first place, specifically > > > > irq_create_mapping(). How about if we passed 'irq_domain' as a parameter > > > > when requesting the IRQ? That way we can pass the correct IRQ without > > > > worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem > > > > can do the necessary conversions. If 'irq_domain' is NULL it continues to > > > > use the requested IRQ as a virq. > > > > This is totally orthogonal to doing the mapping in the MFD subsystem > > > which is the issue here. > > > Again, I only mentioned this because you said you wanted it to be handled > > by the irqdomain. > > The *mapping* should be being handled in irqdomain. > > > I'll code up the second suggestion now. > > I've already done this. What have you done already? Why make suggestions if you're just going to do the work yourself?
On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote: > On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote: > > > another way to get hold of the domain, because the only way to obtain > > > it without having direct access is via a device node. > > This doesn't actually hold. > Okay, besides irq_find_host(struct device_node *np), how else can you fetch > a domain from the irqdomain? I'm not sure I can parse the above, sorry - I'm not sure I can distinguish "domain" and "irqdomain". > What have you done already? Implemented a patch for this which I've now tested a bit and will probably post in the next hour or so. > Why make suggestions if you're just going to do the work yourself? I made the suggestion then later on realised that this was actively going to break things I care about so I actually need it fixing.
On Tue, Aug 21, 2012 at 05:52:07PM +0100, Mark Brown wrote: > On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote: > > On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote: > > > > > another way to get hold of the domain, because the only way to obtain > > > > it without having direct access is via a device node. > > > > This doesn't actually hold. > > > Okay, besides irq_find_host(struct device_node *np), how else can you fetch > > a domain from the irqdomain? > > I'm not sure I can parse the above, sorry - I'm not sure I can > distinguish "domain" and "irqdomain". Are you being intentionally awkward on this? Picking holes when you know exactly what I'm trying to say? Not that it matters now, but just out of principle, let me try to put it in a very clear way so there can be no misinterpretation. I was saying that in order for the MFD core to carry out the hwirq->virq conversion, it needed to obtain the irqdomain pointer pertaining to the provided hwirq. The only helper function the irqdomain subsystem provides requires a device node pointer to be passed as an argument, hence the mention of 'irq_find_host(struct device_node *np)'. Then the Device Tree is traversed until a specified 'interrupt-controller' is stumbled upon or is pointed to by the 'interrupt-parent' property. Hence, we have to find another way to find the irqdomain pointers for non-DT based MFDs. To which we now have a solution. > > What have you done already? > > Implemented a patch for this which I've now tested a bit and will > probably post in the next hour or so. > > > Why make suggestions if you're just going to do the work yourself? > > I made the suggestion then later on realised that this was actively > going to break things I care about so I actually need it fixing. I'm a little taken aback and annoyed by this. In a previous email thread you categorically requested that I discuss some of the important changes with maintainers and people in-the-know prior to actually writing any code. I was obviously actively working on, had put time into, and was in the mist of discussing this with you. Then you just go ahead and code it (the easy part) yourself, essentially wasting my time. Surely there's some kind of etiquette surrounding such things?
On Wed, Aug 22, 2012 at 09:17:50AM +0100, Lee Jones wrote: > I was saying that in order for the MFD core to carry out the hwirq->virq > conversion, it needed to obtain the irqdomain pointer pertaining to the > provided hwirq. The only helper function the irqdomain subsystem provides > requires a device node pointer to be passed as an argument, hence the > mention of 'irq_find_host(struct device_node *np)'. Then the Device Tree > is traversed until a specified 'interrupt-controller' is stumbled upon > or is pointed to by the 'interrupt-parent' property. Hence, we have to > find another way to find the irqdomain pointers for non-DT based MFDs. To > which we now have a solution. Oh, right. Yes, there's no way to get an irqdomain if you don't already have it or something which has a direct mapping to one like an irqdomain. > > I made the suggestion then later on realised that this was actively > > going to break things I care about so I actually need it fixing. > I'm a little taken aback and annoyed by this. In a previous email thread > you categorically requested that I discuss some of the important changes > with maintainers and people in-the-know prior to actually writing any > code. No, that's not something I've ever said to do. I *have* asked you to communicate more clearly about what you're doing but that doesn't mean to stop sending code, it means to have clearer words around what you're sending. The really bad pattern here is that you're frequently working around issues in your drivers with changes in the subsystem without mentioning that the driver issues even exist - this makes it much harder understand what you are trying to achieve, especially when there is a problem with your subsystem changes and/or the urgency you're attaching to them. > I was obviously actively working on, had put time into, and was in > the mist of discussing this with you. Then you just go ahead and code it > (the easy part) yourself, essentially wasting my time. Surely there's > some kind of etiquette surrounding such things? To be honest in this case I had expected to send the patch out much sooner than I did - several priority interrupts stopped me testing it. Like I say I realised that I really needed a fix and it seemed like the quickest way to accomplish that was to just send the code.
> > > I made the suggestion then later on realised that this was actively > > > going to break things I care about so I actually need it fixing. > > > I'm a little taken aback and annoyed by this. In a previous email thread > > you categorically requested that I discuss some of the important changes > > with maintainers and people in-the-know prior to actually writing any > > code. > > No, that's not something I've ever said to do. > > I *have* asked you to communicate more clearly about what you're doing > but that doesn't mean to stop sending code, it means to have clearer > words around what you're sending. That's not how I interpreted your words: "What you can do here is to commmunicate about what you're doing more. Don't just think about the code, think about the communication surrounding the code - this is the core of the issue." > The really bad pattern here is that > you're frequently working around issues in your drivers with changes in > the subsystem without mentioning that the driver issues even exist - > this makes it much harder understand what you are trying to achieve, > especially when there is a problem with your subsystem changes and/or > the urgency you're attaching to them. Frequently? I've done this once, and yes I did push hard because I thought the subsystem was incorrect (I still think folding an entire driver because of a minor failure is wrong, but we digress). This is completely different to that. This was a subsystem change designed to aid DT enabled MFDs which 'chose' to register themselves in a specific way, by passing their compatible string through the mfd_cell only. It doesn't affect any other MFD unless they wrongly assume the conversion would be automatically done for them. However, the author would know because they would have tested it. All of this was discussed at length with Arnd and this is what we came up with. I think it's great that you like the idea and want to extend that functionality to other MFDs which perhaps don't support DT, or the ones that do but don't want to provide compatible strings or device nodes for all the MFD's child devices. But that is all we're doing here. There was no breakage. It served a purpose and it worked well. So well in fact that you've now provided the intended functionality to other devices. > > I was obviously actively working on, had put time into, and was in > > the mist of discussing this with you. Then you just go ahead and code it > > (the easy part) yourself, essentially wasting my time. Surely there's > > some kind of etiquette surrounding such things? > > To be honest in this case I had expected to send the patch out much > sooner than I did - several priority interrupts stopped me testing it. > Like I say I realised that I really needed a fix and it seemed like the > quickest way to accomplish that was to just send the code. You only noticed it 2 days ago and I had a patch ready to go yesterday. I'm confused because I don't understand why would you even complain about it if you intended to work on it yourself? Surely, "Ah, I see an issue with xyz, patch to follow." Would have been more appropriate, instead of complaining about it, then I go and waste my time trying to fix something you intend on fixing yourself. I guess what's done is done now.
On Wed, Aug 22, 2012 at 12:55:25PM +0100, Lee Jones wrote: > > I *have* asked you to communicate more clearly about what you're doing > > but that doesn't mean to stop sending code, it means to have clearer > > words around what you're sending. > That's not how I interpreted your words: > "What you can do here is to commmunicate about what you're doing more. > Don't just think about the code, think about the communication > surrounding the code - this is the core of the issue." Just to be clear I'd include things like commit messages, cover letters and so on in the general area of communication. > I think it's great that you like the idea and want to extend that > functionality to other MFDs which perhaps don't support DT, or the > ones that do but don't want to provide compatible strings or device > nodes for all the MFD's child devices. But that is all we're doing > here. There was no breakage. It served a purpose and it worked well. > So well in fact that you've now provided the intended functionality > to other devices. I'm looking at this from the point of view of adding the compatible strings and then finding that the core starts remapping things in the case where you're probing from DT - and this behaviour will vary depending on the device tree that the user is using so the driver can't even make a decision based on if device tree is being used by the system. > You only noticed it 2 days ago and I had a patch ready to go yesterday. > I'm confused because I don't understand why would you even complain about > it if you intended to work on it yourself? Surely, "Ah, I see an issue with > xyz, patch to follow." Would have been more appropriate, instead of > complaining about it, then I go and waste my time trying to fix something > you intend on fixing yourself. I didn't originally intend to do anything, if I had done I'd just have sent a patch as you say. Originally I'd just noticed it as being an awkwardly designed interface at the wrong abstraction layer, it was only later on that I realised how it could blow up.
diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c index 7040a00..9899b3f 100644 --- a/drivers/mfd/db8500-prcmu.c +++ b/drivers/mfd/db8500-prcmu.c @@ -270,6 +270,8 @@ static struct { struct prcmu_fw_version version; } fw_info; +static struct irq_domain *db8500_irq_domain; + /* * This vector maps irq numbers to the bits in the bit field used in * communication with the PRCMU firmware. @@ -2583,7 +2585,7 @@ static void prcmu_irq_mask(struct irq_data *d) spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags); - mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE]; + mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->hwirq]; spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags); @@ -2597,7 +2599,7 @@ static void prcmu_irq_unmask(struct irq_data *d) spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags); - mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE]; + mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->hwirq]; spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags); @@ -2637,9 +2639,43 @@ static char *fw_project_name(u8 project) } } +int db8500_irq_get_virq(int irq) +{ + return irq_create_mapping(db8500_irq_domain, irq); +} +EXPORT_SYMBOL_GPL(db8500_irq_get_virq); + +static int db8500_irq_map(struct irq_domain *d, unsigned int virq, + irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(virq, &prcmu_irq_chip, + handle_simple_irq); + set_irq_flags(virq, IRQF_VALID); + + return 0; +} + +static struct irq_domain_ops db8500_irq_ops = { + .map = db8500_irq_map, + .xlate = irq_domain_xlate_twocell, +}; + +static int db8500_irq_init(struct device_node *np) +{ + db8500_irq_domain = irq_domain_add_legacy( + np, NUM_PRCMU_WAKEUPS, IRQ_PRCMU_BASE, + 0, &db8500_irq_ops, NULL); + + if (!db8500_irq_domain) { + pr_err("Failed to create irqdomain\n"); + return -ENOSYS; + } + + return 0; +} + void __init db8500_prcmu_early_init(void) { - unsigned int i; if (cpu_is_u8500v2()) { void *tcpm_base = ioremap_nocache(U8500_PRCMU_TCPM_BASE, SZ_4K); @@ -2683,16 +2719,6 @@ void __init db8500_prcmu_early_init(void) init_completion(&mb5_transfer.work); INIT_WORK(&mb0_transfer.mask_work, prcmu_mask_work); - - /* Initalize irqs. */ - for (i = 0; i < NUM_PRCMU_WAKEUPS; i++) { - unsigned int irq; - - irq = IRQ_PRCMU_BASE + i; - irq_set_chip_and_handler(irq, &prcmu_irq_chip, - handle_simple_irq); - set_irq_flags(irq, IRQF_VALID); - } } static void __init init_prcm_registers(void) @@ -2999,6 +3025,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev) goto no_irq_return; } + db8500_irq_init(np); + for (i = 0; i < ARRAY_SIZE(db8500_prcmu_devs); i++) { if (!strcmp(db8500_prcmu_devs[i].name, "ab8500-core")) { db8500_prcmu_devs[i].platform_data = ab8500_platdata; diff --git a/include/linux/mfd/db8500-prcmu.h b/include/linux/mfd/db8500-prcmu.h index b82f6ee..38494d9 100644 --- a/include/linux/mfd/db8500-prcmu.h +++ b/include/linux/mfd/db8500-prcmu.h @@ -571,6 +571,8 @@ u32 db8500_prcmu_read(unsigned int reg); void db8500_prcmu_write(unsigned int reg, u32 value); void db8500_prcmu_write_masked(unsigned int reg, u32 mask, u32 value); +int db8500_irq_get_virq(int irq); + #else /* !CONFIG_MFD_DB8500_PRCMU */ static inline void db8500_prcmu_early_init(void) {}
The PRCMU has its own USB, Thermal, GPIO, Modem, HSI and RTC drivers, amongst other things. This patch allows those subordinate devices to use it as an interrupt controller as and when they are DT enabled. CC: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/db8500-prcmu.c | 54 +++++++++++++++++++++++++++++--------- include/linux/mfd/db8500-prcmu.h | 2 ++ 2 files changed, 43 insertions(+), 13 deletions(-)