Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Arnaud Pouliquen <arnaud.pouliquen@st.com>]

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@st.com>
>>>
>>> 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