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

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

On 26 November 2016 at 10:52, Marcel Hasler <mahasler@gmail.com> wrote:
> 2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 27 October 2016 at 17:35, Marcel Hasler <mahasler@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 <mahasler@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, 0x0000); /* 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?
>

Well, if the user passes a gain, requesting a non-default value, it is
expected that the gain be set by the driver. So printing a message
for something that is just as expected as "the driver will set the
parameter you told him to set".

User messages should only be printed when *unexpected* or otherwise
relevant events happen.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar