* [PATCH] ASoC: Add accessor macros for TLV items
@ 2018-03-27 18:39 Ranjani Sridharan
2018-03-27 19:43 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: Ranjani Sridharan @ 2018-03-27 18:39 UTC (permalink / raw)
To: alsa-devel; +Cc: liam.r.girdwood, broonie
This patch adds macros for accessing 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>
---
include/sound/tlv.h | 5 +++++
include/uapi/sound/tlv.h | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/include/sound/tlv.h b/include/sound/tlv.h
index 3677ebb928d5..8fa379e8a568 100644
--- a/include/sound/tlv.h
+++ b/include/sound/tlv.h
@@ -49,6 +49,11 @@
#define TLV_DB_GAIN_MUTE SNDRV_CTL_TLVD_DB_GAIN_MUTE
+#define TLV_ITEM_TYPE SNDRV_CTL_TLVD_ITEM_TYPE
+#define TLV_ITEM_LEN SNDRV_CTL_TLVD_ITEM_LEN
+#define TLV_DB_MIN SNDRV_CTL_TLVD_DB_SCALE_MIN
+#define TLV_DB_MUTE_AND_STEP SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP
+
/*
* The below assumes that each item TLV is 4 words like DB_SCALE or LINEAR.
* This is an old fasion and obsoleted by commit bf1d1c9b6179("ALSA: tlv: add
diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
index be5371f09a62..f3fba49ef6e8 100644
--- a/include/uapi/sound/tlv.h
+++ b/include/uapi/sound/tlv.h
@@ -60,6 +60,13 @@
unsigned int name[] = { \
SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \
}
+/* Accessor macros for TLV type and len */
+#define SNDRV_CTL_TLVD_ITEM_TYPE 0
+#define SNDRV_CTL_TLVD_ITEM_LEN 1
+
+/* Accessor macros for min, mute and step values from dB scale type TLV */
+#define SNDRV_CTL_TLVD_DB_SCALE_MIN 2
+#define SNDRV_CTL_TLVD_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) \
--
2.14.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ASoC: Add accessor macros for TLV items
2018-03-27 18:39 [PATCH] ASoC: Add accessor macros for TLV items Ranjani Sridharan
@ 2018-03-27 19:43 ` Takashi Iwai
2018-03-27 21:08 ` Ranjani Sridharan
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Iwai @ 2018-03-27 19:43 UTC (permalink / raw)
To: Ranjani Sridharan; +Cc: liam.r.girdwood, alsa-devel, broonie
On Tue, 27 Mar 2018 20:39:45 +0200,
Ranjani Sridharan wrote:
>
> This patch adds macros for accessing 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>
The subject is wrongly prefixed. This is ALSA core API stuff, not
about ASoC.
And about the change...
> --- a/include/sound/tlv.h
> +++ b/include/sound/tlv.h
> @@ -49,6 +49,11 @@
>
> #define TLV_DB_GAIN_MUTE SNDRV_CTL_TLVD_DB_GAIN_MUTE
>
> +#define TLV_ITEM_TYPE SNDRV_CTL_TLVD_ITEM_TYPE
> +#define TLV_ITEM_LEN SNDRV_CTL_TLVD_ITEM_LEN
> +#define TLV_DB_MIN SNDRV_CTL_TLVD_DB_SCALE_MIN
> +#define TLV_DB_MUTE_AND_STEP SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP
What's the reason of these redundant redefinitions?
> --- a/include/uapi/sound/tlv.h
> +++ b/include/uapi/sound/tlv.h
> @@ -60,6 +60,13 @@
> unsigned int name[] = { \
> SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \
> }
> +/* Accessor macros for TLV type and len */
> +#define SNDRV_CTL_TLVD_ITEM_TYPE 0
> +#define SNDRV_CTL_TLVD_ITEM_LEN 1
> +
> +/* Accessor macros for min, mute and step values from dB scale type TLV */
> +#define SNDRV_CTL_TLVD_DB_SCALE_MIN 2
> +#define SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP 3
These are no "macros" but constants. And, they use the same prefix
SNDRV_CTL_TLVD_* as other existing macros. SNDTV_CTL_TLVD_*() is a
macro to define/expand a TLV descriptor. It's not for defining the
offset value as added in the patch.
That is, better to choose another unique prefix to distinguish the
definitions clearly.
thanks,
Takashi
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ASoC: Add accessor macros for TLV items
2018-03-27 19:43 ` Takashi Iwai
@ 2018-03-27 21:08 ` Ranjani Sridharan
2018-03-28 1:55 ` Takashi Sakamoto
0 siblings, 1 reply; 4+ messages in thread
From: Ranjani Sridharan @ 2018-03-27 21:08 UTC (permalink / raw)
To: Takashi Iwai; +Cc: liam.r.girdwood, alsa-devel, broonie
On Tue, 2018-03-27 at 21:43 +0200, Takashi Iwai wrote:
> On Tue, 27 Mar 2018 20:39:45 +0200,
> Ranjani Sridharan wrote:
> >
> > This patch adds macros for accessing 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
> > >
>
> The subject is wrongly prefixed. This is ALSA core API stuff, not
> about ASoC.
Will fix this in v2.
>
> And about the change...
>
> > --- a/include/sound/tlv.h
> > +++ b/include/sound/tlv.h
> > @@ -49,6 +49,11 @@
> >
> > #define TLV_DB_GAIN_MUTE SNDRV_CTL_TLVD_DB_GAIN_MUT
> > E
> >
> > +#define TLV_ITEM_TYPE SNDRV_CTL_TLVD_ITEM_T
> > YPE
> > +#define TLV_ITEM_LEN SNDRV_CTL_TLVD_ITEM_LE
> > N
> > +#define TLV_DB_MIN SNDRV_CTL_TLVD_DB_SCALE_
> > MIN
> > +#define TLV_DB_MUTE_AND_STEP SNDRV_CTL_TLVD_DB_SCAL
> > E_MUTE_AND_STEP
>
> What's the reason of these redundant redefinitions?
I followed the other definitions in this file which were aliases to the
ones in uapi/sound/tlv.h. I suppose these can be avoided. I'll fix this
in v2.
>
>
> > --- a/include/uapi/sound/tlv.h
> > +++ b/include/uapi/sound/tlv.h
> > @@ -60,6 +60,13 @@
> > unsigned int name[] = { \
> > SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \
> > }
> > +/* Accessor macros for TLV type and len */
> > +#define SNDRV_CTL_TLVD_ITEM_TYPE 0
> > +#define SNDRV_CTL_TLVD_ITEM_LEN 1
> > +
> > +/* Accessor macros for min, mute and step values from dB scale
> > type TLV */
> > +#define SNDRV_CTL_TLVD_DB_SCALE_MIN 2
> > +#define SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP 3
>
> These are no "macros" but constants. And, they use the same prefix
> SNDRV_CTL_TLVD_* as other existing macros. SNDTV_CTL_TLVD_*() is a
> macro to define/expand a TLV descriptor. It's not for defining the
> offset value as added in the patch.
>
> That is, better to choose another unique prefix to distinguish the
> definitions clearly.
Sure, will change the prefix as well. Thanks!
>
>
> thanks,
>
> Takashi
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ASoC: Add accessor macros for TLV items
2018-03-27 21:08 ` Ranjani Sridharan
@ 2018-03-28 1:55 ` Takashi Sakamoto
0 siblings, 0 replies; 4+ messages in thread
From: Takashi Sakamoto @ 2018-03-28 1:55 UTC (permalink / raw)
To: Ranjani Sridharan, Takashi Iwai; +Cc: liam.r.girdwood, alsa-devel, broonie
Hi,
On Mar 28 2018 06:08, Ranjani Sridharan wrote:
>>> --- a/include/sound/tlv.h
>>> +++ b/include/sound/tlv.h
>>> @@ -49,6 +49,11 @@
>>>
>>> #define TLV_DB_GAIN_MUTE SNDRV_CTL_TLVD_DB_GAIN_MUT
>>> E
>>>
>>> +#define TLV_ITEM_TYPE SNDRV_CTL_TLVD_ITEM_T
>>> YPE
>>> +#define TLV_ITEM_LEN SNDRV_CTL_TLVD_ITEM_LE
>>> N
>>> +#define TLV_DB_MIN SNDRV_CTL_TLVD_DB_SCALE_
>>> MIN
>>> +#define TLV_DB_MUTE_AND_STEP SNDRV_CTL_TLVD_DB_SCAL
>>> E_MUTE_AND_STEP
>>
>> What's the reason of these redundant redefinitions?
>
> I followed the other definitions in this file which were aliases to the
> ones in uapi/sound/tlv.h. I suppose these can be avoided. I'll fix this
> in v2.
When adding new macros relevant to TLV, please add them into
'include/uapi/sound/tlv.h' instead of 'include/sound/tlv.h'. This is
required to share declarations to user space. The latter should be going
to be obsoleted with replacement of all of macros but developers
including me have been lazy to do it for a small concerns.
>>> --- a/include/uapi/sound/tlv.h
>>> +++ b/include/uapi/sound/tlv.h
>>> @@ -60,6 +60,13 @@
>>> unsigned int name[] = { \
>>> SNDRV_CTL_TLVD_DB_SCALE_ITEM(min, step, mute) \
>>> }
>>> +/* Accessor macros for TLV type and len */
>>> +#define SNDRV_CTL_TLVD_ITEM_TYPE 0
>>> +#define SNDRV_CTL_TLVD_ITEM_LEN 1
>>> +
>>> +/* Accessor macros for min, mute and step values from dB scale
>>> type TLV */
>>> +#define SNDRV_CTL_TLVD_DB_SCALE_MIN 2
>>> +#define SNDRV_CTL_TLVD_DB_SCALE_MUTE_AND_STEP 3
>>
>> These are no "macros" but constants. And, they use the same prefix
>> SNDRV_CTL_TLVD_* as other existing macros. SNDTV_CTL_TLVD_*() is a
>> macro to define/expand a TLV descriptor. It's not for defining the
>> offset value as added in the patch.
>>
>> That is, better to choose another unique prefix to distinguish the
>> definitions clearly.
>
> Sure, will change the prefix as well. Thanks!
And in my opinion when adding such new lines, it's better to post actual
usage of them with the same patch serie. Addition of them are easy, but
maintenance of them is not easy as the same.
In short:
"These will be used by drivers to extract the TLV data while loading
topology." from your patch comment.
This comment seems to be just from your in-house tree, in reviewer's
eyes. Please show actual usage of the new macros to evaluate your
changes. At least, it's better ways to add changes into core features.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-28 1:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 18:39 [PATCH] ASoC: Add accessor macros for TLV items Ranjani Sridharan
2018-03-27 19:43 ` Takashi Iwai
2018-03-27 21:08 ` Ranjani Sridharan
2018-03-28 1:55 ` Takashi Sakamoto
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.