From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ranjani Sridharan Subject: Re: [PATCH v3] ALSA: core api: define offsets for TLV items Date: Wed, 09 May 2018 17:40:37 -0700 Message-ID: <1525912837.3597.1.camel@linux.intel.com> References: <20180507210431.4332-1-ranjani.sridharan@linux.intel.com> <16c25e6a-20f2-e390-a7ba-1cb9129dc0bc@sakamocchi.jp> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by alsa0.perex.cz (Postfix) with ESMTP id 64F19266F44 for ; Thu, 10 May 2018 02:40:40 +0200 (CEST) In-Reply-To: <16c25e6a-20f2-e390-a7ba-1cb9129dc0bc@sakamocchi.jp> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Sakamoto , alsa-devel@alsa-project.org Cc: liam.r.girdwood@linux.intel.com List-Id: alsa-devel@alsa-project.org On Thu, 2018-05-10 at 03:06 +0900, Takashi Sakamoto wrote: > Hi, > > On May 8 2018 06:04, Ranjani Sridharan wrote: > > Currently, there are no pre-defined accessors for the elements > > in topology TLV data. In the absence of such offsets, the > > tlv data will have to be decoded using hardwired offset > > numbers 0-N depending on the type of TLV. This patch defines > > accessor offsets for the type, length, min and mute/step items > > in TLV data for DB_SCALE type tlv's. These will be used by drivers > > to > > decode the TLV data while loading topology thereby improving > > code readability. The type and len offsets are common for all TLV > > types. The min and step/mute offsets are specific to DB_SCALE tlv > > type. > > > > Signed-off-by: Ranjani Sridharan > > > > --- > > > > Notes: > > v3: fix prefix for offset definitions > > > > 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..13f425c2abbd 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_TLVO_TYPE 0 > > +#define SNDRV_CTL_TLVO_LEN 1 > > + > > +/* Accessor offsets for min, mute and step items in dB scale type > > TLV */ > > +#define SNDRV_CTL_TLVO_DB_SCALE_MIN 2 > > +#define SNDRV_CTL_TLVO_DB_SCALE_MUTE_AND_STEP 3 > > + > > #endif > > You added these macros in the end of this file. In my opinion, this > is > inconvenient for future when adding more macros for new types. It's > better to place the additional macro to places convenient to find > relevant lines such that: Thanks, Takashi. I've fixed the placement in v4. > - 'SNDRV_CTL_TLVO_TYPE' and 'SNDRV_CTL_TLVO_LEN' following > to comments about layout of TLV data. > - 'SNDRV_CTL_TLVO_DB_SCALE_MIN' and > 'SNDRV_CTL_TLVO_DB_SCALE_MUTE_AND_STEP' following to > 'SNDRV_CTL_TLVO_DB_SCALE_MUTE_AND_STEP'. > > diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h > index be5371f09a62..b5f45b13077e 100644 > --- a/include/uapi/sound/tlv.h > +++ b/include/uapi/sound/tlv.h > @@ -37,6 +37,9 @@ > * block_length = (length + (sizeof(unsigned int) - 1)) & > * ~(sizeof(unsigned int) - 1)) .... > */ > +#define SNDRV_CTL_TLVO_TYPE 0 > +#define SNDRV_CTL_TLVO_LEN 1 > + > #define SNDRV_CTL_TLVD_ITEM(type, ...) \ > (type), SNDRV_CTL_TLVD_LENGTH(__VA_ARGS__), __VA_ARGS__ > #define SNDRV_CTL_TLVD_LENGTH(...) \ > @@ -61,6 +64,9 @@ > SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \ > } > > +#define SNDRV_CTL_TLVO_DB_SCALE_MIN 2 > +#define SNDRV_CTL_TLVO_DB_SCALE_MUTE_AND_STEP 3 > + > /* dB scale specified with min/max values instead of step */ > #define SNDRV_CTL_TLVD_DB_MINMAX_ITEM(min_dB, max_dB) \ > SNDRV_CTL_TLVD_ITEM(SNDRV_CTL_TLVT_DB_MINMAX, (min_dB), > (max_dB)) > > I have another concern that 'O' is hard to distinguish from 'D' as a > quick glance and can easily leads developers and reviewers to > misread 'SNDRV_CTL_TLVD_XXX' and 'SNDRV_CTL_TLVO_XXX'. Of cource, I > know that you intend to use 'O' to express 'offset' and it's > logically > valid. My concern is just for visual. I've left the prefix SNDRV_CTL_TLVO for lack of a better one and adding OFFSET in the prefix would make the names very long. Hope that is OK. > > > Regards > > Takashi Sakamoto