[PATCH v3 0/4] stk1160: Let the driver setup the device's internal AC97 codec

[PATCH v3 0/4] stk1160: Let the driver setup the device's internal AC97 codec

[Marcel Hasler]

This patchset is a result of my attempt to fix a bug (https://bugzilla.kernel.org/show_bug.cgi?id=180071) that eventually turned out to be caused by a missing quirk in snd-usb-audio. My idea was to remove the AC97 interface and setup the codec using the same values and in the same order as the Windows driver does, hoping there might be some "magic" sequence that would make the sound work the way it should. Although this didn't help to fix the problem, I found these changes to be useful nevertheless.

IMHO, having all of the AC97 codec's channels exposed to userspace is confusing since most of them have no meaning for this device anyway. Changing these values in alsamixer has either no effect at all or may even reduce the sound quality since it can actually increase the line-in DC offset (slightly).

In addition, having to re-select the correct capture channel everytime the device has been plugged in is annoying. At least on my systems the mixer setup is only saved if the device is plugged in during shutdown/reboot. I also get error messages in my kernel log when I unplug the device because some process (probably the AC97 driver) ist trying to read from the device after it has been removed. Either way the device should work out-of-the-box without the need for the user to manually setup channels.

The first patch in the set therefore removes the 'stk1160-mixer' and lets the driver setup the AC97 codec using the same values as the Windows driver. Although some of the values seem to be defaults I let the driver set them either way, just to be sure.

The second patch adds a check to determine whether the device is strapped to use the internal 8-bit ADC or an external chip. There's currently no check in place to determine whether the device uses AC-link or I2S, but then again I haven't heard of any of these devices actually using an I2S chip. If the device uses the internal ADC the AC97 setup can be skipped. I implemented the check inside stk1160-ac97. It could just as well be in stk1160-core but this way just seemed cleaner. If at some point the need arises to check other power-on strap values, it might make sense to refactor this then.

The third patch adds a new module parameter for setting the record gain manually since the AC97 chip is no longer exposed to userspace. The Windows driver doesn't allow this value to be changed but instead always sets it to 8 (of 15). While this should be fine for most users, some may prefer something higher.

The fourth patch addresses an issue when reading from the AC97 chip too soon, resulting in corrupt data.

Changes from version 2:
* Added copyright notice
* Added defines for POSVA bytes and bits
* Added check for ACDOUT bit to determine whether audio is disabled completely
* Removed info output for gain setting
* Added fourth patch which had been submitted independently before
* Expanded comment on AC97 read delay

Marcel Hasler (4):
  stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
  stk1160: Check whether to use AC97 codec.
  stk1160: Add module param for setting the record gain.
  stk1160: Give the chip some time to retrieve data from AC97 codec.

 drivers/media/usb/stk1160/Kconfig        |  10 +-
 drivers/media/usb/stk1160/Makefile       |   4 +-
 drivers/media/usb/stk1160/stk1160-ac97.c | 159 +++++++++++++++++--------------
 drivers/media/usb/stk1160/stk1160-core.c |   8 +-
 drivers/media/usb/stk1160/stk1160-reg.h  |   8 ++
 drivers/media/usb/stk1160/stk1160.h      |   9 +-
 6 files changed, 98 insertions(+), 100 deletions(-)

-- 
2.10.2

[PATCH v3 1/4] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.

[Marcel Hasler]

Exposing all the channels of the device's internal AC97 codec to userspace is unnecessary and
confusing. Instead the driver should setup the codec with proper values. This patch removes the
mixer and sets up the codec using optimal values, i.e. the same values set by the Windows
driver. This also makes the device work out-of-the-box, without the need for the user to
reconfigure the device every time it's plugged in.

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/media/usb/stk1160/Kconfig        |  10 +--
 drivers/media/usb/stk1160/Makefile       |   4 +-
 drivers/media/usb/stk1160/stk1160-ac97.c | 124 ++++++++++++-------------------
 drivers/media/usb/stk1160/stk1160-core.c |   5 +-
 drivers/media/usb/stk1160/stk1160.h      |   9 +--
 5 files changed, 50 insertions(+), 102 deletions(-)

diff --git a/drivers/media/usb/stk1160/Kconfig b/drivers/media/usb/stk1160/Kconfig
index 95584c1..22dff4f 100644
--- a/drivers/media/usb/stk1160/Kconfig
+++ b/drivers/media/usb/stk1160/Kconfig
@@ -8,17 +8,9 @@ config VIDEO_STK1160_COMMON
 	  To compile this driver as a module, choose M here: the
 	  module will be called stk1160
 
-config VIDEO_STK1160_AC97
-	bool "STK1160 AC97 codec support"
-	depends on VIDEO_STK1160_COMMON && SND
-
-	---help---
-	  Enables AC97 codec support for stk1160 driver.
-
 config VIDEO_STK1160
 	tristate
-	depends on (!VIDEO_STK1160_AC97 || (SND='n') || SND) && VIDEO_STK1160_COMMON
+	depends on VIDEO_STK1160_COMMON
 	default y
 	select VIDEOBUF2_VMALLOC
 	select VIDEO_SAA711X
-	select SND_AC97_CODEC if SND
diff --git a/drivers/media/usb/stk1160/Makefile b/drivers/media/usb/stk1160/Makefile
index dfe3e90..42d0546 100644
--- a/drivers/media/usb/stk1160/Makefile
+++ b/drivers/media/usb/stk1160/Makefile
@@ -1,10 +1,8 @@
-obj-stk1160-ac97-$(CONFIG_VIDEO_STK1160_AC97) := stk1160-ac97.o
-
 stk1160-y := 	stk1160-core.o \
 		stk1160-v4l.o \
 		stk1160-video.o \
 		stk1160-i2c.o \
-		$(obj-stk1160-ac97-y)
+		stk1160-ac97.o
 
 obj-$(CONFIG_VIDEO_STK1160) += stk1160.o
 
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 2dd308f..63ade1b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -4,6 +4,9 @@
  * Copyright (C) 2012 Ezequiel Garcia
  * <elezegarcia--a.t--gmail.com>
  *
+ * Copyright (C) 2016 Marcel Hasler
+ * <mahasler--a.t--gmail.com>
+ *
  * Based on Easycap driver by R.M. Thomas
  *	Copyright (C) 2010 R.M. Thomas
  *	<rmthomas--a.t--sciolus.org>
@@ -21,19 +24,12 @@
  */
 
 #include <linux/module.h>
-#include <sound/core.h>
-#include <sound/initval.h>
-#include <sound/ac97_codec.h>
 
 #include "stk1160.h"
 #include "stk1160-reg.h"
 
-static struct snd_ac97 *stk1160_ac97;
-
-static void stk1160_write_ac97(struct snd_ac97 *ac97, u16 reg, u16 value)
+static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
 {
-	struct stk1160 *dev = ac97->private_data;
-
 	/* Set codec register address */
 	stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
 
@@ -48,9 +44,9 @@ static void stk1160_write_ac97(struct snd_ac97 *ac97, u16 reg, u16 value)
 	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
 }
 
-static u16 stk1160_read_ac97(struct snd_ac97 *ac97, u16 reg)
+#ifdef DEBUG
+static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
 {
-	struct stk1160 *dev = ac97->private_data;
 	u8 vall = 0;
 	u8 valh = 0;
 
@@ -70,81 +66,53 @@ static u16 stk1160_read_ac97(struct snd_ac97 *ac97, u16 reg)
 	return (valh << 8) | vall;
 }
 
-static void stk1160_reset_ac97(struct snd_ac97 *ac97)
+void stk1160_ac97_dump_regs(struct stk1160 *dev)
 {
-	struct stk1160 *dev = ac97->private_data;
-	/* Two-step reset AC97 interface and hardware codec */
-	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
-	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x88);
+	u16 value;
 
-	/* Set 16-bit audio data and choose L&R channel*/
-	stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01);
-}
+	value = stk1160_read_ac97(dev, 0x12); /* CD volume */
+	stk1160_dbg("0x12 == 0x%04x", value);
 
-static struct snd_ac97_bus_ops stk1160_ac97_ops = {
-	.read	= stk1160_read_ac97,
-	.write	= stk1160_write_ac97,
-	.reset	= stk1160_reset_ac97,
-};
+	value = stk1160_read_ac97(dev, 0x10); /* Line-in volume */
+	stk1160_dbg("0x10 == 0x%04x", value);
 
-int stk1160_ac97_register(struct stk1160 *dev)
-{
-	struct snd_card *card = NULL;
-	struct snd_ac97_bus *ac97_bus;
-	struct snd_ac97_template ac97_template;
-	int rc;
+	value = stk1160_read_ac97(dev, 0x0e); /* MIC volume (mono) */
+	stk1160_dbg("0x0e == 0x%04x", value);
 
-	/*
-	 * Just want a card to access ac96 controls,
-	 * the actual capture interface will be handled by snd-usb-audio
-	 */
-	rc = snd_card_new(dev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
-			  THIS_MODULE, 0, &card);
-	if (rc < 0)
-		return rc;
-
-	/* TODO: I'm not sure where should I get these names :-( */
-	snprintf(card->shortname, sizeof(card->shortname),
-		 "stk1160-mixer");
-	snprintf(card->longname, sizeof(card->longname),
-		 "stk1160 ac97 codec mixer control");
-	strlcpy(card->driver, dev->dev->driver->name, sizeof(card->driver));
-
-	rc = snd_ac97_bus(card, 0, &stk1160_ac97_ops, NULL, &ac97_bus);
-	if (rc)
-		goto err;
-
-	/* We must set private_data before calling snd_ac97_mixer */
-	memset(&ac97_template, 0, sizeof(ac97_template));
-	ac97_template.private_data = dev;
-	ac97_template.scaps = AC97_SCAP_SKIP_MODEM;
-	rc = snd_ac97_mixer(ac97_bus, &ac97_template, &stk1160_ac97);
-	if (rc)
-		goto err;
-
-	dev->snd_card = card;
-	rc = snd_card_register(card);
-	if (rc)
-		goto err;
-
-	return 0;
-
-err:
-	dev->snd_card = NULL;
-	snd_card_free(card);
-	return rc;
+	value = stk1160_read_ac97(dev, 0x16); /* Aux volume */
+	stk1160_dbg("0x16 == 0x%04x", value);
+
+	value = stk1160_read_ac97(dev, 0x1a); /* Record select */
+	stk1160_dbg("0x1a == 0x%04x", value);
+
+	value = stk1160_read_ac97(dev, 0x02); /* Master volume */
+	stk1160_dbg("0x02 == 0x%04x", value);
+
+	value = stk1160_read_ac97(dev, 0x1c); /* Record gain */
+	stk1160_dbg("0x1c == 0x%04x", value);
 }
+#endif
 
-int stk1160_ac97_unregister(struct stk1160 *dev)
+void stk1160_ac97_setup(struct stk1160 *dev)
 {
-	struct snd_card *card = dev->snd_card;
-
-	/*
-	 * We need to check usb_device,
-	 * because ac97 release attempts to communicate with codec
-	 */
-	if (card && dev->udev)
-		snd_card_free(card);
+	/* Two-step reset AC97 interface and hardware codec */
+	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
+	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
 
-	return 0;
+	/* Set 16-bit audio data and choose L&R channel*/
+	stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01);
+	stk1160_write_reg(dev, STK1160_AC97CTL_1 + 3, 0x00);
+
+	/* Setup channels */
+	stk1160_write_ac97(dev, 0x12, 0x8808); /* CD volume */
+	stk1160_write_ac97(dev, 0x10, 0x0808); /* Line-in volume */
+	stk1160_write_ac97(dev, 0x0e, 0x0008); /* MIC volume (mono) */
+	stk1160_write_ac97(dev, 0x16, 0x0808); /* Aux volume */
+	stk1160_write_ac97(dev, 0x1a, 0x0404); /* Record select */
+	stk1160_write_ac97(dev, 0x02, 0x0000); /* Master volume */
+	stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
+
+#ifdef DEBUG
+	stk1160_ac97_dump_regs(dev);
+#endif
 }
diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
index bc02947..f3c9b8a 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -373,7 +373,7 @@ static int stk1160_probe(struct usb_interface *interface,
 	/* select default input */
 	stk1160_select_input(dev);
 
-	stk1160_ac97_register(dev);
+	stk1160_ac97_setup(dev);
 
 	rc = stk1160_video_register(dev);
 	if (rc < 0)
@@ -411,9 +411,6 @@ static void stk1160_disconnect(struct usb_interface *interface)
 	/* Here is the only place where isoc get released */
 	stk1160_uninit_isoc(dev);
 
-	/* ac97 unregister needs to be done before usb_device is cleared */
-	stk1160_ac97_unregister(dev);
-
 	stk1160_clear_queue(dev);
 
 	video_unregister_device(&dev->vdev);
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index 1ed1cc4..e85e12e 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -197,11 +197,4 @@ int stk1160_read_reg_req_len(struct stk1160 *dev, u8 req, u16 reg,
 void stk1160_select_input(struct stk1160 *dev);
 
 /* Provided by stk1160-ac97.c */
-#ifdef CONFIG_VIDEO_STK1160_AC97
-int stk1160_ac97_register(struct stk1160 *dev);
-int stk1160_ac97_unregister(struct stk1160 *dev);
-#else
-static inline int stk1160_ac97_register(struct stk1160 *dev) { return 0; }
-static inline int stk1160_ac97_unregister(struct stk1160 *dev) { return 0; }
-#endif
-
+void stk1160_ac97_setup(struct stk1160 *dev);
-- 
2.10.2

[PATCH v3 2/4] stk1160: Check whether to use AC97 codec.

[Marcel Hasler]

Some STK1160-based devices use the chip's internal 8-bit ADC. This is configured through a strap
pin. The value of this and other pins can be read through the POSVA register. If the internal
ADC is used, or if audio is disabled altogether, there's no point trying to setup the AC97 codec.

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 26 ++++++++++++++++++++++++++
 drivers/media/usb/stk1160/stk1160-core.c |  3 +--
 drivers/media/usb/stk1160/stk1160-reg.h  |  8 ++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 63ade1b..95648ac 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -93,8 +93,34 @@ void stk1160_ac97_dump_regs(struct stk1160 *dev)
 }
 #endif
 
+int stk1160_has_audio(struct stk1160 *dev)
+{
+	u8 value;
+
+	stk1160_read_reg(dev, STK1160_POSV_L, &value);
+	return !(value & STK1160_POSV_L_ACDOUT);
+}
+
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+	u8 value;
+
+	stk1160_read_reg(dev, STK1160_POSV_L, &value);
+	return !(value & STK1160_POSV_L_ACSYNC);
+}
+
 void stk1160_ac97_setup(struct stk1160 *dev)
 {
+	if (!stk1160_has_audio(dev)) {
+		stk1160_info("Device doesn't support audio, skipping AC97 setup.");
+		return;
+	}
+
+	if (!stk1160_has_ac97(dev)) {
+		stk1160_info("Device uses internal 8-bit ADC, skipping AC97 setup.");
+		return;
+	}
+
 	/* Two-step reset AC97 interface and hardware codec */
 	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
 	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c
index f3c9b8a..c86eb61 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -20,8 +20,7 @@
  *
  * TODO:
  *
- * 1. (Try to) detect if we must register ac97 mixer
- * 2. Support stream at lower speed: lower frame rate or lower frame size.
+ * 1. Support stream at lower speed: lower frame rate or lower frame size.
  *
  */
 
diff --git a/drivers/media/usb/stk1160/stk1160-reg.h b/drivers/media/usb/stk1160/stk1160-reg.h
index 81ff3a1..296a9e7 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -26,6 +26,14 @@
 /* Remote Wakup Control */
 #define STK1160_RMCTL			0x00c
 
+/* Power-on Strapping Data */
+#define STK1160_POSVA			0x010
+#define STK1160_POSV_L			0x010
+#define STK1160_POSV_M			0x011
+#define STK1160_POSV_H			0x012
+#define  STK1160_POSV_L_ACDOUT		BIT(3)
+#define  STK1160_POSV_L_ACSYNC		BIT(2)
+
 /*
  * Decoder Control Register:
  * This byte controls capture start/stop
-- 
2.10.2

[PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

Allow setting a custom record gain for the internal AC97 codec (if available). This can be
a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
driver also sets this to 8 without any possibility for changing it.

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 95648ac..60327af 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -28,6 +28,11 @@
 #include "stk1160.h"
 #include "stk1160-reg.h"
 
+static u8 gain = 8;
+
+module_param(gain, byte, 0444);
+MODULE_PARM_DESC(gain, "Set capture gain level if AC97 codec is available (0-15, default: 8)");
+
 static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
 {
 	/* Set codec register address */
@@ -136,7 +141,10 @@ void stk1160_ac97_setup(struct stk1160 *dev)
 	stk1160_write_ac97(dev, 0x16, 0x0808); /* Aux volume */
 	stk1160_write_ac97(dev, 0x1a, 0x0404); /* Record select */
 	stk1160_write_ac97(dev, 0x02, 0x0000); /* Master volume */
-	stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
+
+	/* Record gain */
+	gain = (gain > 15) ? 15 : gain;
+	stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain);
 
 #ifdef DEBUG
 	stk1160_ac97_dump_regs(dev);
-- 
2.10.2

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Mauro Carvalho Chehab]

Em Sun, 27 Nov 2016 12:11:48 +0100
Marcel Hasler <mahasler@gmail.com> escreveu:

> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> driver also sets this to 8 without any possibility for changing it.

The problem of removing the mixer is that you need this kind of
crap to setup the volumes on a non-standard way.

NACK.

Instead, keep the alsa mixer. The way other drivers do (for example, 
em28xx) is that they configure the mixer when an input is selected,
increasing the volume of the active audio channel to 100% and muting
the other audio channels. Yet, as the alsa mixer is exported, users 
can change the mixer settings in runtime using some alsa (or pa)
mixer application.

> 
> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> ---
>  drivers/media/usb/stk1160/stk1160-ac97.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 95648ac..60327af 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -28,6 +28,11 @@
>  #include "stk1160.h"
>  #include "stk1160-reg.h"
>  
> +static u8 gain = 8;
> +
> +module_param(gain, byte, 0444);
> +MODULE_PARM_DESC(gain, "Set capture gain level if AC97 codec is available (0-15, default: 8)");
> +
>  static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>  {
>  	/* Set codec register address */
> @@ -136,7 +141,10 @@ void stk1160_ac97_setup(struct stk1160 *dev)
>  	stk1160_write_ac97(dev, 0x16, 0x0808); /* Aux volume */
>  	stk1160_write_ac97(dev, 0x1a, 0x0404); /* Record select */
>  	stk1160_write_ac97(dev, 0x02, 0x0000); /* Master volume */
> -	stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
> +
> +	/* Record gain */
> +	gain = (gain > 15) ? 15 : gain;
> +	stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain);
>  
>  #ifdef DEBUG
>  	stk1160_ac97_dump_regs(dev);



Thanks,
Mauro

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Ezequiel Garcia]

On 2 December 2016 at 08:05, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> Em Sun, 27 Nov 2016 12:11:48 +0100
> Marcel Hasler <mahasler@gmail.com> escreveu:
>
>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>> driver also sets this to 8 without any possibility for changing it.
>
> The problem of removing the mixer is that you need this kind of
> crap to setup the volumes on a non-standard way.
>

Right, that's a good point.

> NACK.
>
> Instead, keep the alsa mixer. The way other drivers do (for example,
> em28xx) is that they configure the mixer when an input is selected,
> increasing the volume of the active audio channel to 100% and muting
> the other audio channels. Yet, as the alsa mixer is exported, users
> can change the mixer settings in runtime using some alsa (or pa)
> mixer application.
>

Yeah, the AC97 mixer we are currently leveraging
exposes many controls that have no meaning in this device,
so removing that still looks like an improvement.

I guess the proper way is creating our own mixer
(not using snd_ac97_mixer)  exposing only the record
gain knob.

Marcel, what do you think?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

Hello

2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:
>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>
>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>> driver also sets this to 8 without any possibility for changing it.
>>
>> The problem of removing the mixer is that you need this kind of
>> crap to setup the volumes on a non-standard way.
>>
>
> Right, that's a good point.
>
>> NACK.
>>
>> Instead, keep the alsa mixer. The way other drivers do (for example,
>> em28xx) is that they configure the mixer when an input is selected,
>> increasing the volume of the active audio channel to 100% and muting
>> the other audio channels. Yet, as the alsa mixer is exported, users
>> can change the mixer settings in runtime using some alsa (or pa)
>> mixer application.
>>
>
> Yeah, the AC97 mixer we are currently leveraging
> exposes many controls that have no meaning in this device,
> so removing that still looks like an improvement.
>
> I guess the proper way is creating our own mixer
> (not using snd_ac97_mixer)  exposing only the record
> gain knob.
>
> Marcel, what do you think?
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar

As I have written before, the recording gain isn't actually meant to
be changed by the user. In the official Windows driver this value is
hard-coded to 8 and cannot be changed in any way. And there really is
no good reason why anyone should need to mess with it in the first
place. The default value will give the best results in pretty much all
cases and produces approximately the same volume as the internal 8-bit
ADC whose gain cannot be changed at all, not even by a driver.

I had considered writing a mixer but chose not to. If the gain setting
is openly exposed to mixer applications, how do you tell the users
that the value set by the driver already is the optimal and
recommended value and that they shouldn't mess with the controls
unless they really have to? By having a module parameter, this setting
is practically hidden from the normal user but still is available to
power-users if they think they really need it. In the end it's really
just a compromise between hiding it completely and exposing it openly.
Also, this way the driver guarantees reproducible results, since
there's no need to remember the positions of any volume sliders.

Either way, if you still think this solution is "crap", feel free to
modify the patches in any way you see fit. I've wasted too much time
on this already, and since I'm not being paid for it, I don't intend
to put any more effort into this.

Best regards
Marcel

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Ezequiel Garcia]

On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
> Hello
>
> 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> <mchehab@s-opensource.com> wrote:
>>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>>
>>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>>> driver also sets this to 8 without any possibility for changing it.
>>>
>>> The problem of removing the mixer is that you need this kind of
>>> crap to setup the volumes on a non-standard way.
>>>
>>
>> Right, that's a good point.
>>
>>> NACK.
>>>
>>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>> em28xx) is that they configure the mixer when an input is selected,
>>> increasing the volume of the active audio channel to 100% and muting
>>> the other audio channels. Yet, as the alsa mixer is exported, users
>>> can change the mixer settings in runtime using some alsa (or pa)
>>> mixer application.
>>>
>>
>> Yeah, the AC97 mixer we are currently leveraging
>> exposes many controls that have no meaning in this device,
>> so removing that still looks like an improvement.
>>
>> I guess the proper way is creating our own mixer
>> (not using snd_ac97_mixer)  exposing only the record
>> gain knob.
>>
>> Marcel, what do you think?
>> --
>> Ezequiel García, VanguardiaSur
>> www.vanguardiasur.com.ar
>
> As I have written before, the recording gain isn't actually meant to
> be changed by the user. In the official Windows driver this value is
> hard-coded to 8 and cannot be changed in any way. And there really is
> no good reason why anyone should need to mess with it in the first
> place. The default value will give the best results in pretty much all
> cases and produces approximately the same volume as the internal 8-bit
> ADC whose gain cannot be changed at all, not even by a driver.
>
> I had considered writing a mixer but chose not to. If the gain setting
> is openly exposed to mixer applications, how do you tell the users
> that the value set by the driver already is the optimal and
> recommended value and that they shouldn't mess with the controls
> unless they really have to? By having a module parameter, this setting
> is practically hidden from the normal user but still is available to
> power-users if they think they really need it. In the end it's really
> just a compromise between hiding it completely and exposing it openly.
> Also, this way the driver guarantees reproducible results, since
> there's no need to remember the positions of any volume sliders.
>

Hm, right. I've never changed the record gain, and it's true that it
doens't really improve the volume. So, I would be OK with having
a module parameter.

On the other side, we are exposing it currently, through the "Capture"
mixer control:

Simple mixer control 'Capture',0
  Capabilities: cvolume cswitch cswitch-joined
  Capture channels: Front Left - Front Right
  Limits: Capture 0 - 15
  Front Left: Capture 10 [67%] [15.00dB] [on]
  Front Right: Capture 8 [53%] [12.00dB] [on]

So, it would be user-friendly to keep the user interface and continue
to expose the same knob - even if the default is the optimal, etc.

To be completely honest, I don't think any user is really relying
on any REC_GAIN / Capture setting, and I'm completely OK
with having a mixer control or a module parameter. It doesn't matter.

What matters here, is getting rid of the stupid AC97 mixer,
with a dozen of playback and capture controls that have no meaning
whatsoever.

> Either way, if you still think this solution is "crap", feel free to
> modify the patches in any way you see fit. I've wasted too much time
> on this already, and since I'm not being paid for it, I don't intend
> to put any more effort into this.
>

FWIW, I don't think your patches are crap. Quite the opposite,
it's refreshing to see such good stuff being submitted.

After the click noise you fixed in snd-usb-audio, removing the mixer
is the last TODO thing in this driver. So I really appreciate all the time
you have put in it.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Mauro Carvalho Chehab]

Em Sun, 4 Dec 2016 15:25:25 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:

> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
> > Hello
> >
> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:  
> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> >> <mchehab@s-opensource.com> wrote:  
> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
> >>>  
> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> >>>> driver also sets this to 8 without any possibility for changing it.  
> >>>
> >>> The problem of removing the mixer is that you need this kind of
> >>> crap to setup the volumes on a non-standard way.
> >>>  
> >>
> >> Right, that's a good point.
> >>  
> >>> NACK.
> >>>
> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
> >>> em28xx) is that they configure the mixer when an input is selected,
> >>> increasing the volume of the active audio channel to 100% and muting
> >>> the other audio channels. Yet, as the alsa mixer is exported, users
> >>> can change the mixer settings in runtime using some alsa (or pa)
> >>> mixer application.
> >>>  
> >>
> >> Yeah, the AC97 mixer we are currently leveraging
> >> exposes many controls that have no meaning in this device,
> >> so removing that still looks like an improvement.
> >>
> >> I guess the proper way is creating our own mixer
> >> (not using snd_ac97_mixer)  exposing only the record
> >> gain knob.
> >>
> >> Marcel, what do you think?
> >> --
> >> Ezequiel García, VanguardiaSur
> >> www.vanguardiasur.com.ar  
> >
> > As I have written before, the recording gain isn't actually meant to
> > be changed by the user. In the official Windows driver this value is
> > hard-coded to 8 and cannot be changed in any way. And there really is
> > no good reason why anyone should need to mess with it in the first
> > place. The default value will give the best results in pretty much all
> > cases and produces approximately the same volume as the internal 8-bit
> > ADC whose gain cannot be changed at all, not even by a driver.
> >
> > I had considered writing a mixer but chose not to. If the gain setting
> > is openly exposed to mixer applications, how do you tell the users
> > that the value set by the driver already is the optimal and
> > recommended value and that they shouldn't mess with the controls
> > unless they really have to? By having a module parameter, this setting
> > is practically hidden from the normal user but still is available to
> > power-users if they think they really need it. In the end it's really
> > just a compromise between hiding it completely and exposing it openly.
> > Also, this way the driver guarantees reproducible results, since
> > there's no need to remember the positions of any volume sliders.
> >  
> 
> Hm, right. I've never changed the record gain, and it's true that it
> doens't really improve the volume. So, I would be OK with having
> a module parameter.
> 
> On the other side, we are exposing it currently, through the "Capture"
> mixer control:
> 
> Simple mixer control 'Capture',0
>   Capabilities: cvolume cswitch cswitch-joined
>   Capture channels: Front Left - Front Right
>   Limits: Capture 0 - 15
>   Front Left: Capture 10 [67%] [15.00dB] [on]
>   Front Right: Capture 8 [53%] [12.00dB] [on]
> 
> So, it would be user-friendly to keep the user interface and continue
> to expose the same knob - even if the default is the optimal, etc.
> 
> To be completely honest, I don't think any user is really relying
> on any REC_GAIN / Capture setting, and I'm completely OK
> with having a mixer control or a module parameter. It doesn't matter.

If you're positive that *all* stk1160 use the ac97 mixer the
same way, and that there's no sense on having a mixer for it,
then it would be ok to remove it.

In such case, then why you need a modprobe parameter to allow
setting the record level? If this mixer entry is not used,
just set it to zero and be happy with that.

Regards,
Mauro

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Ezequiel Garcia]

On 5 December 2016 at 09:12, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> Em Sun, 4 Dec 2016 15:25:25 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>
>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>> > Hello
>> >
>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> >> <mchehab@s-opensource.com> wrote:
>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>> >>>
>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>> >>>> driver also sets this to 8 without any possibility for changing it.
>> >>>
>> >>> The problem of removing the mixer is that you need this kind of
>> >>> crap to setup the volumes on a non-standard way.
>> >>>
>> >>
>> >> Right, that's a good point.
>> >>
>> >>> NACK.
>> >>>
>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>> >>> em28xx) is that they configure the mixer when an input is selected,
>> >>> increasing the volume of the active audio channel to 100% and muting
>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>> >>> can change the mixer settings in runtime using some alsa (or pa)
>> >>> mixer application.
>> >>>
>> >>
>> >> Yeah, the AC97 mixer we are currently leveraging
>> >> exposes many controls that have no meaning in this device,
>> >> so removing that still looks like an improvement.
>> >>
>> >> I guess the proper way is creating our own mixer
>> >> (not using snd_ac97_mixer)  exposing only the record
>> >> gain knob.
>> >>
>> >> Marcel, what do you think?
>> >> --
>> >> Ezequiel García, VanguardiaSur
>> >> www.vanguardiasur.com.ar
>> >
>> > As I have written before, the recording gain isn't actually meant to
>> > be changed by the user. In the official Windows driver this value is
>> > hard-coded to 8 and cannot be changed in any way. And there really is
>> > no good reason why anyone should need to mess with it in the first
>> > place. The default value will give the best results in pretty much all
>> > cases and produces approximately the same volume as the internal 8-bit
>> > ADC whose gain cannot be changed at all, not even by a driver.
>> >
>> > I had considered writing a mixer but chose not to. If the gain setting
>> > is openly exposed to mixer applications, how do you tell the users
>> > that the value set by the driver already is the optimal and
>> > recommended value and that they shouldn't mess with the controls
>> > unless they really have to? By having a module parameter, this setting
>> > is practically hidden from the normal user but still is available to
>> > power-users if they think they really need it. In the end it's really
>> > just a compromise between hiding it completely and exposing it openly.
>> > Also, this way the driver guarantees reproducible results, since
>> > there's no need to remember the positions of any volume sliders.
>> >
>>
>> Hm, right. I've never changed the record gain, and it's true that it
>> doens't really improve the volume. So, I would be OK with having
>> a module parameter.
>>
>> On the other side, we are exposing it currently, through the "Capture"
>> mixer control:
>>
>> Simple mixer control 'Capture',0
>>   Capabilities: cvolume cswitch cswitch-joined
>>   Capture channels: Front Left - Front Right
>>   Limits: Capture 0 - 15
>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>>
>> So, it would be user-friendly to keep the user interface and continue
>> to expose the same knob - even if the default is the optimal, etc.
>>
>> To be completely honest, I don't think any user is really relying
>> on any REC_GAIN / Capture setting, and I'm completely OK
>> with having a mixer control or a module parameter. It doesn't matter.
>
> If you're positive that *all* stk1160 use the ac97 mixer the
> same way, and that there's no sense on having a mixer for it,
> then it would be ok to remove it.
>

Let's remove it then!

> In such case, then why you need a modprobe parameter to allow
> setting the record level? If this mixer entry is not used,
> just set it to zero and be happy with that.
>

Let's remove the module param too, then.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

Hello

2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:
>> Em Sun, 4 Dec 2016 15:25:25 -0300
>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>>
>>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>>> > Hello
>>> >
>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>> >> <mchehab@s-opensource.com> wrote:
>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>> >>>
>>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>> >>>> driver also sets this to 8 without any possibility for changing it.
>>> >>>
>>> >>> The problem of removing the mixer is that you need this kind of
>>> >>> crap to setup the volumes on a non-standard way.
>>> >>>
>>> >>
>>> >> Right, that's a good point.
>>> >>
>>> >>> NACK.
>>> >>>
>>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>> >>> em28xx) is that they configure the mixer when an input is selected,
>>> >>> increasing the volume of the active audio channel to 100% and muting
>>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>>> >>> can change the mixer settings in runtime using some alsa (or pa)
>>> >>> mixer application.
>>> >>>
>>> >>
>>> >> Yeah, the AC97 mixer we are currently leveraging
>>> >> exposes many controls that have no meaning in this device,
>>> >> so removing that still looks like an improvement.
>>> >>
>>> >> I guess the proper way is creating our own mixer
>>> >> (not using snd_ac97_mixer)  exposing only the record
>>> >> gain knob.
>>> >>
>>> >> Marcel, what do you think?
>>> >> --
>>> >> Ezequiel García, VanguardiaSur
>>> >> www.vanguardiasur.com.ar
>>> >
>>> > As I have written before, the recording gain isn't actually meant to
>>> > be changed by the user. In the official Windows driver this value is
>>> > hard-coded to 8 and cannot be changed in any way. And there really is
>>> > no good reason why anyone should need to mess with it in the first
>>> > place. The default value will give the best results in pretty much all
>>> > cases and produces approximately the same volume as the internal 8-bit
>>> > ADC whose gain cannot be changed at all, not even by a driver.
>>> >
>>> > I had considered writing a mixer but chose not to. If the gain setting
>>> > is openly exposed to mixer applications, how do you tell the users
>>> > that the value set by the driver already is the optimal and
>>> > recommended value and that they shouldn't mess with the controls
>>> > unless they really have to? By having a module parameter, this setting
>>> > is practically hidden from the normal user but still is available to
>>> > power-users if they think they really need it. In the end it's really
>>> > just a compromise between hiding it completely and exposing it openly.
>>> > Also, this way the driver guarantees reproducible results, since
>>> > there's no need to remember the positions of any volume sliders.
>>> >
>>>
>>> Hm, right. I've never changed the record gain, and it's true that it
>>> doens't really improve the volume. So, I would be OK with having
>>> a module parameter.
>>>
>>> On the other side, we are exposing it currently, through the "Capture"
>>> mixer control:
>>>
>>> Simple mixer control 'Capture',0
>>>   Capabilities: cvolume cswitch cswitch-joined
>>>   Capture channels: Front Left - Front Right
>>>   Limits: Capture 0 - 15
>>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>>>
>>> So, it would be user-friendly to keep the user interface and continue
>>> to expose the same knob - even if the default is the optimal, etc.
>>>
>>> To be completely honest, I don't think any user is really relying
>>> on any REC_GAIN / Capture setting, and I'm completely OK
>>> with having a mixer control or a module parameter. It doesn't matter.
>>
>> If you're positive that *all* stk1160 use the ac97 mixer the
>> same way, and that there's no sense on having a mixer for it,
>> then it would be ok to remove it.
>>
>
> Let's remove it then!
>
>> In such case, then why you need a modprobe parameter to allow
>> setting the record level? If this mixer entry is not used,
>> just set it to zero and be happy with that.
>>
>
> Let's remove the module param too, then.

I'm okay with that.

>
> Thanks,
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar

I'm willing to prepare one final patchset, provided we can agree on
and resolve all issues beforehand.

So far the changes would be to remove the module param and to poll
STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
to also poll it before writing, although that never caused problems.

I'll post some code for review before actually submitting patches.
Mauro, is there anything else that you think should be changed? If so,
please tell me now. Thanks.

Best regards
Marcel

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

2016-12-05 22:06 GMT+01:00 Marcel Hasler <mahasler@gmail.com>:
> Hello
>
> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> <mchehab@s-opensource.com> wrote:
>>> Em Sun, 4 Dec 2016 15:25:25 -0300
>>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>>>
>>>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>>>> > Hello
>>>> >
>>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>>> >> <mchehab@s-opensource.com> wrote:
>>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>>> >>>
>>>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>>> >>>> driver also sets this to 8 without any possibility for changing it.
>>>> >>>
>>>> >>> The problem of removing the mixer is that you need this kind of
>>>> >>> crap to setup the volumes on a non-standard way.
>>>> >>>
>>>> >>
>>>> >> Right, that's a good point.
>>>> >>
>>>> >>> NACK.
>>>> >>>
>>>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>>> >>> em28xx) is that they configure the mixer when an input is selected,
>>>> >>> increasing the volume of the active audio channel to 100% and muting
>>>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>>>> >>> can change the mixer settings in runtime using some alsa (or pa)
>>>> >>> mixer application.
>>>> >>>
>>>> >>
>>>> >> Yeah, the AC97 mixer we are currently leveraging
>>>> >> exposes many controls that have no meaning in this device,
>>>> >> so removing that still looks like an improvement.
>>>> >>
>>>> >> I guess the proper way is creating our own mixer
>>>> >> (not using snd_ac97_mixer)  exposing only the record
>>>> >> gain knob.
>>>> >>
>>>> >> Marcel, what do you think?
>>>> >> --
>>>> >> Ezequiel García, VanguardiaSur
>>>> >> www.vanguardiasur.com.ar
>>>> >
>>>> > As I have written before, the recording gain isn't actually meant to
>>>> > be changed by the user. In the official Windows driver this value is
>>>> > hard-coded to 8 and cannot be changed in any way. And there really is
>>>> > no good reason why anyone should need to mess with it in the first
>>>> > place. The default value will give the best results in pretty much all
>>>> > cases and produces approximately the same volume as the internal 8-bit
>>>> > ADC whose gain cannot be changed at all, not even by a driver.
>>>> >
>>>> > I had considered writing a mixer but chose not to. If the gain setting
>>>> > is openly exposed to mixer applications, how do you tell the users
>>>> > that the value set by the driver already is the optimal and
>>>> > recommended value and that they shouldn't mess with the controls
>>>> > unless they really have to? By having a module parameter, this setting
>>>> > is practically hidden from the normal user but still is available to
>>>> > power-users if they think they really need it. In the end it's really
>>>> > just a compromise between hiding it completely and exposing it openly.
>>>> > Also, this way the driver guarantees reproducible results, since
>>>> > there's no need to remember the positions of any volume sliders.
>>>> >
>>>>
>>>> Hm, right. I've never changed the record gain, and it's true that it
>>>> doens't really improve the volume. So, I would be OK with having
>>>> a module parameter.
>>>>
>>>> On the other side, we are exposing it currently, through the "Capture"
>>>> mixer control:
>>>>
>>>> Simple mixer control 'Capture',0
>>>>   Capabilities: cvolume cswitch cswitch-joined
>>>>   Capture channels: Front Left - Front Right
>>>>   Limits: Capture 0 - 15
>>>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>>>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>>>>
>>>> So, it would be user-friendly to keep the user interface and continue
>>>> to expose the same knob - even if the default is the optimal, etc.
>>>>
>>>> To be completely honest, I don't think any user is really relying
>>>> on any REC_GAIN / Capture setting, and I'm completely OK
>>>> with having a mixer control or a module parameter. It doesn't matter.
>>>
>>> If you're positive that *all* stk1160 use the ac97 mixer the
>>> same way, and that there's no sense on having a mixer for it,
>>> then it would be ok to remove it.
>>>
>>
>> Let's remove it then!
>>
>>> In such case, then why you need a modprobe parameter to allow
>>> setting the record level? If this mixer entry is not used,
>>> just set it to zero and be happy with that.
>>>
>>
>> Let's remove the module param too, then.
>
> I'm okay with that.
>
>>
>> Thanks,
>> --
>> Ezequiel García, VanguardiaSur
>> www.vanguardiasur.com.ar
>
> I'm willing to prepare one final patchset, provided we can agree on
> and resolve all issues beforehand.
>
> So far the changes would be to remove the module param and to poll
> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
> to also poll it before writing, although that never caused problems.
>
> I'll post some code for review before actually submitting patches.
> Mauro, is there anything else that you think should be changed? If so,
> please tell me now. Thanks.
>
> Best regards
> Marcel

One more thing...

The driver currently uses a lot of "magic numbers", both for the AC97
register addresses as well as the STK1160 register contents. That
makes it a bit difficult to read unless you happen to have the
datasheet open. Would it maybe be better to add defines for those,
especially if we're going to poll individual bits? I usually prefer
that approach myself. Would you put the defines for the AC97 chip
registers into stk1160-reg.h or keep them in stk1160-ac97.c since
they're only used there?

Best regards
Marcel

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Ezequiel Garcia]

On 5 December 2016 at 18:18, Marcel Hasler <mahasler@gmail.com> wrote:
> 2016-12-05 22:06 GMT+01:00 Marcel Hasler <mahasler@gmail.com>:
>> Hello
>>
>> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>>> <mchehab@s-opensource.com> wrote:
>>>> Em Sun, 4 Dec 2016 15:25:25 -0300
>>>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>>>>
>>>>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>>>>> > Hello
>>>>> >
>>>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>>>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>>>> >> <mchehab@s-opensource.com> wrote:
>>>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>>>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>>>> >>>
>>>>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>>>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>>>> >>>> driver also sets this to 8 without any possibility for changing it.
>>>>> >>>
>>>>> >>> The problem of removing the mixer is that you need this kind of
>>>>> >>> crap to setup the volumes on a non-standard way.
>>>>> >>>
>>>>> >>
>>>>> >> Right, that's a good point.
>>>>> >>
>>>>> >>> NACK.
>>>>> >>>
>>>>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>>>> >>> em28xx) is that they configure the mixer when an input is selected,
>>>>> >>> increasing the volume of the active audio channel to 100% and muting
>>>>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>>>>> >>> can change the mixer settings in runtime using some alsa (or pa)
>>>>> >>> mixer application.
>>>>> >>>
>>>>> >>
>>>>> >> Yeah, the AC97 mixer we are currently leveraging
>>>>> >> exposes many controls that have no meaning in this device,
>>>>> >> so removing that still looks like an improvement.
>>>>> >>
>>>>> >> I guess the proper way is creating our own mixer
>>>>> >> (not using snd_ac97_mixer)  exposing only the record
>>>>> >> gain knob.
>>>>> >>
>>>>> >> Marcel, what do you think?
>>>>> >> --
>>>>> >> Ezequiel García, VanguardiaSur
>>>>> >> www.vanguardiasur.com.ar
>>>>> >
>>>>> > As I have written before, the recording gain isn't actually meant to
>>>>> > be changed by the user. In the official Windows driver this value is
>>>>> > hard-coded to 8 and cannot be changed in any way. And there really is
>>>>> > no good reason why anyone should need to mess with it in the first
>>>>> > place. The default value will give the best results in pretty much all
>>>>> > cases and produces approximately the same volume as the internal 8-bit
>>>>> > ADC whose gain cannot be changed at all, not even by a driver.
>>>>> >
>>>>> > I had considered writing a mixer but chose not to. If the gain setting
>>>>> > is openly exposed to mixer applications, how do you tell the users
>>>>> > that the value set by the driver already is the optimal and
>>>>> > recommended value and that they shouldn't mess with the controls
>>>>> > unless they really have to? By having a module parameter, this setting
>>>>> > is practically hidden from the normal user but still is available to
>>>>> > power-users if they think they really need it. In the end it's really
>>>>> > just a compromise between hiding it completely and exposing it openly.
>>>>> > Also, this way the driver guarantees reproducible results, since
>>>>> > there's no need to remember the positions of any volume sliders.
>>>>> >
>>>>>
>>>>> Hm, right. I've never changed the record gain, and it's true that it
>>>>> doens't really improve the volume. So, I would be OK with having
>>>>> a module parameter.
>>>>>
>>>>> On the other side, we are exposing it currently, through the "Capture"
>>>>> mixer control:
>>>>>
>>>>> Simple mixer control 'Capture',0
>>>>>   Capabilities: cvolume cswitch cswitch-joined
>>>>>   Capture channels: Front Left - Front Right
>>>>>   Limits: Capture 0 - 15
>>>>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>>>>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>>>>>
>>>>> So, it would be user-friendly to keep the user interface and continue
>>>>> to expose the same knob - even if the default is the optimal, etc.
>>>>>
>>>>> To be completely honest, I don't think any user is really relying
>>>>> on any REC_GAIN / Capture setting, and I'm completely OK
>>>>> with having a mixer control or a module parameter. It doesn't matter.
>>>>
>>>> If you're positive that *all* stk1160 use the ac97 mixer the
>>>> same way, and that there's no sense on having a mixer for it,
>>>> then it would be ok to remove it.
>>>>
>>>
>>> Let's remove it then!
>>>
>>>> In such case, then why you need a modprobe parameter to allow
>>>> setting the record level? If this mixer entry is not used,
>>>> just set it to zero and be happy with that.
>>>>
>>>
>>> Let's remove the module param too, then.
>>
>> I'm okay with that.
>>
>>>
>>> Thanks,
>>> --
>>> Ezequiel García, VanguardiaSur
>>> www.vanguardiasur.com.ar
>>
>> I'm willing to prepare one final patchset, provided we can agree on
>> and resolve all issues beforehand.
>>
>> So far the changes would be to remove the module param and to poll
>> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
>> to also poll it before writing, although that never caused problems.
>>
>> I'll post some code for review before actually submitting patches.
>> Mauro, is there anything else that you think should be changed? If so,
>> please tell me now. Thanks.
>>
>> Best regards
>> Marcel
>
> One more thing...
>
> The driver currently uses a lot of "magic numbers", both for the AC97
> register addresses as well as the STK1160 register contents. That
> makes it a bit difficult to read unless you happen to have the
> datasheet open. Would it maybe be better to add defines for those,
> especially if we're going to poll individual bits?

Yes, but we don't want to put that on the same series.
Let's this out first, and then we can work on more changes.

> I usually prefer
> that approach myself. Would you put the defines for the AC97 chip
> registers into stk1160-reg.h or keep them in stk1160-ac97.c since
> they're only used there?
>

I'm fine either way. For consistency with how it's written currently,
I'd go with stk1160-reg.h
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Ezequiel Garcia]

On 5 December 2016 at 18:06, Marcel Hasler <mahasler@gmail.com> wrote:
> Hello
>
> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> <mchehab@s-opensource.com> wrote:
>>> Em Sun, 4 Dec 2016 15:25:25 -0300
>>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>>>
>>>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>>>> > Hello
>>>> >
>>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>>> >> <mchehab@s-opensource.com> wrote:
>>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>>> >>>
>>>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>>> >>>> driver also sets this to 8 without any possibility for changing it.
>>>> >>>
>>>> >>> The problem of removing the mixer is that you need this kind of
>>>> >>> crap to setup the volumes on a non-standard way.
>>>> >>>
>>>> >>
>>>> >> Right, that's a good point.
>>>> >>
>>>> >>> NACK.
>>>> >>>
>>>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>>> >>> em28xx) is that they configure the mixer when an input is selected,
>>>> >>> increasing the volume of the active audio channel to 100% and muting
>>>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>>>> >>> can change the mixer settings in runtime using some alsa (or pa)
>>>> >>> mixer application.
>>>> >>>
>>>> >>
>>>> >> Yeah, the AC97 mixer we are currently leveraging
>>>> >> exposes many controls that have no meaning in this device,
>>>> >> so removing that still looks like an improvement.
>>>> >>
>>>> >> I guess the proper way is creating our own mixer
>>>> >> (not using snd_ac97_mixer)  exposing only the record
>>>> >> gain knob.
>>>> >>
>>>> >> Marcel, what do you think?
>>>> >> --
>>>> >> Ezequiel García, VanguardiaSur
>>>> >> www.vanguardiasur.com.ar
>>>> >
>>>> > As I have written before, the recording gain isn't actually meant to
>>>> > be changed by the user. In the official Windows driver this value is
>>>> > hard-coded to 8 and cannot be changed in any way. And there really is
>>>> > no good reason why anyone should need to mess with it in the first
>>>> > place. The default value will give the best results in pretty much all
>>>> > cases and produces approximately the same volume as the internal 8-bit
>>>> > ADC whose gain cannot be changed at all, not even by a driver.
>>>> >
>>>> > I had considered writing a mixer but chose not to. If the gain setting
>>>> > is openly exposed to mixer applications, how do you tell the users
>>>> > that the value set by the driver already is the optimal and
>>>> > recommended value and that they shouldn't mess with the controls
>>>> > unless they really have to? By having a module parameter, this setting
>>>> > is practically hidden from the normal user but still is available to
>>>> > power-users if they think they really need it. In the end it's really
>>>> > just a compromise between hiding it completely and exposing it openly.
>>>> > Also, this way the driver guarantees reproducible results, since
>>>> > there's no need to remember the positions of any volume sliders.
>>>> >
>>>>
>>>> Hm, right. I've never changed the record gain, and it's true that it
>>>> doens't really improve the volume. So, I would be OK with having
>>>> a module parameter.
>>>>
>>>> On the other side, we are exposing it currently, through the "Capture"
>>>> mixer control:
>>>>
>>>> Simple mixer control 'Capture',0
>>>>   Capabilities: cvolume cswitch cswitch-joined
>>>>   Capture channels: Front Left - Front Right
>>>>   Limits: Capture 0 - 15
>>>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>>>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>>>>
>>>> So, it would be user-friendly to keep the user interface and continue
>>>> to expose the same knob - even if the default is the optimal, etc.
>>>>
>>>> To be completely honest, I don't think any user is really relying
>>>> on any REC_GAIN / Capture setting, and I'm completely OK
>>>> with having a mixer control or a module parameter. It doesn't matter.
>>>
>>> If you're positive that *all* stk1160 use the ac97 mixer the
>>> same way, and that there's no sense on having a mixer for it,
>>> then it would be ok to remove it.
>>>
>>
>> Let's remove it then!
>>
>>> In such case, then why you need a modprobe parameter to allow
>>> setting the record level? If this mixer entry is not used,
>>> just set it to zero and be happy with that.
>>>
>>
>> Let's remove the module param too, then.
>
> I'm okay with that.
>
>>
>> Thanks,
>> --
>> Ezequiel García, VanguardiaSur
>> www.vanguardiasur.com.ar
>
> I'm willing to prepare one final patchset, provided we can agree on
> and resolve all issues beforehand.
>
> So far the changes would be to remove the module param and to poll
> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
> to also poll it before writing, although that never caused problems.
>

If that's too much trouble or too much of a hassle,
imaybe just remove the read_reg. It's not really
used right now, except for dumping the registers.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Mauro Carvalho Chehab]

Em Mon, 5 Dec 2016 22:06:59 +0100
Marcel Hasler <mahasler@gmail.com> escreveu:

> Hello
> 
> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> > <mchehab@s-opensource.com> wrote:  
> >> Em Sun, 4 Dec 2016 15:25:25 -0300
> >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
> >>  
> >>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:  
> >>> > Hello
> >>> >
> >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:  
> >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> >>> >> <mchehab@s-opensource.com> wrote:  
> >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> >>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
> >>> >>>  
> >>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> >>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> >>> >>>> driver also sets this to 8 without any possibility for changing it.  
> >>> >>>
> >>> >>> The problem of removing the mixer is that you need this kind of
> >>> >>> crap to setup the volumes on a non-standard way.
> >>> >>>  
> >>> >>
> >>> >> Right, that's a good point.
> >>> >>  
> >>> >>> NACK.
> >>> >>>
> >>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
> >>> >>> em28xx) is that they configure the mixer when an input is selected,
> >>> >>> increasing the volume of the active audio channel to 100% and muting
> >>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
> >>> >>> can change the mixer settings in runtime using some alsa (or pa)
> >>> >>> mixer application.
> >>> >>>  
> >>> >>
> >>> >> Yeah, the AC97 mixer we are currently leveraging
> >>> >> exposes many controls that have no meaning in this device,
> >>> >> so removing that still looks like an improvement.
> >>> >>
> >>> >> I guess the proper way is creating our own mixer
> >>> >> (not using snd_ac97_mixer)  exposing only the record
> >>> >> gain knob.
> >>> >>
> >>> >> Marcel, what do you think?
> >>> >> --
> >>> >> Ezequiel García, VanguardiaSur
> >>> >> www.vanguardiasur.com.ar  
> >>> >
> >>> > As I have written before, the recording gain isn't actually meant to
> >>> > be changed by the user. In the official Windows driver this value is
> >>> > hard-coded to 8 and cannot be changed in any way. And there really is
> >>> > no good reason why anyone should need to mess with it in the first
> >>> > place. The default value will give the best results in pretty much all
> >>> > cases and produces approximately the same volume as the internal 8-bit
> >>> > ADC whose gain cannot be changed at all, not even by a driver.
> >>> >
> >>> > I had considered writing a mixer but chose not to. If the gain setting
> >>> > is openly exposed to mixer applications, how do you tell the users
> >>> > that the value set by the driver already is the optimal and
> >>> > recommended value and that they shouldn't mess with the controls
> >>> > unless they really have to? By having a module parameter, this setting
> >>> > is practically hidden from the normal user but still is available to
> >>> > power-users if they think they really need it. In the end it's really
> >>> > just a compromise between hiding it completely and exposing it openly.
> >>> > Also, this way the driver guarantees reproducible results, since
> >>> > there's no need to remember the positions of any volume sliders.
> >>> >  
> >>>
> >>> Hm, right. I've never changed the record gain, and it's true that it
> >>> doens't really improve the volume. So, I would be OK with having
> >>> a module parameter.
> >>>
> >>> On the other side, we are exposing it currently, through the "Capture"
> >>> mixer control:
> >>>
> >>> Simple mixer control 'Capture',0
> >>>   Capabilities: cvolume cswitch cswitch-joined
> >>>   Capture channels: Front Left - Front Right
> >>>   Limits: Capture 0 - 15
> >>>   Front Left: Capture 10 [67%] [15.00dB] [on]
> >>>   Front Right: Capture 8 [53%] [12.00dB] [on]
> >>>
> >>> So, it would be user-friendly to keep the user interface and continue
> >>> to expose the same knob - even if the default is the optimal, etc.
> >>>
> >>> To be completely honest, I don't think any user is really relying
> >>> on any REC_GAIN / Capture setting, and I'm completely OK
> >>> with having a mixer control or a module parameter. It doesn't matter.  
> >>
> >> If you're positive that *all* stk1160 use the ac97 mixer the
> >> same way, and that there's no sense on having a mixer for it,
> >> then it would be ok to remove it.
> >>  
> >
> > Let's remove it then!
> >  
> >> In such case, then why you need a modprobe parameter to allow
> >> setting the record level? If this mixer entry is not used,
> >> just set it to zero and be happy with that.
> >>  
> >
> > Let's remove the module param too, then.  
> 
> I'm okay with that.
> 
> >
> > Thanks,
> > --
> > Ezequiel García, VanguardiaSur
> > www.vanguardiasur.com.ar  
> 
> I'm willing to prepare one final patchset, provided we can agree on
> and resolve all issues beforehand.
> 
> So far the changes would be to remove the module param and to poll
> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
> to also poll it before writing, although that never caused problems.

Sounds ok. My experience with AC97 on em28xx is that, as new devices
were added, the delay needed for AC97 varied on some of those new
devices. That's why checking if AC97 is ready before writing was
added to its code.

> 
> I'll post some code for review before actually submitting patches.
> Mauro, is there anything else that you think should be changed? If so,
> please tell me now. Thanks.
> 
> Best regards
> Marcel



Thanks,
Mauro

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

Hello

2016-12-06 13:56 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> Em Mon, 5 Dec 2016 22:06:59 +0100
> Marcel Hasler <mahasler@gmail.com> escreveu:
>
>> Hello
>>
>> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> > <mchehab@s-opensource.com> wrote:
>> >> Em Sun, 4 Dec 2016 15:25:25 -0300
>> >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>> >>
>> >>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>> >>> > Hello
>> >>> >
>> >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> >>> >> <mchehab@s-opensource.com> wrote:
>> >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> >>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>> >>> >>>
>> >>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>> >>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>> >>> >>>> driver also sets this to 8 without any possibility for changing it.
>> >>> >>>
>> >>> >>> The problem of removing the mixer is that you need this kind of
>> >>> >>> crap to setup the volumes on a non-standard way.
>> >>> >>>
>> >>> >>
>> >>> >> Right, that's a good point.
>> >>> >>
>> >>> >>> NACK.
>> >>> >>>
>> >>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>> >>> >>> em28xx) is that they configure the mixer when an input is selected,
>> >>> >>> increasing the volume of the active audio channel to 100% and muting
>> >>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>> >>> >>> can change the mixer settings in runtime using some alsa (or pa)
>> >>> >>> mixer application.
>> >>> >>>
>> >>> >>
>> >>> >> Yeah, the AC97 mixer we are currently leveraging
>> >>> >> exposes many controls that have no meaning in this device,
>> >>> >> so removing that still looks like an improvement.
>> >>> >>
>> >>> >> I guess the proper way is creating our own mixer
>> >>> >> (not using snd_ac97_mixer)  exposing only the record
>> >>> >> gain knob.
>> >>> >>
>> >>> >> Marcel, what do you think?
>> >>> >> --
>> >>> >> Ezequiel García, VanguardiaSur
>> >>> >> www.vanguardiasur.com.ar
>> >>> >
>> >>> > As I have written before, the recording gain isn't actually meant to
>> >>> > be changed by the user. In the official Windows driver this value is
>> >>> > hard-coded to 8 and cannot be changed in any way. And there really is
>> >>> > no good reason why anyone should need to mess with it in the first
>> >>> > place. The default value will give the best results in pretty much all
>> >>> > cases and produces approximately the same volume as the internal 8-bit
>> >>> > ADC whose gain cannot be changed at all, not even by a driver.
>> >>> >
>> >>> > I had considered writing a mixer but chose not to. If the gain setting
>> >>> > is openly exposed to mixer applications, how do you tell the users
>> >>> > that the value set by the driver already is the optimal and
>> >>> > recommended value and that they shouldn't mess with the controls
>> >>> > unless they really have to? By having a module parameter, this setting
>> >>> > is practically hidden from the normal user but still is available to
>> >>> > power-users if they think they really need it. In the end it's really
>> >>> > just a compromise between hiding it completely and exposing it openly.
>> >>> > Also, this way the driver guarantees reproducible results, since
>> >>> > there's no need to remember the positions of any volume sliders.
>> >>> >
>> >>>
>> >>> Hm, right. I've never changed the record gain, and it's true that it
>> >>> doens't really improve the volume. So, I would be OK with having
>> >>> a module parameter.
>> >>>
>> >>> On the other side, we are exposing it currently, through the "Capture"
>> >>> mixer control:
>> >>>
>> >>> Simple mixer control 'Capture',0
>> >>>   Capabilities: cvolume cswitch cswitch-joined
>> >>>   Capture channels: Front Left - Front Right
>> >>>   Limits: Capture 0 - 15
>> >>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>> >>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>> >>>
>> >>> So, it would be user-friendly to keep the user interface and continue
>> >>> to expose the same knob - even if the default is the optimal, etc.
>> >>>
>> >>> To be completely honest, I don't think any user is really relying
>> >>> on any REC_GAIN / Capture setting, and I'm completely OK
>> >>> with having a mixer control or a module parameter. It doesn't matter.
>> >>
>> >> If you're positive that *all* stk1160 use the ac97 mixer the
>> >> same way, and that there's no sense on having a mixer for it,
>> >> then it would be ok to remove it.
>> >>
>> >
>> > Let's remove it then!
>> >
>> >> In such case, then why you need a modprobe parameter to allow
>> >> setting the record level? If this mixer entry is not used,
>> >> just set it to zero and be happy with that.
>> >>
>> >
>> > Let's remove the module param too, then.
>>
>> I'm okay with that.
>>
>> >
>> > Thanks,
>> > --
>> > Ezequiel García, VanguardiaSur
>> > www.vanguardiasur.com.ar
>>
>> I'm willing to prepare one final patchset, provided we can agree on
>> and resolve all issues beforehand.
>>
>> So far the changes would be to remove the module param and to poll
>> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
>> to also poll it before writing, although that never caused problems.
>
> Sounds ok. My experience with AC97 on em28xx is that, as new devices
> were added, the delay needed for AC97 varied on some of those new
> devices. That's why checking if AC97 is ready before writing was
> added to its code.
>
>>
>> I'll post some code for review before actually submitting patches.
>> Mauro, is there anything else that you think should be changed? If so,
>> please tell me now. Thanks.
>>
>> Best regards
>> Marcel
>
>
>
> Thanks,
> Mauro

I've implemented the polling function. I'm using it in a slightly
different manner than em28xx, in order to be consistent with the
device's logic as documented in the datasheet. Namely I let the driver
wait after the write instead of before, to make sure
stk1160_write_ac97() only returns once the write has actually
completed. Please tell me if this is okay for you, before I commit and
create the new patchset. Here's the diff:

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c
b/drivers/media/usb/stk1160/stk1160-ac97.c
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 95648ac..708792b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -23,11 +23,30 @@
  *
  */

-#include <linux/module.h>
+#include <linux/delay.h>

 #include "stk1160.h"
 #include "stk1160-reg.h"

+static int stk1160_ac97_wait_transfer_complete(struct stk1160 *dev)
+{
+       unsigned long timeout = jiffies +
msecs_to_jiffies(STK1160_AC97_TIMEOUT);
+       u8 value;
+
+       /* Wait for AC97 transfer to complete */
+       while (time_is_after_jiffies(timeout)) {
+               stk1160_read_reg(dev, STK1160_AC97CTL_0, &value);
+
+               if (!(value & (STK1160_AC97CTL_0_CR | STK1160_AC97CTL_0_CW)))
+                       return 0;
+
+               msleep(1);
+       }
+
+       stk1160_err("AC97 transfer took too long, this should never happen!");
+       return -EBUSY;
+}
+
 static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
 {
        /* Set codec register address */
@@ -37,11 +56,11 @@ static void stk1160_write_ac97(struct stk1160
*dev, u16 reg, u16 value)
        stk1160_write_reg(dev, STK1160_AC97_CMD, value & 0xff);
        stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value & 0xff00) >> 8);

-       /*
-        * Set command write bit to initiate write operation.
-        * The bit will be cleared when transfer is done.
-        */
+       /* Set command write bit to initiate write operation */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
+
+       /* Wait for command write bit to be cleared */
+       stk1160_ac97_wait_transfer_complete(dev);
 }

 #ifdef DEBUG
@@ -53,12 +72,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
        /* Set codec register address */
        stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);

-       /*
-        * Set command read bit to initiate read operation.
-        * The bit will be cleared when transfer is done.
-        */
+       /* Set command read bit to initiate read operation */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);

+       /* Wait for command read bit to be cleared */
+       if (stk1160_ac97_wait_transfer_complete(dev) < 0) {
+               return 0;
+       }
+
        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
diff --git a/drivers/media/usb/stk1160/stk1160-reg.h
b/drivers/media/usb/stk1160/stk1160-reg.h
index 296a9e7..7b08a3c 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -122,6 +122,8 @@
 /* AC97 Audio Control */
 #define STK1160_AC97CTL_0              0x500
 #define STK1160_AC97CTL_1              0x504
+#define  STK1160_AC97CTL_0_CR          BIT(1)
+#define  STK1160_AC97CTL_0_CW          BIT(2)

 /* Use [0:6] bits of register 0x504 to set codec command address */
 #define STK1160_AC97_ADDR              0x504
diff --git a/drivers/media/usb/stk1160/stk1160.h
b/drivers/media/usb/stk1160/stk1160.h
index e85e12e..acd1c81 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -50,6 +50,8 @@
 #define STK1160_MAX_INPUT 4
 #define STK1160_SVIDEO_INPUT 4

+#define STK1160_AC97_TIMEOUT 50
+
 #define STK1160_I2C_TIMEOUT 100

Best regards
Marcel

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[mahasler]

Sorry about the broken formatting. Here's the diff once more:

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 95648ac..708792b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -23,11 +23,30 @@
  *
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
 
 #include "stk1160.h"
 #include "stk1160-reg.h"
 
+static int stk1160_ac97_wait_transfer_complete(struct stk1160 *dev)
+{
+       unsigned long timeout = jiffies + msecs_to_jiffies(STK1160_AC97_TIMEOUT);
+       u8 value;
+
+       /* Wait for AC97 transfer to complete */
+       while (time_is_after_jiffies(timeout)) {
+               stk1160_read_reg(dev, STK1160_AC97CTL_0, &value);
+
+               if (!(value & (STK1160_AC97CTL_0_CR | STK1160_AC97CTL_0_CW)))
+                       return 0;
+
+               msleep(1);
+       }
+
+       stk1160_err("AC97 transfer took too long, this should never happen!");
+       return -EBUSY;
+}
+
 static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
 {
        /* Set codec register address */
@@ -37,11 +56,11 @@ static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
        stk1160_write_reg(dev, STK1160_AC97_CMD, value & 0xff);
        stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value & 0xff00) >> 8);
 
-       /*
-        * Set command write bit to initiate write operation.
-        * The bit will be cleared when transfer is done.
-        */
+       /* Set command write bit to initiate write operation */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
+
+       /* Wait for command write bit to be cleared */
+       stk1160_ac97_wait_transfer_complete(dev);
 }
 
 #ifdef DEBUG
@@ -53,12 +72,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
        /* Set codec register address */
        stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
 
-       /*
-        * Set command read bit to initiate read operation.
-        * The bit will be cleared when transfer is done.
-        */
+       /* Set command read bit to initiate read operation */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+       /* Wait for command read bit to be cleared */
+       if (stk1160_ac97_wait_transfer_complete(dev) < 0) {
+               return 0;
+       }
+
        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
diff --git a/drivers/media/usb/stk1160/stk1160-reg.h b/drivers/media/usb/stk1160/stk1160-reg.h
index 296a9e7..7b08a3c 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -122,6 +122,8 @@
 /* AC97 Audio Control */
 #define STK1160_AC97CTL_0              0x500
 #define STK1160_AC97CTL_1              0x504
+#define  STK1160_AC97CTL_0_CR          BIT(1)
+#define  STK1160_AC97CTL_0_CW          BIT(2)
 
 /* Use [0:6] bits of register 0x504 to set codec command address */
 #define STK1160_AC97_ADDR              0x504
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index e85e12e..acd1c81 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -50,6 +50,8 @@
 #define STK1160_MAX_INPUT 4
 #define STK1160_SVIDEO_INPUT 4
 
+#define STK1160_AC97_TIMEOUT 50
+
 #define STK1160_I2C_TIMEOUT 100
 
 

On Tue, Dec 06, 2016 at 10:56:26AM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Dec 2016 22:06:59 +0100
> Marcel Hasler <mahasler@gmail.com> escreveu:
> 
> > Hello
> > 
> > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> > > <mchehab@s-opensource.com> wrote:  
> > >> Em Sun, 4 Dec 2016 15:25:25 -0300
> > >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
> > >>  
> > >>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:  
> > >>> > Hello
> > >>> >
> > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:  
> > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> > >>> >> <mchehab@s-opensource.com> wrote:  
> > >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> > >>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
> > >>> >>>  
> > >>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> > >>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> > >>> >>>> driver also sets this to 8 without any possibility for changing it.  
> > >>> >>>
> > >>> >>> The problem of removing the mixer is that you need this kind of
> > >>> >>> crap to setup the volumes on a non-standard way.
> > >>> >>>  
> > >>> >>
> > >>> >> Right, that's a good point.
> > >>> >>  
> > >>> >>> NACK.
> > >>> >>>
> > >>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
> > >>> >>> em28xx) is that they configure the mixer when an input is selected,
> > >>> >>> increasing the volume of the active audio channel to 100% and muting
> > >>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
> > >>> >>> can change the mixer settings in runtime using some alsa (or pa)
> > >>> >>> mixer application.
> > >>> >>>  
> > >>> >>
> > >>> >> Yeah, the AC97 mixer we are currently leveraging
> > >>> >> exposes many controls that have no meaning in this device,
> > >>> >> so removing that still looks like an improvement.
> > >>> >>
> > >>> >> I guess the proper way is creating our own mixer
> > >>> >> (not using snd_ac97_mixer)  exposing only the record
> > >>> >> gain knob.
> > >>> >>
> > >>> >> Marcel, what do you think?
> > >>> >> --
> > >>> >> Ezequiel García, VanguardiaSur
> > >>> >> www.vanguardiasur.com.ar  
> > >>> >
> > >>> > As I have written before, the recording gain isn't actually meant to
> > >>> > be changed by the user. In the official Windows driver this value is
> > >>> > hard-coded to 8 and cannot be changed in any way. And there really is
> > >>> > no good reason why anyone should need to mess with it in the first
> > >>> > place. The default value will give the best results in pretty much all
> > >>> > cases and produces approximately the same volume as the internal 8-bit
> > >>> > ADC whose gain cannot be changed at all, not even by a driver.
> > >>> >
> > >>> > I had considered writing a mixer but chose not to. If the gain setting
> > >>> > is openly exposed to mixer applications, how do you tell the users
> > >>> > that the value set by the driver already is the optimal and
> > >>> > recommended value and that they shouldn't mess with the controls
> > >>> > unless they really have to? By having a module parameter, this setting
> > >>> > is practically hidden from the normal user but still is available to
> > >>> > power-users if they think they really need it. In the end it's really
> > >>> > just a compromise between hiding it completely and exposing it openly.
> > >>> > Also, this way the driver guarantees reproducible results, since
> > >>> > there's no need to remember the positions of any volume sliders.
> > >>> >  
> > >>>
> > >>> Hm, right. I've never changed the record gain, and it's true that it
> > >>> doens't really improve the volume. So, I would be OK with having
> > >>> a module parameter.
> > >>>
> > >>> On the other side, we are exposing it currently, through the "Capture"
> > >>> mixer control:
> > >>>
> > >>> Simple mixer control 'Capture',0
> > >>>   Capabilities: cvolume cswitch cswitch-joined
> > >>>   Capture channels: Front Left - Front Right
> > >>>   Limits: Capture 0 - 15
> > >>>   Front Left: Capture 10 [67%] [15.00dB] [on]
> > >>>   Front Right: Capture 8 [53%] [12.00dB] [on]
> > >>>
> > >>> So, it would be user-friendly to keep the user interface and continue
> > >>> to expose the same knob - even if the default is the optimal, etc.
> > >>>
> > >>> To be completely honest, I don't think any user is really relying
> > >>> on any REC_GAIN / Capture setting, and I'm completely OK
> > >>> with having a mixer control or a module parameter. It doesn't matter.  
> > >>
> > >> If you're positive that *all* stk1160 use the ac97 mixer the
> > >> same way, and that there's no sense on having a mixer for it,
> > >> then it would be ok to remove it.
> > >>  
> > >
> > > Let's remove it then!
> > >  
> > >> In such case, then why you need a modprobe parameter to allow
> > >> setting the record level? If this mixer entry is not used,
> > >> just set it to zero and be happy with that.
> > >>  
> > >
> > > Let's remove the module param too, then.  
> > 
> > I'm okay with that.
> > 
> > >
> > > Thanks,
> > > --
> > > Ezequiel García, VanguardiaSur
> > > www.vanguardiasur.com.ar  
> > 
> > I'm willing to prepare one final patchset, provided we can agree on
> > and resolve all issues beforehand.
> > 
> > So far the changes would be to remove the module param and to poll
> > STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
> > to also poll it before writing, although that never caused problems.
> 
> Sounds ok. My experience with AC97 on em28xx is that, as new devices
> were added, the delay needed for AC97 varied on some of those new
> devices. That's why checking if AC97 is ready before writing was
> added to its code.
> 
> > 
> > I'll post some code for review before actually submitting patches.
> > Mauro, is there anything else that you think should be changed? If so,
> > please tell me now. Thanks.
> > 
> > Best regards
> > Marcel
> 
> 
> 
> Thanks,
> Mauro

Marcel

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Mauro Carvalho Chehab]

Em Sun, 11 Dec 2016 13:20:06 +0100
mahasler@gmail.com escreveu:

> Sorry about the broken formatting. Here's the diff once more:

The patch itself looks ok. Just a few comments.

> 
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 95648ac..708792b 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -23,11 +23,30 @@
>   *
>   */
>  
> -#include <linux/module.h>

This change seems to be unrelated.

> +#include <linux/delay.h>
>  
>  #include "stk1160.h"
>  #include "stk1160-reg.h"
>  
> +static int stk1160_ac97_wait_transfer_complete(struct stk1160 *dev)
> +{
> +       unsigned long timeout = jiffies + msecs_to_jiffies(STK1160_AC97_TIMEOUT);
> +       u8 value;
> +
> +       /* Wait for AC97 transfer to complete */
> +       while (time_is_after_jiffies(timeout)) {
> +               stk1160_read_reg(dev, STK1160_AC97CTL_0, &value);
> +
> +               if (!(value & (STK1160_AC97CTL_0_CR | STK1160_AC97CTL_0_CW)))
> +                       return 0;
> +
> +               msleep(1);

It will likely sleep ~10ms. Maybe you likely need to use usleep_range().
Please read:
	Documentation/timers/timers-howto.txt

> +       }
> +
> +       stk1160_err("AC97 transfer took too long, this should never happen!");
> +       return -EBUSY;
> +}
> +
>  static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>  {
>         /* Set codec register address */
> @@ -37,11 +56,11 @@ static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>         stk1160_write_reg(dev, STK1160_AC97_CMD, value & 0xff);
>         stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value & 0xff00) >> 8);
>  
> -       /*
> -        * Set command write bit to initiate write operation.
> -        * The bit will be cleared when transfer is done.
> -        */
> +       /* Set command write bit to initiate write operation */
>         stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
> +
> +       /* Wait for command write bit to be cleared */
> +       stk1160_ac97_wait_transfer_complete(dev);
>  }
>  
>  #ifdef DEBUG
> @@ -53,12 +72,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
>         /* Set codec register address */
>         stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
>  
> -       /*
> -        * Set command read bit to initiate read operation.
> -        * The bit will be cleared when transfer is done.
> -        */
> +       /* Set command read bit to initiate read operation */
>         stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
>  
> +       /* Wait for command read bit to be cleared */
> +       if (stk1160_ac97_wait_transfer_complete(dev) < 0) {
> +               return 0;
> +       }
> +
>         /* Retrieve register value */
>         stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
>         stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
> diff --git a/drivers/media/usb/stk1160/stk1160-reg.h b/drivers/media/usb/stk1160/stk1160-reg.h
> index 296a9e7..7b08a3c 100644
> --- a/drivers/media/usb/stk1160/stk1160-reg.h
> +++ b/drivers/media/usb/stk1160/stk1160-reg.h
> @@ -122,6 +122,8 @@
>  /* AC97 Audio Control */
>  #define STK1160_AC97CTL_0              0x500
>  #define STK1160_AC97CTL_1              0x504
> +#define  STK1160_AC97CTL_0_CR          BIT(1)
> +#define  STK1160_AC97CTL_0_CW          BIT(2)
>  
>  /* Use [0:6] bits of register 0x504 to set codec command address */
>  #define STK1160_AC97_ADDR              0x504
> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
> index e85e12e..acd1c81 100644
> --- a/drivers/media/usb/stk1160/stk1160.h
> +++ b/drivers/media/usb/stk1160/stk1160.h
> @@ -50,6 +50,8 @@
>  #define STK1160_MAX_INPUT 4
>  #define STK1160_SVIDEO_INPUT 4
>  
> +#define STK1160_AC97_TIMEOUT 50
> +
>  #define STK1160_I2C_TIMEOUT 100
>  
>  
> 
> On Tue, Dec 06, 2016 at 10:56:26AM -0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 5 Dec 2016 22:06:59 +0100
> > Marcel Hasler <mahasler@gmail.com> escreveu:
> >   
> > > Hello
> > > 
> > > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:  
> > > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> > > > <mchehab@s-opensource.com> wrote:    
> > > >> Em Sun, 4 Dec 2016 15:25:25 -0300
> > > >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
> > > >>    
> > > >>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:    
> > > >>> > Hello
> > > >>> >
> > > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:    
> > > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> > > >>> >> <mchehab@s-opensource.com> wrote:    
> > > >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> > > >>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
> > > >>> >>>    
> > > >>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> > > >>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> > > >>> >>>> driver also sets this to 8 without any possibility for changing it.    
> > > >>> >>>
> > > >>> >>> The problem of removing the mixer is that you need this kind of
> > > >>> >>> crap to setup the volumes on a non-standard way.
> > > >>> >>>    
> > > >>> >>
> > > >>> >> Right, that's a good point.
> > > >>> >>    
> > > >>> >>> NACK.
> > > >>> >>>
> > > >>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
> > > >>> >>> em28xx) is that they configure the mixer when an input is selected,
> > > >>> >>> increasing the volume of the active audio channel to 100% and muting
> > > >>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
> > > >>> >>> can change the mixer settings in runtime using some alsa (or pa)
> > > >>> >>> mixer application.
> > > >>> >>>    
> > > >>> >>
> > > >>> >> Yeah, the AC97 mixer we are currently leveraging
> > > >>> >> exposes many controls that have no meaning in this device,
> > > >>> >> so removing that still looks like an improvement.
> > > >>> >>
> > > >>> >> I guess the proper way is creating our own mixer
> > > >>> >> (not using snd_ac97_mixer)  exposing only the record
> > > >>> >> gain knob.
> > > >>> >>
> > > >>> >> Marcel, what do you think?
> > > >>> >> --
> > > >>> >> Ezequiel García, VanguardiaSur
> > > >>> >> www.vanguardiasur.com.ar    
> > > >>> >
> > > >>> > As I have written before, the recording gain isn't actually meant to
> > > >>> > be changed by the user. In the official Windows driver this value is
> > > >>> > hard-coded to 8 and cannot be changed in any way. And there really is
> > > >>> > no good reason why anyone should need to mess with it in the first
> > > >>> > place. The default value will give the best results in pretty much all
> > > >>> > cases and produces approximately the same volume as the internal 8-bit
> > > >>> > ADC whose gain cannot be changed at all, not even by a driver.
> > > >>> >
> > > >>> > I had considered writing a mixer but chose not to. If the gain setting
> > > >>> > is openly exposed to mixer applications, how do you tell the users
> > > >>> > that the value set by the driver already is the optimal and
> > > >>> > recommended value and that they shouldn't mess with the controls
> > > >>> > unless they really have to? By having a module parameter, this setting
> > > >>> > is practically hidden from the normal user but still is available to
> > > >>> > power-users if they think they really need it. In the end it's really
> > > >>> > just a compromise between hiding it completely and exposing it openly.
> > > >>> > Also, this way the driver guarantees reproducible results, since
> > > >>> > there's no need to remember the positions of any volume sliders.
> > > >>> >    
> > > >>>
> > > >>> Hm, right. I've never changed the record gain, and it's true that it
> > > >>> doens't really improve the volume. So, I would be OK with having
> > > >>> a module parameter.
> > > >>>
> > > >>> On the other side, we are exposing it currently, through the "Capture"
> > > >>> mixer control:
> > > >>>
> > > >>> Simple mixer control 'Capture',0
> > > >>>   Capabilities: cvolume cswitch cswitch-joined
> > > >>>   Capture channels: Front Left - Front Right
> > > >>>   Limits: Capture 0 - 15
> > > >>>   Front Left: Capture 10 [67%] [15.00dB] [on]
> > > >>>   Front Right: Capture 8 [53%] [12.00dB] [on]
> > > >>>
> > > >>> So, it would be user-friendly to keep the user interface and continue
> > > >>> to expose the same knob - even if the default is the optimal, etc.
> > > >>>
> > > >>> To be completely honest, I don't think any user is really relying
> > > >>> on any REC_GAIN / Capture setting, and I'm completely OK
> > > >>> with having a mixer control or a module parameter. It doesn't matter.    
> > > >>
> > > >> If you're positive that *all* stk1160 use the ac97 mixer the
> > > >> same way, and that there's no sense on having a mixer for it,
> > > >> then it would be ok to remove it.
> > > >>    
> > > >
> > > > Let's remove it then!
> > > >    
> > > >> In such case, then why you need a modprobe parameter to allow
> > > >> setting the record level? If this mixer entry is not used,
> > > >> just set it to zero and be happy with that.
> > > >>    
> > > >
> > > > Let's remove the module param too, then.    
> > > 
> > > I'm okay with that.
> > >   
> > > >
> > > > Thanks,
> > > > --
> > > > Ezequiel García, VanguardiaSur
> > > > www.vanguardiasur.com.ar    
> > > 
> > > I'm willing to prepare one final patchset, provided we can agree on
> > > and resolve all issues beforehand.
> > > 
> > > So far the changes would be to remove the module param and to poll
> > > STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
> > > to also poll it before writing, although that never caused problems.  
> > 
> > Sounds ok. My experience with AC97 on em28xx is that, as new devices
> > were added, the delay needed for AC97 varied on some of those new
> > devices. That's why checking if AC97 is ready before writing was
> > added to its code.
> >   
> > > 
> > > I'll post some code for review before actually submitting patches.
> > > Mauro, is there anything else that you think should be changed? If so,
> > > please tell me now. Thanks.
> > > 
> > > Best regards
> > > Marcel  
> > 
> > 
> > 
> > Thanks,
> > Mauro  
> 
> Marcel



Thanks,
Mauro

Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.

[Marcel Hasler]

Hello

2016-12-12 12:21 GMT+01:00 Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> Em Sun, 11 Dec 2016 13:20:06 +0100
> mahasler@gmail.com escreveu:
>
>> Sorry about the broken formatting. Here's the diff once more:
>
> The patch itself looks ok. Just a few comments.
>
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 95648ac..708792b 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -23,11 +23,30 @@
>>   *
>>   */
>>
>> -#include <linux/module.h>
>
> This change seems to be unrelated.
>

You are right, this was a left-over after the first patch. I can fix that.

>> +#include <linux/delay.h>
>>
>>  #include "stk1160.h"
>>  #include "stk1160-reg.h"
>>
>> +static int stk1160_ac97_wait_transfer_complete(struct stk1160 *dev)
>> +{
>> +       unsigned long timeout = jiffies + msecs_to_jiffies(STK1160_AC97_TIMEOUT);
>> +       u8 value;
>> +
>> +       /* Wait for AC97 transfer to complete */
>> +       while (time_is_after_jiffies(timeout)) {
>> +               stk1160_read_reg(dev, STK1160_AC97CTL_0, &value);
>> +
>> +               if (!(value & (STK1160_AC97CTL_0_CR | STK1160_AC97CTL_0_CW)))
>> +                       return 0;
>> +
>> +               msleep(1);
>
> It will likely sleep ~10ms. Maybe you likely need to use usleep_range().
> Please read:
>         Documentation/timers/timers-howto.txt
>

Thanks for the hint. I'll change that.

>> +       }
>> +
>> +       stk1160_err("AC97 transfer took too long, this should never happen!");
>> +       return -EBUSY;
>> +}
>> +
>>  static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>>  {
>>         /* Set codec register address */
>> @@ -37,11 +56,11 @@ static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>>         stk1160_write_reg(dev, STK1160_AC97_CMD, value & 0xff);
>>         stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value & 0xff00) >> 8);
>>
>> -       /*
>> -        * Set command write bit to initiate write operation.
>> -        * The bit will be cleared when transfer is done.
>> -        */
>> +       /* Set command write bit to initiate write operation */
>>         stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
>> +
>> +       /* Wait for command write bit to be cleared */
>> +       stk1160_ac97_wait_transfer_complete(dev);
>>  }
>>
>>  #ifdef DEBUG
>> @@ -53,12 +72,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
>>         /* Set codec register address */
>>         stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
>>
>> -       /*
>> -        * Set command read bit to initiate read operation.
>> -        * The bit will be cleared when transfer is done.
>> -        */
>> +       /* Set command read bit to initiate read operation */
>>         stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
>>
>> +       /* Wait for command read bit to be cleared */
>> +       if (stk1160_ac97_wait_transfer_complete(dev) < 0) {
>> +               return 0;
>> +       }
>> +
>>         /* Retrieve register value */
>>         stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
>>         stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
>> diff --git a/drivers/media/usb/stk1160/stk1160-reg.h b/drivers/media/usb/stk1160/stk1160-reg.h
>> index 296a9e7..7b08a3c 100644
>> --- a/drivers/media/usb/stk1160/stk1160-reg.h
>> +++ b/drivers/media/usb/stk1160/stk1160-reg.h
>> @@ -122,6 +122,8 @@
>>  /* AC97 Audio Control */
>>  #define STK1160_AC97CTL_0              0x500
>>  #define STK1160_AC97CTL_1              0x504
>> +#define  STK1160_AC97CTL_0_CR          BIT(1)
>> +#define  STK1160_AC97CTL_0_CW          BIT(2)
>>
>>  /* Use [0:6] bits of register 0x504 to set codec command address */
>>  #define STK1160_AC97_ADDR              0x504
>> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
>> index e85e12e..acd1c81 100644
>> --- a/drivers/media/usb/stk1160/stk1160.h
>> +++ b/drivers/media/usb/stk1160/stk1160.h
>> @@ -50,6 +50,8 @@
>>  #define STK1160_MAX_INPUT 4
>>  #define STK1160_SVIDEO_INPUT 4
>>
>> +#define STK1160_AC97_TIMEOUT 50
>> +
>>  #define STK1160_I2C_TIMEOUT 100
>>
>>
>>
>> On Tue, Dec 06, 2016 at 10:56:26AM -0200, Mauro Carvalho Chehab wrote:
>> > Em Mon, 5 Dec 2016 22:06:59 +0100
>> > Marcel Hasler <mahasler@gmail.com> escreveu:
>> >
>> > > Hello
>> > >
>> > > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> > > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> > > > <mchehab@s-opensource.com> wrote:
>> > > >> Em Sun, 4 Dec 2016 15:25:25 -0300
>> > > >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>> > > >>
>> > > >>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
>> > > >>> > Hello
>> > > >>> >
>> > > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> > > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> > > >>> >> <mchehab@s-opensource.com> wrote:
>> > > >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> > > >>> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
>> > > >>> >>>
>> > > >>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>> > > >>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>> > > >>> >>>> driver also sets this to 8 without any possibility for changing it.
>> > > >>> >>>
>> > > >>> >>> The problem of removing the mixer is that you need this kind of
>> > > >>> >>> crap to setup the volumes on a non-standard way.
>> > > >>> >>>
>> > > >>> >>
>> > > >>> >> Right, that's a good point.
>> > > >>> >>
>> > > >>> >>> NACK.
>> > > >>> >>>
>> > > >>> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
>> > > >>> >>> em28xx) is that they configure the mixer when an input is selected,
>> > > >>> >>> increasing the volume of the active audio channel to 100% and muting
>> > > >>> >>> the other audio channels. Yet, as the alsa mixer is exported, users
>> > > >>> >>> can change the mixer settings in runtime using some alsa (or pa)
>> > > >>> >>> mixer application.
>> > > >>> >>>
>> > > >>> >>
>> > > >>> >> Yeah, the AC97 mixer we are currently leveraging
>> > > >>> >> exposes many controls that have no meaning in this device,
>> > > >>> >> so removing that still looks like an improvement.
>> > > >>> >>
>> > > >>> >> I guess the proper way is creating our own mixer
>> > > >>> >> (not using snd_ac97_mixer)  exposing only the record
>> > > >>> >> gain knob.
>> > > >>> >>
>> > > >>> >> Marcel, what do you think?
>> > > >>> >> --
>> > > >>> >> Ezequiel García, VanguardiaSur
>> > > >>> >> www.vanguardiasur.com.ar
>> > > >>> >
>> > > >>> > As I have written before, the recording gain isn't actually meant to
>> > > >>> > be changed by the user. In the official Windows driver this value is
>> > > >>> > hard-coded to 8 and cannot be changed in any way. And there really is
>> > > >>> > no good reason why anyone should need to mess with it in the first
>> > > >>> > place. The default value will give the best results in pretty much all
>> > > >>> > cases and produces approximately the same volume as the internal 8-bit
>> > > >>> > ADC whose gain cannot be changed at all, not even by a driver.
>> > > >>> >
>> > > >>> > I had considered writing a mixer but chose not to. If the gain setting
>> > > >>> > is openly exposed to mixer applications, how do you tell the users
>> > > >>> > that the value set by the driver already is the optimal and
>> > > >>> > recommended value and that they shouldn't mess with the controls
>> > > >>> > unless they really have to? By having a module parameter, this setting
>> > > >>> > is practically hidden from the normal user but still is available to
>> > > >>> > power-users if they think they really need it. In the end it's really
>> > > >>> > just a compromise between hiding it completely and exposing it openly.
>> > > >>> > Also, this way the driver guarantees reproducible results, since
>> > > >>> > there's no need to remember the positions of any volume sliders.
>> > > >>> >
>> > > >>>
>> > > >>> Hm, right. I've never changed the record gain, and it's true that it
>> > > >>> doens't really improve the volume. So, I would be OK with having
>> > > >>> a module parameter.
>> > > >>>
>> > > >>> On the other side, we are exposing it currently, through the "Capture"
>> > > >>> mixer control:
>> > > >>>
>> > > >>> Simple mixer control 'Capture',0
>> > > >>>   Capabilities: cvolume cswitch cswitch-joined
>> > > >>>   Capture channels: Front Left - Front Right
>> > > >>>   Limits: Capture 0 - 15
>> > > >>>   Front Left: Capture 10 [67%] [15.00dB] [on]
>> > > >>>   Front Right: Capture 8 [53%] [12.00dB] [on]
>> > > >>>
>> > > >>> So, it would be user-friendly to keep the user interface and continue
>> > > >>> to expose the same knob - even if the default is the optimal, etc.
>> > > >>>
>> > > >>> To be completely honest, I don't think any user is really relying
>> > > >>> on any REC_GAIN / Capture setting, and I'm completely OK
>> > > >>> with having a mixer control or a module parameter. It doesn't matter.
>> > > >>
>> > > >> If you're positive that *all* stk1160 use the ac97 mixer the
>> > > >> same way, and that there's no sense on having a mixer for it,
>> > > >> then it would be ok to remove it.
>> > > >>
>> > > >
>> > > > Let's remove it then!
>> > > >
>> > > >> In such case, then why you need a modprobe parameter to allow
>> > > >> setting the record level? If this mixer entry is not used,
>> > > >> just set it to zero and be happy with that.
>> > > >>
>> > > >
>> > > > Let's remove the module param too, then.
>> > >
>> > > I'm okay with that.
>> > >
>> > > >
>> > > > Thanks,
>> > > > --
>> > > > Ezequiel García, VanguardiaSur
>> > > > www.vanguardiasur.com.ar
>> > >
>> > > I'm willing to prepare one final patchset, provided we can agree on
>> > > and resolve all issues beforehand.
>> > >
>> > > So far the changes would be to remove the module param and to poll
>> > > STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better
>> > > to also poll it before writing, although that never caused problems.
>> >
>> > Sounds ok. My experience with AC97 on em28xx is that, as new devices
>> > were added, the delay needed for AC97 varied on some of those new
>> > devices. That's why checking if AC97 is ready before writing was
>> > added to its code.
>> >
>> > >
>> > > I'll post some code for review before actually submitting patches.
>> > > Mauro, is there anything else that you think should be changed? If so,
>> > > please tell me now. Thanks.
>> > >
>> > > Best regards
>> > > Marcel
>> >
>> >
>> >
>> > Thanks,
>> > Mauro
>>
>> Marcel
>
>
>
> Thanks,
> Mauro

Best regards
Marcel

[PATCH v3 4/4] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Marcel Hasler]

The STK1160 needs some time to transfer data from the AC97 registers into its own. On some
systems reading the chip's own registers to soon will return wrong values. The "proper" way to
handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the
command bit has been cleared, but this may not be worth the hassle.

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 60327af..b39f51b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -23,6 +23,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"
@@ -64,6 +65,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
 	 */
 	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+	/*
+	 * Give the chip some time to transfer the data.
+	 * The proper way would be to poll STK1160_AC97CTL_0
+	 * until the command bit has been cleared, but this
+	 * may not be worth the hassle.
+	 */
+	usleep_range(20, 40);
+
 	/* Retrieve register value */
 	stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
 	stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
-- 
2.10.2

Re: [PATCH v3 4/4] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Mauro Carvalho Chehab]

Em Sun, 27 Nov 2016 12:12:36 +0100
Marcel Hasler <mahasler@gmail.com> escreveu:

> The STK1160 needs some time to transfer data from the AC97 registers into its own. On some
> systems reading the chip's own registers to soon will return wrong values. The "proper" way to
> handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the
> command bit has been cleared, but this may not be worth the hassle.
> 
> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> ---
>  drivers/media/usb/stk1160/stk1160-ac97.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 60327af..b39f51b 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -23,6 +23,7 @@
>   *
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/module.h>
>  
>  #include "stk1160.h"
> @@ -64,6 +65,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
>  	 */
>  	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
>  
> +	/*
> +	 * Give the chip some time to transfer the data.
> +	 * The proper way would be to poll STK1160_AC97CTL_0
> +	 * until the command bit has been cleared, but this
> +	 * may not be worth the hassle.

Why not? Relying on a fixed amount time is not nice.

Take a look at em28xx_is_ac97_ready() function, at
drivers/media/usb/em28xx/em28xx-core.c to see how this could be
implemented instead.


> +	 */
> +	usleep_range(20, 40);
> +

>  	/* Retrieve register value */
>  	stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
>  	stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);



Thanks,
Mauro

Re: [PATCH v3 4/4] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Ezequiel Garcia]

On 2 December 2016 at 08:09, Mauro Carvalho Chehab
<mchehab@s-opensource.com> wrote:
> Em Sun, 27 Nov 2016 12:12:36 +0100
> Marcel Hasler <mahasler@gmail.com> escreveu:
>
>> The STK1160 needs some time to transfer data from the AC97 registers into its own. On some
>> systems reading the chip's own registers to soon will return wrong values. The "proper" way to
>> handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the
>> command bit has been cleared, but this may not be worth the hassle.
>>
>> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 60327af..b39f51b 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -23,6 +23,7 @@
>>   *
>>   */
>>
>> +#include <linux/delay.h>
>>  #include <linux/module.h>
>>
>>  #include "stk1160.h"
>> @@ -64,6 +65,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
>>        */
>>       stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
>>
>> +     /*
>> +      * Give the chip some time to transfer the data.
>> +      * The proper way would be to poll STK1160_AC97CTL_0
>> +      * until the command bit has been cleared, but this
>> +      * may not be worth the hassle.
>
> Why not? Relying on a fixed amount time is not nice.
>
> Take a look at em28xx_is_ac97_ready() function, at
> drivers/media/usb/em28xx/em28xx-core.c to see how this could be
> implemented instead.
>

We were reluctant to implement this properly because the read reg
function is only used for debugging purposes, to dump registers.

That said, it's not too much of a hassle to do it properly, and
we might have to if we are going to have our own mixer.

>
>> +      */
>> +     usleep_range(20, 40);
>> +
>
>>       /* Retrieve register value */
>>       stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
>>       stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
>
>
>
> Thanks,
> Mauro



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v3 0/4] stk1160: Let the driver setup the device's internal AC97 codec

[Ezequiel Garcia]

On 27 November 2016 at 08:07, Marcel Hasler <mahasler@gmail.com> wrote:
> This patchset is a result of my attempt to fix a bug (https://bugzilla.kernel.org/show_bug.cgi?id=180071) that eventually turned out to be caused by a missing quirk in snd-usb-audio. My idea was to remove the AC97 interface and setup the codec using the same values and in the same order as the Windows driver does, hoping there might be some "magic" sequence that would make the sound work the way it should. Although this didn't help to fix the problem, I found these changes to be useful nevertheless.
>
> IMHO, having all of the AC97 codec's channels exposed to userspace is confusing since most of them have no meaning for this device anyway. Changing these values in alsamixer has either no effect at all or may even reduce the sound quality since it can actually increase the line-in DC offset (slightly).
>
> In addition, having to re-select the correct capture channel everytime the device has been plugged in is annoying. At least on my systems the mixer setup is only saved if the device is plugged in during shutdown/reboot. I also get error messages in my kernel log when I unplug the device because some process (probably the AC97 driver) ist trying to read from the device after it has been removed. Either way the device should work out-of-the-box without the need for the user to manually setup channels.
>
> The first patch in the set therefore removes the 'stk1160-mixer' and lets the driver setup the AC97 codec using the same values as the Windows driver. Although some of the values seem to be defaults I let the driver set them either way, just to be sure.
>
> The second patch adds a check to determine whether the device is strapped to use the internal 8-bit ADC or an external chip. There's currently no check in place to determine whether the device uses AC-link or I2S, but then again I haven't heard of any of these devices actually using an I2S chip. If the device uses the internal ADC the AC97 setup can be skipped. I implemented the check inside stk1160-ac97. It could just as well be in stk1160-core but this way just seemed cleaner. If at some point the need arises to check other power-on strap values, it might make sense to refactor this then.
>
> The third patch adds a new module parameter for setting the record gain manually since the AC97 chip is no longer exposed to userspace. The Windows driver doesn't allow this value to be changed but instead always sets it to 8 (of 15). While this should be fine for most users, some may prefer something higher.
>
> The fourth patch addresses an issue when reading from the AC97 chip too soon, resulting in corrupt data.
>
> Changes from version 2:
> * Added copyright notice
> * Added defines for POSVA bytes and bits
> * Added check for ACDOUT bit to determine whether audio is disabled completely
> * Removed info output for gain setting
> * Added fourth patch which had been submitted independently before
> * Expanded comment on AC97 read delay
>
> Marcel Hasler (4):
>   stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
>   stk1160: Check whether to use AC97 codec.
>   stk1160: Add module param for setting the record gain.
>   stk1160: Give the chip some time to retrieve data from AC97 codec.
>

For the whole set:

Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar