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

[Ranjani Sridharan <ranjani.sridharan@linux.intel.com>]

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