| Message ID | 1453484912-24547-3-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
On Fri, Jan 22, 2016 at 06:48:28PM +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 thought I had already commented about the mutex - maybe not. Please don't make these conditional like this: if you need the mutex to eliminate a race condition, then you need it to eliminate the race condition and it can't be "optional". Elimination of race conditions is never optional! Please get rid of all these conditions and make it mandatory that a mutex is supplied. Thanks.
On 02/16/2016 09:17 PM, Jyri Sarha wrote: > 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(). oops... Seems that i need to enable some debug configs in my environment... My apologize for this copy/past and thanks for the test! Anyway i need to perform further tests before deliver as patches instead of rfc. But i prefer to wait a decision on the HDMI codec before finalizing it.
On 02/17/2016 01:31 AM, Russell King - ARM Linux wrote: > On Fri, Jan 22, 2016 at 06:48:28PM +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 thought I had already commented about the mutex - maybe not. Please > don't make these conditional like this: if you need the mutex to > eliminate a race condition, then you need it to eliminate the race > condition and it can't be "optional". Elimination of race conditions > is never optional! > > Please get rid of all these conditions and make it mandatory that a > mutex is supplied. > > Thanks. Ok, i will suppress condition. I also need to use a spinlock instead of the mutex, to be able to support race condition with atomic callbacks (trigger, pointer, and ack)
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); + 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);
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(+)