from:"Marcel Hasler"

[PATCH 2/2] stk1160: Check whether to use AC97 codec or internal ADC.

2016-10-27 Thread 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, there's no point trying to setup the unavailable AC97 codec.

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

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index d3665ce..0199fad 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -90,8 +90,23 @@ void stk1160_ac97_dump_regs(struct stk1160 *dev)
 }
 #endif
 
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+   u8 value;
+
+   stk1160_read_reg(dev, STK1160_POSVA, );
+
+   /* Bit 2 high means internal ADC */
+   return !(value & 0x04);
+}
+
 void stk1160_ac97_setup(struct stk1160 *dev)
 {
+   if (!stk1160_has_ac97(dev)) {
+   stk1160_dbg("Device uses internal 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..a4ab586 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -26,6 +26,9 @@
 /* Remote Wakup Control */
 #define STK1160_RMCTL  0x00c
 
+/* Power-on Strapping Data */
+#define STK1160_POSVA  0x010
+
 /*
  * Decoder Control Register:
  * This byte controls capture start/stop
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] stk1160: Remove VIDEO_STK1160_AC97 and SND_AC97_CODEC from Kconfig and Makefile.

2016-10-27 Thread Marcel Hasler

Hi

Sure, I'll clean up the patches later on today and resubmit. The last
one should be merged with the first one anyway, I missed that at
first.

Regards
Marcel

2016-10-27 14:38 GMT+02:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
>
> Marcel,
>
> Thanks a lot for all your stk1160 fixes. They are much appreciated! In
> particular,
> the click noise was something we really wanted to get rid of:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113981.html
>
> Regarding the linux-media fixes, is there any chance you re-submit
> this set of patches, in way that they are properly numbered
> PATCH 1, PATCH 2, PATCH 3...
>
> git-format-patch is able to do that for you automatically.
>
> You may also include a cover letter (it's optional) to explain
> what stuff you are fixing, how you tested, where are the patches
> based, and anything else you want to mention.
>
> (And don't forget to Cc the media mailing list)
>
> Thanks again!
> Ezequiel
>
> On 27 October 2016 at 06:10, Marcel Hasler <mahas...@gmail.com> wrote:
> > The VIDEO_STK1160_AC97 option is no longer needed after the removal of 
> > stk1160-mixer. For the
> > same reason SND and SND_AC97_CODEC are no longer required.
> >
> > Signed-off-by: Marcel Hasler <mahas...@gmail.com>
> > ---
> >  drivers/media/usb/stk1160/Kconfig  | 3 +--
> >  drivers/media/usb/stk1160/Makefile | 4 +---
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/usb/stk1160/Kconfig 
> > b/drivers/media/usb/stk1160/Kconfig
> > index 53617da..22dff4f 100644
> > --- a/drivers/media/usb/stk1160/Kconfig
> > +++ b/drivers/media/usb/stk1160/Kconfig
> > @@ -10,8 +10,7 @@ config VIDEO_STK1160_COMMON
> >
> >  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
> >
> > --
> > 2.10.1
> >
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar


2016-10-27 14:38 GMT+02:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> Marcel,
>
> Thanks a lot for all your stk1160 fixes. They are much appreciated! In
> particular,
> the click noise was something we really wanted to get rid of:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113981.html
>
> Regarding the linux-media fixes, is there any chance you re-submit
> this set of patches, in way that they are properly numbered
> PATCH 1, PATCH 2, PATCH 3...
>
> git-format-patch is able to do that for you automatically.
>
> You may also include a cover letter (it's optional) to explain
> what stuff you are fixing, how you tested, where are the patches
> based, and anything else you want to mention.
>
> (And don't forget to Cc the media mailing list)
>
> Thanks again!
> Ezequiel
>
> On 27 October 2016 at 06:10, Marcel Hasler <mahas...@gmail.com> wrote:
>> The VIDEO_STK1160_AC97 option is no longer needed after the removal of 
>> stk1160-mixer. For the
>> same reason SND and SND_AC97_CODEC are no longer required.
>>
>> Signed-off-by: Marcel Hasler <mahas...@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/Kconfig  | 3 +--
>>  drivers/media/usb/stk1160/Makefile | 4 +---
>>  2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/usb/stk1160/Kconfig 
>> b/drivers/media/usb/stk1160/Kconfig
>> index 53617da..22dff4f 100644
>> --- a/drivers/media/usb/stk1160/Kconfig
>> +++ b/drivers/media/usb/stk1160/Kconfig
>> @@ -10,8 +10,7 @@ config VIDEO_STK1160_COMMON
>>
>>  config VIDEO_STK1160
>> tristate
>> -   depends on (!VIDEO_STK1160_AC97 || (SND='n') || SND) && 
>> VIDEO_STK1160_COMMON

[PATCH 3/3] stk1160: Remove VIDEO_STK1160_AC97 and SND_AC97_CODEC from Kconfig and Makefile.

2016-10-27 Thread Marcel Hasler

The VIDEO_STK1160_AC97 option is no longer needed after the removal of 
stk1160-mixer. For the
same reason SND and SND_AC97_CODEC are no longer required.

Signed-off-by: Marcel Hasler <mahas...@gmail.com>
---
 drivers/media/usb/stk1160/Kconfig  | 3 +--
 drivers/media/usb/stk1160/Makefile | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/stk1160/Kconfig 
b/drivers/media/usb/stk1160/Kconfig
index 53617da..22dff4f 100644
--- a/drivers/media/usb/stk1160/Kconfig
+++ b/drivers/media/usb/stk1160/Kconfig
@@ -10,8 +10,7 @@ config VIDEO_STK1160_COMMON
 
 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
 
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-27 Thread Marcel Hasler

Exposing the device's internal AC97 codec to userspace seems rather pointless. 
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 <mahas...@gmail.com>
---
 drivers/media/usb/stk1160/Kconfig|   7 --
 drivers/media/usb/stk1160/stk1160-ac97.c | 121 +++
 drivers/media/usb/stk1160/stk1160-core.c |   5 +-
 drivers/media/usb/stk1160/stk1160.h  |   9 +--
 4 files changed, 45 insertions(+), 97 deletions(-)

diff --git a/drivers/media/usb/stk1160/Kconfig 
b/drivers/media/usb/stk1160/Kconfig
index 95584c1..53617da 100644
--- a/drivers/media/usb/stk1160/Kconfig
+++ b/drivers/media/usb/stk1160/Kconfig
@@ -8,13 +8,6 @@ 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
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 2dd308f..d3665ce 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -21,19 +21,12 @@
  */
 
 #include 
-#include 
-#include 
-#include 
 
 #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 +41,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 +63,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 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, );
-   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, _ac97_ops, NULL, _bus);
-   if (rc)
-   goto err;
-
-   /* We must set private_data before calling snd_ac97_mixer */
-   memset(_template, 0, sizeof(ac97_template));
-   ac97_template.private_data = dev;
-   ac97_template.scaps = AC97_SCAP_SKIP_MODEM;
-   rc = snd_ac97_mixer(ac97_bus, _template, _ac97);
-

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

2016-10-27 Thread Marcel Hasler

As requested, I've cleaned up my previous patchset for resubmission.

This patchset is really 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.

Marcel Hasler (3):
  stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
  stk1160: Check whether to use AC97 codec or internal ADC.
  stk1160: Add module param for setting the record gain.

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

-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-27 Thread 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 <mahas...@gmail.com>
---
 drivers/media/usb/stk1160/Kconfig|  10 +--
 drivers/media/usb/stk1160/Makefile   |   4 +-
 drivers/media/usb/stk1160/stk1160-ac97.c | 121 +++
 drivers/media/usb/stk1160/stk1160-core.c |   5 +-
 drivers/media/usb/stk1160/stk1160.h  |   9 +--
 5 files changed, 47 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..d3665ce 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -21,19 +21,12 @@
  */
 
 #include 
-#include 
-#include 
-#include 
 
 #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 +41,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 +63,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 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, );
-   if (rc < 0)
-   return rc;
-
-   /* TODO: I'm

[PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC.

2016-10-27 Thread 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, there's no point trying to setup the unavailable AC97 codec.

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

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index d3665ce..6dbc39f 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -90,8 +90,23 @@ void stk1160_ac97_dump_regs(struct stk1160 *dev)
 }
 #endif
 
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+   u8 value;
+
+   stk1160_read_reg(dev, STK1160_POSVA, );
+
+   /* Bit 2 high means internal ADC */
+   return !(value & 0x04);
+}
+
 void stk1160_ac97_setup(struct stk1160 *dev)
 {
+   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..a4ab586 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -26,6 +26,9 @@
 /* Remote Wakup Control */
 #define STK1160_RMCTL  0x00c
 
+/* Power-on Strapping Data */
+#define STK1160_POSVA  0x010
+
 /*
  * Decoder Control Register:
  * This byte controls capture start/stop
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-27 Thread 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 <mahas...@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 6dbc39f..31bdd60d 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -25,6 +25,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 */
@@ -122,7 +127,11 @@ 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, 0x); /* Master volume */
-   stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
+
+   /* Record gain */
+   gain = (gain > 15) ? 15 : gain;
+   stk1160_info("Setting capture gain to %d.", gain);
+   stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain);
 
 #ifdef DEBUG
stk1160_ac97_dump_regs(dev);
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-26 Thread Marcel Hasler

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 28 October 2016 at 05:52, Marcel Hasler <mahas...@gmail.com> wrote:
>> 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 <mahas...@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
>> b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 31bdd60d..caa65a8 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -20,6 +20,7 @@
>>   *
>>   */
>>
>> +#include 
>>  #include 
>>
>>  #include "stk1160.h"
>> @@ -61,6 +62,9 @@ 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 data */
>> +   usleep_range(20, 40);
>> +
>
> I don't recall any issues with this. In any case, we only read the registers
> for debugging purposes, so it's not a big deal.
>

I actually just re-tested this, as I recently replaced my computer's
main board. I didn't happen with my old one, but it does with my new
one, just as with both of my notebooks.

> Maybe it would be better to expand the comment a little bit,
> using your commit log:
>
> ""
> 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.
> ""
>
> This way, if the sleep proves problematic in the future,
> the "proper way" is already documented.
>
>> /* Retrieve register value */
>> stk1160_read_reg(dev, STK1160_AC97_CMD, );
>> stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
>> --
>> 2.10.1
>>
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-26 Thread Marcel Hasler

2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 27 October 2016 at 17:35, Marcel Hasler <mahas...@gmail.com> wrote:
>> 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 <mahas...@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 11 ++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
>> b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 6dbc39f..31bdd60d 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -25,6 +25,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 */
>> @@ -122,7 +127,11 @@ 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, 0x); /* Master volume */
>> -   stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
>> +
>> +   /* Record gain */
>> +   gain = (gain > 15) ? 15 : gain;
>> +   stk1160_info("Setting capture gain to %d.", gain);
>
> This message doesn't add anything useful, can we drop it?
>

I think it would make sense to have some kind of feedback, at least
when the default value has been overridden. How about making this
conditional?

>> +   stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain);
>>
>>  #ifdef DEBUG
>> stk1160_ac97_dump_regs(dev);
>> --
>> 2.10.1
>>
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.

2016-11-26 Thread Marcel Hasler

Hello, and thanks for your feedback.

2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> Marcel,
>
> On 27 October 2016 at 17:34, Marcel Hasler <mahas...@gmail.com> wrote:
>> 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 <mahas...@gmail.com>
>
> This patch is *awesome*.
>
> You've re-written the file ;-), so if you want to put your
> copyright on stk1160-ac97.c, be my guest.
>

Thanks, will do :-)

> Also, just a minor comment, see below.
>
>> ---
>>  drivers/media/usb/stk1160/Kconfig|  10 +--
>>  drivers/media/usb/stk1160/Makefile   |   4 +-
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 121 
>> +++
>>  drivers/media/usb/stk1160/stk1160-core.c |   5 +-
>>  drivers/media/usb/stk1160/stk1160.h  |   9 +--
>>  5 files changed, 47 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..d3665ce 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -21,19 +21,12 @@
>>   */
>>
>>  #include 
>> -#include 
>> -#include 
>> -#include 
>>
>>  #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 +41,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 +63,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)
>
> static void stk1160_ac97_dump_regs ?
>

Righ

Re: [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC.

2016-11-26 Thread Marcel Hasler

2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 27 October 2016 at 17:34, Marcel Hasler <mahas...@gmail.com> wrote:
>> 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, there's no point trying to setup the unavailable AC97 codec.
>>
>> Signed-off-by: Marcel Hasler <mahas...@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 15 +++
>>  drivers/media/usb/stk1160/stk1160-core.c |  3 +--
>>  drivers/media/usb/stk1160/stk1160-reg.h  |  3 +++
>>  3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
>> b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index d3665ce..6dbc39f 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -90,8 +90,23 @@ void stk1160_ac97_dump_regs(struct stk1160 *dev)
>>  }
>>  #endif
>>
>> +int stk1160_has_ac97(struct stk1160 *dev)
>> +{
>> +   u8 value;
>> +
>> +   stk1160_read_reg(dev, STK1160_POSVA, );
>> +
>> +   /* Bit 2 high means internal ADC */
>> +   return !(value & 0x04);
>
> How about define a macro such as:
>
> diff --git a/drivers/media/usb/stk1160/stk1160-reg.h
> b/drivers/media/usb/stk1160/stk1160-reg.h
> index a4ab586fcee1..4922249d7d34 100644
> --- a/drivers/media/usb/stk1160/stk1160-reg.h
> +++ b/drivers/media/usb/stk1160/stk1160-reg.h
> @@ -28,6 +28,7 @@
>
>  /* Power-on Strapping Data */
>  #define STK1160_POSVA  0x010
> +#define  STK1160_POSVA_ACSYNC  BIT(2)
>

Good idea, I'll do that.

> Also, the spec mentions another POSVA bit relevant
> to audio ACDOUT. Should we check that too?
>

Yes, that would make sense.

>> +}
>> +
>>  void stk1160_ac97_setup(struct stk1160 *dev)
>>  {
>> +   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..a4ab586 100644
>> --- a/drivers/media/usb/stk1160/stk1160-reg.h
>> +++ b/drivers/media/usb/stk1160/stk1160-reg.h
>> @@ -26,6 +26,9 @@
>>  /* Remote Wakup Control */
>>  #define STK1160_RMCTL  0x00c
>>
>> +/* Power-on Strapping Data */
>> +#define STK1160_POSVA  0x010
>> +
>>  /*
>>   * Decoder Control Register:
>>   * This byte controls capture start/stop
>> --
>> 2.10.1
>>
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-26 Thread Marcel Hasler

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 28 October 2016 at 05:52, Marcel Hasler <mahas...@gmail.com> wrote:
>> 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 <mahas...@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
>> b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 31bdd60d..caa65a8 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -20,6 +20,7 @@
>>   *
>>   */
>>
>> +#include 
>>  #include 
>>
>>  #include "stk1160.h"
>> @@ -61,6 +62,9 @@ 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 data */
>> +   usleep_range(20, 40);
>> +
>
> I don't recall any issues with this. In any case, we only read the registers
> for debugging purposes, so it's not a big deal.
>
> Maybe it would be better to expand the comment a little bit,
> using your commit log:
>
> ""
> 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.
> ""
>
> This way, if the sleep proves problematic in the future,
> the "proper way" is already documented.
>

Of course, since the register dump isn't needed anymore, this function
could also be dropped. But then again, I think it would make sense to
keep it, even if it's just for documentation. There might be use for
it in the future. I'll add a comment as suggested. Let me know whether
I should remove the dump, I guess there's no need to keep it. I'll
then prepare a new patchset with all four patches as soon as I can.

>> /* Retrieve register value */
>> stk1160_read_reg(dev, STK1160_AC97_CMD, );
>> stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
>> --
>> 2.10.1
>>
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar

Best regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-27 Thread 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 <mahas...@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
  * 
  *
+ * Copyright (C) 2016 Marcel Hasler
+ * 
+ *
  * Based on Easycap driver by R.M. Thomas
  * Copyright (C) 2010 R.M. Thomas
  * 
@@ -21,19 +24,12 @@
  */
 
 #include 
-#include 
-#include 
-#include 
 
 #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 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

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

2016-11-27 Thread 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 <mahas...@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, );
+   return !(value & STK1160_POSV_L_ACDOUT);
+}
+
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+   u8 value;
+
+   stk1160_read_reg(dev, STK1160_POSV_L, );
+   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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-27 Thread 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 <mahas...@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, 0x); /* 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-27 Thread 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 <mahas...@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 
 #include 
 
 #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, );
stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-11-27 Thread 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-28 Thread 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 <mahas...@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 31bdd60d..caa65a8 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -20,6 +20,7 @@
  *
  */
 
+#include 
 #include 
 
 #include "stk1160.h"
@@ -61,6 +62,9 @@ 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 data */
+   usleep_range(20, 40);
+
/* Retrieve register value */
stk1160_read_reg(dev, STK1160_AC97_CMD, );
stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-28 Thread Marcel Hasler

This patch might need some explaining. I actually noticed this problem
early on while trying to fix the sound problem, but it was only this
morning that I realized the (trivial) cause of it.

I first noticed something strange going on when I read the AC97
registers from /proc/asound/cardX/codec97#0/ac97#0-0+regs using the
current version of the driver. Every time I read that file I would get
slightly different values, not only for one register but for several
of them. Also, every time I plugged in the device and opened alsamixer
I would be presented with a different set of mixer controls. So
obviously something was going wrong while talking to the AC97 chip.

When analyzing the USB trace I took from Windows (on VirtualBox) I
found long delays (2 ms) between control packets and wondered whether
those might be set by the driver on purpose. So I tried adding delays
in stk1160_[read|write]_reg, and sure enough, the problem disappeared.

In retrospective I suspect those long delays to really be the result
of virtualization overhead. I actually tried getting a native trace
using USBpcap, but unfortunately its timer resolution is so low that
it's impossible to get any useful data.

Once I realized what the actual problem was I removed the delays in
stk1160_[read|write]_reg and instead experimented with different
delays in stk1160_read_ac97 and found 20 us to be perfectly sufficient
to get reliable reads.

Now the strange thing about this problem is that it occurs on both of
my notebooks, but not on my desktop computer. I can only speculate
about the reason for this. My theory is that is has something to do
with the way different USB host controllers handle/buffer outgoing
control packets. Both of my notebooks are recent models by Acer (a
normal notebook and a cloudbook) and most likely use the same host
controller. My desktop motherboard on the other hand is a bit older.

So I wonder, have you experienced this problem on your own systems?

Best regards
Marcel

2016-10-28 10:52 GMT+02:00 Marcel Hasler <mahas...@gmail.com>:
> 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 <mahas...@gmail.com>
> ---
>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
> b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 31bdd60d..caa65a8 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -20,6 +20,7 @@
>   *
>   */
>
> +#include 
>  #include 
>
>  #include "stk1160.h"
> @@ -61,6 +62,9 @@ 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 data */
> +   usleep_range(20, 40);
> +
> /* Retrieve register value */
> stk1160_read_reg(dev, STK1160_AC97_CMD, );
> stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
> --
> 2.10.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-12-11 Thread Marcel Hasler

Hello

2016-12-06 13:56 GMT+01:00 Mauro Carvalho Chehab <mche...@s-opensource.com>:
> Em Mon, 5 Dec 2016 22:06:59 +0100
> Marcel Hasler <mahas...@gmail.com> escreveu:
>
>> Hello
>>
>> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
>> > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> > <mche...@s-opensource.com> wrote:
>> >> Em Sun, 4 Dec 2016 15:25:25 -0300
>> >> Ezequiel Garcia <ezequ...@vanguardiasur.com.ar> escreveu:
>> >>
>> >>> On 4 December 2016 at 10:01, Marcel Hasler <mahas...@gmail.com> wrote:
>> >>> > Hello
>> >>> >
>> >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
>> >>> > <ezequ...@vanguardiasur.com.ar>:
>> >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> >>> >> <mche...@s-opensource.com> wrote:
>> >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> >>> >>> Marcel Hasler <mahas...@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, t

[PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec

2016-12-15 Thread Marcel Hasler

automatically.
Reply-To: 
In-Reply-To: <20161215221146.GA9398@arch-desktop>

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 <mahas...@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
  * 
  *
+ * Copyright (C) 2016 Marcel Hasler
+ * 
+ *
  * Based on Easycap driver by R.M. Thomas
  * Copyright (C) 2010 R.M. Thomas
  * 
@@ -20,20 +23,11 @@
  *
  */
 
-#include 
-#include 
-#include 
-#include 
-
 #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 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 co

[PATCH v4 3/3] stk1160: Wait for completion of transfers to and from AC97 codec.

2016-12-15 Thread Marcel Hasler

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 <mahas...@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 
+
 #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, );
+
+   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, );
stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/3] stk1160: Check whether to use AC97 codec.

2016-12-15 Thread 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 <mahas...@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, );
+   return !(value & STK1160_POSV_L_ACDOUT);
+}
+
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+   u8 value;
+
+   stk1160_read_reg(dev, STK1160_POSV_L, );
+   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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-12-15 Thread 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, 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-12-15 Thread 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 <mahas...@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
  * 
  *
+ * Copyright (C) 2016 Marcel Hasler
+ * 
+ *
  * Based on Easycap driver by R.M. Thomas
  * Copyright (C) 2010 R.M. Thomas
  * 
@@ -20,20 +23,11 @@
  *
  */
 
-#include 
-#include 
-#include 
-#include 
-
 #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 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

Re: [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec

2016-12-15 Thread Marcel Hasler

Sorry, somehow the email header got messed up. Ignore this one.

2016-12-15 23:13 GMT+01:00 Marcel Hasler <mahas...@gmail.com>:
> automatically.
> Reply-To:
> In-Reply-To: <20161215221146.GA9398@arch-desktop>
>
> 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 <mahas...@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
>   * 
>   *
> + * Copyright (C) 2016 Marcel Hasler
> + * 
> + *
>   * Based on Easycap driver by R.M. Thomas
>   * Copyright (C) 2010 R.M. Thomas
>   * 
> @@ -20,20 +23,11 @@
>   *
>   */
>
> -#include 
> -#include 
> -#include 
> -#include 
> -
>  #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 channel*/
> -   stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01);
> -}
> +   value =

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

2016-12-12 Thread Marcel Hasler

Hello

2016-12-12 12:21 GMT+01:00 Mauro Carvalho Chehab <mche...@s-opensource.com>:
> Em Sun, 11 Dec 2016 13:20:06 +0100
> mahas...@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 
>
> This change seems to be unrelated.
>

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

>> +#include 
>>
>>  #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, );
>> +
>> +   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, );
>> stk1160_read_reg(dev, STK1160_AC97_CMD + 1, );
>> 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

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

2016-12-05 Thread Marcel Hasler

Hello

2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> <mche...@s-opensource.com> wrote:
>> Em Sun, 4 Dec 2016 15:25:25 -0300
>> Ezequiel Garcia <ezequ...@vanguardiasur.com.ar> escreveu:
>>
>>> On 4 December 2016 at 10:01, Marcel Hasler <mahas...@gmail.com> wrote:
>>> > Hello
>>> >
>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
>>> > <ezequ...@vanguardiasur.com.ar>:
>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>> >> <mche...@s-opensource.com> wrote:
>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>> >>> Marcel Hasler <mahas...@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

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

2016-12-05 Thread Marcel Hasler

2016-12-05 22:06 GMT+01:00 Marcel Hasler <mahas...@gmail.com>:
> Hello
>
> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> <mche...@s-opensource.com> wrote:
>>> Em Sun, 4 Dec 2016 15:25:25 -0300
>>> Ezequiel Garcia <ezequ...@vanguardiasur.com.ar> escreveu:
>>>
>>>> On 4 December 2016 at 10:01, Marcel Hasler <mahas...@gmail.com> wrote:
>>>> > Hello
>>>> >
>>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
>>>> > <ezequ...@vanguardiasur.com.ar>:
>>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>>> >> <mche...@s-opensource.com> wrote:
>>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>>> >>> Marcel Hasler <mahas...@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 currentl

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

2016-12-04 Thread Marcel Hasler

Hello

2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequ...@vanguardiasur.com.ar>:
> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> <mche...@s-opensource.com> wrote:
>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> Marcel Hasler <mahas...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] stk1160: Check whether to use AC97 codec or internal ADC.

Re: [PATCH 3/3] stk1160: Remove VIDEO_STK1160_AC97 and SND_AC97_CODEC from Kconfig and Makefile.

[PATCH 3/3] stk1160: Remove VIDEO_STK1160_AC97 and SND_AC97_CODEC from Kconfig and Makefile.

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

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

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

[PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC.

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

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

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

Re: [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.

Re: [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC.

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

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

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

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

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

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

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

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

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

[PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec

[PATCH v4 3/3] stk1160: Wait for completion of transfers to and from AC97 codec.

[PATCH v4 2/3] stk1160: Check whether to use AC97 codec.

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

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

Re: [PATCH v4 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec

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

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

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

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

31 matches

Site Navigation

Mail list logo

Footer information