| Message ID | 1453210836-16218-3-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 19, 2016 at 02:40:32PM +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> > --- > include/sound/pcm_iec958.h | 16 +++++++++ > sound/core/pcm_iec958.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 106 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..1d426bc 100644 > --- a/sound/core/pcm_iec958.c > +++ b/sound/core/pcm_iec958.c > @@ -7,11 +7,78 @@ > */ > #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); > + 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. > + 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. > +} > + > +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?
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?
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..1d426bc 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -7,11 +7,78 @@ */ #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); + 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) { + 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; +} + +static const struct snd_kcontrol_new iec958_ctls[] = { + { + .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, + }, + { + .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 +178,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);
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 | 90 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+)