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

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

[Ranjani Sridharan]

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 <ranjani.sridharan@linux.intel.com>
---

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
-- 
2.14.1

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

[Takashi Sakamoto]

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 <ranjani.sridharan@linux.intel.com>
> ---
> 
> 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:
  - '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.


Regards

Takashi Sakamoto

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

[Ranjani Sridharan]

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 <ranjani.sridharan@linux.intel.com
> > >
> > ---
> > 
> > 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