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

Commit Message

Marcel Hasler Dec. 11, 2016, 12:07 p.m. UTC

Hello

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

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

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

Patch

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

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

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

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

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

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

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

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

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

+#define STK1160_AC97_TIMEOUT 50
+
 #define STK1160_I2C_TIMEOUT 100

Best regards