[PATCH 2/3] PNP: add pnp_build_option() to the API

[PATCH 2/3] PNP: add pnp_build_option() to the API

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

Hi Bjorn.

The subsequent AD181x quirk patch would like this as part of the API.
pnp_register_dependent_option() adds to the same dependent chain the
quirk is walking which is fairly unclean. This enables a private
option chain build which it can then just add onto the end when done.

Signed-off-by: Rene Herman <rene.herman@gmail.com>


[-- Attachment #2: pnp-add-pnp_build_option-to-the-api.diff --]
[-- Type: text/plain, Size: 1532 bytes --]

commit 1e16678158431ad9260acbd9ff3e43468b67ab6d
Author: Rene Herman <rene.herman@gmail.com>
Date:   Tue May 6 01:54:43 2008 +0200

    PNP: add pnp_build_option() to the API
    
    The subsequent AD181x quirk patch would like this as part of the API.
    pnp_register_dependent_option() adds to the same dependent chain the
    quirk is walking which is fairly unclean. This enables a private
    option chain build which it can then just add onto the end when done.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/drivers/pnp/base.h b/drivers/pnp/base.h
index 4fe7c58..886dac8 100644
--- a/drivers/pnp/base.h
+++ b/drivers/pnp/base.h
@@ -19,6 +19,7 @@ void pnp_remove_card(struct pnp_card *card);
 int pnp_add_card_device(struct pnp_card *card, struct pnp_dev *dev);
 void pnp_remove_card_device(struct pnp_dev *dev);
 
+struct pnp_option *pnp_build_option(int priority);
 struct pnp_option *pnp_register_independent_option(struct pnp_dev *dev);
 struct pnp_option *pnp_register_dependent_option(struct pnp_dev *dev,
 						 int priority);
diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
index 2041620..390b500 100644
--- a/drivers/pnp/resource.c
+++ b/drivers/pnp/resource.c
@@ -28,7 +28,7 @@ static int pnp_reserve_mem[16] = {[0 ... 15] = -1 };	/* reserve (don't use) some
  * option registration
  */
 
-static struct pnp_option *pnp_build_option(int priority)
+struct pnp_option *pnp_build_option(int priority)
 {
 	struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));
 

[PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

Hi Bjorn.

The AD181x chip doesn't support an IRQ-less MPU401 option but works
fine without one. This adds (priority: functional) IRQ-less options
for each port option to help systems with few available IRQs.

The AD1815 quirk can't use pnp_register_irq_resource() due to doubly
penalizing the IRQ. Also, while not a practical issue due to no IRQ
option being present for the dependents, this needs to add in front,
not back.

Doesn't use pnp_register_port_resource() for symetry with above.

This does not delete the AD1815 independent option even though it
should be empty after the IRQ transfer due to AD1816 coming with an
empty but still present independent option by default.

Was tested on AD1815 and AD1816. The ALSA driver also support the
AZT2002 ID for MPU401 but this doesn't as I was unable to test it.

Signed-off-by: Rene Herman <rene.herman@gmail.com>


[-- Attachment #2: pnp-add-ad1815-and-ad1816-quirks.diff --]
[-- Type: text/plain, Size: 3901 bytes --]

commit effa66095a52cecd415c3f34043ed505561a5139
Author: Rene Herman <rene.herman@gmail.com>
Date:   Tue May 6 02:07:59 2008 +0200

    PNP: add AD1815 and AD1816 quirks.
    
    The AD181x chip doesn't support an IRQ-less MPU401 option but works
    fine without one. This adds (priority: functional) IRQ-less options
    for each port option to help systems with few available IRQs.
    
    The AD1815 quirk can't use pnp_register_irq_resource() due to doubly
    penalizing the IRQ. Also, while not a practical issue due to no IRQ
    option being present for the dependents, this needs to add in front,
    not back.
    
    Doesn't use pnp_register_port_resource() for symetry with above.
    
    This does not delete the AD1815 independent option even though it
    should be empty after the IRQ transfer due to AD1816 coming with an
    empty but still present independent option by default.
    
    Was tested on AD1815 and AD1816. The ALSA driver also support the
    AZT2002 ID for MPU401 but this doesn't as I was unable to test it.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
index 10b1295..e4b049c 100644
--- a/drivers/pnp/quirks.c
+++ b/drivers/pnp/quirks.c
@@ -108,6 +108,114 @@ static void quirk_sb16audio_resources(struct pnp_dev *dev)
 		       "pnp: SB audio device quirk - increasing port range\n");
 }
 
+static struct pnp_option *quirk_ad181x_mpu_resources(struct pnp_dev *dev)
+{
+	struct pnp_option *head = NULL;
+	struct pnp_option *prev = NULL;
+	struct pnp_option *res;
+
+	/*
+	 * Build a functional IRQ-less variant of each MPU option.
+	 */
+
+	for (res = dev->dependent; res; res = res->next) {
+		struct pnp_option *curr;
+		struct pnp_port *port;
+		struct pnp_port *copy;
+
+		port = res->port;
+		if (!port || !res->irq)
+			continue;
+
+		copy = pnp_alloc(sizeof *copy);
+		if (!copy)
+			break;
+
+		copy->min = port->min;
+		copy->max = port->max;
+		copy->align = port->align;
+		copy->size = port->size;
+		copy->flags = port->flags;
+
+		curr = pnp_build_option(PNP_RES_PRIORITY_FUNCTIONAL);
+		if (!curr) {
+			kfree(copy);
+			break;
+		}
+		curr->port = copy;
+
+		if (prev)
+			prev->next = curr;
+		else
+			head = curr;
+		prev = curr;
+	}
+	return head;
+}
+
+static void quirk_ad1815_mpu_resources(struct pnp_dev *dev)
+{
+	struct pnp_option *res;
+	struct pnp_irq *irq;
+
+	/*
+	 * Distribute the independent IRQ over the dependent options
+	 */
+
+	res = dev->independent;
+	if (!res)
+		return;
+
+	irq = res->irq;
+	if (!irq || irq->next)
+		return;
+
+	res = dev->dependent;
+	if (!res)
+		return;
+
+	while (1) {
+		struct pnp_irq *copy;
+
+		copy = pnp_alloc(sizeof *copy);
+		if (!copy)
+			return;
+
+		memcpy(copy->map, irq->map, sizeof copy->map);
+		copy->flags = irq->flags;
+
+		copy->next = res->irq; /* Yes, this is NULL */
+		res->irq = copy;
+
+		if (!res->next)
+			break;
+		res = res->next;
+	}
+	kfree(irq);
+
+	res->next = quirk_ad181x_mpu_resources(dev);
+
+	res = dev->independent;
+	res->irq = NULL;
+
+	printk(KERN_INFO "pnp: adding IRQ-less AD1815 MPU options\n");
+}
+
+static void quirk_ad1816_mpu_resources(struct pnp_dev *dev)
+{
+	struct pnp_option *res;
+
+	res = dev->dependent;
+	if (!res)
+		return;
+
+	while (res->next)
+		res = res->next;
+
+	res->next = quirk_ad181x_mpu_resources(dev);
+
+	printk(KERN_INFO "pnp: adding IRQ-less AD1816 MPU options\n");
+}
 
 #include <linux/pci.h>
 
@@ -202,6 +310,10 @@ static struct pnp_fixup pnp_fixups[] = {
 	{"CTL0043", quirk_sb16audio_resources},
 	{"CTL0044", quirk_sb16audio_resources},
 	{"CTL0045", quirk_sb16audio_resources},
+	/* Add IRQ-less AD1815 MPU options */
+	{"ADS7151", quirk_ad1815_mpu_resources},
+	/* Add IRQ-less AD1816 MPU options */
+	{"ADS7181", quirk_ad1816_mpu_resources},
 	{"PNP0c01", quirk_system_pci_resources},
 	{"PNP0c02", quirk_system_pci_resources},
 	{""}

Re: [PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Bjorn Helgaas]

On Monday 05 May 2008 07:08:19 pm Rene Herman wrote:
> The AD181x chip doesn't support an IRQ-less MPU401 option but works
> fine without one. This adds (priority: functional) IRQ-less options
> for each port option to help systems with few available IRQs.
> 
> The AD1815 quirk can't use pnp_register_irq_resource() due to doubly
> penalizing the IRQ. Also, while not a practical issue due to no IRQ
> option being present for the dependents, this needs to add in front,
> not back.
> 
> Doesn't use pnp_register_port_resource() for symetry with above.
> 
> This does not delete the AD1815 independent option even though it
> should be empty after the IRQ transfer due to AD1816 coming with an
> empty but still present independent option by default.
> 
> Was tested on AD1815 and AD1816. The ALSA driver also support the
> AZT2002 ID for MPU401 but this doesn't as I was unable to test it.
> 
> Signed-off-by: Rene Herman <rene.herman@gmail.com>

I'm not opposed to this in principle, but I don't understand
it well enough to really have an informed opinion.

IIRC, we give up some seldom-used functionality when running
the MPU401 without an IRQ.  Does the driver mention the fact
that there's something it can't do because we don't have an IRQ?
The user might like to know in case he needs that functionality
and wants to fiddle with other devices to free up IRQs.

I'm vaguely uncomfortable because this quirk isn't really working
around a hardware/firmware issue; it's stepping around the fact
that Linux doesn't know how to allocate IRQs between ISA and PCI.
Does anybody know how Windows handles this?  If Windows can do it
without user intervention, maybe we can too.

If we continue down the quirk path, would you mind using dev_info()
instead of plain printk()?

Bjorn

Re: [PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Rene Herman]

On 06-05-08 21:08, Bjorn Helgaas wrote:

> On Monday 05 May 2008 07:08:19 pm Rene Herman wrote:

>> The AD181x chip doesn't support an IRQ-less MPU401 option but works
>> fine without one. This adds (priority: functional) IRQ-less options
>> for each port option to help systems with few available IRQs.
>>
>> The AD1815 quirk can't use pnp_register_irq_resource() due to doubly
>> penalizing the IRQ. Also, while not a practical issue due to no IRQ
>> option being present for the dependents, this needs to add in front,
>> not back.
>>
>> Doesn't use pnp_register_port_resource() for symetry with above.
>>
>> This does not delete the AD1815 independent option even though it
>> should be empty after the IRQ transfer due to AD1816 coming with an
>> empty but still present independent option by default.
>>
>> Was tested on AD1815 and AD1816. The ALSA driver also support the
>> AZT2002 ID for MPU401 but this doesn't as I was unable to test it.
>>
>> Signed-off-by: Rene Herman <rene.herman@gmail.com>
> 
> I'm not opposed to this in principle, but I don't understand
> it well enough to really have an informed opinion.
> 
> IIRC, we give up some seldom-used functionality when running
> the MPU401 without an IRQ.

Not even that in fact. When assigned one, the MPU401 IRQ is used for MIDI 
recording but the ALSA MPU401 driver can record without problem without an 
assigned IRQ as well...

General information, in case it's useful; the MPU401 is the UART that sits 
behind that 15-pin joystick/midi connector on the back of (older, by now) 
soundcards.

If so inclined, you'd attach for example an external MIDI keyboard (keyboard 
in the piano sense) to it after which you can record what you play and/or 
relay the data to a running software synthesizer to turn your keypresses 
into sound.

The difference between an assigned IRQ and no assigned IRQ is (I suppose, I 
should say) just that in the latter case you might experience some worse 
latency due to the polled operation.

However, not many people have external MIDI gear hooked up to an MPU401 in 
the first place and of those that do, none are still using old ISA cards; 
especially not those that care at all about latency. Those people are in 
fact very explicitly likely to use fairly expensive equipment specifically 
for low latency purposes.

(and modern keyboards connect through USB, not MPU401).

The other direction, playing MIDI _to_ the MPU401, is a bit more applicable 
to old hardware still due to soundcards with their own onboard wavetable 
synthesizer that takes its input from the MPU401 ("hardware MIDI") but in 
that direction, nothing changes at all, not even latency.

> Does the driver mention the fact that there's something it can't do
> because we don't have an IRQ? The user might like to know in case he
> needs that functionality and wants to fiddle with other devices to free
> up IRQs.

So no, there's nothing it can't do without an IRQ and ALSA has always been 
fine with IRQ-less MPU401. It's just that normally the hardware exposes an 
IRQ-less option itself so that the device enables without problem even when 
no IRQ is available for it while the AD181x neglects to do so so that the 
PnP layer complains that it can't enable the device when it's not finding a 
free IRQ -- the situation that drew the complaint/report that led to this 
thread.

Moreover, for any readers, please note that the patch doesn't make the 
MPU401 operate without an IRQ -- it just provides it with the last resort 
_option_ of functioning without one if none are available (or if the user 
specifically picks that option manually ofcourse).

> I'm vaguely uncomfortable because this quirk isn't really working
> around a hardware/firmware issue; it's stepping around the fact
> that Linux doesn't know how to allocate IRQs between ISA and PCI.
> Does anybody know how Windows handles this?  If Windows can do it
> without user intervention, maybe we can too.

Yes, I can appreciate the vague uncomfort. This is improving the hardware 
more then it is fixing it and that might feel like a bit of a misuse of 
quirks. However, given Linux's ability to operate the MPU401 fully without 
an assigned IRQ I feel it does make sense. The hardware shouldn't become 
unavailable just because the system can't find a free IRQ for it.

As such, it's not really stepping around much of anything; it's just being 
friendly. Do feel it makes sense. Also feel it makes sense to treat an ISA 
card that insists on an MPU401 IRQ to a ride in a blender but there are in 
fact some nice-ish AD1816A cards such as the TerraTec EWS64S still around.

However, with respect to your direct worry I get sort of foggy. All my ISA 
capable systems seem to have a BIOS that assigns IRQs to each PCI card that 
I put in as evidenced by the neat list of them that most of them print out 
just before the screen says "Verifying DMI Pool Data" and loading linux, an 
OS which I don't recall having ever seen deviate from the BIOS assigned 
values either.

That is, I'm not sure in how far Linux is involved at all :-?

> If we continue down the quirk path, would you mind using dev_info()
> instead of plain printk()?

Yes. Will regenerate against current mainline. My ISA test machine is 
chugging away on things at the moment. Will repost when it's done.

Rene.

Re: [PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Bjorn Helgaas]

On Tuesday 06 May 2008 03:06:14 pm Rene Herman wrote:
> ...
> > I'm vaguely uncomfortable because this quirk isn't really working
> > around a hardware/firmware issue; it's stepping around the fact
> > that Linux doesn't know how to allocate IRQs between ISA and PCI.
> > Does anybody know how Windows handles this?  If Windows can do it
> > without user intervention, maybe we can too.
> 
> Yes, I can appreciate the vague uncomfort. This is improving the hardware 
> more then it is fixing it and that might feel like a bit of a misuse of 
> quirks. However, given Linux's ability to operate the MPU401 fully without 
> an assigned IRQ I feel it does make sense. The hardware shouldn't become 
> unavailable just because the system can't find a free IRQ for it.

OK, thanks a lot for the explanation.  It makes a lot more sense
now, and I'm happy with your approach now.  It shouldn't make anything
worse, and it should make the card useful in some cases where it
wasn't before.

> However, with respect to your direct worry I get sort of foggy. All my ISA 
> capable systems seem to have a BIOS that assigns IRQs to each PCI card that 
> I put in as evidenced by the neat list of them that most of them print out 
> just before the screen says "Verifying DMI Pool Data" and loading linux, an 
> OS which I don't recall having ever seen deviate from the BIOS assigned 
> values either.
> 
> That is, I'm not sure in how far Linux is involved at all :-?

This part is just my general unhappiness about options like
"acpi_irq_nobalance" and "acpi_irq_isa=".  I was just wondering
what Windows does when it can't assign PNP IRQs.  Does it assign
them all before PCI, so they know they have enough?  Do it
dynamically move PCI interrupts?  Do it require explicit boot
flags like Linux does?  Or maybe nobody even needs those flags
on Linux, and the flags are effectively unused.

Bjorn

Re: [PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Rene Herman]

On 06-05-08 23:27, Bjorn Helgaas wrote:

> This part is just my general unhappiness about options like
> "acpi_irq_nobalance" and "acpi_irq_isa=".  I was just wondering
> what Windows does when it can't assign PNP IRQs.  Does it assign
> them all before PCI, so they know they have enough?  Do it
> dynamically move PCI interrupts?  Do it require explicit boot
> flags like Linux does?  Or maybe nobody even needs those flags
> on Linux, and the flags are effectively unused.

That's for Len. I haven't used Windows in eight or nine years now and am 
consequently progressively ignorant of it.

Rene.

Re: [PATCH 3/3] PNP: add AD1815 and AD1816 quirks

[Rene Herman]

On 06-05-08 23:06, Rene Herman wrote:

> Yes. Will regenerate against current mainline. My ISA test machine is 
> chugging away on things at the moment. Will repost when it's done.

Ofcourse, on .26-rc1 nothing actually works anymore. One of its 1643 other 
regressions is getting in the way I presume. It's quite impossible to test 
-rc1 kernels on something that actually takes some time to do so...

And especially .26-rc1. Anyways, will need some time. Can also not test your 
dynamic resource code on that machine until then.

Rene.

[PATCH 1/3] PNP: cleanup pnp_fixup_device()

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

Hi Bjorn.

In response to off-list AD181x thread, small cleanup to quirk handling.

Patch 3/3 does the actual work. Mind if I feed this stuff through you? And,
mind this, period? Not wed to it or anything...

Signed-off-by: Rene Herman <rene.herman@gmail.com>


[-- Attachment #2: pnp-cleanup-pnp_fixup_device.diff --]
[-- Type: text/plain, Size: 1067 bytes --]

commit 4ed0cb4cef2b45a257b6d7d6cdc987e3511ddf58
Author: Rene Herman <rene.herman@gmail.com>
Date:   Tue May 6 01:51:47 2008 +0200

    PNP: cleanup pnp_fixup_device()
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
index 3f2d34a..10b1295 100644
--- a/drivers/pnp/quirks.c
+++ b/drivers/pnp/quirks.c
@@ -209,20 +209,12 @@ static struct pnp_fixup pnp_fixups[] = {
 
 void pnp_fixup_device(struct pnp_dev *dev)
 {
-	int i = 0;
-	void (*quirk)(struct pnp_dev *);
-
-	while (*pnp_fixups[i].id) {
-		if (compare_pnp_id(dev->id, pnp_fixups[i].id)) {
-			quirk = pnp_fixups[i].quirk_function;
-
-#ifdef DEBUG
-			dev_dbg(&dev->dev, "calling quirk 0x%p", quirk);
-			print_fn_descriptor_symbol(": %s()\n",
-				(unsigned long) *quirk);
-#endif
-			(*quirk)(dev);
-		}
-		i++;
+	int i;
+
+	for (i = 0; *pnp_fixups[i].id; i++) {
+		if (!compare_pnp_id(dev->id, pnp_fixups[i].id))
+			continue;
+		dev_dbg(&dev->dev, "calling quirk for %s\n", pnp_fixups[i].id);
+		pnp_fixups[i].quirk_function(dev);
 	}
 }

Re: [PATCH 1/3] PNP: cleanup pnp_fixup_device()

[Bjorn Helgaas]

On Monday 05 May 2008 07:08:09 pm Rene Herman wrote:

[It's easier to comment if the patch is inline rather than attached.]

> PNP: cleanup pnp_fixup_device()
>    
>     Signed-off-by: Rene Herman <rene.herman@gmail.com>
>
> diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
> index 3f2d34a..10b1295 100644
> --- a/drivers/pnp/quirks.c
> +++ b/drivers/pnp/quirks.c
> @@ -209,20 +209,12 @@ static struct pnp_fixup pnp_fixups[] = {
>  
>  void pnp_fixup_device(struct pnp_dev *dev)
>  {
> -       int i = 0;
> -       void (*quirk)(struct pnp_dev *);
> -
> -       while (*pnp_fixups[i].id) {
> -               if (compare_pnp_id(dev->id, pnp_fixups[i].id)) {
> -                       quirk = pnp_fixups[i].quirk_function;
> -
> -#ifdef DEBUG
> -                       dev_dbg(&dev->dev, "calling quirk 0x%p", quirk);
> -                       print_fn_descriptor_symbol(": %s()\n",
> -                               (unsigned long) *quirk); 
> -#endif
> -                       (*quirk)(dev);
> -               }
> -               i++;
> +       int i;
> +
> +       for (i = 0; *pnp_fixups[i].id; i++) {
> +               if (!compare_pnp_id(dev->id, pnp_fixups[i].id))
> +                       continue;
> +               dev_dbg(&dev->dev, "calling quirk for %s\n", pnp_fixups[i].id);
> +               pnp_fixups[i].quirk_function(dev);
>         }
>  }

I'm sort of attached to keeping this similar to pci_do_fixups()
and keeping the symbol, even though the #ifdef is ugly.  I do
like getting rid of the "0x%p" and adding the PNP ID.

[PATCH 2/3] PNP: add pnp_build_option() to the API

[Rene Herman]

Hi Bjorn,

===
PNP: add pnp_build_option() to the API

The subsequent ISAPnP MPU quirk patch would like this as part of the
API. pnp_register_dependent_option() adds to the same dependent chain
the quirk is walking which is fairly unclean. This enables a private
option chain build which it can then just add onto the end when done.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
drivers/pnp/base.h     |    1 +
drivers/pnp/resource.c |    2 +-
2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/pnp/base.h b/drivers/pnp/base.h
index 4fe7c58..886dac8 100644
--- a/drivers/pnp/base.h
+++ b/drivers/pnp/base.h
@@ -19,6 +19,7 @@ void pnp_remove_card(struct pnp_card *card);
int pnp_add_card_device(struct pnp_card *card, struct pnp_dev *dev);
void pnp_remove_card_device(struct pnp_dev *dev);

+struct pnp_option *pnp_build_option(int priority);
struct pnp_option *pnp_register_independent_option(struct pnp_dev *dev);
struct pnp_option *pnp_register_dependent_option(struct pnp_dev *dev,
						 int priority);
diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
index 2041620..390b500 100644
--- a/drivers/pnp/resource.c
+++ b/drivers/pnp/resource.c
@@ -28,7 +28,7 @@ static int pnp_reserve_mem[16] = {[0 ... 15] = -1 };	/* reserve (don't use) some
* option registration
*/

-static struct pnp_option *pnp_build_option(int priority)
+struct pnp_option *pnp_build_option(int priority)
{
	struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));

-- 
1.5.2.2