From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnaud Pouliquen Subject: Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper Date: Tue, 1 Mar 2016 14:34:04 +0100 Message-ID: <56D59A4C.8080105@st.com> References: <1456820357-2545-1-git-send-email-arnaud.pouliquen@st.com> <1456820357-2545-2-git-send-email-arnaud.pouliquen@st.com> <56D5731E.102@st.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx07-00178001.pphosted.com (mx07-00178001.pphosted.com [62.209.51.94]) by alsa0.perex.cz (Postfix) with ESMTP id 8B1FF2614C2 for ; Tue, 1 Mar 2016 14:34:09 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: Jean-Francois Moine , "alsa-devel@alsa-project.org" , Lars-Peter Clausen , Russell King - ARM Linux , David Airlie , Liam Girdwood , Jyri Sarha , Mark Brown , Philipp Zabel , Moise GERGAUD List-Id: alsa-devel@alsa-project.org On 03/01/2016 11:57 AM, Takashi Iwai wrote: > On Tue, 01 Mar 2016 11:46:54 +0100, > Arnaud Pouliquen wrote: >> >> >> >> On 03/01/2016 10:12 AM, Takashi Iwai wrote: >>> On Tue, 01 Mar 2016 09:19:14 +0100, >>> Arnaud Pouliquen wrote: >>>> >>>> Add IEC958 channel status helper that creates control to handle the >>>> IEC60958 status bits. >>>> >>>> Signed-off-by: Arnaud Pouliquen >>> >>> What is the reason to make the mutex pointer instead of the own one? >>> Any need for sharing the mutex? >> Yes, need to share mutex to avoid race condition between control update >> and action on pcm stream ( hw_param or prepare) > > Hrm.... I don't know whether this is in a good form. At least, it's > a big confusing to me. In general, a mandatory mutex is usually > assigned to its own, not referring to an external one. > Severals drivers that use this control use a mutex (e.g ac97_codec.c). So need to shared it between driver and the help function. In my first version mutex was optional (used if not null). I have made it mandatory after discussions with Russel. Forcing driver to use it to avoid race condition make also sense... Now,I have no fixed idea on it, just need a consensus. >>> And, if it has to be assigned explicitly by user, you have to write it >>> explicitly, too. >> ok, if not enough explicit, i will add comment in snd_pcm_iec958_params >> definition >>> >>> Another small nitpicking: >> >>>> +static int snd_pcm_iec958_put(struct snd_kcontrol *kcontrol, >>>> + struct snd_ctl_elem_value *uctl) >>>> +{ >>>> + struct snd_pcm_iec958_params *params = snd_kcontrol_chip(kcontrol); >>>> + int err = 0; >>>> + >>>> + if (!params->mutex) >>>> + return -EINVAL; >>>> + >>>> + mutex_lock(params->mutex); >>>> + if (params->ctrl_set) >>>> + err = params->ctrl_set(params->pdata, >>>> + uctl->value.iec958.status); >>> >>> So, in your design, ctrl_set isn't mandatory? >> Hypothesis is that for some hardwares, callback is not >> needed, only channels status values are needed. As example >> some hardwares could not want to support switch from audio to none-audio >> mode without stopping PCM. > > OK, fair enough. Then put the comment that this callback is > optional. > >>>> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm, >>>> + struct snd_pcm_iec958_params *params, int stream) >>>> +{ >>>> + struct snd_kcontrol_new knew; >>>> + >>>> + if (stream > SNDRV_PCM_STREAM_LAST) >>>> + return -EINVAL; >>>> + >>>> + knew = iec958_ctls[stream]; >>>> + knew.device = pcm->device; >>>> + knew.index = pcm->device; >>>> + knew.count = pcm->streams[stream].substream_count; >>> >>> Hmm, this doesn't always work. It will create the substream_count >>> ctls starting from the pcm dev# as index. What if there are 2 PCM >>> devices where both contain 4 substreams? >>> >>> I admit that the current ctl <-> PCM mapping is messing. There are >>> some heuristics and you're trying to follow that. But blindly >>> applying to all cases doesn't seem to work. >>> >> yes this is not clean. >> i'm not very familiar with substream usage, so any suggestion is welcome. >> The only use case I have in mind is a HDMI connected through 4 I2S data >> wire... >> I can see 2 options: >> - Associate control only to pcm device. >> - Create a control per sub-device >> >> Do we really need to associate one IEC control per substream? > > This pretty much depends on the hardware design. If each substream is > really individual, you'd need to give the control for each substream. > > I think you can pass the decision to the caller side: instead of > defining it in the function there, give via arguments. Ok, one argument to determine if control has to be associated to device or sub-devices is sufficient? or should i define one argument per sub device? Thanks Arnaud