[PATCH v3] ES938 support for ES18xx driver

[PATCH v3] ES938 support for ES18xx driver

[Ondrej Zary]

Add support for 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 code opens/closes the MIDI device everytime a mixer control value is
changed so userspace apps can still use the MIDI port. Changing the mixer
controls will fail when the MIDI port is open by an application.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
Changes in v3:
 - constify buf argument to snd_rawmidi_kernel_write
 - remove double if from probe
 - spelling fix
Changes in v2:
 - debug message when ES938 detected
 - reworked sysex messages to use structs
 - ktime instead of jiffies for timeout
---
 sound/isa/Kconfig  |    4 +
 sound/isa/Makefile |    2 +
 sound/isa/es18xx.c |   13 +++-
 sound/isa/es938.c  |  217 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 sound/isa/es938.h  |   48 ++++++++++++
 5 files changed, 283 insertions(+), 1 deletion(-)
 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 0216475..db0b678 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 6faaac6..72dad69 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;
@@ -2167,7 +2169,16 @@ 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;
+
+	/* no error if this fails because ES938 is optional */
+	if (mpu_port[dev] > 0 && mpu_port[dev] != SNDRV_AUTO_PORT &&
+				snd_es938_init(&chip->es938, card, 0, 0) == 0)
+		snd_printk(KERN_DEBUG "found ES938 audio processor\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..75d1fbe
--- /dev/null
+++ b/sound/isa/es938.c
@@ -0,0 +1,217 @@
+/*
+ *  Driver for ESS ES938 3-D Audio Effects Processor
+ *  Copyright (c) 2014 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)
+{
+	int i = 0, res;
+	const struct snd_es938_sysex_reg req = {
+		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
+		.id = ES938_ID,
+		.cmd = ES938_CMD_REG_R,
+		.reg = reg,
+		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
+	};
+	struct snd_es938_sysex_regval reply = { };
+	u8 *p = (void *)&reply;
+	ktime_t end_time;
+
+	snd_rawmidi_kernel_write(chip->rfile.output,
+				(const void *)&req, sizeof(req));
+
+	end_time = ktime_add_ms(ktime_get(), 100);
+	while (i < sizeof(reply)) {
+		res = snd_rawmidi_kernel_read(chip->rfile.input, p + i,
+					      sizeof(reply) - i);
+		if (res > 0)
+			i += res;
+		if (ktime_after(ktime_get(), end_time))
+			return -1;
+	}
+
+	/* check if the reply is ours and has SYSEX_END at the end */
+	if (memcmp(&reply, &req, sizeof(req) - 1) ||
+				reply.midi_end != MIDI_CMD_COMMON_SYSEX_END)
+		return -1;
+
+	if (out)
+		*out = reply.val;
+
+	return 0;
+}
+
+static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
+{
+	const struct snd_es938_sysex_regval req = {
+		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
+		.id = ES938_ID,
+		.cmd = ES938_CMD_REG_W,
+		.reg = reg,
+		.val = val,
+		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
+	};
+
+	snd_rawmidi_kernel_write(chip->rfile.output,
+				(const void *)&req, sizeof(req));
+	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..08148ff
--- /dev/null
+++ b/sound/isa/es938.h
@@ -0,0 +1,48 @@
+#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;
+};
+
+struct snd_es938_sysex_reg {
+	u8 midi_cmd;
+	u8 zeros[2];
+	u8 id;
+	u8 cmd;
+	u8 reg;
+	u8 midi_end;
+};
+
+struct snd_es938_sysex_regval {
+	u8 midi_cmd;
+	u8 zeros[2];
+	u8 id;
+	u8 cmd;
+	u8 reg;
+	u8 val;
+	u8 midi_end;
+};
+
+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: [PATCH v3] ES938 support for ES18xx driver

[Andreas Mohr]

On Mon, Sep 29, 2014 at 11:44:39PM +0200, Ondrej Zary wrote:
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
> Changes in v3:
>  - constify buf argument to snd_rawmidi_kernel_write
>  - remove double if from probe
>  - spelling fix
> Changes in v2:
>  - debug message when ES938 detected
>  - reworked sysex messages to use structs
>  - ktime instead of jiffies for timeout
> ---

Nice!

Reviewed-by: Andreas Mohr <andim2@users.sf.net>

Andreas Mohr

Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver

[Takashi Iwai]

At Mon, 29 Sep 2014 23:44:39 +0200,
Ondrej Zary wrote:
> 
> Add support for ES938 3-D Audio Effects Processor found on some ES18xx
> (and possibly other) sound cards, doing bass/treble and 3D control.

As this is seen only on es18xx, we don't need to make it as an
individual module.  Just merge into snd-es18xx module.

> ES938 is controlled by MIDI SysEx commands sent through card's MPU401 port.
> 
> The code opens/closes the MIDI device everytime a mixer control value is
> changed so userspace apps can still use the MIDI port. Changing the mixer
> controls will fail when the MIDI port is open by an application.

Why the kernel open/close is needed for snd_es938_init(), too?
Also, adding controls after snd_card_register() call isn't good in
most cases.


thanks,

Takashi

> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

> ---
> Changes in v3:
>  - constify buf argument to snd_rawmidi_kernel_write
>  - remove double if from probe
>  - spelling fix
> Changes in v2:
>  - debug message when ES938 detected
>  - reworked sysex messages to use structs
>  - ktime instead of jiffies for timeout
> ---
>  sound/isa/Kconfig  |    4 +
>  sound/isa/Makefile |    2 +
>  sound/isa/es18xx.c |   13 +++-
>  sound/isa/es938.c  |  217 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sound/isa/es938.h  |   48 ++++++++++++
>  5 files changed, 283 insertions(+), 1 deletion(-)
>  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 0216475..db0b678 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 6faaac6..72dad69 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;
> @@ -2167,7 +2169,16 @@ 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;
> +
> +	/* no error if this fails because ES938 is optional */
> +	if (mpu_port[dev] > 0 && mpu_port[dev] != SNDRV_AUTO_PORT &&
> +				snd_es938_init(&chip->es938, card, 0, 0) == 0)
> +		snd_printk(KERN_DEBUG "found ES938 audio processor\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..75d1fbe
> --- /dev/null
> +++ b/sound/isa/es938.c
> @@ -0,0 +1,217 @@
> +/*
> + *  Driver for ESS ES938 3-D Audio Effects Processor
> + *  Copyright (c) 2014 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)
> +{
> +	int i = 0, res;
> +	const struct snd_es938_sysex_reg req = {
> +		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
> +		.id = ES938_ID,
> +		.cmd = ES938_CMD_REG_R,
> +		.reg = reg,
> +		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
> +	};
> +	struct snd_es938_sysex_regval reply = { };
> +	u8 *p = (void *)&reply;
> +	ktime_t end_time;
> +
> +	snd_rawmidi_kernel_write(chip->rfile.output,
> +				(const void *)&req, sizeof(req));
> +
> +	end_time = ktime_add_ms(ktime_get(), 100);
> +	while (i < sizeof(reply)) {
> +		res = snd_rawmidi_kernel_read(chip->rfile.input, p + i,
> +					      sizeof(reply) - i);
> +		if (res > 0)
> +			i += res;
> +		if (ktime_after(ktime_get(), end_time))
> +			return -1;
> +	}
> +
> +	/* check if the reply is ours and has SYSEX_END at the end */
> +	if (memcmp(&reply, &req, sizeof(req) - 1) ||
> +				reply.midi_end != MIDI_CMD_COMMON_SYSEX_END)
> +		return -1;
> +
> +	if (out)
> +		*out = reply.val;
> +
> +	return 0;
> +}
> +
> +static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
> +{
> +	const struct snd_es938_sysex_regval req = {
> +		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
> +		.id = ES938_ID,
> +		.cmd = ES938_CMD_REG_W,
> +		.reg = reg,
> +		.val = val,
> +		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
> +	};
> +
> +	snd_rawmidi_kernel_write(chip->rfile.output,
> +				(const void *)&req, sizeof(req));
> +	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..08148ff
> --- /dev/null
> +++ b/sound/isa/es938.h
> @@ -0,0 +1,48 @@
> +#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;
> +};
> +
> +struct snd_es938_sysex_reg {
> +	u8 midi_cmd;
> +	u8 zeros[2];
> +	u8 id;
> +	u8 cmd;
> +	u8 reg;
> +	u8 midi_end;
> +};
> +
> +struct snd_es938_sysex_regval {
> +	u8 midi_cmd;
> +	u8 zeros[2];
> +	u8 id;
> +	u8 cmd;
> +	u8 reg;
> +	u8 val;
> +	u8 midi_end;
> +};
> +
> +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
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver

[Ondrej Zary]

On Monday 06 October 2014, Takashi Iwai wrote:
> At Mon, 29 Sep 2014 23:44:39 +0200,
>
> Ondrej Zary wrote:
> > Add support for ES938 3-D Audio Effects Processor found on some ES18xx
> > (and possibly other) sound cards, doing bass/treble and 3D control.
>
> As this is seen only on es18xx, we don't need to make it as an
> individual module.  Just merge into snd-es18xx module.

ES938 does not depend on ES18xx and can be connected to any device with MIDI 
interface. Maybe there are some other cards with this chip.

> > ES938 is controlled by MIDI SysEx commands sent through card's MPU401
> > port.
> >
> > The code opens/closes the MIDI device everytime a mixer control value is
> > changed so userspace apps can still use the MIDI port. Changing the mixer
> > controls will fail when the MIDI port is open by an application.
>
> Why the kernel open/close is needed for snd_es938_init(), too?
> Also, adding controls after snd_card_register() call isn't good in
> most cases.

chip->rfile must be a valid snd_rawmidi_file handle for snd_es938_write_reg() 
and snd_es938_write_reg() to work.

IIRC, there is a chicken-and-egg problem with adding controls and sound card's 
MIDI interface initialization:
we need a working MIDI interface to detect ES938 presence - that's after 
snd_card_register() is called

vs.

we need to add conttrols before snd_card_register() is called

>
> thanks,
>
> Takashi
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > ---
> > Changes in v3:
> >  - constify buf argument to snd_rawmidi_kernel_write
> >  - remove double if from probe
> >  - spelling fix
> > Changes in v2:
> >  - debug message when ES938 detected
> >  - reworked sysex messages to use structs
> >  - ktime instead of jiffies for timeout
> > ---
> >  sound/isa/Kconfig  |    4 +
> >  sound/isa/Makefile |    2 +
> >  sound/isa/es18xx.c |   13 +++-
> >  sound/isa/es938.c  |  217
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++ sound/isa/es938.h  |
> >   48 ++++++++++++
> >  5 files changed, 283 insertions(+), 1 deletion(-)
> >  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 0216475..db0b678 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 6faaac6..72dad69 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;
> > @@ -2167,7 +2169,16 @@ 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;
> > +
> > +	/* no error if this fails because ES938 is optional */
> > +	if (mpu_port[dev] > 0 && mpu_port[dev] != SNDRV_AUTO_PORT &&
> > +				snd_es938_init(&chip->es938, card, 0, 0) == 0)
> > +		snd_printk(KERN_DEBUG "found ES938 audio processor\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..75d1fbe
> > --- /dev/null
> > +++ b/sound/isa/es938.c
> > @@ -0,0 +1,217 @@
> > +/*
> > + *  Driver for ESS ES938 3-D Audio Effects Processor
> > + *  Copyright (c) 2014 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)
> > +{
> > +	int i = 0, res;
> > +	const struct snd_es938_sysex_reg req = {
> > +		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
> > +		.id = ES938_ID,
> > +		.cmd = ES938_CMD_REG_R,
> > +		.reg = reg,
> > +		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
> > +	};
> > +	struct snd_es938_sysex_regval reply = { };
> > +	u8 *p = (void *)&reply;
> > +	ktime_t end_time;
> > +
> > +	snd_rawmidi_kernel_write(chip->rfile.output,
> > +				(const void *)&req, sizeof(req));
> > +
> > +	end_time = ktime_add_ms(ktime_get(), 100);
> > +	while (i < sizeof(reply)) {
> > +		res = snd_rawmidi_kernel_read(chip->rfile.input, p + i,
> > +					      sizeof(reply) - i);
> > +		if (res > 0)
> > +			i += res;
> > +		if (ktime_after(ktime_get(), end_time))
> > +			return -1;
> > +	}
> > +
> > +	/* check if the reply is ours and has SYSEX_END at the end */
> > +	if (memcmp(&reply, &req, sizeof(req) - 1) ||
> > +				reply.midi_end != MIDI_CMD_COMMON_SYSEX_END)
> > +		return -1;
> > +
> > +	if (out)
> > +		*out = reply.val;
> > +
> > +	return 0;
> > +}
> > +
> > +static void snd_es938_write_reg(struct snd_es938 *chip, u8 reg, u8 val)
> > +{
> > +	const struct snd_es938_sysex_regval req = {
> > +		.midi_cmd = MIDI_CMD_COMMON_SYSEX,
> > +		.id = ES938_ID,
> > +		.cmd = ES938_CMD_REG_W,
> > +		.reg = reg,
> > +		.val = val,
> > +		.midi_end = MIDI_CMD_COMMON_SYSEX_END,
> > +	};
> > +
> > +	snd_rawmidi_kernel_write(chip->rfile.output,
> > +				(const void *)&req, sizeof(req));
> > +	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..08148ff
> > --- /dev/null
> > +++ b/sound/isa/es938.h
> > @@ -0,0 +1,48 @@
> > +#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;
> > +};
> > +
> > +struct snd_es938_sysex_reg {
> > +	u8 midi_cmd;
> > +	u8 zeros[2];
> > +	u8 id;
> > +	u8 cmd;
> > +	u8 reg;
> > +	u8 midi_end;
> > +};
> > +
> > +struct snd_es938_sysex_regval {
> > +	u8 midi_cmd;
> > +	u8 zeros[2];
> > +	u8 id;
> > +	u8 cmd;
> > +	u8 reg;
> > +	u8 val;
> > +	u8 midi_end;
> > +};
> > +
> > +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
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Ondrej Zary

Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver

[Takashi Iwai]

At Mon, 6 Oct 2014 15:55:18 +0200,
Ondrej Zary wrote:
> 
> On Monday 06 October 2014, Takashi Iwai wrote:
> > At Mon, 29 Sep 2014 23:44:39 +0200,
> >
> > Ondrej Zary wrote:
> > > Add support for ES938 3-D Audio Effects Processor found on some ES18xx
> > > (and possibly other) sound cards, doing bass/treble and 3D control.
> >
> > As this is seen only on es18xx, we don't need to make it as an
> > individual module.  Just merge into snd-es18xx module.
> 
> ES938 does not depend on ES18xx and can be connected to any device with MIDI 
> interface. Maybe there are some other cards with this chip.

Please prove it :)

> > > ES938 is controlled by MIDI SysEx commands sent through card's MPU401
> > > port.
> > >
> > > The code opens/closes the MIDI device everytime a mixer control value is
> > > changed so userspace apps can still use the MIDI port. Changing the mixer
> > > controls will fail when the MIDI port is open by an application.
> >
> > Why the kernel open/close is needed for snd_es938_init(), too?
> > Also, adding controls after snd_card_register() call isn't good in
> > most cases.
> 
> chip->rfile must be a valid snd_rawmidi_file handle for snd_es938_write_reg() 
> and snd_es938_write_reg() to work.
> 
> IIRC, there is a chicken-and-egg problem with adding controls and sound card's 
> MIDI interface initialization:
> we need a working MIDI interface to detect ES938 presence - that's after 
> snd_card_register() is called
> 
> vs.
> 
> we need to add conttrols before snd_card_register() is called

Well, then the interface is somehow abused.  The
snd_rawmidi_kernel_read/write() are mostly only for sequencer core 
stuff, and they aren't supposed to be used by the individual driver.

That said, I'm not willing to merge the patch in the current form.
One of the reason is that this can be implemented pretty easily in
user-space.  Another is that it's a deadly old device, and likely only
handful boards are still running in the world, thus no much motivation
to bloat the kernel code for that.  We're even discussing to get rid
of ISA codes from the kernel tree nowadays.

Sorry, the patch was submitted 10 years too late.


Takashi

Workable vintage driver support mechanism? (Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver)

[Andreas Mohr]

On Mon, Oct 06, 2014 at 04:13:12PM +0200, Takashi Iwai wrote:
> At Mon, 6 Oct 2014 15:55:18 +0200,
> Ondrej Zary wrote:
> > ES938 does not depend on ES18xx and can be connected to any device with MIDI 
> > interface. Maybe there are some other cards with this chip.
> 
> Please prove it :)

:(

> That said, I'm not willing to merge the patch in the current form.
> One of the reason is that this can be implemented pretty easily in
> user-space.  Another is that it's a deadly old device, and likely only
> handful boards are still running in the world, thus no much motivation
> to bloat the kernel code for that.  We're even discussing to get rid
> of ISA codes from the kernel tree nowadays.
> 
> Sorry, the patch was submitted 10 years too late.

:((

Given that I hate the forces that are increasingly coming into play here
(but of course I do see the bigger picture, namely of kernel tree bloat),
I think I have an idea which might be useful to accept:
for every piece of sufficiently "vintage" submission,
people would be tasked with offering (or somehow ensuring)
a sufficiently closely time-related cleanup in other places.

Thus, given a "diffstat penalty" (lines added minus lines removed)
of the vintage support patch:

Additionally offer a cleanup patch
which gets rid of redundantly implemented kernel tree functionality:
- for 15 year old devices: 0.5 * diffstat_penalty
- for 20 year old devices: 1.0 * diffstat_penalty
- for 25 year old devices: 1.5 * diffstat_penalty
- for 30 year old devices: whaaa?

Hmm?

Andreas Mohr

Re: [alsa-devel] Workable vintage driver support mechanism? (Re: [PATCH v3] ES938 support for ES18xx driver)

[Clemens Ladisch]

Andreas Mohr wrote:
> I think I have an idea which might be useful to accept:
> for every piece of sufficiently "vintage" submission,
> people would be tasked with offering (or somehow ensuring)
> a sufficiently closely time-related cleanup in other places.

The problem that such a new driver imposes is not a one-time reduction
in overall kernel quality, but the ongoing maintenance effort.


Regards,
Clemens

Re: Workable vintage driver support mechanism? (Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver)

[Takashi Iwai]

At Mon, 6 Oct 2014 20:41:50 +0200,
Andreas Mohr wrote:
> 
> On Mon, Oct 06, 2014 at 04:13:12PM +0200, Takashi Iwai wrote:
> > At Mon, 6 Oct 2014 15:55:18 +0200,
> > Ondrej Zary wrote:
> > > ES938 does not depend on ES18xx and can be connected to any device with MIDI 
> > > interface. Maybe there are some other cards with this chip.
> > 
> > Please prove it :)
> 
> :(
> 
> > That said, I'm not willing to merge the patch in the current form.
> > One of the reason is that this can be implemented pretty easily in
> > user-space.  Another is that it's a deadly old device, and likely only
> > handful boards are still running in the world, thus no much motivation
> > to bloat the kernel code for that.  We're even discussing to get rid
> > of ISA codes from the kernel tree nowadays.
> > 
> > Sorry, the patch was submitted 10 years too late.
> 
> :((
> 
> Given that I hate the forces that are increasingly coming into play here
> (but of course I do see the bigger picture, namely of kernel tree bloat),
> I think I have an idea which might be useful to accept:
> for every piece of sufficiently "vintage" submission,
> people would be tasked with offering (or somehow ensuring)
> a sufficiently closely time-related cleanup in other places.
> 
> Thus, given a "diffstat penalty" (lines added minus lines removed)
> of the vintage support patch:
> 
> Additionally offer a cleanup patch
> which gets rid of redundantly implemented kernel tree functionality:
> - for 15 year old devices: 0.5 * diffstat_penalty
> - for 20 year old devices: 1.0 * diffstat_penalty
> - for 25 year old devices: 1.5 * diffstat_penalty
> - for 30 year old devices: whaaa?

The kernel bloat is one side, and your proposal would help to keep
figure.  But another side is the maintenance, as Clemens pointed.
It's obviously more difficult for exotic hardware devices.  So, it's
really case-by-case.

Speaking of this particular patch, however, the reason for NAK isn't
(only) the vintage.  There are other concerns as I raised, especially
the ugliness of the interface it uses, and this doesn't have to be a
kernel stuff at all.  You could implement it as a user-space alsa-lib
plugin as well.


Takashi

Re: [alsa-devel] Workable vintage driver support mechanism? (Re: [PATCH v3] ES938 support for ES18xx driver)

[One Thousand Gnomes]

On Mon, 06 Oct 2014 21:41:20 +0200
Clemens Ladisch <clemens@ladisch.de> wrote:

> Andreas Mohr wrote:
> > I think I have an idea which might be useful to accept:
> > for every piece of sufficiently "vintage" submission,
> > people would be tasked with offering (or somehow ensuring)
> > a sufficiently closely time-related cleanup in other places.
> 
> The problem that such a new driver imposes is not a one-time reduction
> in overall kernel quality, but the ongoing maintenance effort.

Vintage is not IMHO a useful test. We have plenty of vintage hardware
with active hands-on maintainers which causes no problem, and plenty of
modern drivers with basically no maintainer that causes endless problems.

I think there is much merit in the Debian approach - if it's not got a
maintainer, and nobody is willing to take it on, throw it out. If it's
got a maintainer leave it in.

Economics will resolve the rest of the problem reasonably efficiently. If
someone cares enough about using it then they'll figure out how to keep
it maintained.

Alan

Re: Workable vintage driver support mechanism? (Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver)

[Ondrej Zary]

On Tuesday 07 October 2014 07:32:18 Takashi Iwai wrote:
> At Mon, 6 Oct 2014 20:41:50 +0200,
>
> Andreas Mohr wrote:
> > On Mon, Oct 06, 2014 at 04:13:12PM +0200, Takashi Iwai wrote:
> > > At Mon, 6 Oct 2014 15:55:18 +0200,
> > >
> > > Ondrej Zary wrote:
> > > > ES938 does not depend on ES18xx and can be connected to any device
> > > > with MIDI interface. Maybe there are some other cards with this chip.
> > >
> > > Please prove it :)
> > >
> > :(
> > :
> > > That said, I'm not willing to merge the patch in the current form.
> > > One of the reason is that this can be implemented pretty easily in
> > > user-space.  Another is that it's a deadly old device, and likely only
> > > handful boards are still running in the world, thus no much motivation
> > > to bloat the kernel code for that.  We're even discussing to get rid
> > > of ISA codes from the kernel tree nowadays.
> > >
> > > Sorry, the patch was submitted 10 years too late.
> > >
> > :((
> >
> > Given that I hate the forces that are increasingly coming into play here
> > (but of course I do see the bigger picture, namely of kernel tree bloat),
> > I think I have an idea which might be useful to accept:
> > for every piece of sufficiently "vintage" submission,
> > people would be tasked with offering (or somehow ensuring)
> > a sufficiently closely time-related cleanup in other places.
> >
> > Thus, given a "diffstat penalty" (lines added minus lines removed)
> > of the vintage support patch:
> >
> > Additionally offer a cleanup patch
> > which gets rid of redundantly implemented kernel tree functionality:
> > - for 15 year old devices: 0.5 * diffstat_penalty
> > - for 20 year old devices: 1.0 * diffstat_penalty
> > - for 25 year old devices: 1.5 * diffstat_penalty
> > - for 30 year old devices: whaaa?
>
> The kernel bloat is one side, and your proposal would help to keep
> figure.  But another side is the maintenance, as Clemens pointed.
> It's obviously more difficult for exotic hardware devices.  So, it's
> really case-by-case.
>
> Speaking of this particular patch, however, the reason for NAK isn't
> (only) the vintage.  There are other concerns as I raised, especially
> the ugliness of the interface it uses, and this doesn't have to be a
> kernel stuff at all.  You could implement it as a user-space alsa-lib
> plugin as well.

So do I understand correctly that there is no (right) way for a kernel driver 
to issue MIDI commands?

Is it possible for an alsa-lib plugin to automatically load when an ES18xx 
sound card is present to check for ES938 (without repeating the detection on 
each application launch)?

-- 
Ondrej Zary

Re: Workable vintage driver support mechanism? (Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver)

[Takashi Iwai]

At Sun, 12 Oct 2014 23:19:04 +0200,
Ondrej Zary wrote:
> 
> 
> 
> On Tuesday 07 October 2014 07:32:18 Takashi Iwai wrote:
> > At Mon, 6 Oct 2014 20:41:50 +0200,
> >
> > Andreas Mohr wrote:
> > > On Mon, Oct 06, 2014 at 04:13:12PM +0200, Takashi Iwai wrote:
> > > > At Mon, 6 Oct 2014 15:55:18 +0200,
> > > >
> > > > Ondrej Zary wrote:
> > > > > ES938 does not depend on ES18xx and can be connected to any device
> > > > > with MIDI interface. Maybe there are some other cards with this chip.
> > > >
> > > > Please prove it :)
> > > >
> > > :(
> > > :
> > > > That said, I'm not willing to merge the patch in the current form.
> > > > One of the reason is that this can be implemented pretty easily in
> > > > user-space.  Another is that it's a deadly old device, and likely only
> > > > handful boards are still running in the world, thus no much motivation
> > > > to bloat the kernel code for that.  We're even discussing to get rid
> > > > of ISA codes from the kernel tree nowadays.
> > > >
> > > > Sorry, the patch was submitted 10 years too late.
> > > >
> > > :((
> > >
> > > Given that I hate the forces that are increasingly coming into play here
> > > (but of course I do see the bigger picture, namely of kernel tree bloat),
> > > I think I have an idea which might be useful to accept:
> > > for every piece of sufficiently "vintage" submission,
> > > people would be tasked with offering (or somehow ensuring)
> > > a sufficiently closely time-related cleanup in other places.
> > >
> > > Thus, given a "diffstat penalty" (lines added minus lines removed)
> > > of the vintage support patch:
> > >
> > > Additionally offer a cleanup patch
> > > which gets rid of redundantly implemented kernel tree functionality:
> > > - for 15 year old devices: 0.5 * diffstat_penalty
> > > - for 20 year old devices: 1.0 * diffstat_penalty
> > > - for 25 year old devices: 1.5 * diffstat_penalty
> > > - for 30 year old devices: whaaa?
> >
> > The kernel bloat is one side, and your proposal would help to keep
> > figure.  But another side is the maintenance, as Clemens pointed.
> > It's obviously more difficult for exotic hardware devices.  So, it's
> > really case-by-case.
> >
> > Speaking of this particular patch, however, the reason for NAK isn't
> > (only) the vintage.  There are other concerns as I raised, especially
> > the ugliness of the interface it uses, and this doesn't have to be a
> > kernel stuff at all.  You could implement it as a user-space alsa-lib
> > plugin as well.
> 
> So do I understand correctly that there is no (right) way for a kernel driver 
> to issue MIDI commands?

Not in the form as you used.  The driver could poke the internal I/O
instead, or introduce a better way to integrate without fs ops.  In
the latter case, we need to introduce a new API, and it's not worth
only for this driver, though.

> Is it possible for an alsa-lib plugin to automatically load when an ES18xx 
> sound card is present to check for ES938 (without repeating the detection on 
> each application launch)?

A good question.  IMO, it's easier to let user choose via the static
configuration in this case.  It's an ISA device, after all, no hotplug
device.  You can fiddle with udev and create a config on the fly at
boot time, too.


Takashi