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

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

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

> Regards
> Takashi Sakamoto

Best regards,
Maciej Szmigiero

  reply	other threads:[~2017-11-22 23:44 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 [this message]
2017-11-23  7:40     ` Takashi Sakamoto
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).