On 26 November 2016 at 10:38, Marcel Hasler <mahasler@xxxxxxxxx> wrote:
> Hello, and thanks for your feedback.
>
> 2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>:
>> Marcel,
>>
>> On 27 October 2016 at 17:34, Marcel Hasler <mahasler@xxxxxxxxx> 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 <mahasler@xxxxxxxxx>
>>
>> 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 <linux/module.h>
>>> -#include <sound/core.h>
>>> -#include <sound/initval.h>
>>> -#include <sound/ac97_codec.h>
>>>
>>> #include "stk1160.h"
>>> #include "stk1160-reg.h"
>>>
>>> -static struct snd_ac97 *stk1160_ac97;
>>> -
>>> -static void stk1160_write_ac97(struct snd_ac97 *ac97, u16 reg, u16 value)
>>> +static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
>>> {
>>> - struct stk1160 *dev = ac97->private_data;
>>> -
>>> /* Set codec register address */
>>> stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
>>>
>>> @@ -48,9 +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 ?
>>
>
> Right, this was just to test the issue addressed in the last patch
> that I submitted separately. I didn't know at that point, whether the
> issue was with writing or reading the registers, or both. This can of
> course be removed, since it's not really needed for anything anymore.
>
I'm not opposed to keeping the dump. Remove it if you think
it's useless, or keep it if you think it has debugging value.
I'm fine either way.
--
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
