On 01/19/2016 06:00 PM, Russell King - ARM Linux wrote:
> On Tue, Jan 19, 2016 at 02:40:32PM +0100, Arnaud Pouliquen wrote:
>> +static int snd_pcm_iec958_get(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *uctl)
>> +{
>> + struct snd_pcm_iec958_params *params = snd_kcontrol_chip(kcontrol);
>> +
>> + if (params->mutex)
>> + mutex_unlock(params->mutex);
>> + uctl->value.iec958.status[0] = params->iec->status[0];
>> + uctl->value.iec958.status[1] = params->iec->status[1];
>> + uctl->value.iec958.status[2] = params->iec->status[2];
>> + uctl->value.iec958.status[3] = params->iec->status[3];
>> + uctl->value.iec958.status[4] = params->iec->status[4];
>> + if (params->mutex)
>> + mutex_unlock(params->mutex);
>
> I'm not sure it makes sense for the mutex to be optional.
I have no use case in mind that justifies optional mutex.
Just did it for flexibility...
I can suppress condition to force drivers to use it.
>
>> + return 0;
>> +}
>> +
>> +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)
>> + mutex_lock(params->mutex);
>> + if (params->ctrl_set)
>> + err = params->ctrl_set(params->pdata,
>> + uctl->value.iec958.status, 5);
>> + if (!err) {
>> + params->iec->status[0] = uctl->value.iec958.status[0];
>> + params->iec->status[1] = uctl->value.iec958.status[1];
>> + params->iec->status[2] = uctl->value.iec958.status[2];
>> + params->iec->status[3] = uctl->value.iec958.status[3];
>> + params->iec->status[4] = uctl->value.iec958.status[4];
>> + }
>> + if (params->mutex)
>> + mutex_unlock(params->mutex);
>> +
>> + return 0;
>
> I think you're supposed to report whether anything changed, rather
> than just zero on success.
right, need to return 1 or err
>
>> +}
>> +
>> +static const struct snd_kcontrol_new iec958_ctls[] = {
>> + {
>> + .iface = SNDRV_CTL_ELEM_IFACE_PCM,
>> + .name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
>
> Shouldn't this have a .access member? What if the audio driver
> modifies the IEC958 status for PCM playback - shouldn't it have
> the ability to have SNDRV_CTL_ELEM_ACCESS_VOLATILE added?
>
i will add READ + VOLATILE access for capture, RW +VOLATILE access for
playback.
Should i also add some other controls by default, like controls for
consumer and professional mask for playback?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
