From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751982AbdKVXoe (ORCPT ); Wed, 22 Nov 2017 18:44:34 -0500 Received: from vps-vb.mhejs.net ([37.28.154.113]:41310 "EHLO vps-vb.mhejs.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751762AbdKVXod (ORCPT ); Wed, 22 Nov 2017 18:44:33 -0500 Subject: Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4 To: Takashi Sakamoto Cc: Timur Tabi , Nicolin Chen , Xiubo Li , Jaroslav Kysela , Takashi Iwai , alsa-devel@alsa-project.org, Liam Girdwood , linux-kernel , Mark Brown , Fabio Estevam , linuxppc-dev@lists.ozlabs.org References: <53d527c0-89f6-d501-09e9-1effea8a5bac@maciej.szmigiero.name> From: "Maciej S. Szmigiero" Message-ID: <4d0f43f5-0161-3dfb-1692-e755ad7fe641@maciej.szmigiero.name> Date: Thu, 23 Nov 2017 00:44:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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] 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). > Regards > > Takashi Sakamoto Best regards, Maciej Szmigiero