LKML Archive on
help / color / mirror / Atom feed
From: Takashi Sakamoto <>
To: "Maciej S. Szmigiero" <>
Cc:, Timur Tabi <>,
	Xiubo Li <>,
	Liam Girdwood <>,
	Takashi Iwai <>,
	Nicolin Chen <>,
	Mark Brown <>,
	Fabio Estevam <>,,
	linux-kernel <>
Subject: Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4
Date: Thu, 23 Nov 2017 16:40:21 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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_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 */
>> 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 */

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

#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);
   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.

> so if we name
> these formats SNDRV_PCM_FORMAT_{S, U}20LSB_4 they are going to have it
> inconsistent with every other one
> (I assume you meant to include such information in a format name?).
> But information about whether this format is MSB or LSB justified can be
> added in a comment so the situation is clear for other developers from
> the definition without needing to read the actual processing code.

For consistency of the other entries, this is not so preferable, in my 
opinion. So I didn't suggest it and just noted.

>> 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];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.



Takashi Sakamoto

  reply	other threads:[~2017-11-23  7:40 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-22 19:17 [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20_4 Maciej S. Szmigiero
2017-11-22 23:27 ` [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4 Takashi Sakamoto
2017-11-22 23:44   ` Maciej S. Szmigiero
2017-11-23  7:40     ` Takashi Sakamoto [this message]
2017-11-23 12:26       ` Maciej S. Szmigiero
2017-11-23  8:08 ` [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20_4 Takashi Iwai
2017-11-23 12:26   ` Maciej S. Szmigiero

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).