[RFC PATCH] ES938 support for ES18xx driver

[RFC PATCH] ES938 support for ES18xx driver

[Ondrej Zary]

Hello,
this is an attempt to support ES938 3-D Audio Effects Processor found on some
ES18xx (and possibly other) sound cards, doing bass/treble and 3D control.

ES938 is controlled by MIDI SysEx commands sent through card's MPU401 port.

The following patch works but has a problem: the midi port cannot be used by
userspace applications. Opening and closing the rawmidi device on each control
change would probably work (as long as the port is not used by an application)
but that just does not seem right. Is there a way to cleanly fix this?

(There are two problems: 3D level does not have any effect and alsamixer does
not display the TLV dB values - but don't know why).

---
 sound/isa/Kconfig  |    4 +
 sound/isa/Makefile |    2 +
 sound/isa/es18xx.c |   12 +++-
 sound/isa/es938.c  |  181 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 sound/isa/es938.h  |   24 +++++++
 5 files changed, 222 insertions(+), 1 deletions(-)
 create mode 100644 sound/isa/es938.c
 create mode 100644 sound/isa/es938.h

diff --git a/sound/isa/Kconfig b/sound/isa/Kconfig
index affa134..a8bf3e3 100644
--- a/sound/isa/Kconfig
+++ b/sound/isa/Kconfig
@@ -17,6 +17,9 @@ config SND_SB16_DSP
         select SND_PCM
         select SND_SB_COMMON
 
+config SND_ES938
+	tristate
+
 menuconfig SND_ISA
 	bool "ISA sound devices"
 	depends on ISA && ISA_DMA_API
@@ -183,6 +186,7 @@ config SND_ES18XX
 	select SND_OPL3_LIB
 	select SND_MPU401_UART
 	select SND_PCM
+	select SND_ES938
 	help
 	  Say Y here to include support for ESS AudioDrive ES18xx chips.
 
diff --git a/sound/isa/Makefile b/sound/isa/Makefile
index 9a15f14..d59e0bf 100644
--- a/sound/isa/Makefile
+++ b/sound/isa/Makefile
@@ -8,6 +8,7 @@ snd-als100-objs := als100.o
 snd-azt2320-objs := azt2320.o
 snd-cmi8328-objs := cmi8328.o
 snd-cmi8330-objs := cmi8330.o
+snd-es938-objs := es938.o
 snd-es18xx-objs := es18xx.o
 snd-opl3sa2-objs := opl3sa2.o
 snd-sc6000-objs := sc6000.o
@@ -19,6 +20,7 @@ obj-$(CONFIG_SND_ALS100) += snd-als100.o
 obj-$(CONFIG_SND_AZT2320) += snd-azt2320.o
 obj-$(CONFIG_SND_CMI8328) += snd-cmi8328.o
 obj-$(CONFIG_SND_CMI8330) += snd-cmi8330.o
+obj-$(CONFIG_SND_ES938) += snd-es938.o
 obj-$(CONFIG_SND_ES18XX) += snd-es18xx.o
 obj-$(CONFIG_SND_OPL3SA2) += snd-opl3sa2.o
 obj-$(CONFIG_SND_SC6000) += snd-sc6000.o
diff --git a/sound/isa/es18xx.c b/sound/isa/es18xx.c
index 12978b8..c502aa0 100644
--- a/sound/isa/es18xx.c
+++ b/sound/isa/es18xx.c
@@ -96,6 +96,7 @@
 #define SNDRV_LEGACY_FIND_FREE_IRQ
 #define SNDRV_LEGACY_FIND_FREE_DMA
 #include <sound/initval.h>
+#include "es938.h"
 
 #define PFX "es18xx: "
 
@@ -122,6 +123,7 @@ struct snd_es18xx {
 	struct snd_pcm_substream *playback_b_substream;
 
 	struct snd_rawmidi *rmidi;
+	struct snd_es938 es938;
 
 	struct snd_kcontrol *hw_volume;
 	struct snd_kcontrol *hw_switch;
@@ -2166,7 +2168,15 @@ static int snd_audiodrive_probe(struct snd_card *card, int dev)
 			return err;
 	}
 
-	return snd_card_register(card);
+	err = snd_card_register(card);
+	if (err < 0)
+		return err;
+
+	if (mpu_port[dev] > 0 && mpu_port[dev] != SNDRV_AUTO_PORT)
+		if (snd_es938_init(&chip->es938, card, 0, 0) == 0)
+			printk("es938 found!\n");
+
+	return 0;
 }
 
 static int snd_es18xx_isa_match(struct device *pdev, unsigned int dev)
diff --git a/sound/isa/es938.c b/sound/isa/es938.c
new file mode 100644
index 0000000..cfe8ae7
--- /dev/null
+++ b/sound/isa/es938.c
@@ -0,0 +1,181 @@
+/*
+ *  Driver for ESS ES938 3-D Audio Effects Processor
+ *  Copyright (c) 2013 Ondrej Zary
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+//#include <linux/init.h>
+#include <linux/module.h>
+#include <sound/asoundef.h>
+#include <sound/control.h>
+#include <sound/core.h>
+#include <sound/rawmidi.h>
+#include "es938.h"
+
+#define PFX "es938: "
+
+MODULE_AUTHOR("Ondrej Zary");  
+MODULE_DESCRIPTION("ESS ES938");
+MODULE_LICENSE("GPL");
+
+static int snd_es938_read_reg(struct snd_es938 *chip, u8 reg, u8 *out)
+{
+	u8 buf[8];
+	int i = 0, res;
+	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, ES938_ID, ES938_CMD_REG_R, reg,
+		       MIDI_CMD_COMMON_SYSEX_END };
+
+	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));
+
+	memset(buf, 0, sizeof(buf));
+	while (i < sizeof(buf)) {
+		res = snd_rawmidi_kernel_read(chip->rfile.input, buf + i, sizeof(buf) - i);
+		if (res > 0)
+			i+= res;
+	}
+
+	/* check reply */
+	if (memcmp(buf, sysex, 6) || buf[7] != MIDI_CMD_COMMON_SYSEX_END)
+		return -1;
+
+	if (out)
+		*out = buf[6];
+
+	return 0;
+}
+
+static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
+{
+	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, ES938_ID, ES938_CMD_REG_W, reg,
+		       val, MIDI_CMD_COMMON_SYSEX_END };
+
+	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));
+	chip->regs[reg] = val;
+}
+
+static void snd_es938_write_reg_mask(struct snd_es938 *chip, u8 reg, u8 val, u8 mask)
+{
+	u8 oldval = chip->regs[reg];
+	oldval &= ~mask;
+	snd_es938_write_reg(chip, reg, oldval | (val & mask));
+}
+
+#define ES938_MIXER(xname, xindex, reg, shift, mask) \
+{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, .index = xindex, \
+  .info = snd_es938_info_mixer, \
+  .get = snd_es938_get_mixer, .put = snd_es938_put_mixer, \
+  .private_value = reg | (shift << 8) | (mask << 16) }
+
+#define ES938_MIXER_TLV(xname, xindex, reg, shift, mask, xtlv) \
+{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, .index = xindex, \
+  .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
+  .info = snd_es938_info_mixer, \
+  .get = snd_es938_get_mixer, .put = snd_es938_put_mixer, \
+  .private_value = reg | (shift << 8) | (mask << 16), \
+  .tlv = { .p = (xtlv) } }
+
+static const DECLARE_TLV_DB_SCALE(db_scale_tone, -900, 300, 0);
+
+static struct snd_kcontrol_new snd_es938_controls[] = {
+ES938_MIXER_TLV("Tone Control - Bass", 0, ES938_REG_TONE, 0, 7, db_scale_tone),
+ES938_MIXER_TLV("Tone Control - Treble", 0, ES938_REG_TONE, 4, 7, db_scale_tone),
+ES938_MIXER("3D Control - Level", 0, ES938_REG_SPATIAL, 1, 63),
+ES938_MIXER("3D Control - Switch", 0, ES938_REG_SPATIAL_EN, 0, 1),
+};
+
+int snd_es938_init(struct snd_es938 *chip, struct snd_card *card, int device,
+		   int subdevice)
+{
+	int err, i;
+
+	if ((err = snd_rawmidi_kernel_open(card, device, subdevice,
+			SNDRV_RAWMIDI_LFLG_OPEN | SNDRV_RAWMIDI_LFLG_APPEND,
+			&chip->rfile)) < 0) {
+		snd_printk(KERN_WARNING PFX "unable to open MIDI device\n");
+		return err;
+	}
+
+	/* try to read a register to detect chip presence */
+	if (snd_es938_read_reg(chip, ES938_REG_MISC, NULL) < 0)
+		return -ENODEV;
+
+	/* write default values (there's no reset) */
+	snd_es938_write_reg(chip, ES938_REG_MISC, 0x49);
+	snd_es938_write_reg(chip, ES938_REG_TONE, 0x33);
+	snd_es938_write_reg(chip, ES938_REG_SPATIAL, 0x00);
+	/* datasheet specifies invalid value 0x00 as default */
+	snd_es938_write_reg(chip, ES938_REG_SPATIAL_EN, 0x02);
+	snd_es938_write_reg(chip, ES938_REG_POWER, 0x0e);
+
+	strlcat(card->mixername, " + ES938", sizeof(card->mixername));
+	for (i = 0; i < ARRAY_SIZE(snd_es938_controls); i++)
+		if ((err = snd_ctl_add(card, snd_ctl_new1(&snd_es938_controls[i], chip))) < 0)
+			return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(snd_es938_init);
+
+void snd_es938_release(struct snd_es938 *chip)
+{
+	snd_rawmidi_kernel_release(&chip->rfile);
+}
+EXPORT_SYMBOL(snd_es938_release);
+
+
+int snd_es938_info_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo)
+{
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+
+	uinfo->type = mask == 1 ? SNDRV_CTL_ELEM_TYPE_BOOLEAN : SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = mask;
+	return 0;
+}
+EXPORT_SYMBOL(snd_es938_info_mixer);
+
+int snd_es938_get_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_es938 *chip = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	int shift = (kcontrol->private_value >> 8) & 0xff;
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+	u8 val = chip->regs[reg];
+	
+	ucontrol->value.integer.value[0] = (val >> shift) & mask;
+	return 0;
+}
+EXPORT_SYMBOL(snd_es938_get_mixer);
+
+int snd_es938_put_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_es938 *chip = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	int shift = (kcontrol->private_value >> 8) & 0xff;
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+	u8 val = ucontrol->value.integer.value[0] & mask;
+	u8 oldval = chip->regs[reg];
+
+	mask <<= shift;
+	val <<= shift;
+	
+	snd_es938_write_reg_mask(chip, reg, val, mask);
+	return chip->regs[reg] != oldval;
+}
+EXPORT_SYMBOL(snd_es938_put_mixer);
diff --git a/sound/isa/es938.h b/sound/isa/es938.h
new file mode 100644
index 0000000..1f8595f
--- /dev/null
+++ b/sound/isa/es938.h
@@ -0,0 +1,24 @@
+#include <sound/tlv.h>
+
+#define ES938_ID	0x00, 0x00, 0x7b
+
+#define ES938_CMD_REG_R		0x7e
+#define ES938_CMD_REG_W		0x7f
+
+#define ES938_REG_MISC		0
+#define ES938_REG_TONE		1
+#define ES938_REG_SPATIAL	5
+#define ES938_REG_SPATIAL_EN	6
+#define ES938_REG_POWER		7
+
+struct snd_es938 {
+	u8 regs[8];
+	struct snd_rawmidi_file rfile;
+};
+
+int snd_es938_init(struct snd_es938 *chip, struct snd_card *card, int device,
+		   int subdevice);
+void snd_es938_release(struct snd_es938 *chip);
+int snd_es938_info_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo);
+int snd_es938_get_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol);
+int snd_es938_put_mixer(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol);
-- 
Ondrej Zary

Re: [RFC PATCH] ES938 support for ES18xx driver

[Andreas Mohr]

Hi,

> ES938 is controlled by MIDI SysEx commands sent through card's MPU401 port.
> 
> The following patch works but has a problem: the midi port cannot be used by
> userspace applications. Opening and closing the rawmidi device on each control
> change would probably work (as long as the port is not used by an application)
> but that just does not seem right. Is there a way to cleanly fix this?

Just to clarify matters:

The story here was a bit split, but then I got it:
you're saying that since ES938 control is via MIDI SysEx,
these operations will occupy MIDI resources at least while controlling ES938
and therefore they're unavailable for userspace.

Don't tell me we'd require a MIDI access multiplexing layer which perhaps does not exist yet...


> (There are two problems: 3D level does not have any effect and alsamixer does
> not display the TLV dB values - but don't know why).

Hmm, I'm rather in the dark here as well - however from my driver experiences
the actually sort-of standardized 3D level control
may have some "weird" deviations (at least when talking AC97 or pseudo-AC97)
such as different bitmask indices or possibly even inverted volume operation
(background: azt3328 is pseudo-AC97, thus it has some deviations).
Sorry for not knowing any more helpful hints...

Hmm, or perhaps ES938_REG_POWER needs some special values configured
to have 3D sound effects powered up, too?
OTOH if this is a simple non-DSP implementation then 3D probably is implemented
via delay lines etc., so perhaps it is so simple that it does not have a
special "enable power" switch after all.

Oh, and BTW, my azt3328.c said:

 *  - (non-bug) "Bass/Treble or 3D settings don't work" - they do get evaluated
 *    if you set PCM output switch to "pre 3D" instead of "post 3D".
 *    If this can't be set, then get a mixer application that Isn't Stupid (tm)
 *    (e.g. kmix, gamix) - unfortunately several are!!

So perhaps ES938 has (perhaps since it might more or less conform to certain
standards specs?) another control for PCM output switch as well
which would need implementing or simply isn't set properly?
(to clarify, we're talking about a switch which would cause output signal
to be grabbed pre or post the 3D manipulation block)


> +		if (snd_es938_init(&chip->es938, card, 0, 0) == 0)
> +			printk("es938 found!\n");

Perhaps we could have a more verbose message here?
That ES938 surely must have some human-readable characteristics...
(hmm, what about "ES938 3D audio effects processor found!"?)
((or "Detected ..."))


> +	/* try to read a register to detect chip presence */
> +	if (snd_es938_read_reg(chip, ES938_REG_MISC, NULL) < 0)
> +		return -ENODEV;

Perhaps that check is not specific enough, i.e. it might be useful
to figure out a more precise check?


Thanks a ton for enhancing the feature set of certain sound cards :))

Andreas Mohr

Re: [RFC PATCH] ES938 support for ES18xx driver

[Ondrej Zary]

On Sunday 15 September 2013 20:49:02 Andreas Mohr wrote:
> Hi,
>
> > ES938 is controlled by MIDI SysEx commands sent through card's MPU401
> > port.
> >
> > The following patch works but has a problem: the midi port cannot be used
> > by userspace applications. Opening and closing the rawmidi device on each
> > control change would probably work (as long as the port is not used by an
> > application) but that just does not seem right. Is there a way to cleanly
> > fix this?
>
> Just to clarify matters:
>
> The story here was a bit split, but then I got it:
> you're saying that since ES938 control is via MIDI SysEx,
> these operations will occupy MIDI resources at least while controlling
> ES938 and therefore they're unavailable for userspace.
>
> Don't tell me we'd require a MIDI access multiplexing layer which perhaps
> does not exist yet...

Probably some hack that would allow the driver to send MIDI data behind 
everyone's back would be enough. Receiving is not needed (because the read 
register command is used only once for device detection). The 
SNDRV_RAWMIDI_LFLG_APPEND thing looks like that but does not seem to work.

And another bad thing I forgot to mention before: the driver cannot be 
unloaded because the MIDI device is open (circular dependency between 
snd-es18xx and snd-es938).

Maybe rawmidi_open_priv() would work, only if it was exported (and not _priv).

> > (There are two problems: 3D level does not have any effect and alsamixer
> > does not display the TLV dB values - but don't know why).
>
> Hmm, I'm rather in the dark here as well - however from my driver
> experiences the actually sort-of standardized 3D level control
> may have some "weird" deviations (at least when talking AC97 or
> pseudo-AC97) such as different bitmask indices or possibly even inverted
> volume operation (background: azt3328 is pseudo-AC97, thus it has some
> deviations). Sorry for not knowing any more helpful hints...
>
> Hmm, or perhaps ES938_REG_POWER needs some special values configured
> to have 3D sound effects powered up, too?
> OTOH if this is a simple non-DSP implementation then 3D probably is
> implemented via delay lines etc., so perhaps it is so simple that it does
> not have a special "enable power" switch after all.
>
> Oh, and BTW, my azt3328.c said:
>
> *  - (non-bug) "Bass/Treble or 3D settings don't work" - they do get
> evaluated 
> *    if you set PCM output switch to "pre 3D" instead of "post 3D".
> *    If this can't be set, then get a mixer application that Isn't Stupid
> (tm) 
> *    (e.g. kmix, gamix) - unfortunately several are!!

BTW. I've got an AZT3328 card and it works fine with your driver. Thanks for 
it.
I'm thinking about doing some reverse-engineering of thunderbird-based sound 
cards (VLSI Thunderbird - Aztech PCI 368-DSP, Thunderbird Avenger - Philips 
Acoustic/Seismic/Rhythmic Edge).

> So perhaps ES938 has (perhaps since it might more or less conform to
> certain standards specs?) another control for PCM output switch as well
> which would need implementing or simply isn't set properly?
> (to clarify, we're talking about a switch which would cause output signal
> to be grabbed pre or post the 3D manipulation block)

The good thing is that there's a datasheet available for ES938 - e.g. at 
http://www.datasheetarchive.com/ES938-datasheet.html

The 3D switch works but the level doesn't. Maybe I should use only the 3 
specific level values from the datasheet and not a variable scale. Or just 
remove the level control completely as Windows driver does (there's only a 3D 
switch).

> > +		if (snd_es938_init(&chip->es938, card, 0, 0) == 0)
> > +			printk("es938 found!\n");
>
> Perhaps we could have a more verbose message here?
> That ES938 surely must have some human-readable characteristics...
> (hmm, what about "ES938 3D audio effects processor found!"?)
> ((or "Detected ..."))

Yes, that's wrong - it's only for testing purposes. There should probably be 
no message at all as the es18xx driver itself does not display anything if 
everything is OK.

> > +	/* try to read a register to detect chip presence */
> > +	if (snd_es938_read_reg(chip, ES938_REG_MISC, NULL) < 0)
> > +		return -ENODEV;
>
> Perhaps that check is not specific enough, i.e. it might be useful
> to figure out a more precise check?

snd_es938_read_reg() checks that the returned data match the format used by 
ES938. Maybe we can reread all the register values back after writing to be 
sure.

-- 
Ondrej Zary

Re: [RFC PATCH] ES938 support for ES18xx driver

[Andreas Mohr]

Hi,

On Sun, Sep 15, 2013 at 10:25:45PM +0200, Ondrej Zary wrote:
> On Sunday 15 September 2013 20:49:02 Andreas Mohr wrote:
> BTW. I've got an AZT3328 card and it works fine with your driver. Thanks for 
> it.

What!?!? I didn't quite expect any kernel dev to have that one, too ;)

(got several commits sitting here and waiting for submission though)

> I'm thinking about doing some reverse-engineering of thunderbird-based sound 
> cards (VLSI Thunderbird - Aztech PCI 368-DSP, Thunderbird Avenger - Philips 
> Acoustic/Seismic/Rhythmic Edge).

Oh, nice. I've got the Aztech PCI 368-DSP, too (that was somewhat obligatory
due to having written another Aztech card driver...).

And yeah, the PCI 368 (etc.) series does seem to be some rather heavy
support hole indeed, IIRC (especially as it's more modern PCI-based and not ISA).


> > So perhaps ES938 has (perhaps since it might more or less conform to
> > certain standards specs?) another control for PCM output switch as well
> > which would need implementing or simply isn't set properly?
> > (to clarify, we're talking about a switch which would cause output signal
> > to be grabbed pre or post the 3D manipulation block)
> 
> The good thing is that there's a datasheet available for ES938 - e.g. at 
> http://www.datasheetarchive.com/ES938-datasheet.html
> 
> The 3D switch works but the level doesn't. Maybe I should use only the 3 
> specific level values from the datasheet and not a variable scale. Or just 
> remove the level control completely as Windows driver does (there's only a 3D 
> switch).

Hmm. Perhaps they removed it "Since It Does Not Work"? ;-P


Thanks for hinting at the TLV control dB values thingy! Didn't know that such
thing existed, thus azt3328 does not have it (yet?).


> > Perhaps we could have a more verbose message here?
> > That ES938 surely must have some human-readable characteristics...
> > (hmm, what about "ES938 3D audio effects processor found!"?)
> > ((or "Detected ..."))
> 
> Yes, that's wrong - it's only for testing purposes. There should probably be 
> no message at all as the es18xx driver itself does not display anything if 
> everything is OK.

...or that, yeah.


> > Perhaps that check is not specific enough, i.e. it might be useful
> > to figure out a more precise check?
> 
> snd_es938_read_reg() checks that the returned data match the format used by 
> ES938. Maybe we can reread all the register values back after writing to be 
> sure.

Is there any chip ID/version register to be identified?

Andreas Mohr

Re: [RFC PATCH] ES938 support for ES18xx driver

[Ondrej Zary]

On Sunday 15 September 2013 22:35:20 Andreas Mohr wrote:
> Hi,
>
> On Sun, Sep 15, 2013 at 10:25:45PM +0200, Ondrej Zary wrote:
> > On Sunday 15 September 2013 20:49:02 Andreas Mohr wrote:
> > BTW. I've got an AZT3328 card and it works fine with your driver. Thanks
> > for it.
>
> What!?!? I didn't quite expect any kernel dev to have that one, too ;)

Don't remember where I got it from but it looked nice so I bought it :)

> (got several commits sitting here and waiting for submission though)
>
> > I'm thinking about doing some reverse-engineering of thunderbird-based
> > sound cards (VLSI Thunderbird - Aztech PCI 368-DSP, Thunderbird Avenger -
> > Philips Acoustic/Seismic/Rhythmic Edge).
>
> Oh, nice. I've got the Aztech PCI 368-DSP, too (that was somewhat
> obligatory due to having written another Aztech card driver...).
>
> And yeah, the PCI 368 (etc.) series does seem to be some rather heavy
> support hole indeed, IIRC (especially as it's more modern PCI-based and not
> ISA).

Got it on eBay but the Philips Edge cards are not that uncommon - they were 
sold here (in Slovakia) too, IIRC. I have Seismic Edge PSC705 (but bought it 
used).

> > > So perhaps ES938 has (perhaps since it might more or less conform to
> > > certain standards specs?) another control for PCM output switch as well
> > > which would need implementing or simply isn't set properly?
> > > (to clarify, we're talking about a switch which would cause output
> > > signal to be grabbed pre or post the 3D manipulation block)
> >
> > The good thing is that there's a datasheet available for ES938 - e.g. at
> > http://www.datasheetarchive.com/ES938-datasheet.html
> >
> > The 3D switch works but the level doesn't. Maybe I should use only the 3
> > specific level values from the datasheet and not a variable scale. Or
> > just remove the level control completely as Windows driver does (there's
> > only a 3D switch).
>
> Hmm. Perhaps they removed it "Since It Does Not Work"? ;-P

Maybe. The value is configured in the INF file and never changed.

> Thanks for hinting at the TLV control dB values thingy! Didn't know that
> such thing existed, thus azt3328 does not have it (yet?).

It's a nice thing, especially for cards that can go above 0 dB. You then know 
that the sound can be distorted when you set e.g. PCM volume too high.

> > > Perhaps we could have a more verbose message here?
> > > That ES938 surely must have some human-readable characteristics...
> > > (hmm, what about "ES938 3D audio effects processor found!"?)
> > > ((or "Detected ..."))
> >
> > Yes, that's wrong - it's only for testing purposes. There should probably
> > be no message at all as the es18xx driver itself does not display
> > anything if everything is OK.
>
> ...or that, yeah.
>
> > > Perhaps that check is not specific enough, i.e. it might be useful
> > > to figure out a more precise check?
> >
> > snd_es938_read_reg() checks that the returned data match the format used
> > by ES938. Maybe we can reread all the register values back after writing
> > to be sure.
>
> Is there any chip ID/version register to be identified?

No, there are only 8 registers, all(? - haven't tested) R/W.

-- 
Ondrej Zary

Re: [RFC PATCH] ES938 support for ES18xx driver

[Andreas Mohr]

On Sun, Sep 15, 2013 at 11:04:26PM +0200, Ondrej Zary wrote:
> On Sunday 15 September 2013 22:35:20 Andreas Mohr wrote:
> > What!?!? I didn't quite expect any kernel dev to have that one, too ;)
> 
> Don't remember where I got it from but it looked nice so I bought it :)

Yup, e.g. the builtin amp is quite nice.

> > Thanks for hinting at the TLV control dB values thingy! Didn't know that
> > such thing existed, thus azt3328 does not have it (yet?).
> 
> It's a nice thing, especially for cards that can go above 0 dB. You then know 
> that the sound can be distorted when you set e.g. PCM volume too high.

Hmm, any hint how to precisely do dB values normalization scaling,
for a card where this is not documented? Or perhaps that's actually easy -
haven't thought about it...

> > Is there any chip ID/version register to be identified?
> 
> No, there are only 8 registers, all(? - haven't tested) R/W.

OK, but possibly they happen to be creating another virtual register mapping range?
(i.e. index/data register combo?)

Andreas Mohr

Re: [RFC PATCH] ES938 support for ES18xx driver

[Ondrej Zary]

On Sunday 15 September 2013 23:23:58 Andreas Mohr wrote:
> On Sun, Sep 15, 2013 at 11:04:26PM +0200, Ondrej Zary wrote:
> > On Sunday 15 September 2013 22:35:20 Andreas Mohr wrote:
> > > What!?!? I didn't quite expect any kernel dev to have that one, too ;)
> >
> > Don't remember where I got it from but it looked nice so I bought it :)
>
> Yup, e.g. the builtin amp is quite nice.
>
> > > Thanks for hinting at the TLV control dB values thingy! Didn't know
> > > that such thing existed, thus azt3328 does not have it (yet?).
> >
> > It's a nice thing, especially for cards that can go above 0 dB. You then
> > know that the sound can be distorted when you set e.g. PCM volume too
> > high.
>
> Hmm, any hint how to precisely do dB values normalization scaling,
> for a card where this is not documented? Or perhaps that's actually easy -
> haven't thought about it...

Perhaps with 3 sound cards: one for recording, one with known dB values and 
one unknown.
First play some test signal at known dB value and record that.
Then play the signal using on the unknown card and ajdust its mixer so that 
the recording revel is the same as before.
Repeat a few times to find out the dB value of mixer step.

> > > Is there any chip ID/version register to be identified?
> >
> > No, there are only 8 registers, all(? - haven't tested) R/W.
>
> OK, but possibly they happen to be creating another virtual register
> mapping range? (i.e. index/data register combo?)

Datasheet says that there are no more registers - just 0..7 (2,3,4 are 
reserved).

-- 
Ondrej Zary

Re: [RFC PATCH] ES938 support for ES18xx driver

[Ondrej Zary]

This is v2, which opens/closes the MIDI device everytime a mixer control
value is changed. Userspace apps can now use the MIDI port. Changing the mixer
controls will fail when the MIDI port is open.

Also the 3D level control now works - seems that the reserved bit 0 must be
set.

---
 sound/isa/Kconfig  |    4 +
 sound/isa/Makefile |    2 +
 sound/isa/es18xx.c |   11 +++-
 sound/isa/es938.c  |  203 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 sound/isa/es938.h  |   29 ++++++++
 5 files changed, 248 insertions(+), 1 deletions(-)
 create mode 100644 sound/isa/es938.c
 create mode 100644 sound/isa/es938.h

diff --git a/sound/isa/Kconfig b/sound/isa/Kconfig
index affa134..a8bf3e3 100644
--- a/sound/isa/Kconfig
+++ b/sound/isa/Kconfig
@@ -17,6 +17,9 @@ config SND_SB16_DSP
         select SND_PCM
         select SND_SB_COMMON
 
+config SND_ES938
+	tristate
+
 menuconfig SND_ISA
 	bool "ISA sound devices"
 	depends on ISA && ISA_DMA_API
@@ -183,6 +186,7 @@ config SND_ES18XX
 	select SND_OPL3_LIB
 	select SND_MPU401_UART
 	select SND_PCM
+	select SND_ES938
 	help
 	  Say Y here to include support for ESS AudioDrive ES18xx chips.
 
diff --git a/sound/isa/Makefile b/sound/isa/Makefile
index 9a15f14..d59e0bf 100644
--- a/sound/isa/Makefile
+++ b/sound/isa/Makefile
@@ -8,6 +8,7 @@ snd-als100-objs := als100.o
 snd-azt2320-objs := azt2320.o
 snd-cmi8328-objs := cmi8328.o
 snd-cmi8330-objs := cmi8330.o
+snd-es938-objs := es938.o
 snd-es18xx-objs := es18xx.o
 snd-opl3sa2-objs := opl3sa2.o
 snd-sc6000-objs := sc6000.o
@@ -19,6 +20,7 @@ obj-$(CONFIG_SND_ALS100) += snd-als100.o
 obj-$(CONFIG_SND_AZT2320) += snd-azt2320.o
 obj-$(CONFIG_SND_CMI8328) += snd-cmi8328.o
 obj-$(CONFIG_SND_CMI8330) += snd-cmi8330.o
+obj-$(CONFIG_SND_ES938) += snd-es938.o
 obj-$(CONFIG_SND_ES18XX) += snd-es18xx.o
 obj-$(CONFIG_SND_OPL3SA2) += snd-opl3sa2.o
 obj-$(CONFIG_SND_SC6000) += snd-sc6000.o
diff --git a/sound/isa/es18xx.c b/sound/isa/es18xx.c
index 12978b8..5e496a7 100644
--- a/sound/isa/es18xx.c
+++ b/sound/isa/es18xx.c
@@ -96,6 +96,7 @@
 #define SNDRV_LEGACY_FIND_FREE_IRQ
 #define SNDRV_LEGACY_FIND_FREE_DMA
 #include <sound/initval.h>
+#include "es938.h"
 
 #define PFX "es18xx: "
 
@@ -122,6 +123,7 @@ struct snd_es18xx {
 	struct snd_pcm_substream *playback_b_substream;
 
 	struct snd_rawmidi *rmidi;
+	struct snd_es938 es938;
 
 	struct snd_kcontrol *hw_volume;
 	struct snd_kcontrol *hw_switch;
@@ -2166,7 +2168,14 @@ static int snd_audiodrive_probe(struct snd_card *card, int dev)
 			return err;
 	}
 
-	return snd_card_register(card);
+	err = snd_card_register(card);
+	if (err < 0)
+		return err;
+
+	if (mpu_port[dev] > 0 && mpu_port[dev] != SNDRV_AUTO_PORT)
+		snd_es938_init(&chip->es938, card, 0, 0);
+
+	return 0;
 }
 
 static int snd_es18xx_isa_match(struct device *pdev, unsigned int dev)
diff --git a/sound/isa/es938.c b/sound/isa/es938.c
new file mode 100644
index 0000000..a01f887
--- /dev/null
+++ b/sound/isa/es938.c
@@ -0,0 +1,203 @@
+/*
+ *  Driver for ESS ES938 3-D Audio Effects Processor
+ *  Copyright (c) 2013 Ondrej Zary
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#include <linux/module.h>
+#include <sound/asoundef.h>
+#include <sound/control.h>
+#include <sound/core.h>
+#include <sound/rawmidi.h>
+#include "es938.h"
+
+MODULE_AUTHOR("Ondrej Zary");
+MODULE_DESCRIPTION("ESS ES938");
+MODULE_LICENSE("GPL");
+
+static int snd_es938_read_reg(struct snd_es938 *chip, u8 reg, u8 *out)
+{
+	u8 buf[8];
+	int i = 0, res;
+	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, 0x00, 0x00, ES938_ID,
+		       ES938_CMD_REG_R, reg, MIDI_CMD_COMMON_SYSEX_END };
+	unsigned long end_time;
+
+	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));
+
+	memset(buf, 0, sizeof(buf));
+	end_time = jiffies + msecs_to_jiffies(100);
+	while (i < sizeof(buf)) {
+		res = snd_rawmidi_kernel_read(chip->rfile.input, buf + i,
+					      sizeof(buf) - i);
+		if (res > 0)
+			i += res;
+		if (time_after(jiffies, end_time))
+			return -1;
+	}
+
+	/* check reply */
+	if (memcmp(buf, sysex, 6) || buf[7] != MIDI_CMD_COMMON_SYSEX_END)
+		return -1;
+
+	if (out)
+		*out = buf[6];
+
+	return 0;
+}
+
+static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
+{
+	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, 0x00, 0x00, ES938_ID,
+		       ES938_CMD_REG_W, reg, val, MIDI_CMD_COMMON_SYSEX_END };
+
+	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));
+	chip->regs[reg] = val;
+}
+
+#define ES938_MIXER(xname, xindex, reg, shift, mask) \
+{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, .index = xindex, \
+.info = snd_es938_info_mixer, \
+.get = snd_es938_get_mixer, .put = snd_es938_put_mixer, \
+.private_value = reg | (shift << 8) | (mask << 16) }
+
+#define ES938_MIXER_TLV(xname, xindex, reg, shift, mask, xtlv) \
+{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, .index = xindex, \
+.access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
+.info = snd_es938_info_mixer, \
+.get = snd_es938_get_mixer, .put = snd_es938_put_mixer, \
+.private_value = reg | (shift << 8) | (mask << 16), \
+.tlv = { .p = (xtlv) } }
+
+static const DECLARE_TLV_DB_SCALE(db_scale_tone, -900, 300, 0);
+
+static struct snd_kcontrol_new snd_es938_controls[] = {
+ES938_MIXER_TLV("Tone Control - Bass", 0, ES938_REG_TONE, 0, 7, db_scale_tone),
+ES938_MIXER_TLV("Tone Control - Treble", 0, ES938_REG_TONE, 4, 7, db_scale_tone),
+ES938_MIXER("3D Control - Level", 0, ES938_REG_SPATIAL, 1, 63),
+ES938_MIXER("3D Control - Switch", 0, ES938_REG_SPATIAL_EN, 0, 1),
+};
+
+int snd_es938_init(struct snd_es938 *chip, struct snd_card *card, int device,
+		   int subdevice)
+{
+	int ret, i;
+
+	ret = snd_rawmidi_kernel_open(card, device, subdevice,
+			SNDRV_RAWMIDI_LFLG_OPEN | SNDRV_RAWMIDI_LFLG_APPEND,
+			&chip->rfile);
+	if (ret < 0) {
+		snd_printk(KERN_WARNING "es938: unable to open MIDI device\n");
+		return ret;
+	}
+
+	/* try to read a register to detect chip presence */
+	if (snd_es938_read_reg(chip, ES938_REG_MISC, NULL) < 0) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* write default values (there's no reset) */
+	snd_es938_write_reg(chip, ES938_REG_MISC, 0x49);
+	snd_es938_write_reg(chip, ES938_REG_TONE, 0x33);
+	/* reserved bit 0 must be set for 3D to work */
+	snd_es938_write_reg(chip, ES938_REG_SPATIAL, 0x01);
+	/* datasheet specifies invalid value 0x00 as default */
+	snd_es938_write_reg(chip, ES938_REG_SPATIAL_EN, 0x02);
+	snd_es938_write_reg(chip, ES938_REG_POWER, 0x0e);
+
+	strlcat(card->mixername, " + ES938", sizeof(card->mixername));
+	for (i = 0; i < ARRAY_SIZE(snd_es938_controls); i++) {
+		ret = snd_ctl_add(card,
+				  snd_ctl_new1(&snd_es938_controls[i], chip));
+		if (ret < 0)
+			goto err;
+	}
+
+	chip->card = card;
+	chip->device = device;
+	chip->subdevice = subdevice;
+err:
+	snd_rawmidi_kernel_release(&chip->rfile);
+
+	return ret;
+}
+EXPORT_SYMBOL(snd_es938_init);
+
+int snd_es938_info_mixer(struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_info *uinfo)
+{
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+
+	uinfo->type = mask == 1 ? SNDRV_CTL_ELEM_TYPE_BOOLEAN :
+				  SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = mask;
+	return 0;
+}
+EXPORT_SYMBOL(snd_es938_info_mixer);
+
+int snd_es938_get_mixer(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_es938 *chip = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	int shift = (kcontrol->private_value >> 8) & 0xff;
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+	u8 val = chip->regs[reg];
+
+	ucontrol->value.integer.value[0] = (val >> shift) & mask;
+	return 0;
+}
+EXPORT_SYMBOL(snd_es938_get_mixer);
+
+int snd_es938_put_mixer(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_es938 *chip = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	int shift = (kcontrol->private_value >> 8) & 0xff;
+	int mask = (kcontrol->private_value >> 16) & 0xff;
+	u8 val = ucontrol->value.integer.value[0] & mask;
+	u8 oldval = chip->regs[reg];
+	u8 newval;
+	int err;
+
+	mask <<= shift;
+	val <<= shift;
+
+	newval = (oldval & ~mask) | val;
+	if (newval == oldval)
+		return 0;
+
+	/* this will fail if the MIDI port is used by an application */
+	err = snd_rawmidi_kernel_open(chip->card, chip->device,
+			chip->subdevice,
+			SNDRV_RAWMIDI_LFLG_OPEN | SNDRV_RAWMIDI_LFLG_APPEND,
+			&chip->rfile);
+	if (err < 0)
+		return err;
+
+	snd_es938_write_reg(chip, reg, newval);
+
+	snd_rawmidi_kernel_release(&chip->rfile);
+
+	return 1;
+}
+EXPORT_SYMBOL(snd_es938_put_mixer);
diff --git a/sound/isa/es938.h b/sound/isa/es938.h
new file mode 100644
index 0000000..b67dfc7
--- /dev/null
+++ b/sound/isa/es938.h
@@ -0,0 +1,29 @@
+#include <sound/tlv.h>
+
+#define ES938_ID		0x7b
+
+#define ES938_CMD_REG_R		0x7e
+#define ES938_CMD_REG_W		0x7f
+
+#define ES938_REG_MISC		0
+#define ES938_REG_TONE		1
+#define ES938_REG_SPATIAL	5
+#define ES938_REG_SPATIAL_EN	6
+#define ES938_REG_POWER		7
+
+struct snd_es938 {
+	u8 regs[8];
+	struct snd_card *card;
+	int device;
+	int subdevice;
+	struct snd_rawmidi_file rfile;
+};
+
+int snd_es938_init(struct snd_es938 *chip, struct snd_card *card, int device,
+		   int subdevice);
+int snd_es938_info_mixer(struct snd_kcontrol *kcontrol,
+			 struct snd_ctl_elem_info *uinfo);
+int snd_es938_get_mixer(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol);
+int snd_es938_put_mixer(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol);
-- 
Ondrej Zary

Re: [RFC PATCH] ES938 support for ES18xx driver

[Andreas Mohr]

Hi,

On Mon, Sep 16, 2013 at 10:20:54PM +0200, Ondrej Zary wrote:
> +static int snd_es938_read_reg(struct snd_es938 *chip, u8 reg, u8 *out)
> +{
> +	u8 buf[8];
> +	int i = 0, res;
> +	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, 0x00, 0x00, ES938_ID,
> +		       ES938_CMD_REG_R, reg, MIDI_CMD_COMMON_SYSEX_END };
> +	unsigned long end_time;
> +
> +	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));
> +
> +	memset(buf, 0, sizeof(buf));
> +	end_time = jiffies + msecs_to_jiffies(100);
> +	while (i < sizeof(buf)) {
> +		res = snd_rawmidi_kernel_read(chip->rfile.input, buf + i,
> +					      sizeof(buf) - i);
> +		if (res > 0)
> +			i += res;
> +		if (time_after(jiffies, end_time))
> +			return -1;
> +	}

Forgive me, but I seem to faintly remember that it's preferred for
user code to tend towards non-jiffies-based timing (IIRC due to
NOHZ efforts etc.).
So if that actually is the case, then one might want to change it to
less jiffies-dependent handling (use non-jiffies helpers), but then how?

> +static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
> +{
> +	u8 sysex[] = { MIDI_CMD_COMMON_SYSEX, 0x00, 0x00, ES938_ID,
> +		       ES938_CMD_REG_W, reg, val, MIDI_CMD_COMMON_SYSEX_END };
> +
> +	snd_rawmidi_kernel_write(chip->rfile.output, sysex, sizeof(sysex));

snd_rawmidi_kernel_write() is prototyped as const unsigned char *buf,
so perhaps...
const unsigned char buf[] = ...;?
(static const probably not so useful
e.g. since static const may be rather prone to cache misses)

Though personally I do favour u8, since "char" seems so unsuitably stringy -
but the prototype currently doesn't offer it typed that way...

(Almost-)Reviewed-By: Andreas Mohr <andim2@users.sf.net>

Andreas Mohr