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

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

[Ranjani Sridharan]

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@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
+
+/* 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
-- 
2.14.1

Re: [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@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

Re: [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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ALSA: core api: define offsets for TLV items

[Ranjani Sridharan]

On Thu, 2018-03-29 at 04:30 +0900, Takashi Sakamoto wrote:
> 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.

Thanks for your comments, Takashi. 
Maybe this patch was a bit shortsighted, after all.

Let me step back and explain my motivation.

While working on decoding the tlv data in topology in the sof driver,
it was suggested that it would make the code easier to follow if the 
offsets were replaced with constants defining what each offset stood
for. 
At the moment, sof supports only dB scale type TLV, so thats the reason
for me not considering the other types.
> 
> 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.

Yes, single declaration is only one of interest to me at the moment. So
I'd appreciate suggestions. Thanks again!

> 
> 
> Regards
> 
> Takashi Sakamoto