Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Mark Brown]

On Wed, Aug 01, 2012 at 07:05:15PM +0100, Anthony Olech wrote:

> if the irq_base is set to -1 when calling regmap_add_irq_chip()
> then allow the IRQ to be mapped even if the allocated irq_base
> is actually zero.

> This restores the behaviour seen in v3.4, and I assume that the
> tidy-ups just made in v3.5 INADVERTENTLY introduce this change
> in behaviour.

Please pay MORE attention to the changelog - obviously there's no
problem mapping automatically allocated IRQs, there's only any effect if
they happen to GET allocated at zero.

The only real issue I see with the current code is that if the user
explicitly wants to statically allocate an IRQ range at zero they can't.
The current intended behaviour is that we use a linear domain unless a
positive IRQ base is specified, though we're not quite doing that right
now as a transitional measure until drivers are updated.

The current da9052 driver usage seems to have quite a few problems, I do
recall having to fix some problems that make me doubt if it ever worked
well.  Looking at the code now I see it's using hard coded references to
absolute IRQ numbers which is an issue...  It should be being converted
to use regmap_irq_get_virq().

RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Opensource [Anthony Olech]]

Hi Mark,

I am not testing the da9052 driver, but the da9058 that I recently submitted.

The problem is that the auto allocated "irq_base" comes back as zero.

I repeat - in v3.4 the daa9058 driver work, but fails (without my patch) in v3.5

If my suggested fix is not acceptable, then is it possible to fix the bug in some other way?

thanks,

Tony Olech

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: 04 August 2012 11:54
To: Opensource [Anthony Olech]
Cc: LKML; Anthony Olech; David Dajun Chen
Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

On Wed, Aug 01, 2012 at 07:05:15PM +0100, Anthony Olech wrote:

> if the irq_base is set to -1 when calling regmap_add_irq_chip() then 
> allow the IRQ to be mapped even if the allocated irq_base is actually 
> zero.

> This restores the behaviour seen in v3.4, and I assume that the 
> tidy-ups just made in v3.5 INADVERTENTLY introduce this change in 
> behaviour.

Please pay MORE attention to the changelog - obviously there's no problem mapping automatically allocated IRQs, there's only any effect if they happen to GET allocated at zero.

The only real issue I see with the current code is that if the user explicitly wants to statically allocate an IRQ range at zero they can't.
The current intended behaviour is that we use a linear domain unless a positive IRQ base is specified, though we're not quite doing that right now as a transitional measure until drivers are updated.

The current da9052 driver usage seems to have quite a few problems, I do recall having to fix some problems that make me doubt if it ever worked well.  Looking at the code now I see it's using hard coded references to absolute IRQ numbers which is an issue...  It should be being converted to use regmap_irq_get_virq().

Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Mark Brown]

On Sun, Aug 05, 2012 at 07:57:19PM +0000, Opensource [Anthony Olech] wrote:
> Hi Mark,

Don't send top posted mails and fix your mail client ti wrod wrap within
paragraphs.

> The problem is that the auto allocated "irq_base" comes back as zero.

> I repeat - in v3.4 the daa9058 driver work, but fails (without my
> patch) in v3.5

> If my suggested fix is not acceptable, then is it possible to fix the
> bug in some other way?

To repeat, your driver is buggy, any irq_base should be specified by
platform data.  Your driver should stop doing whatever it is doing that
relies on having an irq_base.

RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Opensource [Anthony Olech]]

if you don't TOP POST how can you tell who wrote what?

see my comments embedded below

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: 04 August 2012 11:54
To: Opensource [Anthony Olech]
Cc: LKML; Anthony Olech; David Dajun Chen
Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

On Wed, Aug 01, 2012 at 07:05:15PM +0100, Anthony Olech wrote:

> if the irq_base is set to -1 when calling regmap_add_irq_chip() then 
> allow the IRQ to be mapped even if the allocated irq_base is actually 
> zero.

> This restores the behaviour seen in v3.4, and I assume that the 
> tidy-ups just made in v3.5 INADVERTENTLY introduce this change in 
> behaviour.

Please pay MORE attention to the changelog - obviously there's no problem mapping automatically allocated IRQs, there's only any effect if they happen to GET allocated at zero.

That is the problem - they are allocated at zero, and hence my patch

The only real issue I see with the current code is that if the user explicitly wants to statically allocate an IRQ range at zero they can't.

I don't want to explicitly allocate at zero.

The current intended behaviour is that we use a linear domain unless a positive IRQ base is specified, though we're not quite doing that right now as a transitional measure until drivers are updated.

The fact remains that my patch enables the DA9058 driver to work in v3.5

The current da9052 driver usage seems to have quite a few problems, I do recall having to fix some problems that make me doubt if it ever worked well.  Looking at the code now I see it's using hard coded references to absolute IRQ numbers which is an issue...  It should be being converted to use regmap_irq_get_virq().

Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Mark Brown]

On Tue, Aug 07, 2012 at 11:18:20AM +0000, Opensource [Anthony Olech] wrote:

> if you don't TOP POST how can you tell who wrote what?

Well, it's not clear who wrote what in your current e-mail since there's
no indication of what's quoted and what's new text...  Take a look at
all the other mails on the list - your mail should be in a similar style
to them.  There's some advice for common e-mail clients in email-clients.txt
under Documentation.

The bottom line here is that if your driver requires a dynamically
allocated legacy domain it is broken.

RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: 07 August 2012 15:03
> On Tue, Aug 07, 2012 at 11:18:20AM +0000, Opensource [Anthony Olech]
> wrote:
> > if you don't TOP POST how can you tell who wrote what?
> Well, it's not clear who wrote what in your current e-mail since there's no
> indication of what's quoted and what's new text...  Take a look at all the other
> mails on the list - your mail should be in a similar style to them.  There's some
> advice for common e-mail clients in email-clients.txt under Documentation.

I found the option to quote/indent the email original, sorry about that but we
are forced to use Microsoft Outlook and the default were set up strangely.
 
> The bottom line here is that if your driver requires a dynamically allocated
> legacy domain it is broken.

I am trying to use the latest REGMAP API, and I do not understand why you
say the DSA9058 driver "requires" a dynamically allocated legacy domain.
Surely the virtual IRQs that the PMIC component drivers use must be
dynamically allocated. It is only the single GPIO line designated as an
interrupt line in the machne drivert that is fixed by the hardware. That
surely means the "irq_base" parameter to regmap_add_irq_chip() must
be set to "-1". What else could it be set to??

I am beginning to suspect that I have misunderstood something. The
regmap-irq API seemed taylor-made for our PMIC with one real h/w
interrupt line with several PMIC chip irq sources controlled by a set
of registers that seemed to slot into the "regmap_add_irq_chip" struct
perfectly. Why should that set of virtual irqs be given a specific base??

I really hope that you can help me clear this issue up, as there are not
many examples of drivers that use the regmap-irq API in linux-release
GIT repository.

Tony Olech

Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Mark Brown]

On Tue, Aug 07, 2012 at 02:37:37PM +0000, Opensource [Anthony Olech] wrote:

> > The bottom line here is that if your driver requires a dynamically allocated
> > legacy domain it is broken.

> I am trying to use the latest REGMAP API, and I do not understand why you
> say the DSA9058 driver "requires" a dynamically allocated legacy domain.

If you care if you got a linear or a legacy domain then that shows
you're reyling on having a legacy domain (indeed, my statement above
should've been stronger - if anything in the driver itself cares if it's
got a linear or a legacy domain the driver is buggy).

> Surely the virtual IRQs that the PMIC component drivers use must be
> dynamically allocated. It is only the single GPIO line designated as an
> interrupt line in the machne drivert that is fixed by the hardware. That
> surely means the "irq_base" parameter to regmap_add_irq_chip() must
> be set to "-1". What else could it be set to??

If the driver doesn't have any inputs which could be used as an
interrupt by another device then it should be set to -1, yes, and
nothing in the code should ever care how the specific virq values are
related to each other.

If the driver does support another device using it as an interrupt
controller then unfortunately for non-DT systems platform data would
need to configure an irq_base so that the interrupt can be supplied to
whatever the other device is but in all other circumstances it should be
set to -1.

> I am beginning to suspect that I have misunderstood something. The
> regmap-irq API seemed taylor-made for our PMIC with one real h/w
> interrupt line with several PMIC chip irq sources controlled by a set
> of registers that seemed to slot into the "regmap_add_irq_chip" struct
> perfectly. Why should that set of virtual irqs be given a specific base??

It shouldn't, this is what I'm saying.  The IRQs shouldn't have a base
at all and should instead be using a linear domain (which doesn't have
a base but instead maps each IRQ on demand).  What your patch does is to
stop that happening and instead always allocate a legacy domain even
when linear is OK.

It sounds like your code to allocate the IRQs is fine but the code using
the IRQs is buggy as it's relying on the linear domain.

RE: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: 07 August 2012 18:02
> To: Opensource [Anthony Olech]
> Cc: LKML; David Dajun Chen
> Subject: Re: [PATCH] regmap-irq: allow auto-allocated IRQs to be mapped
> On Tue, Aug 07, 2012 at 02:37:37PM +0000, Opensource [Anthony Olech]
> wrote: 
> > > The bottom line here is that if your driver requires a dynamically
> > > allocated legacy domain it is broken.
> > I am trying to use the latest REGMAP API, and I do not understand why
> > you say the DSA9058 driver "requires" a dynamically allocated legacy
> domain.
> If you care if you got a linear or a legacy domain then that shows you're reyling
> on having a legacy domain (indeed, my statement above should've been
> stronger - if anything in the driver itself cares if it's got a linear or a legacy
> domain the driver is buggy).
> > Surely the virtual IRQs that the PMIC component drivers use must be
> > dynamically allocated. It is only the single GPIO line designated as
> > an interrupt line in the machne drivert that is fixed by the hardware.
> > That surely means the "irq_base" parameter to regmap_add_irq_chip()
> > must be set to "-1". What else could it be set to??
> If the driver doesn't have any inputs which could be used as an interrupt by
> another device then it should be set to -1, yes, and nothing in the code should
> ever care how the specific virq values are related to each other.
> If the driver does support another device using it as an interrupt controller then
> unfortunately for non-DT systems platform data would need to configure an
> irq_base so that the interrupt can be supplied to whatever the other device is
> but in all other circumstances it should be set to -1.
> > I am beginning to suspect that I have misunderstood something. The
> > regmap-irq API seemed taylor-made for our PMIC with one real h/w
> > interrupt line with several PMIC chip irq sources controlled by a set
> > of registers that seemed to slot into the "regmap_add_irq_chip" struct
> > perfectly. Why should that set of virtual irqs be given a specific base??
> It shouldn't, this is what I'm saying.  The IRQs shouldn't have a base at all and
> should instead be using a linear domain (which doesn't have a base but instead
> maps each IRQ on demand).  What your patch does is to stop that happening
> and instead always allocate a legacy domain even when linear is OK.
> It sounds like your code to allocate the IRQs is fine but the code using the IRQs
> is buggy as it's relying on the linear domain.

Thanks Mark, now we are getting somewhere - it must be my use of the IRQ domain
IRQs is wrong. There are exactly 3 mfd drivers that specify a base of '-1', namely:
	palmas.c, 88pm805.c, arizona-irq.c
so I will examine how they use the IRQs

thanks for your help
Tony Olech

[PATCH] regmap-irq: allow auto-allocated IRQs to be mapped

[Anthony Olech]

if the irq_base is set to -1 when calling regmap_add_irq_chip()
then allow the IRQ to be mapped even if the allocated irq_base
is actually zero.

This restores the behaviour seen in v3.4, and I assume that the
tidy-ups just made in v3.5 INADVERTENTLY introduce this change
in behaviour.

Signed-off-by: Anthony Olech (Opensource) <anthony.olech.opensource@diasemi.com>
---
 drivers/base/regmap/regmap-irq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index a897346..2441785 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -238,6 +238,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	struct regmap_irq_chip_data *d;
 	int i;
 	int ret = -ENOMEM;
+	int orig_irq_base = irq_base;
 
 	for (i = 0; i < chip->num_irqs; i++) {
 		if (chip->irqs[i].reg_offset % map->reg_stride)
@@ -313,7 +314,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 		}
 	}
 
-	if (irq_base)
+	if (irq_base || orig_irq_base)
 		d->domain = irq_domain_add_legacy(map->dev->of_node,
 						  chip->num_irqs, irq_base, 0,
 						  &regmap_domain_ops, d);
-- 
end-of-patch for regmap-irq-bug-fix