Message-Id: <201410061555.19007.linux@rainbow-software.org>
Date:	Mon, 6 Oct 2014 15:55:18 +0200
From:	Ondrej Zary <linux@...nbow-software.org>
To:	Takashi Iwai <tiwai@...e.de>
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

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@...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/



-- 
Ondrej Zary
--
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