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 <arnaud.pouliquen@xxxxxx>
>>>
>>> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
