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 :
> 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 Carvalho Chehab wrote:
>> > Em Mon, 5 Dec 2016 22:06:59 +0100
>> > Marcel Hasler  escreveu:
>> >
>> > > Hello
>> > >
>> > > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia 
>> > > :
>> > > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>> > > >  wrote:
>> > > >> Em Sun, 4 Dec 2016 15:25:25 -0300
>> > > >> Ezequiel Garcia  escreveu:
>> > > >>
>> > > >>> On 4 December 2016 at 10:01, Marcel Hasler  
>> > > >>> wrote:
>> > > >>> > Hello
>> > > >>> >
>> > > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
>> > > >>> > :
>> > > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> >

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

2016-12-12 Thread Mauro Carvalho Chehab 
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.

> +#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

> +   }
> +
> +   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 Carvalho Chehab wrote:
> > Em Mon, 5 Dec 2016 22:06:59 +0100
> > Marcel Hasler  escreveu:
> >   
> > > Hello
> > > 
> > > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia 
> > > :  
> > > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> > > >  wrote:
> > > >> Em Sun, 4 Dec 2016 15:25:25 -0300
> > > >> Ezequiel Garcia  escreveu:
> > > >>
> > > >>> On 4 December 2016 at 10:01, Marcel Hasler  
> > > >>> wrote:
> > > >>> > Hello
> > > >>> >
> > > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
> > > >>> > :
> > > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> > > >>> >>  wrote:
> > > >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> > > >>> >>> Marcel Hasler  escreveu:
> > > >>> >>>
> > > >>>  Allow setting a custom record gain for the internal AC97 codec 
> > > >>>  (if available). This can be
> > > >>>  a

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

2016-12-11 Thread mahasler 
Sorry about the broken formatting. Here's the diff once more:

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c 
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 95648ac..708792b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -23,11 +23,30 @@
  *
  */
 
-#include 
+#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);
+   }
+
+   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 Carvalho Chehab wrote:
> Em Mon, 5 Dec 2016 22:06:59 +0100
> Marcel Hasler  escreveu:
> 
> > Hello
> > 
> > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia :
> > > On 5 December 2016 at 09:12, Mauro Carvalho Chehab
> > >  wrote:  
> > >> Em Sun, 4 Dec 2016 15:25:25 -0300
> > >> Ezequiel Garcia  escreveu:
> > >>  
> > >>> On 4 December 2016 at 10:01, Marcel Hasler  wrote:  
> > >>> > Hello
> > >>> >
> > >>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
> > >>> > :  
> > >>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> > >>> >>  wrote:  
> > >>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> > >>> >>> Marcel Hasler  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

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

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

2016-12-06 Thread Mauro Carvalho Chehab 
Em Mon, 5 Dec 2016 22:06:59 +0100
Marcel Hasler  escreveu:

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

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

2016-12-05 Thread Ezequiel Garcia 
On 5 December 2016 at 18:18, Marcel Hasler  wrote:
> 2016-12-05 22:06 GMT+01:00 Marcel Hasler :
>> Hello
>>
>> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia :
>>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>>>  wrote:
 Em Sun, 4 Dec 2016 15:25:25 -0300
 Ezequiel Garcia  escreveu:

> On 4 December 2016 at 10:01, Marcel Hasler  wrote:
> > Hello
> >
> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
> > :
> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> >>  wrote:
> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> >>> Marcel Hasler  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

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

2016-12-05 Thread Ezequiel Garcia 
On 5 December 2016 at 18:06, Marcel Hasler  wrote:
> Hello
>
> 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia :
>> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>>  wrote:
>>> Em Sun, 4 Dec 2016 15:25:25 -0300
>>> Ezequiel Garcia  escreveu:
>>>
 On 4 December 2016 at 10:01, Marcel Hasler  wrote:
 > Hello
 >
 > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
 > :
 >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
 >>  wrote:
 >>> Em Sun, 27 Nov 2016 12:11:48 +0100
 >>> Marcel Hasler  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

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

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 :
> On 5 December 2016 at 09:12, Mauro Carvalho Chehab
>  wrote:
>> Em Sun, 4 Dec 2016 15:25:25 -0300
>> Ezequiel Garcia  escreveu:
>>
>>> On 4 December 2016 at 10:01, Marcel Hasler  wrote:
>>> > Hello
>>> >
>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia 
>>> > :
>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>>> >>  wrote:
>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>> >>> Marcel Hasler  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

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

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

Let's remove it then!

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

Let's remove the module param too, then.

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
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-05 Thread Mauro Carvalho Chehab 
Em Sun, 4 Dec 2016 15:25:25 -0300
Ezequiel Garcia  escreveu:

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

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

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

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

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

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

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

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

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

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

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

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

After the click noise you fixed in snd-usb-audio, removing the mixer
is the last TODO thing in this driver. So I really appreciate all the time
you have put in it.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
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-04 Thread Marcel Hasler 
Hello

2016-12-03 21:46 GMT+01:00 Ezequiel Garcia :
> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>  wrote:
>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> Marcel Hasler  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

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

2016-12-03 Thread Ezequiel Garcia 
On 2 December 2016 at 08:05, Mauro Carvalho Chehab
 wrote:
> Em Sun, 27 Nov 2016 12:11:48 +0100
> Marcel Hasler  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
--
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-02 Thread Mauro Carvalho Chehab 
Em Sun, 27 Nov 2016 12:11:48 +0100
Marcel Hasler  escreveu:

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

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

NACK.

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

> 
> Signed-off-by: Marcel Hasler 
> ---
>  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);



Thanks,
Mauro
--
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