* [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec
@ 2016-12-15 22:11 Marcel Hasler
2016-12-15 22:13 ` [PATCH v4 2/3] stk1160: Check whether to use " Marcel Hasler
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Marcel Hasler @ 2016-12-15 22:11 UTC (permalink / raw)
To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media
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, or whether audio is disabled altogether. 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 addresses an issue when reading from the AC97 chip too soon, resulting in corrupt data.
Changes from version 3:
* Removed module param
* Implemented polling read/write bits
Marcel Hasler (3):
stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
stk1160: Check whether to use AC97 codec.
stk1160: Wait for completion of transfers to and from AC97 codec.
drivers/media/usb/stk1160/Kconfig | 10 +-
drivers/media/usb/stk1160/Makefile | 4 +-
drivers/media/usb/stk1160/stk1160-ac97.c | 183 +++++++++++++++++--------------
drivers/media/usb/stk1160/stk1160-core.c | 8 +-
drivers/media/usb/stk1160/stk1160-reg.h | 10 ++
drivers/media/usb/stk1160/stk1160.h | 11 +-
6 files changed, 116 insertions(+), 110 deletions(-)
--
2.10.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v4 2/3] stk1160: Check whether to use AC97 codec.
2016-12-15 22:11 [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Marcel Hasler
@ 2016-12-15 22:13 ` Marcel Hasler
2016-12-15 22:14 ` [PATCH v4 3/3] stk1160: Wait for completion of transfers to and from " Marcel Hasler
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Marcel Hasler @ 2016-12-15 22:13 UTC (permalink / raw)
To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media
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 502fc8e..5e9b76e 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -91,8 +91,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 3/3] stk1160: Wait for completion of transfers to and from AC97 codec.
2016-12-15 22:11 [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Marcel Hasler
2016-12-15 22:13 ` [PATCH v4 2/3] stk1160: Check whether to use " Marcel Hasler
@ 2016-12-15 22:14 ` Marcel Hasler
2016-12-15 22:17 ` [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler
2017-01-01 16:11 ` [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Ezequiel Garcia
3 siblings, 0 replies; 5+ messages in thread
From: Marcel Hasler @ 2016-12-15 22:14 UTC (permalink / raw)
To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media
The STK1160 needs some time to transfer data to and from the AC97 codec. The transfer completion
is indicated by command read/write bits in the chip's audio control register. The driver should
poll these bits and wait until they have been cleared by hardware before trying to retrive the
results of a read operation or setting a new write command.
Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
drivers/media/usb/stk1160/stk1160-ac97.c | 39 +++++++++++++++++++++++++-------
drivers/media/usb/stk1160/stk1160-reg.h | 2 ++
drivers/media/usb/stk1160/stk1160.h | 2 ++
3 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 5e9b76e..439d1b7 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -23,9 +23,30 @@
*
*/
+#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;
+
+ usleep_range(50, 100);
+ }
+
+ 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 */
@@ -35,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
@@ -51,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
/* TODO: Print helpers
--
2.10.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
2016-12-15 22:11 [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Marcel Hasler
2016-12-15 22:13 ` [PATCH v4 2/3] stk1160: Check whether to use " Marcel Hasler
2016-12-15 22:14 ` [PATCH v4 3/3] stk1160: Wait for completion of transfers to and from " Marcel Hasler
@ 2016-12-15 22:17 ` Marcel Hasler
2017-01-01 16:11 ` [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Ezequiel Garcia
3 siblings, 0 replies; 5+ messages in thread
From: Marcel Hasler @ 2016-12-15 22:17 UTC (permalink / raw)
To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media
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 | 126 +++++++++++--------------------
drivers/media/usb/stk1160/stk1160-core.c | 5 +-
drivers/media/usb/stk1160/stk1160.h | 9 +--
5 files changed, 50 insertions(+), 104 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..502fc8e 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>
@@ -20,20 +23,11 @@
*
*/
-#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 +42,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 +64,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec
2016-12-15 22:11 [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Marcel Hasler
` (2 preceding siblings ...)
2016-12-15 22:17 ` [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler
@ 2017-01-01 16:11 ` Ezequiel Garcia
3 siblings, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2017-01-01 16:11 UTC (permalink / raw)
To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media
On 15 December 2016 at 19:11, 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, or whether audio is disabled altogether. 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 addresses an issue when reading from the AC97 chip too soon, resulting in corrupt data.
>
> Changes from version 3:
> * Removed module param
> * Implemented polling read/write bits
>
> Marcel Hasler (3):
> stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
> stk1160: Check whether to use AC97 codec.
> stk1160: Wait for completion of transfers to and from AC97 codec.
>
For the whole set:
Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-01 16:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-15 22:11 [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Marcel Hasler
2016-12-15 22:13 ` [PATCH v4 2/3] stk1160: Check whether to use " Marcel Hasler
2016-12-15 22:14 ` [PATCH v4 3/3] stk1160: Wait for completion of transfers to and from " Marcel Hasler
2016-12-15 22:17 ` [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler
2017-01-01 16:11 ` [PATCH v4 0/3] stk1160: Let the driver setup the device's internal AC97 codec Ezequiel Garcia
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).