| Message ID | 1456820357-2545-2-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? And, if it has to be assigned explicitly by user, you have to write it explicitly, too. Another small nitpicking: > --- > include/sound/pcm_iec958.h | 15 +++++++ > sound/core/pcm_iec958.c | 107 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 122 insertions(+) > > diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h > index 0eed397..08e4bab 100644 > --- a/include/sound/pcm_iec958.h > +++ b/include/sound/pcm_iec958.h > @@ -3,7 +3,22 @@ > > #include <linux/types.h> > > +/* > + * IEC 60958 controls parameters > + * Describes channel status and associated callback > + */ > +struct snd_pcm_iec958_params { > + /* call under mutex protection, when control is updated by user */ > + int (*ctrl_set)(void *pdata, u8 *status); > + > + 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_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 36b2d7a..80e7e47 100644 > --- a/sound/core/pcm_iec958.c > +++ b/sound/core/pcm_iec958.c > @@ -7,10 +7,93 @@ > */ > #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_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; > +} No static? > + > +/** > + * IEC958 channel status default controls callbacks > + * > + * Callbacks are protected by a mutex provided by user. > + */ You don't need the kernel-doc comment "/**" for static functions, in general. It's basically for API, not for internal ones. > +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) > + return -EINVAL; > + > + mutex_lock(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]; A loop might be better :) Let compiler optimize it. > + 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) > + 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? > + if (err < 0) { > + 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]; > + > + mutex_unlock(params->mutex); > + > + return 1; Strictly speaking, the return value of put callback should be zero if the value isn't changed. This will avoid the ctl value change notification. > +} > + > +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, > + }, > +}; > + > /** > * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status > * @runtime: pcm runtime structure with ->rate filled in > @@ -93,3 +176,27 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, > return len; > } > EXPORT_SYMBOL(snd_pcm_create_iec958_consumer); > + > +/** > + * 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 contains callbacks > + * and channel status buffer > + * stream: stream type SNDRV_PCM_STREAM_PLAYBACK or SNDRV_PCM_STREAM_CATURE Put "@" prefix for arguments in kernel-doc. > + * 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.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. thanks, Takashi
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) > > 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. > >> +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? Thanks Arnaud
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. > > 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. Takashi
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
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..08e4bab 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -3,7 +3,22 @@ #include <linux/types.h> +/* + * IEC 60958 controls parameters + * Describes channel status and associated callback + */ +struct snd_pcm_iec958_params { + /* call under mutex protection, when control is updated by user */ + int (*ctrl_set)(void *pdata, u8 *status); + + 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_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 36b2d7a..80e7e47 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -7,10 +7,93 @@ */ #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_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; +} + +/** + * IEC958 channel status default controls callbacks + * + * Callbacks are protected by a mutex provided by user. + */ +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) + return -EINVAL; + + mutex_lock(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]; + 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) + return -EINVAL; + + mutex_lock(params->mutex); + if (params->ctrl_set) + err = params->ctrl_set(params->pdata, + uctl->value.iec958.status); + if (err < 0) { + 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]; + + 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, + }, +}; + /** * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status * @runtime: pcm runtime structure with ->rate filled in @@ -93,3 +176,27 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, return len; } EXPORT_SYMBOL(snd_pcm_create_iec958_consumer); + +/** + * 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 contains 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.index = 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 | 15 +++++++ sound/core/pcm_iec958.c | 107 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+)