[alsa-devel] [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Moise Gergaud]

On 12/07/2015 10:51 AM, Takashi Iwai wrote:
> On Mon, 07 Dec 2015 09:46:02 +0100,
> Moise Gergaud wrote:
>>
>> On 12/03/2015 01:03 PM, Takashi Iwai wrote:
>>> On Thu, 03 Dec 2015 12:13:04 +0100,
>>> Russell King - ARM Linux wrote:
>>>>
>>>> On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
>>>>> On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
>>>>>> Again, I'm not happy with this change.  The intention here is that we
>>>>>> validate all arguments before we change anything.  This breaks that
>>>>>> principle.
>>>>>>
>>>>> We propose this change for compressed mode (IEC61937/non linear PCM).
>>>>> In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
>>>>> cannot use snd_pcm_create_iec958_consumer.
>>>>
>>>> Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
>>>> are allowed to modify the values you get afterwards in order to handle
>>>> this case if you so wish to toggle this bit.
>>>>
>>>> However, if you are using data mode, you really should be using the AES
>>>> bytes passed by the application through alsalib and into the kernel via
>>>> the IEC958 controls.  There are three controls specified for this:
>>>>
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
>>>> changed for consumer mode.
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
>>>> changed for professional mode.
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
>>>> bits.
>>>>
>>>> When using this, it is userspace's responsibility to set the sample rate
>>>> appropriately for the stream.
>>>>
>>>> The only case to use snd_pcm_create_iec958_consumer() is to generate the
>>>> status bytes for PCM audio, where the userspace API doesn't facilitiate
>>>> generating and/or passing the AES status bytes.
>>>
>>> That's true.  OTOH, in most drivers, we try to be kind not to be too
>>> strict for this requirement and adjust the status bits accordingly.
>>> This comes from the lessons we learned how user-space doesn't care the
>>> things.
>>>
>>> So, if we want to keep that good uncle attitude, this change doesn't
>>> look so bad to me.
>>>
>> shall I deliver a new version of this patch with header correction or
>> shall I abandon this patch?
>
> It's up to you whether you want further convincing Russell :)
> I'd happily merge this once when we get his ack.
>
>
> thanks,
>
> Takashi
>

I share Takashi's view; unfortunately, we cannot always trust user space.
And so, for robustness reason, I think drivers shall ensure that channel 
status params are correct. For that purpose, the update helper function 
could be useful.