* [PATCH v2] ALSA: core api: define offsets for TLV items @ 2018-03-28 17:09 Ranjani Sridharan 2018-03-28 19:05 ` Takashi Sakamoto 0 siblings, 1 reply; 4+ messages in thread From: Ranjani Sridharan @ 2018-03-28 17:09 UTC (permalink / raw) To: alsa-devel; +Cc: liam.r.girdwood 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: core api: define offsets for TLV items 2018-03-28 17:09 [PATCH v2] ALSA: core api: define offsets for TLV items Ranjani Sridharan @ 2018-03-28 19:05 ` Takashi Sakamoto 2018-03-28 19:30 ` Takashi Sakamoto 0 siblings, 1 reply; 4+ messages in thread From: Takashi Sakamoto @ 2018-03-28 19:05 UTC (permalink / raw) To: Ranjani Sridharan, alsa-devel; +Cc: liam.r.girdwood 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: core api: define offsets for TLV items 2018-03-28 19:05 ` Takashi Sakamoto @ 2018-03-28 19:30 ` Takashi Sakamoto 2018-03-29 4:07 ` Ranjani Sridharan 0 siblings, 1 reply; 4+ messages in thread From: Takashi Sakamoto @ 2018-03-28 19:30 UTC (permalink / raw) To: Ranjani Sridharan, alsa-devel; +Cc: liam.r.girdwood 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ALSA: core api: define offsets for TLV items 2018-03-28 19:30 ` Takashi Sakamoto @ 2018-03-29 4:07 ` Ranjani Sridharan 0 siblings, 0 replies; 4+ messages in thread From: Ranjani Sridharan @ 2018-03-29 4:07 UTC (permalink / raw) To: Takashi Sakamoto, alsa-devel; +Cc: liam.r.girdwood 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-29 4:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-28 17:09 [PATCH v2] ALSA: core api: define offsets for TLV items Ranjani Sridharan 2018-03-28 19:05 ` Takashi Sakamoto 2018-03-28 19:30 ` Takashi Sakamoto 2018-03-29 4:07 ` Ranjani Sridharan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).