diff mbox

ES938 support for ES18xx driver

Message ID 1411755805-8165-1-git-send-email-linux@rainbow-software.org (mailing list archive)
State Rejected
Delegated to: Takashi Iwai
Headers show

Commit Message

Ondrej Zary Sept. 26, 2014, 6:23 p.m. UTC
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>
---
 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 deletion(-)
 create mode 100644 sound/isa/es938.c
 create mode 100644 sound/isa/es938.h

Comments

Andreas Mohr Sept. 26, 2014, 9:12 p.m. UTC | #1
Hi,

On Fri, Sep 26, 2014 at 08:23:25PM +0200, Ondrej Zary wrote:
> -	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)

Should probably add comment here, like:
		/* Since ES938 support is an optional component,
		   we choose to not take into account its failure result codes
		   as reason for fatal abort of soundcard registration. */
> +		snd_es938_init(&chip->es938, card, 0, 0);
> +
> +	return 0;
>  }

OTOH completely ignoring es938 result might be undesirable,
thus it could be suitable to at least add announcing failure via a dev_warn()
in addition to the comment.


> +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));

long snd_rawmidi_kernel_write(struct snd_rawmidi_substream *substream,
                              const unsigned char *buf, long count);

--> constify sysex!

Also, there is a "unsigned char" vs. "u8" type ""violation"".

> +	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;
> +	}

Could we somehow manage to get rid of basing the calculation on jiffies?
AFAIK the kernel is strongly steering away from jiffies-based
calculation (normalizing things on ktime instead, AFAIK, or seconds-based),
for hopefully good reasons...
(ktime is an abstract, future-proof "kernel time",
and "msecs" is an obvious human-parseable unit,
whereas jiffies is a rather hardware-specific and ugly thingy).

> +
> +	/* check reply */
> +	if (memcmp(buf, sysex, 6) || buf[7] != MIDI_CMD_COMMON_SYSEX_END)
> +		return -1;
> +
> +	if (out)
> +		*out = buf[6];

A wee bit too many literals here for my taste...
(but it's not an overly clear-cut case of a simple direct sizeof(x)
replacement OTOH, so I'm still left wondering)

> +
> +	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 };

See case above.

> +static const DECLARE_TLV_DB_SCALE(db_scale_tone, -900, 300, 0);

Yup, I also wanted to make use of TLV controls in my driver -
good to see that you're doing that already :)

> +
> +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;
> +	}

I've been told the Fashionable Logger API Of The Week is dev_warn(),
since it's providing proper implicitly dev context decorated logging,
thus changing all affected code might be useful.
OTOH prior code happens to be making use of snd_printk currently,
so there should be an earlier commit to first convert existing parts to new API,
and then have your ES938 commit, doing a dev_warn();
or simply have some resignation and do add this line as snd_printk(), too ;)
(for a later commit to then change everything to dev_warn())


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

Thanks for such an interesting addition,

Andreas Mohr
diff mbox

Patch

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..139cd3b 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,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..ea1b25c
--- /dev/null
+++ b/sound/isa/es938.c
@@ -0,0 +1,203 @@ 
+/*
+ *  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)
+{
+	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);