Message-ID: <s5h7g0dzgd5.wl-tiwai@suse.de>
Date:	Mon, 06 Oct 2014 11:39:50 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Ondrej Zary <linux@...nbow-software.org>
Cc:	alsa-devel@...a-project.org,
	Kernel development list <linux-kernel@...r.kernel.org>,
	Andreas Mohr <andi@...as.de>
Subject: Re: [alsa-devel] [PATCH v3] ES938 support for ES18xx driver

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@...nbow-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@...a-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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists