From patchwork Sun Dec 11 12:07:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Marcel Hasler X-Patchwork-Id: 9469715 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6144360760 for ; Sun, 11 Dec 2016 12:08:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3CCC92830A for ; Sun, 11 Dec 2016 12:08:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1CE0928375; Sun, 11 Dec 2016 12:08:23 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1D6EF2830A for ; Sun, 11 Dec 2016 12:08:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752207AbcLKMIU (ORCPT ); Sun, 11 Dec 2016 07:08:20 -0500 Received: from mail-io0-f196.google.com ([209.85.223.196]:36662 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752184AbcLKMIT (ORCPT ); Sun, 11 Dec 2016 07:08:19 -0500 Received: by mail-io0-f196.google.com with SMTP id b194so15993178ioa.3 for ; Sun, 11 Dec 2016 04:08:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=0b8cO1HvsgIcefyxjqLZ/5ybChV9wEHztN7NksavrvI=; b=bUh6Fs6qBlWDMv17n1a4nYHgz9Zceo6dEZEmSuLRwLllQlk/J1emhOESTTvxlSdtoc vBeP/UYq+FswUlPdrRFv8fdODV2p87yB6iqvNcgPYfKPkJWSJTbOtx7t+TJLLgELHz+G v5digsZ0bRiMPoW7BHI8Pms3cW8C0pOFgGAHNvWbXenLHv/L6j1gmp+XCc2PZ/xnBGSU KIBidOq39PYeygRQzKyXsdTo+Ls+vNhTjZNvEDKbaGTnGusNyYQeyT25xMUSiMUGCbN6 LeiAYSb9x3pjs3eha2KN4lONj8YChpGLZgjTpr30SIe7lppUg2dq2DvVlrg5Sa8Mk2N9 hsQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=0b8cO1HvsgIcefyxjqLZ/5ybChV9wEHztN7NksavrvI=; b=JZ7oyqKzUxFwe2Z+RufZaPRD0HyB5I5D6C7EoCsgd/h16Qb77REtUQQeXly3Pnj3YE mM+tCYvE2FL5SJKzJnxgDmXbUYbS5UNwInIybbTDNtjP0JN57X5sAsLeL5QbxS/wEDBI 9N1nuqMdQzSjn5H00NEZECLCUNgtUWgfAvW/B9Gh/WYHc8K02oSm7omxBCRax10NskYq nrwwIiGirvX+mYMW26TBxJGruZpbRkevyeGn9ieO9HDLOe9gdRVNjmhrv/yE6jpslbZW iaem6Q8p1e/u4DabIPMwTEgU6tGBem7bdv0m1sPjdDMq5Qg19VBFJ21Og7ntjpwBjceC Cvqw== X-Gm-Message-State: AKaTC00HLCzZhmLGBx3znJ6YWXgVhDjcZoUA67KFt9ERdYtOCiOcVY3rbx5k12hUq1nSTMeWmcXyqBs5IX+4Gw== X-Received: by 10.36.219.196 with SMTP id c187mr14235574itg.23.1481458098166; Sun, 11 Dec 2016 04:08:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.182.5 with HTTP; Sun, 11 Dec 2016 04:07:37 -0800 (PST) In-Reply-To: <20161206105626.7de242a3@vento.lan> References: <20161127110732.GA5338@arch-desktop> <20161127111148.GA30483@arch-desktop> <20161202090558.29931492@vento.lan> <20161205101221.53613e57@vento.lan> <20161206105626.7de242a3@vento.lan> From: Marcel Hasler Date: Sun, 11 Dec 2016 13:07:37 +0100 Message-ID: Subject: Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain. To: Mauro Carvalho Chehab Cc: Ezequiel Garcia , Mauro Carvalho Chehab , linux-media Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 is not used, >> >> just set it to zero and be happy with that. >> >> >> > >> > Let's remove the module param too, then. >> >> I'm okay with that. >> >> > >> > Thanks, >> > -- >> > Ezequiel García, VanguardiaSur >> > www.vanguardiasur.com.ar >> >> I'm willing to prepare one final patchset, provided we can agree on >> and resolve all issues beforehand. >> >> So far the changes would be to remove the module param and to poll >> STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better >> to also poll it before writing, although that never caused problems. > > Sounds ok. My experience with AC97 on em28xx is that, as new devices > were added, the delay needed for AC97 varied on some of those new > devices. That's why checking if AC97 is ready before writing was > added to its code. > >> >> I'll post some code for review before actually submitting patches. >> Mauro, is there anything else that you think should be changed? If so, >> please tell me now. Thanks. >> >> Best regards >> Marcel > > > > Thanks, > Mauro I've implemented the polling function. I'm using it in a slightly different manner than em28xx, in order to be consistent with the device's logic as documented in the datasheet. Namely I let the driver wait after the write instead of before, to make sure stk1160_write_ac97() only returns once the write has actually completed. Please tell me if this is okay for you, before I commit and create the new patchset. Here's the diff: Marcel --- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c index 95648ac..708792b 100644 --- a/drivers/media/usb/stk1160/stk1160-ac97.c +++ b/drivers/media/usb/stk1160/stk1160-ac97.c @@ -23,11 +23,30 @@ * */ -#include +#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, &value); + + if (!(value & (STK1160_AC97CTL_0_CR | STK1160_AC97CTL_0_CW))) + return 0; + + msleep(1); + } + + stk1160_err("AC97 transfer took too long, this should never happen!"); + return -EBUSY; +} + static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value) { /* Set codec register address */ @@ -37,11 +56,11 @@ static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value) stk1160_write_reg(dev, STK1160_AC97_CMD, value & 0xff); stk1160_write_reg(dev, STK1160_AC97_CMD + 1, (value & 0xff00) >> 8); - /* - * Set command write bit to initiate write operation. - * The bit will be cleared when transfer is done. - */ + /* Set command write bit to initiate write operation */ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c); + + /* Wait for command write bit to be cleared */ + stk1160_ac97_wait_transfer_complete(dev); } #ifdef DEBUG @@ -53,12 +72,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg) /* Set codec register address */ stk1160_write_reg(dev, STK1160_AC97_ADDR, reg); - /* - * Set command read bit to initiate read operation. - * The bit will be cleared when transfer is done. - */ + /* Set command read bit to initiate read operation */ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b); + /* Wait for command read bit to be cleared */ + if (stk1160_ac97_wait_transfer_complete(dev) < 0) { + return 0; + } + /* Retrieve register value */ stk1160_read_reg(dev, STK1160_AC97_CMD, &vall); stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh); diff --git a/drivers/media/usb/stk1160/stk1160-reg.h b/drivers/media/usb/stk1160/stk1160-reg.h index 296a9e7..7b08a3c 100644 --- a/drivers/media/usb/stk1160/stk1160-reg.h +++ b/drivers/media/usb/stk1160/stk1160-reg.h @@ -122,6 +122,8 @@ /* AC97 Audio Control */ #define STK1160_AC97CTL_0 0x500 #define STK1160_AC97CTL_1 0x504 +#define STK1160_AC97CTL_0_CR BIT(1) +#define STK1160_AC97CTL_0_CW BIT(2) /* Use [0:6] bits of register 0x504 to set codec command address */ #define STK1160_AC97_ADDR 0x504 diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h index e85e12e..acd1c81 100644 --- a/drivers/media/usb/stk1160/stk1160.h +++ b/drivers/media/usb/stk1160/stk1160.h @@ -50,6 +50,8 @@ #define STK1160_MAX_INPUT 4 #define STK1160_SVIDEO_INPUT 4 +#define STK1160_AC97_TIMEOUT 50 + #define STK1160_I2C_TIMEOUT 100 Best regards