[PATCH] opti931: additional check for OPL3 device

[PATCH] opti931: additional check for OPL3 device

[Krzysztof Helt]

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

From: Krzysztof Helt <krzysztof.h1@wp.pl>

This patch adds additional check for OPL3 device. I found that PNP
region returned
by the card is 0x380 - 0x38f but the OPL3 device is located at 0x388
(standard FM
device address). The driver checks start of the range (0x380) which is
incorrect.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
---

The same problem appears on two cards I have with Opti chipset:
opti931 and opti933.
If someone can test if the opti92x PnP has the same problem I would be grateful.

Regards,
Krzysztof

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: opti-fm.diff --]
[-- Type: text/x-patch; name="opti-fm.diff", Size: 1219 bytes --]

diff -urp linux-2.6.22.old/sound/isa/opti9xx/opti92x-ad1848.c linux-2.6.22/sound/isa/opti9xx/opti92x-ad1848.c
--- linux-2.6.22.old/sound/isa/opti9xx/opti92x-ad1848.c	2007-09-08 13:37:59.000000000 +0200
+++ linux-2.6.22/sound/isa/opti9xx/opti92x-ad1848.c	2007-09-13 23:54:02.000000000 +0200
@@ -1890,12 +1890,26 @@ static int __devinit snd_opti9xx_probe(s
 			}
 		}
 #endif	/* !OPTi93X */
-		if (!opl3 && snd_opl3_create(card,
-					     chip->fm_port,
-					     chip->fm_port + 2,
-					     OPL3_HW_AUTO, 0, &opl3) < 0) {
-			snd_printk(KERN_WARNING "no OPL device at 0x%lx-0x%lx\n",
-				   chip->fm_port, chip->fm_port + 4 - 1);
+		if (!opl3) {
+			error = snd_opl3_create(card,
+						chip->fm_port,
+						chip->fm_port + 2,
+						OPL3_HW_AUTO, 0, &opl3);
+			if (error < 0 && chip->fm_port == 0x380) {
+				error = snd_opl3_create(card,
+							chip->fm_port + 8,
+							chip->fm_port + 10,
+							OPL3_HW_AUTO, 0, &opl3);
+			}
+			if (error < 0) {
+				snd_printk(KERN_WARNING
+					   "no OPL device at 0x%lx-0x%lx "
+					   "or 0x%lx-0x%lx\n",
+					   chip->fm_port,
+					   chip->fm_port + 4 - 1,
+					   chip->fm_port + 8,
+					   chip->fm_port + 12 - 1);
+			}
 		}
 		if (opl3) {
 #ifdef CS4231

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] opti931: additional check for OPL3 device

[Clemens Ladisch]

Krzysztof Helt wrote:
> This patch adds additional check for OPL3 device. I found that PNP
> region returned by the card is 0x380 - 0x38f but the OPL3 device is
> located at 0x388 (standard FM device address).

On cards with an OPL4, it would use the entire range, where that last
eight bytes are the OPL3-compatible registers.

AFAIK the opti93x chips have an integrated OPL3 and cannot be used
with an OPL4.

> If someone can test if the opti92x PnP has the same problem I would be
> grateful.

My card doesn't have PnP, but I guess the OPL4 detection wouldn't work
on 92x either for the same reason.


Regards,
Clemens

Re: [PATCH] opti931: additional check for OPL3 device

[Krzysztof Helt]

On Fri, 14 Sep 2007 16:52:47 +0200
"Clemens Ladisch" <cladisch@fastmail.net> wrote:

> Krzysztof Helt wrote:
> > This patch adds additional check for OPL3 device. I found that PNP
> > region returned by the card is 0x380 - 0x38f but the OPL3 device is
> > located at 0x388 (standard FM device address).
> 
> On cards with an OPL4, it would use the entire range, where that last
> eight bytes are the OPL3-compatible registers.
> 

Ok. I haven't touched the size of the range or the range itself. I just added
a fallback to check range +8 (which is 0x388) after failure to find the OPL3
at the 0x380.

> 
> My card doesn't have PnP, but I guess the OPL4 detection wouldn't work
> on 92x either for the same reason.
> 
> 

Should the OPL4 detection has the same fallback or just all Opti cards
should search the FM chip at the range + 8 address?

Regards,
Krzysztof

Re: [PATCH] opti931: additional check for OPL3 device

[Clemens Ladisch]

Krzysztof Helt wrote:
> "Clemens Ladisch" <cladisch@fastmail.net> wrote:
> > Krzysztof Helt wrote:
> > > This patch adds additional check for OPL3 device. I found that PNP
> > > region returned by the card is 0x380 - 0x38f but the OPL3 device is
> > > located at 0x388 (standard FM device address).
> > 
> > On cards with an OPL4, it would use the entire range, where that last
> > eight bytes are the OPL3-compatible registers.
> 
> Ok. I haven't touched the size of the range or the range itself. I just added
> a fallback to check range +8 (which is 0x388) after failure to find the OPL3
> at the 0x380.
> 
> > My card doesn't have PnP, but I guess the OPL4 detection wouldn't work
> > on 92x either for the same reason.
> 
> Should the OPL4 detection has the same fallback or just all Opti cards
> should search the FM chip at the range + 8 address?

It appears a fallback shoudn't be needed because the chip always returns
a range suitable for an OPL4.

Since an OPL3 range is never larger than 8 bytes, and OPL4 needs 16
bytes, we could add 8 to get the FM address _if_ the range is at least
16 bytes; thus we are safe even if a chip publishes an OPL3-only range.


Regards,
Clemens

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

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

On 09/17/2007 10:14 AM, Clemens Ladisch wrote:

> Krzysztof Helt wrote:

>> Should the OPL4 detection has the same fallback or just all Opti cards
>> should search the FM chip at the range + 8 address?
> 
> It appears a fallback shoudn't be needed because the chip always returns 
> a range suitable for an OPL4.
> 
> Since an OPL3 range is never larger than 8 bytes, and OPL4 needs 16 
> bytes, we could add 8 to get the FM address _if_ the range is at least 16
> bytes; thus we are safe even if a chip publishes an OPL3-only range.

At least 16 won't do -- the OPTi chip provides a 12-byte range. More than 8 
would work, but just +8 is fine. I have all OPTi ISA-PnP chips (924, 925, 
931, 933) and more aren't going to be produced. The 0x380 base is what it 
calls the OPL4Base and ALBase (AdLib) is at 0x388.

The chip can defer to external FM it seems, but even if there are OPTi/OPL4 
combo's out there in the wild (I sort of doubt it) the driver's just doing 
opl3 anyway. ie, still needs just the +8.

Rene.

[-- Attachment #2: opti9xx-fm_port.diff --]
[-- Type: text/plain, Size: 652 bytes --]

diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c
index 60c120f..8bda47a 100644
--- a/sound/isa/opti9xx/opti92x-ad1848.c
+++ b/sound/isa/opti9xx/opti92x-ad1848.c
@@ -1732,11 +1732,11 @@ static int __devinit snd_card_opti9xx_pnp(struct snd_opti9xx *chip,
 
 #ifdef OPTi93X
 	port = pnp_port_start(pdev, 0) - 4;
-	fm_port = pnp_port_start(pdev, 1);
+	fm_port = pnp_port_start(pdev, 1) + 8;
 #else
 	if (pid->driver_data != 0x0924)
 		port = pnp_port_start(pdev, 1);
-	fm_port = pnp_port_start(pdev, 2);
+	fm_port = pnp_port_start(pdev, 2) + 8;
 #endif	/* OPTi93X */
 	irq = pnp_irq(pdev, 0);
 	dma1 = pnp_dma(pdev, 0);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] opti931: additional check for OPL3 device

[Clemens Ladisch]

Rene Herman wrote:
> On 09/17/2007 10:14 AM, Clemens Ladisch wrote:
> > Krzysztof Helt wrote:
> >> Should the OPL4 detection has the same fallback or just all Opti cards
> >> should search the FM chip at the range + 8 address?
> > 
> > It appears a fallback shoudn't be needed because the chip always returns 
> > a range suitable for an OPL4.
> > 
> > Since an OPL3 range is never larger than 8 bytes, and OPL4 needs 16 
> > bytes, we could add 8 to get the FM address _if_ the range is at least 16
> > bytes; thus we are safe even if a chip publishes an OPL3-only range.
> 
> At least 16 won't do -- the OPTi chip provides a 12-byte range. More than 8 
> would work, but just +8 is fine. I have all OPTi ISA-PnP chips (924, 925, 
> 931, 933) and more aren't going to be produced. The 0x380 base is what it 
> calls the OPL4Base and ALBase (AdLib) is at 0x388.

Are there chips that have both OPL4Base and ALBase?

> The chip can defer to external FM it seems, but even if there are OPTi/OPL4 
> combo's out there in the wild (I sort of doubt it)

Why?

> the driver's just doing opl3 anyway. ie, still needs just the +8.

Only the 93x driver; the 92x driver has OPL4 support.


Regards,
Clemens

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

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

On 09/17/2007 12:39 PM, Clemens Ladisch wrote:

> Rene Herman wrote:

>> At least 16 won't do -- the OPTi chip provides a 12-byte range. More
>> than 8 would work, but just +8 is fine. I have all OPTi ISA-PnP chips
>> (924, 925, 931, 933) and more aren't going to be produced. The 0x380
>> base is what it calls the OPL4Base and ALBase (AdLib) is at 0x388.
> 
> Are there chips that have both OPL4Base and ALBase?

Not as seperate PnP resources no. Here are pnpdumps for the 4 PnP chips:

http://members.home.nl/rene.herman/opti/

(yes, OPTi 933 is OPT0931 same as OPTi 931)

>> The chip can defer to external FM it seems, but even if there are
>> OPTi/OPL4 combo's out there in the wild (I sort of doubt it)
> 
> Why?

Because an OPL4 is fairly rarely seen as is and all OPTi (PnP) chips I have 
ever seen are on dirt cheap mass-market soundcards. It does not make a lot 
of sense from a business standpoint to put a cheap chip with integrated OPL3 
on a card and then wire up an expensive OPL4 to it. Admittedly, some of 
early OPTi chips may have been somewhat less junky -- 931/3 are worst in 
that respect.

>> the driver's just doing opl3 anyway. ie, still needs just the +8.
> 
> Only the 93x driver; the 92x driver has OPL4 support.

You're right, somehow managed to miss that. According to the code, there's 
overlap with PnP at the 924 which is unfortunate since it means we can't 
just add 8 (unconditionally, nor when the area is larger than 8) to the PnP 
resource -- an OPL4 would be probed for at the wrong address.

How about this? This adds 8 to fm_port only if we're checking for an OPL3 at 
a 16-byte aligned address (such as the 0x380-0x3f0 from the PnP chips). Is 
still second guessing the user if (s)he supplied the value, but what can you 
do. Ack?

Rene.


[-- Attachment #2: opti9xx-fm_port2.diff --]
[-- Type: text/plain, Size: 910 bytes --]

diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c
index 60c120f..1f5af7b 100644
--- a/sound/isa/opti9xx/opti92x-ad1848.c
+++ b/sound/isa/opti9xx/opti92x-ad1848.c
@@ -1890,12 +1890,15 @@ static int __devinit snd_opti9xx_probe(struct snd_card *card)
 			}
 		}
 #endif	/* !OPTi93X */
-		if (!opl3 && snd_opl3_create(card,
-					     chip->fm_port,
-					     chip->fm_port + 2,
-					     OPL3_HW_AUTO, 0, &opl3) < 0) {
-			snd_printk(KERN_WARNING "no OPL device at 0x%lx-0x%lx\n",
-				   chip->fm_port, chip->fm_port + 4 - 1);
+		if (!opl3) {
+			if ((chip->fm_port & 15) == 0)
+				chip->fm_port += 8;
+			error = snd_opl3_create(card, chip->fm_port,
+						chip->fm_port + 2,
+						OPL3_HW_AUTO, 0, &opl3);
+			if (error < 0)
+				snd_printk(KERN_WARNING "no OPL device at 0x%lx-0x%lx\n",
+					   chip->fm_port, chip->fm_port + 4 - 1);
 		}
 		if (opl3) {
 #ifdef CS4231

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] opti931: additional check for OPL3 device

[Krzysztof Helt]

Guys,

Look at the code. Shifting the fm_port address by 8 (Rene's) is correct way to do as the OPL3 is searched at fm_port and fm_port +2, but the OPL4 is searched at fm_port and fm_port -8
(which will be the fm_port before shifting).

Is the OPL4 located at 0x380 and 0x388 or 0x378 and 0x380? If the former, the Rene's patch is correct.

Regards,
Krzysztof

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

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

On 09/17/2007 02:26 PM, Krzysztof Helt wrote:

> Look at the code. Shifting the fm_port address by 8 (Rene's) is correct
> way to do as the OPL3 is searched at fm_port and fm_port +2, but the OPL4
> is searched at fm_port and fm_port -8 (which will be the fm_port before
> shifting).

Ah, damn, crap. Yes, forget the version I just sent. Original (attached) was 
correct. Thanks for paying attention.

> Is the OPL4 located at 0x380 and 0x388 or 0x378 and 0x380? If the former,
> the Rene's patch is correct.

The OPL4 is at 0x380, with its OPL3 compatible part at 0x388. Could you add 
an Acked-by?

===
opti9xx: adjust OPL3 FM resource value

The OPTi ISA-PnP chips advertise their OPL4 base at 0x380 (to 0x3f0) through 
  pnp and put their on-chip OPL3 at +8. The driver assumes the provided 
value is the ALBase (OPL3 address) though and checks for an OPL4 at -8, 
which means that simply adding 8 to the pnp provides value works to fix 
detection of both OPL3 and OPL4.

Problem spotted on 931 and 933 by Krzysztof Helt and confirmed on 924 and 
925 (together all OPTi ISA-PnP chips) by me.

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

Rene.


[-- Attachment #2: opti9xx-fm_port.diff --]
[-- Type: text/plain, Size: 652 bytes --]

diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c
index 60c120f..8bda47a 100644
--- a/sound/isa/opti9xx/opti92x-ad1848.c
+++ b/sound/isa/opti9xx/opti92x-ad1848.c
@@ -1732,11 +1732,11 @@ static int __devinit snd_card_opti9xx_pnp(struct snd_opti9xx *chip,
 
 #ifdef OPTi93X
 	port = pnp_port_start(pdev, 0) - 4;
-	fm_port = pnp_port_start(pdev, 1);
+	fm_port = pnp_port_start(pdev, 1) + 8;
 #else
 	if (pid->driver_data != 0x0924)
 		port = pnp_port_start(pdev, 1);
-	fm_port = pnp_port_start(pdev, 2);
+	fm_port = pnp_port_start(pdev, 2) + 8;
 #endif	/* OPTi93X */
 	irq = pnp_irq(pdev, 0);
 	dma1 = pnp_dma(pdev, 0);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

On 09/17/2007 02:40 PM, Rene Herman wrote:

To possibly save you the trouble, yes, tested that the OPL3 address really 
follows along with the OPL4Base PnP resource.

Fixed description and CCed Takashi:

> On 09/17/2007 02:26 PM, Krzysztof Helt wrote:
> 
>> Look at the code. Shifting the fm_port address by 8 (Rene's) is correct
>> way to do as the OPL3 is searched at fm_port and fm_port +2, but the OPL4
>> is searched at fm_port and fm_port -8 (which will be the fm_port before
>> shifting).
> 
> Ah, damn, crap. Yes, forget the version I just sent. Original (attached) 
> was correct. Thanks for paying attention.
> 
>> Is the OPL4 located at 0x380 and 0x388 or 0x378 and 0x380? If the former,
>> the Rene's patch is correct.
> 
> The OPL4 is at 0x380, with its OPL3 compatible part at 0x388. Could you 
> add an Acked-by?

===
opti9xx: adjust OPL3 FM resource value

The OPTi ISA-PnP chips advertise their OPL4Base resource value at 0x380
(to 0x3f0) through PnP and put their on-chip OPL3 at ALBase = OPL4Base + 8.
The driver assumes the PnP advertised value is the ALBase though and checks
for an OPL4 at -8, which means that simply adding 8 to the PnP advertised
value works to fix both OPL3 and OPL4 support.

Problem spotted on 931 and 933 by Krzysztof Helt and confirmed on 924 and
925 (together all OPTi ISA-PnP chips) by me. Has been tested.

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

Rene.

Re: [PATCH] opti931: additional check for OPL3 device

[Krzysztof Helt]

On Mon, 17 Sep 2007 14:40:51 +0200
Rene Herman <rene.herman@gmail.com> wrote:

> The OPL4 is at 0x380, with its OPL3 compatible part at 0x388. Could you add 
> an Acked-by?
> 
You are inpatient. I have tested it on my opti933.

> ===
> opti9xx: adjust OPL3 FM resource value
> 
> The OPTi ISA-PnP chips advertise their OPL4 base at 0x380 (to 0x3f0) through 
>   pnp and put their on-chip OPL3 at +8. The driver assumes the provided 
> value is the ALBase (OPL3 address) though and checks for an OPL4 at -8, 
> which means that simply adding 8 to the pnp provides value works to fix 
> detection of both OPL3 and OPL4.
> 
> Problem spotted on 931 and 933 by Krzysztof Helt and confirmed on 924 and 
> 925 (together all OPTi ISA-PnP chips) by me.
> 
> Signed-off-by; Rene Herman <rene.herman@gmail.com>

Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>

Re: [PATCH] opti931: additional check for OPL3 device

[Takashi Iwai]

At Mon, 17 Sep 2007 16:45:55 +0200,
Krzysztof Helt wrote:
> 
> On Mon, 17 Sep 2007 14:40:51 +0200
> Rene Herman <rene.herman@gmail.com> wrote:
> 
> > The OPL4 is at 0x380, with its OPL3 compatible part at 0x388. Could you add 
> > an Acked-by?
> > 
> You are inpatient. I have tested it on my opti933.
> 
> > ===
> > opti9xx: adjust OPL3 FM resource value
> > 
> > The OPTi ISA-PnP chips advertise their OPL4 base at 0x380 (to 0x3f0) through 
> >   pnp and put their on-chip OPL3 at +8. The driver assumes the provided 
> > value is the ALBase (OPL3 address) though and checks for an OPL4 at -8, 
> > which means that simply adding 8 to the pnp provides value works to fix 
> > detection of both OPL3 and OPL4.
> > 
> > Problem spotted on 931 and 933 by Krzysztof Helt and confirmed on 924 and 
> > 925 (together all OPTi ISA-PnP chips) by me.
> > 
> > Signed-off-by; Rene Herman <rene.herman@gmail.com>
> 
> Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>

Guys, let's relax.  Now it's merged to HG tree.


thanks,

Takashi

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

On 09/17/2007 04:45 PM, Krzysztof Helt wrote:

> On Mon, 17 Sep 2007 14:40:51 +0200
> Rene Herman <rene.herman@gmail.com> wrote:
> 
>> The OPL4 is at 0x380, with its OPL3 compatible part at 0x388. Could you add 
>> an Acked-by?
>>
> You are inpatient. I have tested it on my opti933.

Impatient? Didn't mean "immediately" or anything... just a request. Thanks.

Rene.

Re: [PATCH] opti931: additional check for OPL3 device

[Rene Herman]

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

On 09/14/2007 04:35 PM, Krzysztof Helt wrote:

> This patch adds additional check for OPL3 device. I found that PNP region
> returned by the card is 0x380 - 0x38f but the OPL3 device is located at
> 0x388 (standard FM device address). The driver checks start of the range
> (0x380) which is incorrect.

> The same problem appears on two cards I have with Opti chipset: opti931
> and opti933. If someone can test if the opti92x PnP has the same problem
> I would be grateful.

Yes, same for OPT0924/OPT0925 (928, 929 and 930 are Non-PnP).

I'd rather simply adjust the resource in snd_card_opti9xx_pnp though -- that 
way, you don't go second guessing a user-specified value.

Untested, but looks as though this should do it:

Rene.

[-- Attachment #2: opti9xx-fm_port.diff --]
[-- Type: text/plain, Size: 652 bytes --]

diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c
index 60c120f..8bda47a 100644
--- a/sound/isa/opti9xx/opti92x-ad1848.c
+++ b/sound/isa/opti9xx/opti92x-ad1848.c
@@ -1732,11 +1732,11 @@ static int __devinit snd_card_opti9xx_pnp(struct snd_opti9xx *chip,
 
 #ifdef OPTi93X
 	port = pnp_port_start(pdev, 0) - 4;
-	fm_port = pnp_port_start(pdev, 1);
+	fm_port = pnp_port_start(pdev, 1) + 8;
 #else
 	if (pid->driver_data != 0x0924)
 		port = pnp_port_start(pdev, 1);
-	fm_port = pnp_port_start(pdev, 2);
+	fm_port = pnp_port_start(pdev, 2) + 8;
 #endif	/* OPTi93X */
 	irq = pnp_irq(pdev, 0);
 	dma1 = pnp_dma(pdev, 0);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel