[alsa-devel] [PATCH] ASoC: Add accessor macros for TLV items

[Takashi Sakamoto]

Hi,

On Mar 28 2018 06:08, Ranjani Sridharan wrote:
>>> --- a/include/sound/tlv.h
>>> +++ b/include/sound/tlv.h
>>> @@ -49,6 +49,11 @@
>>>   
>>>   #define TLV_DB_GAIN_MUTE		SNDRV_CTL_TLVD_DB_GAIN_MUT
>>> E
>>>   
>>> +#define TLV_ITEM_TYPE			SNDRV_CTL_TLVD_ITEM_T
>>> YPE
>>> +#define TLV_ITEM_LEN			SNDRV_CTL_TLVD_ITEM_LE
>>> N
>>> +#define TLV_DB_MIN			SNDRV_CTL_TLVD_DB_SCALE_
>>> MIN
>>> +#define TLV_DB_MUTE_AND_STEP		SNDRV_CTL_TLVD_DB_SCAL
>>> E_MUTE_AND_STEP
>>
>> What's the reason of these redundant redefinitions?
> 
> I followed the other definitions in this file which were aliases to the
> ones in uapi/sound/tlv.h. I suppose these can be avoided. I'll fix this
>   in v2.

When adding new macros relevant to TLV, please add them into 
'include/uapi/sound/tlv.h' instead of 'include/sound/tlv.h'. This is 
required to share declarations to user space. The latter should be going 
to be obsoleted with replacement of all of macros but developers 
including me have been lazy to do it for a small concerns.

>>> --- a/include/uapi/sound/tlv.h
>>> +++ b/include/uapi/sound/tlv.h
>>> @@ -60,6 +60,13 @@
>>>   	unsigned int name[] = { \
>>>   		SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \
>>>   	}
>>> +/* Accessor macros for TLV type and len */
>>> +#define SNDRV_CTL_TLVD_ITEM_TYPE		0
>>> +#define SNDRV_CTL_TLVD_ITEM_LEN			1
>>> +
>>> +/* Accessor macros for min, mute and step values from dB scale
>>> type TLV */
>>> +#define SNDRV_CTL_TLVD_DB_SCALE_MIN		2
>>> +#define SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP	3
>>
>> These are no "macros" but constants.  And, they use the same prefix
>> SNDRV_CTL_TLVD_* as other existing macros.  SNDTV_CTL_TLVD_*() is a
>> macro to define/expand a TLV descriptor.  It's not for defining the
>> offset value as added in the patch.
>>
>> That is, better to choose another unique prefix to distinguish the
>> definitions clearly.
> 
> Sure, will change the prefix as well. Thanks!

And in my opinion when adding such new lines, it's better to post actual 
usage of them with the same patch serie. Addition of them are easy, but 
maintenance of them is not easy as the same.

In short:

"These will be used by drivers to extract the TLV data while loading 
topology." from your patch comment.

This comment seems to be just from your in-house tree, in reviewer's 
eyes. Please show actual usage of the new macros to evaluate your 
changes. At least, it's better ways to add changes into core features.


Regards

Takashi Sakamoto