[alsa-devel] [PATCH v2] ALSA: core api: define offsets for TLV items

[Takashi Sakamoto]

Hi,

On Mar 29 2018 02:09, Ranjani Sridharan wrote:
> This patch defines accessor offsets for the type, length, min,
> mute and step items in TLV data. These will be used by drivers to
> extract the TLV data while loading topology.
> 
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> ---
> V2: removed redundant redefinitions and updated prefix
> ---
>   include/uapi/sound/tlv.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
> index be5371f09a62..1fd786a5e57b 100644
> --- a/include/uapi/sound/tlv.h
> +++ b/include/uapi/sound/tlv.h
> @@ -98,4 +98,12 @@
>   
>   #define SNDRV_CTL_TLVD_DB_GAIN_MUTE	-9999999
>   
> +/* Accessor offsets for TLV data items */
> +#define SNDRV_CTL_TLV_OFFSET_TYPE		0
> +#define SNDRV_CTL_TLV_OFFSET_LEN		1

I suggest you to use these macros in declaration of 
'SNDRV_CTL_TLVD_ITEM()' for self-described header. In short, I mean:

$ diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
index be5371f09a62..76598609f349 100644
--- a/include/uapi/sound/tlv.h
+++ b/include/uapi/sound/tlv.h
@@ -37,8 +37,15 @@
   *        block_length = (length + (sizeof(unsigned int) - 1)) &
   *                       ~(sizeof(unsigned int) - 1)) ....
   */
+
+/* Accessor offsets for TLV data items */
+#define SNDRV_CTL_TLV_OFFSET_TYPE      0
+#define SNDRV_CTL_TLV_OFFSET_LEN       1
+
  #define SNDRV_CTL_TLVD_ITEM(type, ...) \
-       (type), SNDRV_CTL_TLVD_LENGTH(__VA_ARGS__), __VA_ARGS__
+       [SNDRV_CTL_TLV_OFFSET_TYPE] = (type), \
+       [SNDRV_CTL_TLV_OFFSET_LEN] = SNDRV_CTL_TLVD_LENGTH(__VA_ARGS__), \
+       __VA_ARGS__
  #define SNDRV_CTL_TLVD_LENGTH(...) \
         ((unsigned int)sizeof((const unsigned int[]) { __VA_ARGS__ }))

> +/* Accessor offsets for min, mute and step items in dB scale type TLV */
> +#define SNDRV_CTL_TLV_OFFSET_DB_MIN		2
> +#define SNDRV_CTL_TLV_OFFSET_DB_MUTE_AND_STEP	3
> +
>   #endif

I wonder why you add the above macros just for data of 
'SNDRV_CTL_TLVT_DB_MINMAX_MUTE'. In fact, this header defines below 
types of TLV data.
  - SNDRV_CTL_TLVT_CONTAINER
  - SNDRV_CTL_TLVT_DB_SCALE
  - SNDRV_CTL_TLVT_DB_LINEAR
  - SNDRV_CTL_TLVT_DB_RANGE
  - SNDRV_CTL_TLVT_DB_MINMAX
  - SNDRV_CTL_TLVT_DB_MINMAX_MUTE

Would I request you to your reason not to add such offset macros for the 
others? At least, we should have a care for 'SNDRV_CTL_TLVT_DB_LINEAR' 
to keep consistency of defined macros.

Regards

Takashi Sakamoto