[alsa-devel] [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Takashi Iwai]

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 at 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