[PATCH 1/2] ALSA: emu10k1: use high-level I/O functions also during init

[PATCH 1/2] ALSA: emu10k1: use high-level I/O functions also during init

[Oswald Buddenhagen]

... and also use more pre-defined constants on the way (some of which
required adjustment).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---

This patch series needs to be applied on top of the patch titled "ALSA:
emu10k1: use more existing defines instead of open-coded numbers".
---
 include/sound/emu10k1.h          |  9 +++++----
 sound/pci/emu10k1/emu10k1_main.c | 20 ++++++--------------
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 5958cae819fd..05a09826eef0 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -852,10 +852,11 @@
 #define A_SPDIF_MUTED		0x000000c0
 
 #define A_I2S_CAPTURE_RATE_MASK	0x00000e00	/* This sets the capture PCM rate, but it is    */
-#define A_I2S_CAPTURE_48000	0x00000000	/* unclear if this sets the ADC rate as well.	*/
-#define A_I2S_CAPTURE_192000	0x00000200
-#define A_I2S_CAPTURE_96000	0x00000400
-#define A_I2S_CAPTURE_44100	0x00000800
+#define A_I2S_CAPTURE_RATE	0x03090076	/* unclear if this sets the ADC rate as well.	*/
+#define A_I2S_CAPTURE_48000	0x0
+#define A_I2S_CAPTURE_192000	0x1
+#define A_I2S_CAPTURE_96000	0x2
+#define A_I2S_CAPTURE_44100	0x4
 
 #define A_EHC_SRC48_MASK	0x0000e000	/* This sets the playback PCM rate on the P16V	*/
 #define A_EHC_SRC48_BYPASS	0x00000000
diff --git a/sound/pci/emu10k1/emu10k1_main.c b/sound/pci/emu10k1/emu10k1_main.c
index e53eb7fd0883..3aca01c70ccb 100644
--- a/sound/pci/emu10k1/emu10k1_main.c
+++ b/sound/pci/emu10k1/emu10k1_main.c
@@ -186,10 +186,7 @@ static int snd_emu10k1_init(struct snd_emu10k1 *emu, int enable_ir)
 	if (emu->card_capabilities->ca0151_chip) { /* audigy2 */
 		/* Hacks for Alice3 to work independent of haP16V driver */
 		/* Setup SRCMulti_I2S SamplingRate */
-		tmp = snd_emu10k1_ptr_read(emu, A_SPDIF_SAMPLERATE, 0);
-		tmp &= 0xfffff1ff;
-		tmp |= (0x2<<9);
-		snd_emu10k1_ptr_write(emu, A_SPDIF_SAMPLERATE, 0, tmp);
+		snd_emu10k1_ptr_write(emu, A_I2S_CAPTURE_RATE, 0, A_I2S_CAPTURE_96000);
 
 		/* Setup SRCSel (Enable Spdif,I2S SRCMulti) */
 		snd_emu10k1_ptr20_write(emu, SRCSel, 0, 0x14);
@@ -206,25 +203,20 @@ static int snd_emu10k1_init(struct snd_emu10k1 *emu, int enable_ir)
 		/* Hacks for Alice3 to work independent of haP16V driver */
 		dev_info(emu->card->dev, "Audigy2 value: Special config.\n");
 		/* Setup SRCMulti_I2S SamplingRate */
-		tmp = snd_emu10k1_ptr_read(emu, A_SPDIF_SAMPLERATE, 0);
-		tmp &= 0xfffff1ff;
-		tmp |= (0x2<<9);
-		snd_emu10k1_ptr_write(emu, A_SPDIF_SAMPLERATE, 0, tmp);
+		snd_emu10k1_ptr_write(emu, A_I2S_CAPTURE_RATE, 0, A_I2S_CAPTURE_96000);
 
 		/* Setup SRCSel (Enable Spdif,I2S SRCMulti) */
-		outl(0x600000, emu->port + 0x20);
-		outl(0x14, emu->port + 0x24);
+		snd_emu10k1_ptr20_write(emu, P17V_SRCSel, 0, 0x14);
 
 		/* Setup SRCMulti Input Audio Enable */
-		outl(0x7b0000, emu->port + 0x20);
-		outl(0xFF000000, emu->port + 0x24);
+		snd_emu10k1_ptr20_write(emu, P17V_MIXER_I2S_ENABLE, 0, 0xFF000000);
 
 		/* Setup SPDIF Out Audio Enable */
 		/* The Audigy 2 Value has a separate SPDIF out,
 		 * so no need for a mixer switch
 		 */
-		outl(0x7a0000, emu->port + 0x20);
-		outl(0xFF000000, emu->port + 0x24);
+		snd_emu10k1_ptr20_write(emu, P17V_MIXER_SPDIF_ENABLE, 0, 0xFF000000);
+
 		tmp = inw(emu->port + A_IOCFG) & ~0x8; /* Clear bit 3 */
 		outw(tmp, emu->port + A_IOCFG);
 	}
-- 
2.40.0.152.g15d061e6df

[PATCH 2/2] ALSA: emu10k1: use high-level I/O in set_filterQ()

[Oswald Buddenhagen]

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/sound/emu10k1.h              | 3 ++-
 sound/pci/emu10k1/emu10k1_callback.c | 5 +----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/sound/emu10k1.h b/include/sound/emu10k1.h
index 05a09826eef0..8fe80dcee71b 100644
--- a/include/sound/emu10k1.h
+++ b/include/sound/emu10k1.h
@@ -429,7 +429,8 @@
 #define DSL_LOOPENDADDR		0x18000007
 
 #define CCCA			0x08		/* Filter Q, interp. ROM, byte size, cur. addr register */
-#define CCCA_RESONANCE		0xf0000000	/* Lowpass filter resonance (Q) height			*/
+#define CCCA_RESONANCE_MASK	0xf0000000	/* Lowpass filter resonance (Q) height			*/
+#define CCCA_RESONANCE		0x041c0008
 #define CCCA_INTERPROM_MASK	0x0e000000	/* Selects passband of interpolation ROM		*/
 						/* 1 == full band, 7 == lowpass				*/
 						/* ROM 0 is used when pitch shifting downward or less	*/
diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index 44f2a61c6be8..9455df18f7b2 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -532,8 +532,5 @@ set_fm2frq2(struct snd_emu10k1 *hw, struct snd_emux_voice *vp)
 static void
 set_filterQ(struct snd_emu10k1 *hw, struct snd_emux_voice *vp)
 {
-	unsigned int val;
-	val = snd_emu10k1_ptr_read(hw, CCCA, vp->ch) & ~CCCA_RESONANCE;
-	val |= (vp->reg.parm.filterQ << 28);
-	snd_emu10k1_ptr_write(hw, CCCA, vp->ch, val);
+	snd_emu10k1_ptr_write(hw, CCCA_RESONANCE, vp->ch, vp->reg.parm.filterQ);
 }
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 2/2] ALSA: emu10k1: use high-level I/O in set_filterQ()

[Takashi Iwai]

On Sat, 22 Apr 2023 18:10:21 +0200,
Oswald Buddenhagen wrote:
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Again, you must have a bit more say here...
For example, you didn't write why this change is needed.
You thought it obvious?  No, readers don't know.

BTW, it would be really better if we define some macro for the
highlevel I/O definition.  It's cumbersome to decode and check
manually at review whether the conversion is correct, and it's
error-prone.


thanks,

Takashi

Re: [PATCH 1/2] ALSA: emu10k1: use high-level I/O functions also during init

[Takashi Iwai]

On Sat, 22 Apr 2023 18:10:20 +0200,
Oswald Buddenhagen wrote:
> 
> ... and also use more pre-defined constants on the way (some of which
> required adjustment).
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied this one, but skipped the patch 2.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: emu10k1: use high-level I/O in set_filterQ()

[Oswald Buddenhagen]

On Sun, Apr 23, 2023 at 09:25:39AM +0200, Takashi Iwai wrote:
>Again, you must have a bit more say here...
>For example, you didn't write why this change is needed.
>You thought it obvious?  No, readers don't know.
>
it is obvious from the patch - the code becomes much shorter and more 
legible. and someone who just reads the log/blame wouldn't care, because 
it doesn't actually explain anything. but whatever.

On Sun, Apr 23, 2023 at 09:35:46AM +0200, Takashi Iwai wrote:
>On Sat, 22 Apr 2023 18:10:20 +0200,
>Oswald Buddenhagen wrote:
>> 
>> ... and also use more pre-defined constants on the way (some of which
>> required adjustment).
>> 
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
>Applied this one, but skipped the patch 2.
>
which is funny, because that commit message misses the obvious "why" 
part as well - it just mentions an additional thing that is unique to 
this patch.

so to be consistent, you should reject both patches and wait for an 
update.

>BTW, it would be really better if we define some macro for the
>highlevel I/O definition.  It's cumbersome to decode and check
>manually at review whether the conversion is correct, and it's
>error-prone.
>
yes, i considered that.
i also considered many more refactorings, and had to hold myself back -
there are enough nice-to-have patches in this series as-is.
i mean, 15 years ago it would have made sense to go crazy, but now the 
hardware is a bit too obsolete to go much beyond what i actually need 
for my project. i'm assuming some people outside the western sphere are 
still using our scrap with linux, but we rarely hear from them, so it's 
hard to know ...

regards

Re: [PATCH 2/2] ALSA: emu10k1: use high-level I/O in set_filterQ()

[Takashi Iwai]

On Sun, 23 Apr 2023 10:51:38 +0200,
Oswald Buddenhagen wrote:
> 
> On Sun, Apr 23, 2023 at 09:25:39AM +0200, Takashi Iwai wrote:
> > Again, you must have a bit more say here...
> > For example, you didn't write why this change is needed.
> > You thought it obvious?  No, readers don't know.
> > 
> it is obvious from the patch - the code becomes much shorter and more
> legible.

It's not obvious unless you read the code changes.  Not obvious
whether it's a code refactoring without any functional change, etc.
Such info can be well put in the patch description.

> and someone who just reads the log/blame wouldn't care,
> because it doesn't actually explain anything. but whatever.

Someone already cared.  See?

> On Sun, Apr 23, 2023 at 09:35:46AM +0200, Takashi Iwai wrote:
> > On Sat, 22 Apr 2023 18:10:20 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> ... and also use more pre-defined constants on the way (some of which
> >> required adjustment).
> >> 
> >> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> > 
> > Applied this one, but skipped the patch 2.
> > 
> which is funny, because that commit message misses the obvious "why"
> part as well - it just mentions an additional thing that is unique to
> this patch.
> 
> so to be consistent, you should reject both patches and wait for an
> update.

Right, it was enough for me to reply the same thing again, so I wanted
just to reduce the pile of XXXX.  I'd reject all at the next time :)

> > BTW, it would be really better if we define some macro for the
> > highlevel I/O definition.  It's cumbersome to decode and check
> > manually at review whether the conversion is correct, and it's
> > error-prone.
> > 
> yes, i considered that.
> i also considered many more refactorings, and had to hold myself back -
> there are enough nice-to-have patches in this series as-is.
> i mean, 15 years ago it would have made sense to go crazy, but now the
> hardware is a bit too obsolete to go much beyond what i actually need
> for my project. i'm assuming some people outside the western sphere
> are still using our scrap with linux, but we rarely hear from them, so
> it's hard to know ...

Yeah, that's a dilemma of maintaining the old legacy stuff.


Takashi