Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4

[Maciej S. Szmigiero]

On 23.11.2017 08:40, Takashi Sakamoto wrote:
> On Nov 23 2017 08:44, Maciej S. Szmigiero wrote:
>> On 23.11.2017 00:27, Takashi Sakamoto wrote:
>>> On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
>> (..)
>>>> --- a/include/uapi/sound/asound.h
>>>> +++ b/include/uapi/sound/asound.h
>>>> @@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
>>>> ÂÂ #defineÂÂÂ SNDRV_PCM_FORMAT_DSD_U32_LEÂÂÂ ((__force snd_pcm_format_t) 50) /* DSD, 4-byte samples DSD (x32), little endian */
>>>> ÂÂ #defineÂÂÂ SNDRV_PCM_FORMAT_DSD_U16_BEÂÂÂ ((__force snd_pcm_format_t) 51) /* DSD, 2-byte samples DSD (x16), big endian */
>>>> ÂÂ #defineÂÂÂ SNDRV_PCM_FORMAT_DSD_U32_BEÂÂÂ ((__force snd_pcm_format_t) 52) /* DSD, 4-byte samples DSD (x32), big endian */
>>>> -#defineÂÂÂ SNDRV_PCM_FORMAT_LASTÂÂÂÂÂÂÂ SNDRV_PCM_FORMAT_DSD_U32_BE
>>>> +#defineÂÂÂ SNDRV_PCM_FORMAT_S20_4LEÂÂÂ ((__force snd_pcm_format_t) 53)ÂÂÂ /* in four bytes */
>>>> +#defineÂÂÂ SNDRV_PCM_FORMAT_S20_4BEÂÂÂ ((__force snd_pcm_format_t) 54)ÂÂÂ /* in four bytes */
>>>> +#defineÂÂÂ SNDRV_PCM_FORMAT_U20_4LEÂÂÂ ((__force snd_pcm_format_t) 55)ÂÂÂ /* in four bytes */
>>>> +#defineÂÂÂ SNDRV_PCM_FORMAT_U20_4BEÂÂÂ ((__force snd_pcm_format_t) 56)ÂÂÂ /* in four bytes */
>>>> +#defineÂÂÂ SNDRV_PCM_FORMAT_LASTÂÂÂÂÂÂÂ SNDRV_PCM_FORMAT_U20_4BE
>>>
>>> In my opinion, for this type of definition, it's better to declare left/right-adjusted or padding side. (Of course, silence definition is already a hint, however the lack of information forces developers to have a careful behaviour to handle entries on the list.
>>> (I note that in current ALSA PCM interface there's no way to deliver MSB/LSB-first information about sample format.)
>>
>> No other sound format includes this information in its name
> 
> You overlook comments in 'SNDRV_PCM_FORMAT_[U|S]24_[LE|BE]'. Let me refer to them [1]:
> 
> 198 #define SNDRV_PCM_FORMAT_S24_LE ((__force snd_pcm_format_t) 6) /* low three bytes */
> 199 #define SNDRV_PCM_FORMAT_S24_BE ((__force snd_pcm_format_t) 7) /* low three bytes */
> 200 #define SNDRV_PCM_FORMAT_U24_LE ((__force snd_pcm_format_t) 8) /* low three bytes */
> 201 #define SNDRV_PCM_FORMAT_U24_BE ((__force snd_pcm_format_t) 9) /* low three bytes */

Yes, I can add this information in a comment, just like these formats are
described as "low three bytes" (in other words, LSB justified formats)

> In your way, these types of format can be represented by 'SNDRV_PCM_FORMAT_[U|S]24_4[LE|BE]', thus for playback direction they
> mean:
> 
> ```
> #include <sound/asound.h>
> #include <endian.h>
> 
> uint32_t *buf;
> uint32_t sample;
> snd_pcm_format_t format;
> 
> sample = generate_a_sample();
> (sample & ~0x00ffffff) /* invalid bits as sample */
> 
> if (format == SNDRV_PCM_FORMAT_[U|S]24_LE) {
> Â buf[0] = htole32(sample);
> else
> Â buf[0] = htobe32(sample);
> 
> /* transfer content of the buf via ALSA kernel stuffs. */
> ```
> 
> The comments are good enough for application developers in an aspect of a position for padding.
> 
> In general, studying from the past is preferable behaviour to be genius, however accumulated history includes mistakes and defects. Just pretending the past is not so genius, without further consideration.
> 
> Actually additions of the rest of entries for PCM format were done without enough cares of what information they give to application developers. Adding new entries is easier than fixing and improving them once exposed. It's a reason that they're left what they're.
> 
> I wish you had enough care to assist applications developers. Without applications, drivers are worthless and just waste of code base.

Right, I will add this information to a comment.

(..)
>>> Additionally, alsa-lib includes some codes related to the definition[1]. If you'd like to thing goes well out of ALSA SoC part, it's better to submit changes to the library as well.
>>>
>>> [1] http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
>>
>> I have alsa-lib changes ready for these formats - they were needed to
>> test these patches, will post them when this is merged on the kernel
>> side (in case some changes are needed which affect both).
> 
> Please pay enough care when writing patch comment. Silence means nothing, at least for reviewers, even if you have good preparations.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.15-rc1#n198

I will add this information (about alsa-lib changes) to commit notes in this patch description.

> Regards
> 
> Takashi Sakamoto

Best regards,
Maciej Szmigiero