* [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically. [not found] <cover.1477592284.git.mahasler@gmail.com> @ 2016-10-27 20:34 ` Marcel Hasler 2016-11-20 17:36 ` Ezequiel Garcia 2016-10-27 20:34 ` [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC Marcel Hasler 2016-10-27 20:35 ` [PATCH v2 3/3] stk1160: Add module param for setting the record gain Marcel Hasler 2 siblings, 1 reply; 11+ messages in thread From: Marcel Hasler @ 2016-10-27 20:34 UTC (permalink / raw) To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media 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@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 <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) { - 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&R 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, &card); - 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, &stk1160_ac97_ops, NULL, &ac97_bus); - if (rc) - goto err; - - /* We must set private_data before calling snd_ac97_mixer */ - memset(&ac97_template, 0, sizeof(ac97_template)); - ac97_template.private_data = dev; - ac97_template.scaps = AC97_SCAP_SKIP_MODEM; - rc = snd_ac97_mixer(ac97_bus, &ac97_template, &stk1160_ac97); - if (rc) - goto err; - - dev->snd_card = card; - rc = snd_card_register(card); - if (rc) - goto err; - - return 0; - -err: - dev->snd_card = NULL; - snd_card_free(card); - return rc; + value = stk1160_read_ac97(dev, 0x16); /* Aux volume */ + stk1160_dbg("0x16 == 0x%04x", value); + + value = stk1160_read_ac97(dev, 0x1a); /* Record select */ + stk1160_dbg("0x1a == 0x%04x", value); + + value = stk1160_read_ac97(dev, 0x02); /* Master volume */ + stk1160_dbg("0x02 == 0x%04x", value); + + value = stk1160_read_ac97(dev, 0x1c); /* Record gain */ + stk1160_dbg("0x1c == 0x%04x", value); } +#endif -int stk1160_ac97_unregister(struct stk1160 *dev) +void stk1160_ac97_setup(struct stk1160 *dev) { - struct snd_card *card = dev->snd_card; - - /* - * We need to check usb_device, - * because ac97 release attempts to communicate with codec - */ - if (card && dev->udev) - snd_card_free(card); + /* Two-step reset AC97 interface and hardware codec */ + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94); + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c); - return 0; + /* Set 16-bit audio data and choose L&R channel*/ + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01); + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 3, 0x00); + + /* Setup channels */ + stk1160_write_ac97(dev, 0x12, 0x8808); /* CD volume */ + stk1160_write_ac97(dev, 0x10, 0x0808); /* Line-in volume */ + stk1160_write_ac97(dev, 0x0e, 0x0008); /* MIC volume (mono) */ + 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 */ + +#ifdef DEBUG + stk1160_ac97_dump_regs(dev); +#endif } diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c index bc02947..f3c9b8a 100644 --- a/drivers/media/usb/stk1160/stk1160-core.c +++ b/drivers/media/usb/stk1160/stk1160-core.c @@ -373,7 +373,7 @@ static int stk1160_probe(struct usb_interface *interface, /* select default input */ stk1160_select_input(dev); - stk1160_ac97_register(dev); + stk1160_ac97_setup(dev); rc = stk1160_video_register(dev); if (rc < 0) @@ -411,9 +411,6 @@ static void stk1160_disconnect(struct usb_interface *interface) /* Here is the only place where isoc get released */ stk1160_uninit_isoc(dev); - /* ac97 unregister needs to be done before usb_device is cleared */ - stk1160_ac97_unregister(dev); - stk1160_clear_queue(dev); video_unregister_device(&dev->vdev); diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h index 1ed1cc4..e85e12e 100644 --- a/drivers/media/usb/stk1160/stk1160.h +++ b/drivers/media/usb/stk1160/stk1160.h @@ -197,11 +197,4 @@ int stk1160_read_reg_req_len(struct stk1160 *dev, u8 req, u16 reg, void stk1160_select_input(struct stk1160 *dev); /* Provided by stk1160-ac97.c */ -#ifdef CONFIG_VIDEO_STK1160_AC97 -int stk1160_ac97_register(struct stk1160 *dev); -int stk1160_ac97_unregister(struct stk1160 *dev); -#else -static inline int stk1160_ac97_register(struct stk1160 *dev) { return 0; } -static inline int stk1160_ac97_unregister(struct stk1160 *dev) { return 0; } -#endif - +void stk1160_ac97_setup(struct stk1160 *dev); -- 2.10.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically. 2016-10-27 20:34 ` [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler @ 2016-11-20 17:36 ` Ezequiel Garcia 2016-11-26 13:38 ` Marcel Hasler 0 siblings, 1 reply; 11+ messages in thread From: Ezequiel Garcia @ 2016-11-20 17:36 UTC (permalink / raw) To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil Marcel, On 27 October 2016 at 17:34, Marcel Hasler <mahasler@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 <mahasler@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. 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 ? > { > - 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&R 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, &card); > - 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, &stk1160_ac97_ops, NULL, &ac97_bus); > - if (rc) > - goto err; > - > - /* We must set private_data before calling snd_ac97_mixer */ > - memset(&ac97_template, 0, sizeof(ac97_template)); > - ac97_template.private_data = dev; > - ac97_template.scaps = AC97_SCAP_SKIP_MODEM; > - rc = snd_ac97_mixer(ac97_bus, &ac97_template, &stk1160_ac97); > - if (rc) > - goto err; > - > - dev->snd_card = card; > - rc = snd_card_register(card); > - if (rc) > - goto err; > - > - return 0; > - > -err: > - dev->snd_card = NULL; > - snd_card_free(card); > - return rc; > + value = stk1160_read_ac97(dev, 0x16); /* Aux volume */ > + stk1160_dbg("0x16 == 0x%04x", value); > + > + value = stk1160_read_ac97(dev, 0x1a); /* Record select */ > + stk1160_dbg("0x1a == 0x%04x", value); > + > + value = stk1160_read_ac97(dev, 0x02); /* Master volume */ > + stk1160_dbg("0x02 == 0x%04x", value); > + > + value = stk1160_read_ac97(dev, 0x1c); /* Record gain */ > + stk1160_dbg("0x1c == 0x%04x", value); > } > +#endif > > -int stk1160_ac97_unregister(struct stk1160 *dev) > +void stk1160_ac97_setup(struct stk1160 *dev) > { > - struct snd_card *card = dev->snd_card; > - > - /* > - * We need to check usb_device, > - * because ac97 release attempts to communicate with codec > - */ > - if (card && dev->udev) > - snd_card_free(card); > + /* Two-step reset AC97 interface and hardware codec */ > + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94); > + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c); > > - return 0; > + /* Set 16-bit audio data and choose L&R channel*/ > + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01); > + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 3, 0x00); > + > + /* Setup channels */ > + stk1160_write_ac97(dev, 0x12, 0x8808); /* CD volume */ > + stk1160_write_ac97(dev, 0x10, 0x0808); /* Line-in volume */ > + stk1160_write_ac97(dev, 0x0e, 0x0008); /* MIC volume (mono) */ > + 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 */ > + > +#ifdef DEBUG > + stk1160_ac97_dump_regs(dev); > +#endif > } > diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c > index bc02947..f3c9b8a 100644 > --- a/drivers/media/usb/stk1160/stk1160-core.c > +++ b/drivers/media/usb/stk1160/stk1160-core.c > @@ -373,7 +373,7 @@ static int stk1160_probe(struct usb_interface *interface, > /* select default input */ > stk1160_select_input(dev); > > - stk1160_ac97_register(dev); > + stk1160_ac97_setup(dev); > > rc = stk1160_video_register(dev); > if (rc < 0) > @@ -411,9 +411,6 @@ static void stk1160_disconnect(struct usb_interface *interface) > /* Here is the only place where isoc get released */ > stk1160_uninit_isoc(dev); > > - /* ac97 unregister needs to be done before usb_device is cleared */ > - stk1160_ac97_unregister(dev); > - > stk1160_clear_queue(dev); > > video_unregister_device(&dev->vdev); > diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h > index 1ed1cc4..e85e12e 100644 > --- a/drivers/media/usb/stk1160/stk1160.h > +++ b/drivers/media/usb/stk1160/stk1160.h > @@ -197,11 +197,4 @@ int stk1160_read_reg_req_len(struct stk1160 *dev, u8 req, u16 reg, > void stk1160_select_input(struct stk1160 *dev); > > /* Provided by stk1160-ac97.c */ > -#ifdef CONFIG_VIDEO_STK1160_AC97 > -int stk1160_ac97_register(struct stk1160 *dev); > -int stk1160_ac97_unregister(struct stk1160 *dev); > -#else > -static inline int stk1160_ac97_register(struct stk1160 *dev) { return 0; } > -static inline int stk1160_ac97_unregister(struct stk1160 *dev) { return 0; } > -#endif > - > +void stk1160_ac97_setup(struct stk1160 *dev); > -- > 2.10.1 > -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically. 2016-11-20 17:36 ` Ezequiel Garcia @ 2016-11-26 13:38 ` Marcel Hasler 2016-11-26 15:04 ` Ezequiel Garcia 0 siblings, 1 reply; 11+ messages in thread From: Marcel Hasler @ 2016-11-26 13:38 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil Hello, and thanks for your feedback. 2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>: > Marcel, > > On 27 October 2016 at 17:34, Marcel Hasler <mahasler@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 <mahasler@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 <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. >> { >> - 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&R 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, &card); >> - 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, &stk1160_ac97_ops, NULL, &ac97_bus); >> - if (rc) >> - goto err; >> - >> - /* We must set private_data before calling snd_ac97_mixer */ >> - memset(&ac97_template, 0, sizeof(ac97_template)); >> - ac97_template.private_data = dev; >> - ac97_template.scaps = AC97_SCAP_SKIP_MODEM; >> - rc = snd_ac97_mixer(ac97_bus, &ac97_template, &stk1160_ac97); >> - if (rc) >> - goto err; >> - >> - dev->snd_card = card; >> - rc = snd_card_register(card); >> - if (rc) >> - goto err; >> - >> - return 0; >> - >> -err: >> - dev->snd_card = NULL; >> - snd_card_free(card); >> - return rc; >> + value = stk1160_read_ac97(dev, 0x16); /* Aux volume */ >> + stk1160_dbg("0x16 == 0x%04x", value); >> + >> + value = stk1160_read_ac97(dev, 0x1a); /* Record select */ >> + stk1160_dbg("0x1a == 0x%04x", value); >> + >> + value = stk1160_read_ac97(dev, 0x02); /* Master volume */ >> + stk1160_dbg("0x02 == 0x%04x", value); >> + >> + value = stk1160_read_ac97(dev, 0x1c); /* Record gain */ >> + stk1160_dbg("0x1c == 0x%04x", value); >> } >> +#endif >> >> -int stk1160_ac97_unregister(struct stk1160 *dev) >> +void stk1160_ac97_setup(struct stk1160 *dev) >> { >> - struct snd_card *card = dev->snd_card; >> - >> - /* >> - * We need to check usb_device, >> - * because ac97 release attempts to communicate with codec >> - */ >> - if (card && dev->udev) >> - snd_card_free(card); >> + /* Two-step reset AC97 interface and hardware codec */ >> + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94); >> + stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c); >> >> - return 0; >> + /* Set 16-bit audio data and choose L&R channel*/ >> + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01); >> + stk1160_write_reg(dev, STK1160_AC97CTL_1 + 3, 0x00); >> + >> + /* Setup channels */ >> + stk1160_write_ac97(dev, 0x12, 0x8808); /* CD volume */ >> + stk1160_write_ac97(dev, 0x10, 0x0808); /* Line-in volume */ >> + stk1160_write_ac97(dev, 0x0e, 0x0008); /* MIC volume (mono) */ >> + 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 */ >> + >> +#ifdef DEBUG >> + stk1160_ac97_dump_regs(dev); >> +#endif >> } >> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c >> index bc02947..f3c9b8a 100644 >> --- a/drivers/media/usb/stk1160/stk1160-core.c >> +++ b/drivers/media/usb/stk1160/stk1160-core.c >> @@ -373,7 +373,7 @@ static int stk1160_probe(struct usb_interface *interface, >> /* select default input */ >> stk1160_select_input(dev); >> >> - stk1160_ac97_register(dev); >> + stk1160_ac97_setup(dev); >> >> rc = stk1160_video_register(dev); >> if (rc < 0) >> @@ -411,9 +411,6 @@ static void stk1160_disconnect(struct usb_interface *interface) >> /* Here is the only place where isoc get released */ >> stk1160_uninit_isoc(dev); >> >> - /* ac97 unregister needs to be done before usb_device is cleared */ >> - stk1160_ac97_unregister(dev); >> - >> stk1160_clear_queue(dev); >> >> video_unregister_device(&dev->vdev); >> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h >> index 1ed1cc4..e85e12e 100644 >> --- a/drivers/media/usb/stk1160/stk1160.h >> +++ b/drivers/media/usb/stk1160/stk1160.h >> @@ -197,11 +197,4 @@ int stk1160_read_reg_req_len(struct stk1160 *dev, u8 req, u16 reg, >> void stk1160_select_input(struct stk1160 *dev); >> >> /* Provided by stk1160-ac97.c */ >> -#ifdef CONFIG_VIDEO_STK1160_AC97 >> -int stk1160_ac97_register(struct stk1160 *dev); >> -int stk1160_ac97_unregister(struct stk1160 *dev); >> -#else >> -static inline int stk1160_ac97_register(struct stk1160 *dev) { return 0; } >> -static inline int stk1160_ac97_unregister(struct stk1160 *dev) { return 0; } >> -#endif >> - >> +void stk1160_ac97_setup(struct stk1160 *dev); >> -- >> 2.10.1 >> > > > > -- > Ezequiel García, VanguardiaSur > www.vanguardiasur.com.ar Marcel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically. 2016-11-26 13:38 ` Marcel Hasler @ 2016-11-26 15:04 ` Ezequiel Garcia 0 siblings, 0 replies; 11+ messages in thread From: Ezequiel Garcia @ 2016-11-26 15:04 UTC (permalink / raw) To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil On 26 November 2016 at 10:38, Marcel Hasler <mahasler@gmail.com> wrote: > Hello, and thanks for your feedback. > > 2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>: >> Marcel, >> >> On 27 October 2016 at 17:34, Marcel Hasler <mahasler@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 <mahasler@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 <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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC. [not found] <cover.1477592284.git.mahasler@gmail.com> 2016-10-27 20:34 ` [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler @ 2016-10-27 20:34 ` Marcel Hasler 2016-11-20 17:36 ` Ezequiel Garcia 2016-10-27 20:35 ` [PATCH v2 3/3] stk1160: Add module param for setting the record gain Marcel Hasler 2 siblings, 1 reply; 11+ messages in thread From: Marcel Hasler @ 2016-10-27 20:34 UTC (permalink / raw) To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media 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 <mahasler@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, &value); + + /* 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC. 2016-10-27 20:34 ` [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC Marcel Hasler @ 2016-11-20 17:36 ` Ezequiel Garcia 2016-11-26 13:49 ` Marcel Hasler 0 siblings, 1 reply; 11+ messages in thread From: Ezequiel Garcia @ 2016-11-20 17:36 UTC (permalink / raw) To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil On 27 October 2016 at 17:34, Marcel Hasler <mahasler@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 <mahasler@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, &value); > + > + /* 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) Also, the spec mentions another POSVA bit relevant to audio ACDOUT. Should we check that too? > +} > + > 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC. 2016-11-20 17:36 ` Ezequiel Garcia @ 2016-11-26 13:49 ` Marcel Hasler 0 siblings, 0 replies; 11+ messages in thread From: Marcel Hasler @ 2016-11-26 13:49 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil 2016-11-20 18:36 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>: > On 27 October 2016 at 17:34, Marcel Hasler <mahasler@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 <mahasler@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, &value); >> + >> + /* 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] stk1160: Add module param for setting the record gain. [not found] <cover.1477592284.git.mahasler@gmail.com> 2016-10-27 20:34 ` [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler 2016-10-27 20:34 ` [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC Marcel Hasler @ 2016-10-27 20:35 ` Marcel Hasler 2016-11-20 17:36 ` Ezequiel Garcia 2 siblings, 1 reply; 11+ messages in thread From: Marcel Hasler @ 2016-10-27 20:35 UTC (permalink / raw) To: Ezequiel Garcia, Mauro Carvalho Chehab; +Cc: linux-media 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); + stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain); #ifdef DEBUG stk1160_ac97_dump_regs(dev); -- 2.10.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] stk1160: Add module param for setting the record gain. 2016-10-27 20:35 ` [PATCH v2 3/3] stk1160: Add module param for setting the record gain Marcel Hasler @ 2016-11-20 17:36 ` Ezequiel Garcia 2016-11-26 13:52 ` Marcel Hasler 0 siblings, 1 reply; 11+ messages in thread From: Ezequiel Garcia @ 2016-11-20 17:36 UTC (permalink / raw) To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil 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? > + 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] stk1160: Add module param for setting the record gain. 2016-11-20 17:36 ` Ezequiel Garcia @ 2016-11-26 13:52 ` Marcel Hasler 2016-11-26 15:08 ` Ezequiel Garcia 0 siblings, 1 reply; 11+ messages in thread From: Marcel Hasler @ 2016-11-26 13:52 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil 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? >> + 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] stk1160: Add module param for setting the record gain. 2016-11-26 13:52 ` Marcel Hasler @ 2016-11-26 15:08 ` Ezequiel Garcia 0 siblings, 0 replies; 11+ messages in thread From: Ezequiel Garcia @ 2016-11-26 15:08 UTC (permalink / raw) To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-26 15:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1477592284.git.mahasler@gmail.com>
2016-10-27 20:34 ` [PATCH v2 1/3] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically Marcel Hasler
2016-11-20 17:36 ` Ezequiel Garcia
2016-11-26 13:38 ` Marcel Hasler
2016-11-26 15:04 ` Ezequiel Garcia
2016-10-27 20:34 ` [PATCH v2 2/3] stk1160: Check whether to use AC97 codec or internal ADC Marcel Hasler
2016-11-20 17:36 ` Ezequiel Garcia
2016-11-26 13:49 ` Marcel Hasler
2016-10-27 20:35 ` [PATCH v2 3/3] stk1160: Add module param for setting the record gain Marcel Hasler
2016-11-20 17:36 ` Ezequiel Garcia
2016-11-26 13:52 ` Marcel Hasler
2016-11-26 15:08 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).