Re: [RFC v2 2/6] ALSA: pcm: add IEC958 channel status control helper

[Jyri Sarha <jsarha@ti.com>]

On 01/22/16 19:48, 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>
> ---
>   include/sound/pcm_iec958.h | 16 ++++++++
>   sound/core/pcm_iec958.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 115 insertions(+)
>
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 36f023a..7453ace 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -3,9 +3,25 @@
>
>   #include <linux/types.h>
>
> +/*
> + * IEC 60958 controls parameters
> + * Describes channel status and associated callback
> + */
> +struct snd_pcm_iec958_params {
> +	/* call when control is updated by user */
> +	int (*ctrl_set)(void *pdata, u8 *status, u8 len);
> +
> +	struct snd_aes_iec958 *iec;
> +	void *pdata; /* user private data to retrieve context */
> +	struct mutex *mutex; /* use to avoid race condition */
> +};
> +
>   int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>   	size_t len);
>
>   int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
>   					     u8 *cs, size_t len);
> +
> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
> +			      struct snd_pcm_iec958_params *params, int stream);
>   #endif
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index c9f8b66..8dd0415 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -7,11 +7,87 @@
>    */
>   #include <linux/export.h>
>   #include <linux/types.h>
> +#include <linux/wait.h>
>   #include <sound/asoundef.h>
> +#include <sound/control.h>
>   #include <sound/pcm.h>
>   #include <sound/pcm_params.h>
>   #include <sound/pcm_iec958.h>
>
> +int snd_pcm_iec958_info(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
> +	uinfo->count = 1;
> +	return 0;
> +}
> +
> +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);

This here should be mutex_lock().

Cheers,
Jyri

> +	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);
> +	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 < 0) {
> +		if (params->mutex)
> +			mutex_unlock(params->mutex);
> +		return 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 1;
> +}
> +
> +static const struct snd_kcontrol_new iec958_ctls[] = {
> +	{
> +		.access = (SNDRV_CTL_ELEM_ACCESS_READWRITE |
> +			   SNDRV_CTL_ELEM_ACCESS_VOLATILE),
> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
> +		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
> +		.info = snd_pcm_iec958_info,
> +		.get = snd_pcm_iec958_get,
> +		.put = snd_pcm_iec958_put,
> +	},
> +	{
> +		.access = (SNDRV_CTL_ELEM_ACCESS_READ |
> +			   SNDRV_CTL_ELEM_ACCESS_VOLATILE),
> +		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
> +		.name = SNDRV_CTL_NAME_IEC958("", CAPTURE, DEFAULT),
> +		.info = snd_pcm_iec958_info,
> +		.get = snd_pcm_iec958_get,
> +	},
> +};
> +
>   static int create_iec958_consumer(uint rate, uint sample_width,
>   				  u8 *cs, size_t len)
>   {
> @@ -111,3 +187,26 @@ int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
>   				      cs, len);
>   }
>   EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
> +
> +/**
> + * snd_pcm_create_iec958_ctl - create IEC958 channel status default control
> + * pcm: pcm device to associate to the control.
> + * iec958: snd_pcm_iec958_params structure that cntains callbacks
> + *         and channel status buffer
> + * stream: stream type SNDRV_PCM_STREAM_PLAYBACK or SNDRV_PCM_STREAM_CATURE
> + * Returns:  negative error code if something failed.
> + */
> +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.count = pcm->streams[stream].substream_count;
> +	return snd_ctl_add(pcm->card, snd_ctl_new1(&knew, params));
> +}
> +EXPORT_SYMBOL(snd_pcm_create_iec958_ctl);
>