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

[Takashi Sakamoto]

On Mar 29 2018 04:05, Takashi Sakamoto wrote:
>> 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.

Oops. I missed nested-data such like 'SNDRV_CTL_TLVT_CONTAINER' or 
'SNDRV_CTL_TLVT_DB_RANGE'. These macros expands data including data 
expanded by the other macros, thus offsets on the element data are 
different from a case of single declaration. I mean:

static const SNDRV_CTL_TLVD_DECLARE_DB_RANGE(range,
          0,   4, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-5940, 44, 1),
          4,  22, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-5636, 60, 0),
         22,  33, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-4556, 76, 0),
         33,  37, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-4072, 44, 0),
         37,  48, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-3832, 76, 0),
         48,  66, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-2996, 60, 0),
         66,  84, SNDRV_CTL_TLVD_DB_SCALE_ITEM(-2204, 44, 0),
         84,  95, SNDRV_CTL_TLVD_DB_SCALE_ITEM( -836, 60, 0),
         95,  99, SNDRV_CTL_TLVD_DB_SCALE_ITEM( -176, 76, 0),
         100, 10000, SNDRV_CTL_TLVD_DB_SCALE_ITEM(0, 0, 0),
);

This kind of declaration includes several elements for each type of 
data. Here, my concern is that your macros can confuse developers in 
userspace in this case or not. If you assume quite simple usecases of 
TLV macros (single declaration), it's better to have more discussion 
about your changes.


Regards

Takashi Sakamoto