All of lore.kernel.org
 help / color / mirror / Atom feed
* [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set
@ 2016-06-12  8:16 Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 01/10] ctl: add an overview for design of ALSA control interface Takashi Sakamoto
                   ` (9 more replies)
  0 siblings, 10 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

Hi,

This patchset is revised version of below one:

[alsa-devel] [alsa-lib][PATCH 0/6] control: add APIs for control element set
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-February/104795.html

In this time, I add a program to test the feature.

Changes:
 - Use a model which consists of element set/element/member/value.
 - Add an description about the design of ALSA ctl feature.
 - Obsolete old APIs due to the lack of const semantics.
 - Add an description about threshold level feature and topology framework.
 - Improve comments to threshold level APIs.
 - Add a test program.

Before starting any discussions, please read description of the model, to stand
on the same basis of understanding.


I note that execution of the test program causes an abort to PulseAudio. I can
see this line below.

pulseaudio: hcontrol.c:745: snd_hctl_handle_event: Assertion `res >= 0 && dir == 0' failed.
 
I guess that quick addition/removal of control element set causes this
assertion, but I have little time to investigate the cause. I wish to get helps
of ALSA developers in PulseAudio side.


If you need an practical example of the feature, please test one of Python 3    
samples in my 'alsa-gi'.
https://github.com/takaswie/alsa-gi/

This works without alsa-lib, by using kernel/userspace interface directly.
Therefore, this patchet doesn't affect the implementation.


Takashi Sakamoto (10):
  ctl: add an overview for design of ALSA control interface
  ctl: improve comments for handling element data
  ctl: add functions to add an element set
  ctl: improve comments for API to add an element of IEC958 type
  ctl: change former APIs as wrapper functions of element set APIs
  ctl: deprecate APIs to add an element set with a single element
  pcm: use new APIs to add a control element set for softvol plugin
  ctl: add explaination about threshold level feature
  ctl: improve API documentation for threshold level operations
  ctl: add test program for control element set

 include/control.h           |  36 +-
 src/control/control.c       | 829 ++++++++++++++++++++++++++++++++------------
 src/pcm/pcm_softvol.c       |   7 +-
 test/Makefile.am            |   5 +-
 test/user-ctl-element-set.c | 561 ++++++++++++++++++++++++++++++
 5 files changed, 1216 insertions(+), 222 deletions(-)
 create mode 100644 test/user-ctl-element-set.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 01/10] ctl: add an overview for design of ALSA control interface
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 02/10] ctl: improve comments for handling element data Takashi Sakamoto
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

This commit adds a description about the design of ALSA control interface
for  developers to understand a few components of low level.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/control/control.c b/src/control/control.c
index ae78843..504da1f 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -35,9 +35,29 @@ also interface notifying about control and structure changes.
 
 \section control_general_overview General overview
 
-The primitive controls can be integer, inter64, boolean, enumerators, bytes
-and IEC958 structure.
-
+In ALSA control feature, each sound card can have control elements. The elements
+are managed according to below model.
+
+ - element set
+   - A set of elements with the same attribute (i.e. name, get/put operations).
+     Some element sets can be added to a sound card by drivers in kernel and
+     userspace applications.
+ - element
+   - An element can be identified by userspace applications. Each element has
+     own identical information.
+ - member
+   - An element includes some members to have a value. The value of each member
+     can be changed by both of userspace applications and drivers in kernel.
+
+Each element can be identified by two ways; a combination of name and index, or
+numerical number (numid).
+
+The type of element set is one of integer, integerr64, boolean, enumerators,
+bytes and IEC958 structure. This indicates the type of value for each member in
+elements included in the element set.
+
+When the value of member is changed, corresponding events are transferred to
+userspace applications. The applications should subscribe any events in advance.
 */
 
 #include <stdio.h>
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 02/10] ctl: improve comments for handling element data
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 01/10] ctl: add an overview for design of ALSA control interface Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 03/10] ctl: add functions to add an element set Takashi Sakamoto
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

Some parts of control API documentation are described with core-developer
friendly explanations. To usual developer such as me, they're quite hard
to understand.

This commit improves such comments for a part of APIs to handle data of
each element.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 246 +++++++++++++++++++++++++++-----------------------
 1 file changed, 131 insertions(+), 115 deletions(-)

diff --git a/src/control/control.c b/src/control/control.c
index 504da1f..7d1e412 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -482,7 +482,7 @@ int snd_ctl_elem_remove(snd_ctl_t *ctl, snd_ctl_elem_id_t *id)
 /**
  * \brief Get CTL element value
  * \param ctl CTL handle
- * \param control CTL element id/value pointer
+ * \param obj Data of an element.
  * \return 0 on success otherwise a negative error code
  */
 int snd_ctl_elem_read(snd_ctl_t *ctl, snd_ctl_elem_value_t *control)
@@ -494,7 +494,7 @@ int snd_ctl_elem_read(snd_ctl_t *ctl, snd_ctl_elem_value_t *control)
 /**
  * \brief Set CTL element value
  * \param ctl CTL handle
- * \param control CTL element id/value pointer
+ * \param obj Data of an element.
  * \retval 0 on success
  * \retval >0 on success when value was changed
  * \retval <0 a negative error code
@@ -2295,8 +2295,8 @@ void snd_ctl_elem_info_set_index(snd_ctl_elem_info_t *obj, unsigned int val)
 }
 
 /**
- * \brief get size of #snd_ctl_elem_value_t
- * \return size in bytes
+ * \brief Get size of data structure for an element.
+ * \return Size in bytes.
  */
 size_t snd_ctl_elem_value_sizeof()
 {
@@ -2304,9 +2304,9 @@ size_t snd_ctl_elem_value_sizeof()
 }
 
 /**
- * \brief allocate an invalid #snd_ctl_elem_value_t using standard malloc
- * \param ptr returned pointer
- * \return 0 on success otherwise negative error code
+ * \brief Allocate an invalid #snd_ctl_elem_value_t using standard malloc(3).
+ * \param ptr Returned pointer for data of an element.
+ * \return 0 on success otherwise negative error code.
  */
 int snd_ctl_elem_value_malloc(snd_ctl_elem_value_t **ptr)
 {
@@ -2318,8 +2318,8 @@ int snd_ctl_elem_value_malloc(snd_ctl_elem_value_t **ptr)
 }
 
 /**
- * \brief frees a previously allocated #snd_ctl_elem_value_t
- * \param obj pointer to object to free
+ * \brief Frees a previously allocated data of an element.
+ * \param obj Data of an element.
  */
 void snd_ctl_elem_value_free(snd_ctl_elem_value_t *obj)
 {
@@ -2327,8 +2327,8 @@ void snd_ctl_elem_value_free(snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief clear given #snd_ctl_elem_value_t object
- * \param obj pointer to object to clear
+ * \brief Clear given data of an element.
+ * \param obj Data of an element.
  */
 void snd_ctl_elem_value_clear(snd_ctl_elem_value_t *obj)
 {
@@ -2336,32 +2336,34 @@ void snd_ctl_elem_value_clear(snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief copy one #snd_ctl_elem_value_t to another
- * \param dst pointer to destination
- * \param src pointer to source
+ * \brief Copy two data of elements.
+ * \param dst Pointer to destination.
+ * \param src Pointer to source.
  */
-void snd_ctl_elem_value_copy(snd_ctl_elem_value_t *dst, const snd_ctl_elem_value_t *src)
+void snd_ctl_elem_value_copy(snd_ctl_elem_value_t *dst,
+			     const snd_ctl_elem_value_t *src)
 {
 	assert(dst && src);
 	*dst = *src;
 }
 
 /**
- * \brief compare one #snd_ctl_elem_value_t to another
- * \param left pointer to first value
- * \param right pointer to second value
- * \return 0 on match, less than or greater than otherwise, see memcmp
+ * \brief Compare one data of an element to the other.
+ * \param left Pointer to first data.
+ * \param right Pointer to second data.
+ * \return 0 on match, less than or greater than otherwise, see memcmp(3).
  */
-int snd_ctl_elem_value_compare(snd_ctl_elem_value_t *left, const snd_ctl_elem_value_t *right)
+int snd_ctl_elem_value_compare(snd_ctl_elem_value_t *left,
+			       const snd_ctl_elem_value_t *right)
 {
 	assert(left && right);
 	return memcmp(left, right, sizeof(*left));
 }
 
 /**
- * \brief Get CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param ptr Pointer to returned CTL element identifier
+ * \brief Get element identifier from given data of an element.
+ * \param obj Data of an element.
+ * \param ptr Pointer for element identifier.
  */
 void snd_ctl_elem_value_get_id(const snd_ctl_elem_value_t *obj, snd_ctl_elem_id_t *ptr)
 {
@@ -2370,9 +2372,9 @@ void snd_ctl_elem_value_get_id(const snd_ctl_elem_value_t *obj, snd_ctl_elem_id_
 }
 
 /**
- * \brief Get element numeric identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return element numeric identifier
+ * \brief Get element numeric identifier from given data of an element.
+ * \param obj Data of an element.
+ * \return Element numeric identifier.
  */
 unsigned int snd_ctl_elem_value_get_numid(const snd_ctl_elem_value_t *obj)
 {
@@ -2381,9 +2383,10 @@ unsigned int snd_ctl_elem_value_get_numid(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Get interface part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return interface part of element identifier
+ * \brief Get interface part of element identifier from given data of an
+ *	  element.
+ * \param obj Data of an element.
+ * \return Interface part of element identifier.
  */
 snd_ctl_elem_iface_t snd_ctl_elem_value_get_interface(const snd_ctl_elem_value_t *obj)
 {
@@ -2392,9 +2395,9 @@ snd_ctl_elem_iface_t snd_ctl_elem_value_get_interface(const snd_ctl_elem_value_t
 }
 
 /**
- * \brief Get device part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return device part of element identifier
+ * \brief Get device part of element identifier from given data of an element.
+ * \param obj Data of an element.
+ * \return Device part of element identifier.
  */
 unsigned int snd_ctl_elem_value_get_device(const snd_ctl_elem_value_t *obj)
 {
@@ -2403,9 +2406,10 @@ unsigned int snd_ctl_elem_value_get_device(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Get subdevice part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return subdevice part of element identifier
+ * \brief Get subdevice part of element identifier from given data of an
+ *	  element.
+ * \param obj Data of an element.
+ * \return Subdevice part of element identifier.
  */
 unsigned int snd_ctl_elem_value_get_subdevice(const snd_ctl_elem_value_t *obj)
 {
@@ -2414,9 +2418,9 @@ unsigned int snd_ctl_elem_value_get_subdevice(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Get name part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return name part of element identifier
+ * \brief Get name part of element identifier from given data of an element.
+ * \param obj Data of an element.
+ * \return Name part of element identifier.
  */
 const char *snd_ctl_elem_value_get_name(const snd_ctl_elem_value_t *obj)
 {
@@ -2425,9 +2429,9 @@ const char *snd_ctl_elem_value_get_name(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Get index part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \return index part of element identifier
+ * \brief Get index part of element identifier from given data of an element.
+ * \param obj Data of an element.
+ * \return Index part of element identifier.
  */
 unsigned int snd_ctl_elem_value_get_index(const snd_ctl_elem_value_t *obj)
 {
@@ -2436,9 +2440,9 @@ unsigned int snd_ctl_elem_value_get_index(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Set CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param ptr CTL element identifier
+ * \brief Set element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param ptr Pointer to an element identifier.
  */
 void snd_ctl_elem_value_set_id(snd_ctl_elem_value_t *obj, const snd_ctl_elem_id_t *ptr)
 {
@@ -2447,9 +2451,9 @@ void snd_ctl_elem_value_set_id(snd_ctl_elem_value_t *obj, const snd_ctl_elem_id_
 }
 
 /**
- * \brief Set element numeric identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val element numeric identifier
+ * \brief Set numeric identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for numeric identifier.
  */
 void snd_ctl_elem_value_set_numid(snd_ctl_elem_value_t *obj, unsigned int val)
 {
@@ -2458,9 +2462,9 @@ void snd_ctl_elem_value_set_numid(snd_ctl_elem_value_t *obj, unsigned int val)
 }
 
 /**
- * \brief Set interface part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val interface part of element identifier
+ * \brief Set interface part of element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for interface part of element identifier.
  */
 void snd_ctl_elem_value_set_interface(snd_ctl_elem_value_t *obj, snd_ctl_elem_iface_t val)
 {
@@ -2469,9 +2473,9 @@ void snd_ctl_elem_value_set_interface(snd_ctl_elem_value_t *obj, snd_ctl_elem_if
 }
 
 /**
- * \brief Set device part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val device part of element identifier
+ * \brief Set device part of element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for device part of element identifier.
  */
 void snd_ctl_elem_value_set_device(snd_ctl_elem_value_t *obj, unsigned int val)
 {
@@ -2480,9 +2484,9 @@ void snd_ctl_elem_value_set_device(snd_ctl_elem_value_t *obj, unsigned int val)
 }
 
 /**
- * \brief Set subdevice part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val subdevice part of element identifier
+ * \brief Set subdevice part of element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for subdevice part of element identifier.
  */
 void snd_ctl_elem_value_set_subdevice(snd_ctl_elem_value_t *obj, unsigned int val)
 {
@@ -2491,9 +2495,9 @@ void snd_ctl_elem_value_set_subdevice(snd_ctl_elem_value_t *obj, unsigned int va
 }
 
 /**
- * \brief Set name part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val name part of element identifier
+ * \brief Set name part of element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for name part of element identifier,
  */
 void snd_ctl_elem_value_set_name(snd_ctl_elem_value_t *obj, const char *val)
 {
@@ -2502,9 +2506,9 @@ void snd_ctl_elem_value_set_name(snd_ctl_elem_value_t *obj, const char *val)
 }
 
 /**
- * \brief Set index part of CTL element identifier of a CTL element id/value
- * \param obj CTL element id/value
- * \param val index part of element identifier
+ * \brief Set index part of element identifier to given data of an element.
+ * \param obj Data of an element.
+ * \param val Value for index part of element identifier.
  */
 void snd_ctl_elem_value_set_index(snd_ctl_elem_value_t *obj, unsigned int val)
 {
@@ -2513,10 +2517,11 @@ void snd_ctl_elem_value_set_index(snd_ctl_elem_value_t *obj, unsigned int val)
 }
 
 /**
- * \brief Get value for an entry of a #SND_CTL_ELEM_TYPE_BOOLEAN CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \return value for the entry
+ * \brief Get value of a specified member from given data as an element of
+ *	  boolean type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \return Value for the member.
  */ 
 int snd_ctl_elem_value_get_boolean(const snd_ctl_elem_value_t *obj, unsigned int idx)
 {
@@ -2526,10 +2531,11 @@ int snd_ctl_elem_value_get_boolean(const snd_ctl_elem_value_t *obj, unsigned int
 }
 
 /**
- * \brief Get value for an entry of a #SND_CTL_ELEM_TYPE_INTEGER CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \return value for the entry
+ * \brief Get value of a specified member from given data as an element of
+ *	  integer type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \return Value for the member.
  */ 
 long snd_ctl_elem_value_get_integer(const snd_ctl_elem_value_t *obj, unsigned int idx)
 {
@@ -2539,10 +2545,11 @@ long snd_ctl_elem_value_get_integer(const snd_ctl_elem_value_t *obj, unsigned in
 }
 
 /**
- * \brief Get value for an entry of a #SND_CTL_ELEM_TYPE_INTEGER64 CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \return value for the entry
+ * \brief Get value of a specified member from given data as an element of
+ *	  integer64 type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \return Value for the member.
  */ 
 long long snd_ctl_elem_value_get_integer64(const snd_ctl_elem_value_t *obj, unsigned int idx)
 {
@@ -2552,10 +2559,11 @@ long long snd_ctl_elem_value_get_integer64(const snd_ctl_elem_value_t *obj, unsi
 }
 
 /**
- * \brief Get value for an entry of a #SND_CTL_ELEM_TYPE_ENUMERATED CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \return value for the entry
+ * \brief Get value of a specified member from given data as an element of
+ *	  enumerated type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \return Value for the member. This is an index of name set in the element.
  */ 
 unsigned int snd_ctl_elem_value_get_enumerated(const snd_ctl_elem_value_t *obj, unsigned int idx)
 {
@@ -2565,10 +2573,11 @@ unsigned int snd_ctl_elem_value_get_enumerated(const snd_ctl_elem_value_t *obj,
 }
 
 /**
- * \brief Get value for an entry of a #SND_CTL_ELEM_TYPE_BYTES CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \return value for the entry
+ * \brief Get value of a specified member from given data as an element of
+ *	  bytes type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \return Value for the member.
  */ 
 unsigned char snd_ctl_elem_value_get_byte(const snd_ctl_elem_value_t *obj, unsigned int idx)
 {
@@ -2578,10 +2587,11 @@ unsigned char snd_ctl_elem_value_get_byte(const snd_ctl_elem_value_t *obj, unsig
 }
 
 /**
- * \brief Set value for an entry of a #SND_CTL_ELEM_TYPE_BOOLEAN CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \param val value for the entry
+ * \brief Set value of a specified member to given data as an element of
+ *	  boolean type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \param val Value for the member.
  */ 
 void snd_ctl_elem_value_set_boolean(snd_ctl_elem_value_t *obj, unsigned int idx, long val)
 {
@@ -2591,10 +2601,11 @@ void snd_ctl_elem_value_set_boolean(snd_ctl_elem_value_t *obj, unsigned int idx,
 }
 
 /**
- * \brief Set value for an entry of a #SND_CTL_ELEM_TYPE_INTEGER CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \param val value for the entry
+ * \brief Set value of a specified member to given data as an element of
+ *	  integer type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \param val Value for the member.
  */ 
 void snd_ctl_elem_value_set_integer(snd_ctl_elem_value_t *obj, unsigned int idx, long val)
 {
@@ -2604,10 +2615,11 @@ void snd_ctl_elem_value_set_integer(snd_ctl_elem_value_t *obj, unsigned int idx,
 }
 
 /**
- * \brief Set value for an entry of a #SND_CTL_ELEM_TYPE_INTEGER64 CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \param val value for the entry
+ * \brief Set value of a specified member to given data as an element of
+ *	  integer64 type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \param val Value for the member.
  */ 
 void snd_ctl_elem_value_set_integer64(snd_ctl_elem_value_t *obj, unsigned int idx, long long val)
 {
@@ -2617,10 +2629,11 @@ void snd_ctl_elem_value_set_integer64(snd_ctl_elem_value_t *obj, unsigned int id
 }
 
 /**
- * \brief Set value for an entry of a #SND_CTL_ELEM_TYPE_ENUMERATED CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \param val value for the entry
+ * \brief Set value of a specified member to given data as an element of
+ * 	  enumerated type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \param val Value for the member.
  */ 
 void snd_ctl_elem_value_set_enumerated(snd_ctl_elem_value_t *obj, unsigned int idx, unsigned int val)
 {
@@ -2630,10 +2643,11 @@ void snd_ctl_elem_value_set_enumerated(snd_ctl_elem_value_t *obj, unsigned int i
 }
 
 /**
- * \brief Set value for an entry of a #SND_CTL_ELEM_TYPE_BYTES CTL element id/value 
- * \param obj CTL element id/value
- * \param idx Entry index
- * \param val value for the entry
+ * \brief Set value for a specified member to given data as an element of byte
+ *	  type.
+ * \param obj Data of an element.
+ * \param idx Index of member in the element.
+ * \param val Value for the member.
  */ 
 void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, unsigned char val)
 {
@@ -2643,10 +2657,10 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un
 }
 
 /**
- * \brief Set CTL element #SND_CTL_ELEM_TYPE_BYTES value
- * \param obj CTL handle
- * \param data Bytes value
- * \param size Size in bytes
+ * \brief Set values to given data as an element of bytes type.
+ * \param obj Data of an element.
+ * \param data Pointer for byte array.
+ * \param size The number of bytes included in the memory block.
  */
 void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size)
 {
@@ -2656,9 +2670,9 @@ void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size)
 }
 
 /**
- * \brief Get value for a #SND_CTL_ELEM_TYPE_BYTES CTL element id/value 
- * \param obj CTL element id/value
- * \return Pointer to CTL element value
+ * \brief Get memory block from given data as an element of bytes type.
+ * \param obj Data of an element.
+ * \return Pointer for byte array.
  */ 
 const void * snd_ctl_elem_value_get_bytes(const snd_ctl_elem_value_t *obj)
 {
@@ -2667,9 +2681,10 @@ const void * snd_ctl_elem_value_get_bytes(const snd_ctl_elem_value_t *obj)
 }
 
 /**
- * \brief Get value for a #SND_CTL_ELEM_TYPE_IEC958 CTL element id/value 
- * \param obj CTL element id/value
- * \param ptr Pointer to returned CTL element value
+ * \brief Get value from given data to given pointer as an element of IEC958
+ *	  type.
+ * \param obj Data of an element.
+ * \param ptr Pointer to IEC958 data.
  */ 
 void snd_ctl_elem_value_get_iec958(const snd_ctl_elem_value_t *obj, snd_aes_iec958_t *ptr)
 {
@@ -2678,9 +2693,10 @@ void snd_ctl_elem_value_get_iec958(const snd_ctl_elem_value_t *obj, snd_aes_iec9
 }
 
 /**
- * \brief Set value for a #SND_CTL_ELEM_TYPE_IEC958 CTL element id/value 
- * \param obj CTL element id/value
- * \param ptr Pointer to CTL element value
+ * \brief Set value from given pointer to given data as an element of IEC958
+ *	  type.
+ * \param obj Data of an element.
+ * \param ptr Pointer to IEC958 data.
  */ 
 void snd_ctl_elem_value_set_iec958(snd_ctl_elem_value_t *obj, const snd_aes_iec958_t *ptr)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 03/10] ctl: add functions to add an element set
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 01/10] ctl: add an overview for design of ALSA control interface Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 02/10] ctl: improve comments for handling element data Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 04/10] ctl: improve comments for API to add an element of IEC958 type Takashi Sakamoto
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

ALSA control core allows userspace applications to add an element set.
However, in ALSA userspace library, there's no APIs enough to utilize
the feature. The library has APIs just to add an element set with a single
element.

This commit adds functions to add an element set with several elements.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/control.h     |  21 +++
 src/control/control.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 413 insertions(+)

diff --git a/include/control.h b/include/control.h
index 5fdf379..d6069ff 100644
--- a/include/control.h
+++ b/include/control.h
@@ -423,6 +423,27 @@ void snd_ctl_elem_info_set_subdevice(snd_ctl_elem_info_t *obj, unsigned int val)
 void snd_ctl_elem_info_set_name(snd_ctl_elem_info_t *obj, const char *val);
 void snd_ctl_elem_info_set_index(snd_ctl_elem_info_t *obj, unsigned int val);
 
+int snd_ctl_elem_add_integer_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				 unsigned int element_count,
+				 unsigned int member_count,
+				 long min, long max, long step);
+int snd_ctl_elem_add_integer64_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				   unsigned int element_count,
+				   unsigned int member_count,
+				   long long min, long long max,
+				   long long step);
+int snd_ctl_elem_add_boolean_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				 unsigned int element_count,
+				 unsigned int member_count);
+int snd_ctl_elem_add_enumerated_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				    unsigned int element_count,
+				    unsigned int member_count,
+				    unsigned int items,
+				    const char *const labels[]);
+int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+			       unsigned int element_count,
+			       unsigned int member_count);
+
 int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long imin, long imax, long istep);
 int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long long imin, long long imax, long long istep);
 int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count);
diff --git a/src/control/control.c b/src/control/control.c
index 7d1e412..48b0080 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -281,6 +281,398 @@ int snd_ctl_elem_info(snd_ctl_t *ctl, snd_ctl_elem_info_t *info)
 }
 
 /**
+ * \brief Create and add some user-defined control elements of integer type.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of the first new element.
+ * \param element_count The number of elements added by this operation.
+ * \param member_count The number of members which a element has to
+ *			   represent its states.
+ * \param min Minimum value for each member of the elements.
+ * \param max Maximum value for each member of the elements.
+ * \param step The step of value for each member in the elements.
+ * \return Zero on success, otherwise a negative error code.
+ *
+ * This function creates some user elements with integer type. These elements
+ * are not controlled by device drivers in kernel. They can be operated by the
+ * same way as usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify new control
+ * elements. After returning, all fields of \a id are filled. A element can be
+ * identified by the combination of name and index, or by numid.
+ *
+ * All of members in the new elements are locked. The value of each member is
+ * initialized with the minimum value.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>ID has no name, or the number of members is not between 1 to 127.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user elements.
+ * <dt>-ENXIO
+ * <dd>This backend module does not support user elements of integer type.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
+ *
+ * \par Compatibility:
+ * This function is added in version 1.1.2.
+ */
+int snd_ctl_elem_add_integer_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				 unsigned int element_count,
+				 unsigned int member_count,
+				 long min, long max, long step)
+{
+	snd_ctl_elem_info_t *info;
+	snd_ctl_elem_value_t *data;
+	unsigned int i;
+	unsigned int j;
+	unsigned int numid;
+	int err;
+
+	assert(ctl && id && id->name[0]);
+
+	snd_ctl_elem_info_alloca(&info);
+	info->id = *id;
+	info->type = SND_CTL_ELEM_TYPE_INTEGER;
+	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_USER;
+	info->owner = element_count;
+	info->count = member_count;
+	info->value.integer.min = min;
+	info->value.integer.max = max;
+	info->value.integer.step = step;
+
+	err = ctl->ops->element_add(ctl, info);
+	if (err < 0)
+		return err;
+	numid = snd_ctl_elem_id_get_numid(&info->id);
+
+	/* Set initial value to all of members in all of added elements. */
+	snd_ctl_elem_value_alloca(&data);
+	data->id = info->id;
+	for (i = 0; i < element_count; i++) {
+		snd_ctl_elem_id_set_numid(&data->id, numid + i);
+
+		for (j = 0; j < member_count; j++)
+			data->value.integer.value[j] = min;
+
+		err = ctl->ops->element_write(ctl, data);
+		if (err < 0)
+			return err;
+	}
+
+	*id = info->id;
+	return 0;
+}
+
+/**
+ * \brief Create and add some user-defined control elements of integer64 type.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of the first new control element.
+ * \param element_count The number of elements added by this operation.
+ * \param member_count The number of members which a element has to
+ *	    	   represent its states.
+ * \param min Minimum value for each member of the elements.
+ * \param max Maximum value for each member of the elements.
+ * \param step The step of value for each member in the elements.
+ * \return Zero on success, otherwise a negative error code.
+ *
+ * This function creates some user elements with integer64 type. These elements
+ * are not controlled by device drivers in kernel. They can be operated by the
+ * same way as usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify new control
+ * elements. After returning, all fields of \a id are filled. A element can be
+ * identified by the combination of name and index, or by numid.
+ *
+ * All of members in the new elements are locked. The value of each member is
+ * initialized with the minimum value.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>ID has no name, or the number of members is not between 1 to 127.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user elements.
+ * <dt>-ENXIO
+ * <dd>This backend module does not support user elements of integer64 type.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
+ *
+ * \par Compatibility:
+ * This function is added in version 1.1.2.
+ */
+int snd_ctl_elem_add_integer64_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				   unsigned int element_count,
+				   unsigned int member_count,
+				   long long min, long long max, long long step)
+{
+	snd_ctl_elem_info_t *info;
+	snd_ctl_elem_value_t *data;
+	unsigned int i;
+	unsigned int j;
+	unsigned int numid;
+	int err;
+
+	assert(ctl && id && id->name[0]);
+
+	snd_ctl_elem_info_alloca(&info);
+	info->id = *id;
+	info->type = SND_CTL_ELEM_TYPE_INTEGER64;
+	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_USER;
+	info->owner = element_count;
+	info->count = member_count;
+	info->value.integer64.min = min;
+	info->value.integer64.max = max;
+	info->value.integer64.step = step;
+
+	err = ctl->ops->element_add(ctl, info);
+	if (err < 0)
+		return err;
+	numid = snd_ctl_elem_id_get_numid(&info->id);
+
+	/* Set initial value to all of members in all of added elements. */
+	snd_ctl_elem_value_alloca(&data);
+	data->id = info->id;
+	for (i = 0; i < element_count; i++) {
+		snd_ctl_elem_id_set_numid(&data->id, numid + i);
+
+		for (j = 0; j < member_count; j++)
+			data->value.integer64.value[j] = min;
+
+		err = ctl->ops->element_write(ctl, data);
+		if (err < 0)
+			return err;
+	}
+
+	*id = info->id;
+	return 0;
+}
+
+/**
+ * \brief Create and add some user-defined control elements of boolean type.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of the new control element.
+ * \param element_count The number of elements added by this operation.
+ * \param member_count The number of members which a element has to
+ *			   represent its states.
+ *
+ * This function creates some user elements with boolean type. These elements
+ * are not controlled by device drivers in kernel. They can be operated by the
+ * same way as usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify new control
+ * elements. After returning, all fields of \a id are filled. A element can be
+ * identified by the combination of name and index, or by numid.
+ *
+ * All of members in the new elements are locked. The value of each member is
+ * initialized with false.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>ID has no name, or the number of members is not between 1 to 127.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user elements.
+ * <dt>-ENXIO
+ * <dd>This backend module does not support user elements of boolean type.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
+ *
+ * \par Compatibility:
+ * This function is added in version 1.1.2.
+ */
+int snd_ctl_elem_add_boolean_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				 unsigned int element_count,
+				 unsigned int member_count)
+{
+	snd_ctl_elem_info_t *info;
+	int err;
+
+	assert(ctl && id && id->name[0]);
+
+	snd_ctl_elem_info_alloca(&info);
+	info->id = *id;
+	info->type = SND_CTL_ELEM_TYPE_BOOLEAN;
+	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_USER;
+	info->owner = element_count;
+	info->count = member_count;
+	info->value.integer.min = 0;
+	info->value.integer.max = 1;
+
+	err = ctl->ops->element_add(ctl, info);
+	if (err >= 0)
+		*id = info->id;
+
+	return err;
+}
+
+/**
+ * \brief Create and add some user-defined control elements of enumerated type.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of the first new element.
+ * \param element_count The number of elements added by this operation.
+ * \param member_count The number of members which a element has to
+ *	    	   represent its states.
+ * \param items Range of possible values (0 ... \a items - 1).
+ * \param labels An array containing \a items strings.
+ * \return Zero on success, otherwise a negative error code.
+ *
+ * This function creates some user elements with enumerated type. These elements
+ * are not controlled by device drivers in kernel. They can be operated by the
+ * same way as usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify new control
+ * elements. After returning, all fields of \a id are filled. A element can be
+ * identified by the combination of name and index, or by numid.
+ *
+ * All of members in the new elements are locked. The value of each member is
+ * initialized with the first entry of labels.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A control element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>\a element_count is not between 1 to 127, or \a items is not at least
+ *     one, or a string in \a labels is empty, or longer than 63 bytes, or total
+ *     length of the labels requires more than 64 KiB storage.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user control elements.
+ * <dt>-ENXIO
+ * <dd>This driver does not support (enumerated) user controls.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
+ *
+ * \par Compatibility:
+ * This function is added in version 1.1.2.
+ */
+int snd_ctl_elem_add_enumerated_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+				    unsigned int element_count,
+				    unsigned int member_count,
+				    unsigned int items,
+				    const char *const labels[])
+{
+	snd_ctl_elem_info_t *info;
+	unsigned int i, bytes;
+	char *buf, *p;
+	int err;
+
+	assert(ctl && id && id->name[0] && labels);
+
+	snd_ctl_elem_info_alloca(&info);
+	info->id = *id;
+	info->type = SND_CTL_ELEM_TYPE_ENUMERATED;
+	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_USER;
+	info->owner = element_count;
+	info->count = member_count;
+	info->value.enumerated.items = items;
+
+	bytes = 0;
+	for (i = 0; i < items; ++i)
+		bytes += strlen(labels[i]) + 1;
+	if (bytes == 0)
+		return -EINVAL;
+	buf = malloc(bytes);
+	if (buf == NULL)
+		return -ENOMEM;
+	info->value.enumerated.names_ptr = (uintptr_t)buf;
+	info->value.enumerated.names_length = bytes;
+	p = buf;
+	for (i = 0; i < items; ++i) {
+		strcpy(p, labels[i]);
+		p += strlen(labels[i]) + 1;
+	}
+
+	err = ctl->ops->element_add(ctl, info);
+	if (err >= 0)
+		*id = info->id;
+
+	free(buf);
+
+	return err;
+}
+
+/**
+ * \brief Create and add some user-defined control elements of bytes type.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of the first new element.
+ * \param element_count The number of elements added by this operation.
+ * \param member_count The number of members which a element has to
+ *			   represent its states.
+ * \return Zero on success, otherwise a negative error code.
+ *
+ * This function creates some user elements with bytes type. These elements are
+ * not controlled by device drivers in kernel. They can be operated by the same
+ * way as usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify new control
+ * elements. After returning, all fields of \a id are filled. A element can be
+ * identified by the combination of name and index, or by numid.
+ *
+ * All of members in the new elements are locked. The value of each member is
+ * initialized with the minimum value.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>ID has no name, or the number of members is not between 1 to 511.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user elements.
+ * <dt>-ENXIO
+ * <dd>This backend module does not support user elements of bytes type.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
+ *
+ * \par Compatibility:
+ * This function is added in version 1.1.2.
+ */
+int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
+			       unsigned int element_count,
+			       unsigned int member_count)
+{
+	snd_ctl_elem_info_t *info;
+	int err;
+
+	assert(ctl && id && id->name[0]);
+
+	snd_ctl_elem_info_alloca(&info);
+	info->id = *id;
+	info->type = SND_CTL_ELEM_TYPE_BYTES;
+	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		       SNDRV_CTL_ELEM_ACCESS_USER;
+	info->owner = element_count;
+	info->count = member_count;
+
+	err = ctl->ops->element_add(ctl, info);
+	if (err >= 0)
+		*id = info->id;
+
+	return err;
+}
+
+/**
  * \brief Create and add an user INTEGER CTL element
  * \param ctl CTL handle
  * \param id CTL element id to add
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 04/10] ctl: improve comments for API to add an element of IEC958 type
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (2 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 03/10] ctl: add functions to add an element set Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 05/10] ctl: change former APIs as wrapper functions of element set APIs Takashi Sakamoto
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

In ALSA control core, it's not allowed to an element set of IEC 958 type
to have several elements. Therefore, consecutive patchset doesn't touch an
API to add an element of IEC958 type. However, it's better to supplement
comments for the API.

This commit do it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/control/control.c b/src/control/control.c
index 48b0080..9fad0c0 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -842,10 +842,33 @@ int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 }
 
 /**
- * \brief Create and add an user IEC958 CTL element
- * \param ctl CTL handle
- * \param id CTL element info to add
- * \return 0 on success otherwise a negative error code
+ * \brief Create and add a user-defined control element of IEC958 type.
+ * \param[in] ctl A handle of backend module for control interface.
+ * \param[in,out] id ID of the new control element.
+ *
+ * This function creates an user element with IEC958 type. This element is not
+ * controlled by device drivers in kernel. It can be operated by the same way as
+ * usual elements added by the device drivers.
+ *
+ * The name field of \a id must be set with unique value to identify a new
+ * control element. After returning, all fields of \a id are filled. A element
+ * can be identified by the combination of name and index, or by numid.
+ *
+ * A member in the new element is locked and filled with zero.
+ *
+ * \par Errors:
+ * <dl>
+ * <dt>-EBUSY
+ * <dd>A control element with ID \a id already exists.
+ * <dt>-EINVAL
+ * <dd>ID has no name.
+ * <dt>-ENOMEM
+ * <dd>Out of memory, or there are too many user elements.
+ * <dt>-ENXIO
+ * <dd>This backend module does not support user elements of IEC958 type.
+ * <dt>-ENODEV
+ * <dd>Device unplugged.
+ * </dl>
  */
 int snd_ctl_elem_add_iec958(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id)
 {
@@ -855,6 +878,7 @@ int snd_ctl_elem_add_iec958(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id)
 	snd_ctl_elem_info_alloca(&info);
 	info->id = *id;
 	info->type = SND_CTL_ELEM_TYPE_IEC958;
+	info->owner = 1;
 	info->count = 1;
 	return ctl->ops->element_add(ctl, info);
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 05/10] ctl: change former APIs as wrapper functions of element set APIs
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (3 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 04/10] ctl: improve comments for API to add an element of IEC958 type Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element Takashi Sakamoto
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

In former commit, userspace library gets some APIs for element set. Some
existed functions can be simple wrappers of them.

This commit changes these APIs as wrapper functions. Additionally, this
commit also adds local variables for identical information of elements.
This modification is important to keep API consistency.

Some old APIs to add an element have id variables with const type
qualifier, while some new APIs to add element set changes the content of
given parameters. This comes from a change in Linux kernel 4.1.

In this commit [1], in-kernel implementation fills all fields of identical
data in userspace. As a result, when adding a new element set, userspace
applications can get an identical data for the first element in the set.
This lost the semantics of const type qualifier in userspace.

[1] http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/core?id=cab2ed7474bffafd2a68a885e03b85526194abcd

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 183 +++++++++++++-------------------------------------
 1 file changed, 45 insertions(+), 138 deletions(-)

diff --git a/src/control/control.c b/src/control/control.c
index 9fad0c0..fbc5e18 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -673,172 +673,79 @@ int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
 }
 
 /**
- * \brief Create and add an user INTEGER CTL element
- * \param ctl CTL handle
- * \param id CTL element id to add
- * \param count number of elements
- * \param min minimum value
- * \param max maximum value
- * \param step value step
- * \return 0 on success otherwise a negative error code
+ * \brief Create and add an user-defined control element of integer type.
+ *
+ * This function is a wrapper function to snd_ctl_elem_add_integer_set() for
+ * single control element.
  */
 int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
-			     unsigned int count, long min, long max, long step)
+			     unsigned int member_count,
+			     long min, long max, long step)
 {
-	snd_ctl_elem_info_t *info;
-	snd_ctl_elem_value_t *val;
-	unsigned int i;
-	int err;
+	snd_ctl_elem_id_t *local_id;
 
-	assert(ctl && id && id->name[0]);
-	snd_ctl_elem_info_alloca(&info);
-	info->id = *id;
-	info->type = SND_CTL_ELEM_TYPE_INTEGER;
-	info->access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
-		SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE;
-	info->count = count;
-	info->value.integer.min = min;
-	info->value.integer.max = max;
-	info->value.integer.step = step;
-	err = ctl->ops->element_add(ctl, info);
-	if (err < 0)
-		return err;
-	snd_ctl_elem_value_alloca(&val);
-	val->id = info->id;
-	for (i = 0; i < count; i++)
-		val->value.integer.value[i] = min;
-	err = ctl->ops->element_write(ctl, val);
-	return err;
+	snd_ctl_elem_id_alloca(&local_id);
+	*local_id = *id;
+
+	return snd_ctl_elem_add_integer_set(ctl, local_id, 1, member_count,
+					    min, max, step);
 }
 
 /**
- * \brief Create and add an user INTEGER64 CTL element
- * \param ctl CTL handle
- * \param id CTL element id to add
- * \param count number of elements
- * \param min minimum value
- * \param max maximum value
- * \param step value step
- * \return 0 on success otherwise a negative error code
+ * \brief Create and add an user-defined control element of integer64 type.
+ *
+ * This function is a wrapper function to snd_ctl_elem_add_integer64_set() for
+ * single control element.
  */
 int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
-			       unsigned int count, long long min, long long max,
-			       long long step)
+			       unsigned int member_count,
+			       long long min, long long max, long long step)
 {
-	snd_ctl_elem_info_t *info;
-	snd_ctl_elem_value_t *val;
-	unsigned int i;
-	int err;
+	snd_ctl_elem_id_t *local_id;
 
-	assert(ctl && id && id->name[0]);
-	snd_ctl_elem_info_alloca(&info);
-	info->id = *id;
-	info->type = SND_CTL_ELEM_TYPE_INTEGER64;
-	info->count = count;
-	info->value.integer64.min = min;
-	info->value.integer64.max = max;
-	info->value.integer64.step = step;
-	err = ctl->ops->element_add(ctl, info);
-	if (err < 0)
-		return err;
-	snd_ctl_elem_value_alloca(&val);
-	val->id = info->id;
-	for (i = 0; i < count; i++)
-		val->value.integer64.value[i] = min;
-	err = ctl->ops->element_write(ctl, val);
-	return err;
+	snd_ctl_elem_id_alloca(&local_id);
+	*local_id = *id;
+
+	return snd_ctl_elem_add_integer64_set(ctl, local_id, 1, member_count,
+					      min, max, step);
 }
 
 /**
- * \brief Create and add an user BOOLEAN CTL element
- * \param ctl CTL handle
- * \param id CTL element id to add
- * \param count number of elements
- * \return 0 on success otherwise a negative error code
+ * \brief Create and add an user-defined control element of boolean type.
+ *
+ * This function is a wrapper function to snd_ctl_elem_add_boolean_set() for
+ * single control element.
  */
 int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
-			     unsigned int count)
+			     unsigned int member_count)
 {
-	snd_ctl_elem_info_t *info;
+	snd_ctl_elem_id_t *local_id;
 
-	assert(ctl && id && id->name[0]);
-	snd_ctl_elem_info_alloca(&info);
-	info->id = *id;
-	info->type = SND_CTL_ELEM_TYPE_BOOLEAN;
-	info->count = count;
-	info->value.integer.min = 0;
-	info->value.integer.max = 1;
-	return ctl->ops->element_add(ctl, info);
+	snd_ctl_elem_id_alloca(&local_id);
+	*local_id = *id;
+
+	return snd_ctl_elem_add_boolean_set(ctl, local_id, 1, member_count);
 }
 
 /**
- * \brief Create and add a user-defined control element of type enumerated.
- * \param[in] ctl Control device handle.
- * \param[in] id ID of the new control element.
- * \param[in] count Number of element values.
- * \param[in] items Range of possible values (0 ... \a items - 1).
- * \param[in] names An array containing \a items strings.
- * \return Zero on success, otherwise a negative error code.
+ * \brief Create and add a user-defined control element of enumerated type.
  *
- * This function creates a user element, i.e., a control element that is not
- * controlled by the control device's driver but that is just stored together
- * with the other elements of \a ctl.
+ * This function is a wrapper function to snd_ctl_elem_add_enumerated_set() for
+ * single control element.
  *
- * The fields of \a id, except numid, must be set to unique values that
- * identify the new element.
- *
- * The new element is locked; its value is initialized as zero.
- *
- * \par Errors:
- * <dl>
- * <dt>-EBUSY<dd>A control element with ID \a id already exists.
- * <dt>-EINVAL<dd>\a count is not at least one or greater than 128, or \a items
- * 	is not at least one, or a string in \a names is empty or longer than 63
- * 	bytes, or the strings in \a names require more than 64 KB storage.
- * <dt>-ENOMEM<dd>Out of memory, or there are too many user control elements.
- * <dt>-ENXIO<dd>This driver does not support (enumerated) user controls.
- * <dt>-ENODEV<dd>Device unplugged.
- * </dl>
- *
- * \par Compatibility:
- * snd_ctl_elem_add_enumerated() was introduced in ALSA 1.0.25.
+ * This function is added in version 1.0.25.
  */
 int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
-				unsigned int count, unsigned int items,
-				const char *const names[])
+				unsigned int member_count, unsigned int items,
+				const char *const labels[])
 {
-	snd_ctl_elem_info_t *info;
-	unsigned int i, bytes;
-	char *buf, *p;
-	int err;
-
-	assert(ctl && id && id->name[0] && names);
-
-	snd_ctl_elem_info_alloca(&info);
-	info->id = *id;
-	info->type = SND_CTL_ELEM_TYPE_ENUMERATED;
-	info->count = count;
-	info->value.enumerated.items = items;
-
-	bytes = 0;
-	for (i = 0; i < items; ++i)
-		bytes += strlen(names[i]) + 1;
-	buf = bytes ? malloc(bytes) : NULL;
-	if (!buf)
-		return -ENOMEM;
-	info->value.enumerated.names_ptr = (uintptr_t)buf;
-	info->value.enumerated.names_length = bytes;
-	p = buf;
-	for (i = 0; i < items; ++i) {
-		strcpy(p, names[i]);
-		p += strlen(names[i]) + 1;
-	}
+	snd_ctl_elem_id_t *local_id;
 
-	err = ctl->ops->element_add(ctl, info);
+	snd_ctl_elem_id_alloca(&local_id);
+	*local_id = *id;
 
-	free(buf);
-
-	return err;
+	return snd_ctl_elem_add_enumerated_set(ctl, local_id, 1, member_count,
+					       items, labels);
 }
 
 /**
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (4 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 05/10] ctl: change former APIs as wrapper functions of element set APIs Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-13 12:51   ` Takashi Iwai
  2016-06-12  8:16 ` [PATCH 07/10] pcm: use new APIs to add a control element set for softvol plugin Takashi Sakamoto
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

When comparing old APIs (to add a single element) with new APIs (to add
an element set), the latter has an benefit to get full identical
information for a first element in the element set. Furthermore, in
previous commit, the old APIs become simple wrappers to the new APIs.
Therefore, there's few intentions to use the old APIs.

This commit deprecates the old APIs to encourage the usage of new APIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/control.h     | 17 ++++++++++++-----
 src/control/control.c | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/control.h b/include/control.h
index d6069ff..e21a58a 100644
--- a/include/control.h
+++ b/include/control.h
@@ -443,14 +443,21 @@ int snd_ctl_elem_add_enumerated_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
 int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
 			       unsigned int element_count,
 			       unsigned int member_count);
-
-int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long imin, long imax, long istep);
-int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long long imin, long long imax, long long istep);
-int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count);
-int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, unsigned int items, const char *const names[]);
 int snd_ctl_elem_add_iec958(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id);
 int snd_ctl_elem_remove(snd_ctl_t *ctl, snd_ctl_elem_id_t *id);
 
+int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
+		unsigned int count, long imin, long imax,
+		long istep) __attribute__((deprecated));
+int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
+		unsigned int count, long long imin, long long imax,
+		long long istep) __attribute__((deprecated));
+int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
+		unsigned int count) __attribute__((deprecated));
+int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
+		unsigned int count, unsigned int items,
+		const char *const names[]) __attribute__((deprecated));
+
 size_t snd_ctl_elem_value_sizeof(void);
 /** \hideinitializer
  * \brief allocate an invalid #snd_ctl_elem_value_t using standard alloca
diff --git a/src/control/control.c b/src/control/control.c
index fbc5e18..6893012 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -677,6 +677,8 @@ int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
  *
  * This function is a wrapper function to snd_ctl_elem_add_integer_set() for
  * single control element.
+ *
+ * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer_set(), instead.
  */
 int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 			     unsigned int member_count,
@@ -690,12 +692,17 @@ int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 	return snd_ctl_elem_add_integer_set(ctl, local_id, 1, member_count,
 					    min, max, step);
 }
+link_warning(snd_ctl_elem_add_integer,
+	     "Warning: snd_ctl_elem_add_integer is deprecated, "
+	     "Use snd_ctl_elem_add_integer_set, instead.");
 
 /**
  * \brief Create and add an user-defined control element of integer64 type.
  *
  * This function is a wrapper function to snd_ctl_elem_add_integer64_set() for
  * single control element.
+ *
+ * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer64_set(), instead.
  */
 int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 			       unsigned int member_count,
@@ -709,12 +716,17 @@ int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 	return snd_ctl_elem_add_integer64_set(ctl, local_id, 1, member_count,
 					      min, max, step);
 }
+link_warning(snd_ctl_elem_add_integer64,
+	     "Warning: snd_ctl_elem_add_integer64 is deprecated, "
+	     "Use snd_ctl_elem_add_integer64_set, instead.");
 
 /**
  * \brief Create and add an user-defined control element of boolean type.
  *
  * This function is a wrapper function to snd_ctl_elem_add_boolean_set() for
  * single control element.
+ *
+ * \deprecated Since 1.1.2. Use snd_ctl_elem_add_boolean_set(), instead.
  */
 int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 			     unsigned int member_count)
@@ -726,6 +738,9 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 
 	return snd_ctl_elem_add_boolean_set(ctl, local_id, 1, member_count);
 }
+link_warning(snd_ctl_elem_add_boolean,
+	     "Warning: snd_ctl_elem_add_boolean is deprecated, "
+	     "Use snd_ctl_elem_add_boolean_set, instead.");
 
 /**
  * \brief Create and add a user-defined control element of enumerated type.
@@ -734,6 +749,8 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
  * single control element.
  *
  * This function is added in version 1.0.25.
+ *
+ * \deprecated Since 1.1.2. Use snd_ctl_elem_add_enumerated_set(), instead.
  */
 int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 				unsigned int member_count, unsigned int items,
@@ -747,6 +764,9 @@ int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 	return snd_ctl_elem_add_enumerated_set(ctl, local_id, 1, member_count,
 					       items, labels);
 }
+link_warning(snd_ctl_elem_add_enumerated,
+	     "Warning: snd_ctl_elem_add_enumerated is deprecated, "
+	     "Use snd_ctl_elem_add_enumerated_set, instead.");
 
 /**
  * \brief Create and add a user-defined control element of IEC958 type.
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 07/10] pcm: use new APIs to add a control element set for softvol plugin
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (5 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 08/10] ctl: add explaination about threshold level feature Takashi Sakamoto
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

In previous commit, some APIs to add a single element are deprecated.

This commit replaces the usage of the old APIs with new APIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_softvol.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/pcm/pcm_softvol.c b/src/pcm/pcm_softvol.c
index 5492db8..459ff8e 100644
--- a/src/pcm/pcm_softvol.c
+++ b/src/pcm/pcm_softvol.c
@@ -670,10 +670,11 @@ static int add_user_ctl(snd_pcm_softvol_t *svol, snd_ctl_elem_info_t *cinfo, int
 	unsigned int def_val;
 	
 	if (svol->max_val == 1)
-		err = snd_ctl_elem_add_boolean(svol->ctl, &cinfo->id, count);
+		err = snd_ctl_elem_add_boolean_set(svol->ctl, &cinfo->id, 1,
+						   count);
 	else
-		err = snd_ctl_elem_add_integer(svol->ctl, &cinfo->id, count,
-					       0, svol->max_val, 0);
+		err = snd_ctl_elem_add_integer_set(svol->ctl, &cinfo->id, 1,
+						   count, 0, svol->max_val, 0);
 	if (err < 0)
 		return err;
 	if (svol->max_val == 1)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 08/10] ctl: add explaination about threshold level feature
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (6 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 07/10] pcm: use new APIs to add a control element set for softvol plugin Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 09/10] ctl: improve API documentation for threshold level operations Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 10/10] ctl: add test program for control element set Takashi Sakamoto
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

ALSA ctl feature includes threshold level feature. This is introduced in
2006, and there's little resources about it.

This commit adds a simple explanation about the feature.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/control/control.c b/src/control/control.c
index 6893012..1039dc2 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -58,6 +58,28 @@ elements included in the element set.
 
 When the value of member is changed, corresponding events are transferred to
 userspace applications. The applications should subscribe any events in advance.
+
+\section tlv_topology Thredshold level and topology framework
+
+TLV feature is designed to transfer data about threshold level between a driver
+and any userspace applications. The data is for an element set.
+
+At first, this feature was implemented to add pre-defined data readable to
+userspace applications. Soon, it was extended to handle several operations;
+read, write and command. The original implementation remains as the read
+operation. The command operation allows drivers to have own implementations
+against requests from userspace applications. As of 2016, simple write operation
+is not supported yet.
+
+This feature was introduced to ALSA control feature in 2006, at commit
+c7a0708a2362, corresponding to a series of work for Linux kernel (42750b04c5ba
+and 8aa9b586e420).
+
+This feature can transfer arbitrary data in a shape of an array with members of
+unsigned int type. Focusing on this attribute, this feature is utilized as a
+basin of topology framework introduced in 2015. This framework is mainly used to
+describe widget graph of HDA codecs so that ASoC HDA driver can add control
+element sets without retrieve the data from HDA codecs via HDA controller.
 */
 
 #include <stdio.h>
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 09/10] ctl: improve API documentation for threshold level operations
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (7 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 08/10] ctl: add explaination about threshold level feature Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  2016-06-12  8:16 ` [PATCH 10/10] ctl: add test program for control element set Takashi Sakamoto
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

In alsa-lib, threshold level operations require an array of unsigned-int
members, while there's little explanation about how to fill it. To usual
developers such as me, they're quite hard to understand.

This commit adds a few comment for easy understanding about how to use
the APIs.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/control/control.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/control/control.c b/src/control/control.c
index 1039dc2..60b891d 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -898,14 +898,12 @@ static int snd_ctl_tlv_do(snd_ctl_t *ctl, int op_flag,
 	return err;
 }
 
-
-
 /**
- * \brief Get CTL element TLV value
- * \param ctl CTL handle
- * \param id CTL element id pointer
- * \param tlv TLV array pointer to store 
- * \param tlv_size TLV array size in bytes
+ * \brief Set given data to an element as threshold level.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of an element.
+ * \param tlv An array with members of unsigned int type.
+ * \param tlv_size The length of the array.
  * \return 0 on success otherwise a negative error code
  */
 int snd_ctl_elem_tlv_read(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
@@ -929,10 +927,11 @@ int snd_ctl_elem_tlv_read(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 }
 
 /**
- * \brief Set CTL element TLV value
- * \param ctl CTL handle
- * \param id CTL element id pointer
- * \param tlv TLV array pointer to store 
+ * \brief Set given data to an element as threshold level.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of an element.
+ * \param tlv An array with members of unsigned int type. The second member
+ *	      must represent total bytes of the rest of array.
  * \retval 0 on success
  * \retval >0 on success when value was changed
  * \retval <0 a negative error code
@@ -945,10 +944,11 @@ int snd_ctl_elem_tlv_write(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
 }
 
 /**
- * \brief Process CTL element TLV command
- * \param ctl CTL handle
- * \param id CTL element id pointer
- * \param tlv TLV array pointer to process
+ * \brief Set given data to an element as threshold level.
+ * \param ctl A handle of backend module for control interface.
+ * \param id ID of an element.
+ * \param tlv An array with members of unsigned int type. The second member
+ *	      must represent total bytes of the rest of array.
  * \retval 0 on success
  * \retval >0 on success when value was changed
  * \retval <0 a negative error code
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 10/10] ctl: add test program for control element set
  2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
                   ` (8 preceding siblings ...)
  2016-06-12  8:16 ` [PATCH 09/10] ctl: improve API documentation for threshold level operations Takashi Sakamoto
@ 2016-06-12  8:16 ` Takashi Sakamoto
  9 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-12  8:16 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

The feature of control element set has been abandoned for a long time since
firstly introduced in 2003 (approx). Furthermore, there's few applications
to utilize this feature. These situations bring a hard work to the persons
who need the feature. Especially, a lack of test program make it harder to
fix much bugs in this feature.

This commit adds a test program as a sample of the feature. This program
adds element sets of each element type in order; boolean, integer,
enumerated, bytes, IEC958 and integer64. Each iteration includes below
scheme:

1. add an element set with 900 elements. Each of them has maximum number
   of members allowed by ALSA ctl core.
2. check all of events generated by above operation.
3. retrieve information of each element, then validate it.
4. unlock each member of all elements because they're initially locked.
5. write to all of members in all elements and read.
6. check all of events generated by above operation.
7. write information for threshold level to the element set and read it.
8. check all of events generated by above operation.
9. remove the element set.
10.check all of events generated by above operation.

When any of these operations fail, it means regression occurs. Then, added
elements still remain in a certain sound card. In this case, unloading
drivers corresponding to the card is an easy way to recover.

Besides, this program doesn't perform below element operations of ALSA ctl
feature:
 - list
 - lock
 - replace

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 test/Makefile.am            |   5 +-
 test/user-ctl-element-set.c | 561 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 565 insertions(+), 1 deletion(-)
 create mode 100644 test/user-ctl-element-set.c

diff --git a/test/Makefile.am b/test/Makefile.am
index ce6e521..cbf2ab4 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -3,7 +3,7 @@ SUBDIRS=. lsb
 check_PROGRAMS=control pcm pcm_min latency seq \
 	       playmidi1 timer rawmidi midiloop \
 	       oldapi queue_timer namehint client_event_filter \
-	       chmap audio_time
+	       chmap audio_time user-ctl-element-set
 
 control_LDADD=../src/libasound.la
 pcm_LDADD=../src/libasound.la
@@ -24,6 +24,9 @@ code_CFLAGS=-Wall -pipe -g -O2
 chmap_LDADD=../src/libasound.la
 audio_time_LDADD=../src/libasound.la
 
+user_ctl_element_set_LDADD=../src/libasound.la
+user_ctl_element_set_CFLAGS=-Wall -g
+
 AM_CPPFLAGS=-I$(top_srcdir)/include
 AM_CFLAGS=-Wall -pipe -g
 
diff --git a/test/user-ctl-element-set.c b/test/user-ctl-element-set.c
new file mode 100644
index 0000000..0ba2be1
--- /dev/null
+++ b/test/user-ctl-element-set.c
@@ -0,0 +1,561 @@
+/*
+ * user-control-element-set.c - a program to test in-kernel implementation of
+ *                              user-defined control element set.
+ *
+ * Copyright (c) 2015-2016 Takashi Sakamoto
+ *
+ * Licensed under the terms of the GNU General Public License, version 2.
+ */
+
+#include "../include/asoundlib.h"
+
+struct elem_set_trial {
+    snd_ctl_t *handle;
+
+    snd_ctl_elem_type_t type;
+    unsigned int member_count;
+    unsigned int element_count;
+
+    snd_ctl_elem_id_t *id;
+
+    int (*add_elem_set)(struct elem_set_trial *trial);
+    int (*check_elem_props)(struct elem_set_trial *trial,
+                            snd_ctl_elem_info_t *info);
+    void (*change_elem_members)(struct elem_set_trial *trial,
+                                snd_ctl_elem_value_t *elem_data);
+};
+
+/* Operations for elements in an element set with boolean type. */
+static int add_bool_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_boolean_set(trial->handle, trial->id,
+                                        trial->element_count,
+                                        trial->member_count);
+}
+
+static void change_bool_elem_members(struct elem_set_trial *trial,
+                                     snd_ctl_elem_value_t *elem_data)
+{
+    int val;
+    unsigned int i;
+
+    for (i = 0; i < trial->member_count; ++i) {
+        val = snd_ctl_elem_value_get_boolean(elem_data, i);
+        snd_ctl_elem_value_set_boolean(elem_data, i, !val);
+    }
+}
+
+/* Operations for elements in an element set with integer type. */
+static int add_int_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_integer_set(trial->handle, trial->id,
+                                        trial->element_count,
+                                        trial->member_count, 0, 99, 1);
+}
+
+static int check_int_elem_props(struct elem_set_trial *trial,
+                                snd_ctl_elem_info_t *info)
+{
+    if (snd_ctl_elem_info_get_min(info) != 0)
+        return -EIO;
+    if (snd_ctl_elem_info_get_max(info) != 99)
+        return -EIO;
+    if (snd_ctl_elem_info_get_step(info) != 1)
+        return -EIO;
+
+    return 0;
+}
+
+static void change_int_elem_members(struct elem_set_trial *trial,
+                                    snd_ctl_elem_value_t *elem_data)
+{
+    long val;
+    unsigned int i;
+
+    for (i = 0; i < trial->member_count; ++i) {
+        val = snd_ctl_elem_value_get_integer(elem_data, i);
+        snd_ctl_elem_value_set_integer(elem_data, i, ++val);
+    }
+}
+
+/* Operations for elements in an element set with enumerated type. */
+static const char *const labels[] = {
+    "trusty",
+    "utopic",
+    "vivid",
+    "willy",
+    "xenial",
+};
+
+static int add_enum_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_enumerated_set(trial->handle, trial->id,
+                                           trial->element_count,
+                                           trial->member_count,
+                                           sizeof(labels) / sizeof(labels[0]),
+                                           labels);
+}
+
+static int check_enum_elem_props(struct elem_set_trial *trial,
+                                 snd_ctl_elem_info_t *info)
+{
+    unsigned int items;
+    unsigned int i;
+    const char *label;
+    int err;
+
+    items = snd_ctl_elem_info_get_items(info);
+    if (items != sizeof(labels) / sizeof(labels[0]))
+        return -EIO;
+
+    /* Enumerate and validate all of labels registered to this element. */
+    for (i = 0; i < items; ++i) {
+        snd_ctl_elem_info_set_item(info, i);
+        err = snd_ctl_elem_info(trial->handle, info);
+        if (err < 0)
+            return err;
+
+        label = snd_ctl_elem_info_get_item_name(info);
+        if (strncmp(label, labels[i], strlen(labels[i])) != 0)
+            return -EIO;
+    }
+
+    return 0;
+}
+
+static void change_enum_elem_members(struct elem_set_trial *trial,
+                                     snd_ctl_elem_value_t *elem_data)
+{
+    unsigned int val;
+    unsigned int i;
+
+    for (i = 0; i < trial->member_count; ++i) {
+        val = snd_ctl_elem_value_get_enumerated(elem_data, i);
+        snd_ctl_elem_value_set_enumerated(elem_data, i, ++val);
+    }
+}
+
+/* Operations for elements in an element set with bytes type. */
+static int add_bytes_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_bytes_set(trial->handle, trial->id,
+                                      trial->element_count,
+                                      trial->member_count);
+}
+
+static void change_bytes_elem_members(struct elem_set_trial *trial,
+                                      snd_ctl_elem_value_t *elem_data)
+{
+    unsigned char val;
+    unsigned int i;
+
+    for (i = 0; i < trial->member_count; ++i) {
+        val = snd_ctl_elem_value_get_byte(elem_data, i);
+        snd_ctl_elem_value_set_byte(elem_data, i, ++val);
+    }
+}
+
+/* Operations for elements in an element set with iec958 type. */
+static int add_iec958_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_iec958(trial->handle, trial->id);
+}
+
+static void change_iec958_elem_members(struct elem_set_trial *trial,
+                                       snd_ctl_elem_value_t *elem_data)
+{
+    snd_aes_iec958_t data;
+
+    /* To suppress GCC warnings. */
+    trial->element_count = 1;
+
+    snd_ctl_elem_value_get_iec958(elem_data, &data);
+    /* This is an arbitrary number. */
+    data.pad = 10;
+    snd_ctl_elem_value_set_iec958(elem_data, &data);
+}
+
+/* Operations for elements in an element set with integer64 type. */
+static int add_int64_elem_set(struct elem_set_trial *trial)
+{
+    return snd_ctl_elem_add_integer64_set(trial->handle, trial->id,
+                                          trial->element_count,
+                                          trial->member_count, 100, 10000, 30);
+}
+
+static int check_int64_elem_props(struct elem_set_trial *trial,
+                                  snd_ctl_elem_info_t *info)
+{
+    if (snd_ctl_elem_info_get_min64(info) != 100)
+        return -EIO;
+    if (snd_ctl_elem_info_get_max64(info) != 10000)
+        return -EIO;
+    if (snd_ctl_elem_info_get_step64(info) != 30)
+        return -EIO;
+
+    return 0;
+}
+
+static void change_int64_elem_members(struct elem_set_trial *trial,
+                                      snd_ctl_elem_value_t *elem_data)
+{
+    long long val;
+    unsigned int i;
+
+    for (i = 0; i < trial->member_count; ++i) {
+        val = snd_ctl_elem_value_get_integer64(elem_data, i);
+        snd_ctl_elem_value_set_integer64(elem_data, i, ++val);
+    }
+}
+
+/* Common operations. */
+static int add_elem_set(struct elem_set_trial *trial)
+{
+    char name[64] = {0};
+
+    snprintf(name, 64, "userspace-control-element-%s",
+             snd_ctl_elem_type_name(trial->type));
+
+    memset(trial->id, 0, snd_ctl_elem_id_sizeof());
+    snd_ctl_elem_id_set_interface(trial->id, SND_CTL_ELEM_IFACE_MIXER);
+    snd_ctl_elem_id_set_name(trial->id, name);
+
+    return trial->add_elem_set(trial);
+}
+
+static int check_event(struct elem_set_trial *trial, unsigned int mask,
+                       unsigned int expected_count)
+{
+    struct pollfd pfds;
+    int count;
+    unsigned short revents;
+    snd_ctl_event_t *event;
+    int err;
+
+    snd_ctl_event_alloca(&event);
+
+    if (snd_ctl_poll_descriptors_count(trial->handle) != 1)
+        return -ENXIO;
+
+    if (snd_ctl_poll_descriptors(trial->handle, &pfds, 1) != 1)
+        return -ENXIO;
+
+    while (expected_count > 0) {
+        count = poll(&pfds, 1, 1000);
+        if (count < 0)
+            return errno;
+        /* Some events are already supplied. */
+        if (count == 0)
+            return -ETIMEDOUT;
+
+        err = snd_ctl_poll_descriptors_revents(trial->handle, &pfds, count,
+                                               &revents);
+        if (err < 0)
+            return err;
+        if (revents & POLLERR)
+            return -EIO;
+        if (!(revents & POLLIN))
+            continue;
+
+        err = snd_ctl_read(trial->handle, event);
+        if (err < 0)
+            return err;
+        if (snd_ctl_event_get_type(event) != SND_CTL_EVENT_ELEM)
+            continue;
+        /* I expect each event is generated separately to the same element. */
+        if (!(snd_ctl_event_elem_get_mask(event) & mask))
+            continue;
+        --expected_count;
+    }
+
+    if (expected_count != 0)
+        return -EIO;
+
+    return 0;
+}
+
+static int check_elem_set_props(struct elem_set_trial *trial)
+{
+    snd_ctl_elem_id_t *id;
+    snd_ctl_elem_info_t *info;
+    unsigned int numid;
+    unsigned int i;
+    int err;
+
+    snd_ctl_elem_id_alloca(&id);
+    snd_ctl_elem_info_alloca(&info);
+
+    snd_ctl_elem_info_set_id(info, trial->id);
+    numid = snd_ctl_elem_info_get_numid(info);
+
+    for (i = 0; i < trial->element_count; ++i) {
+        snd_ctl_elem_info_set_numid(info, numid + i);
+
+        err = snd_ctl_elem_info(trial->handle, info);
+        if (err < 0)
+            return err;
+
+        /* Check some common properties. */
+        if (snd_ctl_elem_info_get_type(info) != trial->type)
+            return -EIO;
+        if (snd_ctl_elem_info_get_count(info) != trial->member_count)
+            return -EIO;
+        if (snd_ctl_elem_info_get_owner(info) != getpid())
+            return -EIO;
+
+        /*
+         * Just adding an element set by userspace applications, included
+         * elements are initially locked.
+         */
+        if (!snd_ctl_elem_info_is_locked(info))
+            return -EIO;
+
+        /* Check type-specific properties. */
+        if (trial->check_elem_props != NULL) {
+            err = trial->check_elem_props(trial, info);
+            if (err < 0)
+                return err;
+        }
+
+        snd_ctl_elem_info_get_id(info, id);
+        err = snd_ctl_elem_unlock(trial->handle, id);
+        if (err < 0)
+            return err;
+    }
+
+    return 0;
+}
+
+static int check_elems(struct elem_set_trial *trial)
+{
+    snd_ctl_elem_value_t *data;
+    unsigned int numid;
+    unsigned int i;
+    int err;
+
+    snd_ctl_elem_value_alloca(&data);
+
+    snd_ctl_elem_value_set_id(data, trial->id);
+
+    /*
+     * Get the value of numid field in identical information for the first
+     * element of this element set.
+     */
+    numid = snd_ctl_elem_value_get_numid(data);
+
+    for (i = 0; i < trial->element_count; ++i) {
+        /*
+         * Here, I increment numid, while we can also increment offset to
+         * enumerate each element in this element set.
+         */
+        snd_ctl_elem_value_set_numid(data, numid + i);
+
+        err = snd_ctl_elem_read(trial->handle, data);
+        if (err < 0)
+            return err;
+
+        /* Change members of an element in this element set. */
+        trial->change_elem_members(trial, data);
+
+        err = snd_ctl_elem_write(trial->handle, data);
+        if (err < 0)
+            return err;
+    }
+
+    return 0;
+}
+
+static int check_tlv(struct elem_set_trial *trial)
+{
+    unsigned int orig[8], curr[8];
+    int err;
+
+    /*
+     * See a layout of 'struct snd_ctl_tlv'. I don't know the reason to
+     * construct this buffer with the same layout. It should be abstracted inner
+     * userspace library...
+     */
+    orig[0] = snd_ctl_elem_id_get_numid(trial->id);
+    orig[1] = 6 * sizeof(orig[0]);
+    orig[2] = 'a';
+    orig[3] = 'b';
+    orig[4] = 'c';
+    orig[5] = 'd';
+    orig[6] = 'e';
+    orig[7] = 'f';
+
+    /*
+     * In in-kernel implementation, write and command operations are the same
+     * for an element set added by userspace applications. Here, I use write.
+     */
+    err = snd_ctl_elem_tlv_write(trial->handle, trial->id,
+                                 (const unsigned int *)orig);
+    if (err < 0)
+        return err;
+
+    err = snd_ctl_elem_tlv_read(trial->handle, trial->id, curr, sizeof(curr));
+    if (err < 0)
+        return err;
+
+    if (memcmp(curr, orig, sizeof(orig)) != 0)
+        return -EIO;
+
+    return 0;
+}
+
+int main(void)
+{
+    struct elem_set_trial trial = {0};
+    unsigned int i;
+    int err;
+
+    snd_ctl_elem_id_alloca(&trial.id);
+
+    err = snd_ctl_open(&trial.handle, "hw:0", 0);
+    if (err < 0)
+        return EXIT_FAILURE;
+
+    err = snd_ctl_subscribe_events(trial.handle, 1);
+    if (err < 0)
+        return EXIT_FAILURE;
+
+    /* Test all of types. */
+    for (i = 0; i < SND_CTL_ELEM_TYPE_LAST; ++i) {
+        trial.type = i + 1;
+
+        /* Assign type-dependent operations. */
+        switch (trial.type) {
+        case SND_CTL_ELEM_TYPE_BOOLEAN:
+            trial.element_count = 900;
+            trial.member_count = 128;
+            trial.add_elem_set = add_bool_elem_set;
+            trial.check_elem_props = NULL;
+            trial.change_elem_members = change_bool_elem_members;
+            break;
+        case SND_CTL_ELEM_TYPE_INTEGER:
+            trial.element_count = 900;
+            trial.member_count = 128;
+            trial.add_elem_set = add_int_elem_set;
+            trial.check_elem_props = check_int_elem_props;
+            trial.change_elem_members = change_int_elem_members;
+            break;
+        case SND_CTL_ELEM_TYPE_ENUMERATED:
+            trial.element_count = 900;
+            trial.member_count = 128;
+            trial.add_elem_set = add_enum_elem_set;
+            trial.check_elem_props = check_enum_elem_props;
+            trial.change_elem_members = change_enum_elem_members;
+            break;
+        case SND_CTL_ELEM_TYPE_BYTES:
+            trial.element_count = 900;
+            trial.member_count = 512;
+            trial.add_elem_set = add_bytes_elem_set;
+            trial.check_elem_props = NULL;
+            trial.change_elem_members = change_bytes_elem_members;
+            break;
+        case SND_CTL_ELEM_TYPE_IEC958:
+            trial.element_count = 1;
+            trial.member_count = 1;
+            trial.add_elem_set = add_iec958_elem_set;
+            trial.check_elem_props = NULL;
+            trial.change_elem_members = change_iec958_elem_members;
+            break;
+        case SND_CTL_ELEM_TYPE_INTEGER64:
+        default:
+            trial.element_count = 900;
+            trial.member_count = 64;
+            trial.add_elem_set = add_int64_elem_set;
+            trial.check_elem_props = check_int64_elem_props;
+            trial.change_elem_members = change_int64_elem_members;
+            break;
+        }
+
+        /* Test an operation to add an element set. */
+        err = add_elem_set(&trial);
+        if (err < 0) {
+            printf("Fail to add an element set with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+        err = check_event(&trial, SND_CTL_EVENT_MASK_ADD, trial.element_count);
+        if (err < 0) {
+            printf("Fail to check some events to add elements with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+
+        /* Check properties of each element in this element set. */
+        err = check_elem_set_props(&trial);
+        if (err < 0) {
+            printf("Fail to check propetries of each element with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+
+        /*
+         * Test operations to change the state of members in each element in
+         * the element set.
+         */
+        err = check_elems(&trial);
+        if (err < 0) {
+            printf("Fail to change status of each element with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+        err = check_event(&trial, SND_CTL_EVENT_MASK_VALUE,
+                          trial.element_count);
+        if (err < 0) {
+            printf("Fail to check some events to change status of each "
+                   "elements with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+
+        /*
+         * Test an operation to change threshold data of this element set,
+         * except for IEC958 type.
+         */
+        if (trial.type != SND_CTL_ELEM_TYPE_IEC958) {
+            err = check_tlv(&trial);
+            if (err < 0) {
+                printf("Fail to change threshold level of an element set with "
+                       "%s type.\n",
+                       snd_ctl_elem_type_name(trial.type));
+                break;
+            }
+            err = check_event(&trial, SND_CTL_EVENT_MASK_TLV, 1);
+            if (err < 0) {
+                printf("Fail to check an event to change threshold level of an "
+                       "an element set with %s type.\n",
+                       snd_ctl_elem_type_name(trial.type));
+                break;
+            }
+        }
+
+        /* Test an operation to remove elements in this element set. */
+        err = snd_ctl_elem_remove(trial.handle, trial.id);
+        if (err < 0) {
+            printf("Fail to remove elements with %s type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+        err = check_event(&trial, SND_CTL_EVENT_MASK_REMOVE,
+                          trial.element_count);
+        if (err < 0) {
+            printf("Fail to check some events to remove each element with %s "
+                   "type.\n",
+                   snd_ctl_elem_type_name(trial.type));
+            break;
+        }
+    }
+
+    if (err < 0) {
+        printf("%s\n", snd_strerror(err));
+
+        /* To ensure. */
+        snd_ctl_elem_remove(trial.handle, trial.id);
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
+}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-12  8:16 ` [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element Takashi Sakamoto
@ 2016-06-13 12:51   ` Takashi Iwai
  2016-06-14 11:25     ` Takashi Sakamoto
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Iwai @ 2016-06-13 12:51 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, clemens, ffado-devel

On Sun, 12 Jun 2016 10:16:07 +0200,
Takashi Sakamoto wrote:
> 
> When comparing old APIs (to add a single element) with new APIs (to add
> an element set), the latter has an benefit to get full identical
> information for a first element in the element set. Furthermore, in
> previous commit, the old APIs become simple wrappers to the new APIs.
> Therefore, there's few intentions to use the old APIs.
>
> This commit deprecates the old APIs to encourage the usage of new APIs.

In general, it's a bad idea to deprecate an API that has been actually
used, and even a worse idea to give a link warning.  We've done
deprecation for some API functions in the past, and it wasn't a really
smart move.  But it was still justified that they were really unused
API functions.  In this case, however, it's commonly used API.  That's
a big difference.

I know several system libraries like Gtk+ often deprecating API
functions.  But it's more annoying than useful for developers and
users.  Unless you are masochistic, the likely first reaction after
seeing such a warning is: "upstream sucks again".


thanks,

Takashi

> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/control.h     | 17 ++++++++++++-----
>  src/control/control.c | 20 ++++++++++++++++++++
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/include/control.h b/include/control.h
> index d6069ff..e21a58a 100644
> --- a/include/control.h
> +++ b/include/control.h
> @@ -443,14 +443,21 @@ int snd_ctl_elem_add_enumerated_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>  int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>  			       unsigned int element_count,
>  			       unsigned int member_count);
> -
> -int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long imin, long imax, long istep);
> -int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long long imin, long long imax, long long istep);
> -int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count);
> -int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, unsigned int items, const char *const names[]);
>  int snd_ctl_elem_add_iec958(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id);
>  int snd_ctl_elem_remove(snd_ctl_t *ctl, snd_ctl_elem_id_t *id);
>  
> +int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
> +		unsigned int count, long imin, long imax,
> +		long istep) __attribute__((deprecated));
> +int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
> +		unsigned int count, long long imin, long long imax,
> +		long long istep) __attribute__((deprecated));
> +int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
> +		unsigned int count) __attribute__((deprecated));
> +int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
> +		unsigned int count, unsigned int items,
> +		const char *const names[]) __attribute__((deprecated));
> +
>  size_t snd_ctl_elem_value_sizeof(void);
>  /** \hideinitializer
>   * \brief allocate an invalid #snd_ctl_elem_value_t using standard alloca
> diff --git a/src/control/control.c b/src/control/control.c
> index fbc5e18..6893012 100644
> --- a/src/control/control.c
> +++ b/src/control/control.c
> @@ -677,6 +677,8 @@ int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>   *
>   * This function is a wrapper function to snd_ctl_elem_add_integer_set() for
>   * single control element.
> + *
> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer_set(), instead.
>   */
>  int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  			     unsigned int member_count,
> @@ -690,12 +692,17 @@ int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  	return snd_ctl_elem_add_integer_set(ctl, local_id, 1, member_count,
>  					    min, max, step);
>  }
> +link_warning(snd_ctl_elem_add_integer,
> +	     "Warning: snd_ctl_elem_add_integer is deprecated, "
> +	     "Use snd_ctl_elem_add_integer_set, instead.");
>  
>  /**
>   * \brief Create and add an user-defined control element of integer64 type.
>   *
>   * This function is a wrapper function to snd_ctl_elem_add_integer64_set() for
>   * single control element.
> + *
> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer64_set(), instead.
>   */
>  int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  			       unsigned int member_count,
> @@ -709,12 +716,17 @@ int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  	return snd_ctl_elem_add_integer64_set(ctl, local_id, 1, member_count,
>  					      min, max, step);
>  }
> +link_warning(snd_ctl_elem_add_integer64,
> +	     "Warning: snd_ctl_elem_add_integer64 is deprecated, "
> +	     "Use snd_ctl_elem_add_integer64_set, instead.");
>  
>  /**
>   * \brief Create and add an user-defined control element of boolean type.
>   *
>   * This function is a wrapper function to snd_ctl_elem_add_boolean_set() for
>   * single control element.
> + *
> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_boolean_set(), instead.
>   */
>  int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  			     unsigned int member_count)
> @@ -726,6 +738,9 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  
>  	return snd_ctl_elem_add_boolean_set(ctl, local_id, 1, member_count);
>  }
> +link_warning(snd_ctl_elem_add_boolean,
> +	     "Warning: snd_ctl_elem_add_boolean is deprecated, "
> +	     "Use snd_ctl_elem_add_boolean_set, instead.");
>  
>  /**
>   * \brief Create and add a user-defined control element of enumerated type.
> @@ -734,6 +749,8 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>   * single control element.
>   *
>   * This function is added in version 1.0.25.
> + *
> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_enumerated_set(), instead.
>   */
>  int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  				unsigned int member_count, unsigned int items,
> @@ -747,6 +764,9 @@ int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>  	return snd_ctl_elem_add_enumerated_set(ctl, local_id, 1, member_count,
>  					       items, labels);
>  }
> +link_warning(snd_ctl_elem_add_enumerated,
> +	     "Warning: snd_ctl_elem_add_enumerated is deprecated, "
> +	     "Use snd_ctl_elem_add_enumerated_set, instead.");
>  
>  /**
>   * \brief Create and add a user-defined control element of IEC958 type.
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-13 12:51   ` Takashi Iwai
@ 2016-06-14 11:25     ` Takashi Sakamoto
  2016-06-14 11:46       ` Takashi Iwai
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-14 11:25 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, clemens, ffado-devel

Hi,

On Jun 13 2016 21:51, Takashi Iwai wrote:
> On Sun, 12 Jun 2016 10:16:07 +0200,
> Takashi Sakamoto wrote:
>>
>> When comparing old APIs (to add a single element) with new APIs (to add
>> an element set), the latter has an benefit to get full identical
>> information for a first element in the element set. Furthermore, in
>> previous commit, the old APIs become simple wrappers to the new APIs.
>> Therefore, there's few intentions to use the old APIs.
>>
>> This commit deprecates the old APIs to encourage the usage of new APIs.
>
> In general, it's a bad idea to deprecate an API that has been actually
> used, and even a worse idea to give a link warning.  We've done
> deprecation for some API functions in the past, and it wasn't a really
> smart move.  But it was still justified that they were really unused
> API functions.  In this case, however, it's commonly used API.  That's
> a big difference.
>
> I know several system libraries like Gtk+ often deprecating API
> functions.  But it's more annoying than useful for developers and
> users.  Unless you are masochistic, the likely first reaction after
> seeing such a warning is: "upstream sucks again".

I just added the link_warning() by immitating these old APIs such as 
snd_pcm_hwsync(). In short, I assumed that it's a fashion of this 
userspace library to use compiler/linker functionalities even if it's 
against usual ways to maintain APIs in userspace libraries.

(I'm not so strange developer, just foreigner to this project without 
enough documentations.)

For the deprecation, I basically agree with you. But there might be 
exaggeration about usage of these APIs. They're rarely used, I think. If 
you know actual application programs to use them, please inform them to 
me. But I know discussions about the population of these APIs are not a 
good direction in this case.

My main intention of this patchset is to add the new APIs. These APIs 
are completely upper compatible to the old APIs, and have benefits I 
described. In fact, deleting public APIs is not preferable, but 
deprecating them and still maintaining sometimes has advantages to 
motivate users to start using the new ones. For this purpose, I think it 
better to add 'deprecated' comment into documentations of old APIs.


Thanks

Takashi Sakamoto

> thanks,
>
> Takashi
>
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>   include/control.h     | 17 ++++++++++++-----
>>   src/control/control.c | 20 ++++++++++++++++++++
>>   2 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/control.h b/include/control.h
>> index d6069ff..e21a58a 100644
>> --- a/include/control.h
>> +++ b/include/control.h
>> @@ -443,14 +443,21 @@ int snd_ctl_elem_add_enumerated_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>>   int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>>   			       unsigned int element_count,
>>   			       unsigned int member_count);
>> -
>> -int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long imin, long imax, long istep);
>> -int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, long long imin, long long imax, long long istep);
>> -int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count);
>> -int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id, unsigned int count, unsigned int items, const char *const names[]);
>>   int snd_ctl_elem_add_iec958(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id);
>>   int snd_ctl_elem_remove(snd_ctl_t *ctl, snd_ctl_elem_id_t *id);
>>
>> +int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>> +		unsigned int count, long imin, long imax,
>> +		long istep) __attribute__((deprecated));
>> +int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>> +		unsigned int count, long long imin, long long imax,
>> +		long long istep) __attribute__((deprecated));
>> +int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>> +		unsigned int count) __attribute__((deprecated));
>> +int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>> +		unsigned int count, unsigned int items,
>> +		const char *const names[]) __attribute__((deprecated));
>> +
>>   size_t snd_ctl_elem_value_sizeof(void);
>>   /** \hideinitializer
>>    * \brief allocate an invalid #snd_ctl_elem_value_t using standard alloca
>> diff --git a/src/control/control.c b/src/control/control.c
>> index fbc5e18..6893012 100644
>> --- a/src/control/control.c
>> +++ b/src/control/control.c
>> @@ -677,6 +677,8 @@ int snd_ctl_elem_add_bytes_set(snd_ctl_t *ctl, snd_ctl_elem_id_t *id,
>>    *
>>    * This function is a wrapper function to snd_ctl_elem_add_integer_set() for
>>    * single control element.
>> + *
>> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer_set(), instead.
>>    */
>>   int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   			     unsigned int member_count,
>> @@ -690,12 +692,17 @@ int snd_ctl_elem_add_integer(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   	return snd_ctl_elem_add_integer_set(ctl, local_id, 1, member_count,
>>   					    min, max, step);
>>   }
>> +link_warning(snd_ctl_elem_add_integer,
>> +	     "Warning: snd_ctl_elem_add_integer is deprecated, "
>> +	     "Use snd_ctl_elem_add_integer_set, instead.");
>>
>>   /**
>>    * \brief Create and add an user-defined control element of integer64 type.
>>    *
>>    * This function is a wrapper function to snd_ctl_elem_add_integer64_set() for
>>    * single control element.
>> + *
>> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_integer64_set(), instead.
>>    */
>>   int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   			       unsigned int member_count,
>> @@ -709,12 +716,17 @@ int snd_ctl_elem_add_integer64(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   	return snd_ctl_elem_add_integer64_set(ctl, local_id, 1, member_count,
>>   					      min, max, step);
>>   }
>> +link_warning(snd_ctl_elem_add_integer64,
>> +	     "Warning: snd_ctl_elem_add_integer64 is deprecated, "
>> +	     "Use snd_ctl_elem_add_integer64_set, instead.");
>>
>>   /**
>>    * \brief Create and add an user-defined control element of boolean type.
>>    *
>>    * This function is a wrapper function to snd_ctl_elem_add_boolean_set() for
>>    * single control element.
>> + *
>> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_boolean_set(), instead.
>>    */
>>   int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   			     unsigned int member_count)
>> @@ -726,6 +738,9 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>
>>   	return snd_ctl_elem_add_boolean_set(ctl, local_id, 1, member_count);
>>   }
>> +link_warning(snd_ctl_elem_add_boolean,
>> +	     "Warning: snd_ctl_elem_add_boolean is deprecated, "
>> +	     "Use snd_ctl_elem_add_boolean_set, instead.");
>>
>>   /**
>>    * \brief Create and add a user-defined control element of enumerated type.
>> @@ -734,6 +749,8 @@ int snd_ctl_elem_add_boolean(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>    * single control element.
>>    *
>>    * This function is added in version 1.0.25.
>> + *
>> + * \deprecated Since 1.1.2. Use snd_ctl_elem_add_enumerated_set(), instead.
>>    */
>>   int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   				unsigned int member_count, unsigned int items,
>> @@ -747,6 +764,9 @@ int snd_ctl_elem_add_enumerated(snd_ctl_t *ctl, const snd_ctl_elem_id_t *id,
>>   	return snd_ctl_elem_add_enumerated_set(ctl, local_id, 1, member_count,
>>   					       items, labels);
>>   }
>> +link_warning(snd_ctl_elem_add_enumerated,
>> +	     "Warning: snd_ctl_elem_add_enumerated is deprecated, "
>> +	     "Use snd_ctl_elem_add_enumerated_set, instead.");
>>
>>   /**
>>    * \brief Create and add a user-defined control element of IEC958 type.
>> --
>> 2.7.4
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-14 11:25     ` Takashi Sakamoto
@ 2016-06-14 11:46       ` Takashi Iwai
  2016-06-15  6:57         ` Takashi Sakamoto
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Iwai @ 2016-06-14 11:46 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, clemens, ffado-devel

On Tue, 14 Jun 2016 13:25:16 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 13 2016 21:51, Takashi Iwai wrote:
> > On Sun, 12 Jun 2016 10:16:07 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> When comparing old APIs (to add a single element) with new APIs (to add
> >> an element set), the latter has an benefit to get full identical
> >> information for a first element in the element set. Furthermore, in
> >> previous commit, the old APIs become simple wrappers to the new APIs.
> >> Therefore, there's few intentions to use the old APIs.
> >>
> >> This commit deprecates the old APIs to encourage the usage of new APIs.
> >
> > In general, it's a bad idea to deprecate an API that has been actually
> > used, and even a worse idea to give a link warning.  We've done
> > deprecation for some API functions in the past, and it wasn't a really
> > smart move.  But it was still justified that they were really unused
> > API functions.  In this case, however, it's commonly used API.  That's
> > a big difference.
> >
> > I know several system libraries like Gtk+ often deprecating API
> > functions.  But it's more annoying than useful for developers and
> > users.  Unless you are masochistic, the likely first reaction after
> > seeing such a warning is: "upstream sucks again".
> 
> I just added the link_warning() by immitating these old APIs such as
> snd_pcm_hwsync(). In short, I assumed that it's a fashion of this
> userspace library to use compiler/linker functionalities even if it's
> against usual ways to maintain APIs in userspace libraries.

Yes, we have had such things, and as mentioned, it was a bad idea,
after all.  The deprecation doesn't help maintaining the stuff.
You don't need to follow our own bad attitude again.

> (I'm not so strange developer, just foreigner to this project without
> enough documentations.)
> 
> For the deprecation, I basically agree with you. But there might be
> exaggeration about usage of these APIs. They're rarely used, I
> think. If you know actual application programs to use them, please
> inform them to me. But I know discussions about the population of
> these APIs are not a good direction in this case.

Right, it's not the only indication.

> My main intention of this patchset is to add the new APIs. These APIs
> are completely upper compatible to the old APIs, and have benefits I
> described. In fact, deleting public APIs is not preferable, but
> deprecating them and still maintaining sometimes has advantages to
> motivate users to start using the new ones. For this purpose, I think
> it better to add 'deprecated' comment into documentations of old APIs.

It depends.  The deprecation and the recommendation are different
behavior.  The former is needed only when the old API is really
harmful or doesn't work any longer.  In such a case, giving a link
warning is helpful so that user can know of it.

Meanwhile, the latter is done everywhere.  It's a programming
practice, and let users choose a better one.  That being said, it'd be
good to add recommendation for a better API in the documentation. But
it's never meant to deprecate some old API function.

Again, deprecation doesn't help much from maintenance POV.  I can
judge it from my long experiences in both upstream maintainer and
downstream packager sides.  It may help hardening some serious errors
if the old API were really bad.  But that's all.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-14 11:46       ` Takashi Iwai
@ 2016-06-15  6:57         ` Takashi Sakamoto
  2016-06-15  6:59           ` Takashi Iwai
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-15  6:57 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, clemens, ffado-devel

On 2016年06月14日 20:46, Takashi Iwai wrote:
> On Tue, 14 Jun 2016 13:25:16 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Jun 13 2016 21:51, Takashi Iwai wrote:
>>> On Sun, 12 Jun 2016 10:16:07 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> When comparing old APIs (to add a single element) with new APIs (to add
>>>> an element set), the latter has an benefit to get full identical
>>>> information for a first element in the element set. Furthermore, in
>>>> previous commit, the old APIs become simple wrappers to the new APIs.
>>>> Therefore, there's few intentions to use the old APIs.
>>>>
>>>> This commit deprecates the old APIs to encourage the usage of new APIs.
>>>
>>> In general, it's a bad idea to deprecate an API that has been actually
>>> used, and even a worse idea to give a link warning.  We've done
>>> deprecation for some API functions in the past, and it wasn't a really
>>> smart move.  But it was still justified that they were really unused
>>> API functions.  In this case, however, it's commonly used API.  That's
>>> a big difference.
>>>
>>> I know several system libraries like Gtk+ often deprecating API
>>> functions.  But it's more annoying than useful for developers and
>>> users.  Unless you are masochistic, the likely first reaction after
>>> seeing such a warning is: "upstream sucks again".
>>
>> I just added the link_warning() by immitating these old APIs such as
>> snd_pcm_hwsync(). In short, I assumed that it's a fashion of this
>> userspace library to use compiler/linker functionalities even if it's
>> against usual ways to maintain APIs in userspace libraries.
>
> Yes, we have had such things, and as mentioned, it was a bad idea,
> after all.  The deprecation doesn't help maintaining the stuff.
> You don't need to follow our own bad attitude again.
>
>> (I'm not so strange developer, just foreigner to this project without
>> enough documentations.)
>>
>> For the deprecation, I basically agree with you. But there might be
>> exaggeration about usage of these APIs. They're rarely used, I
>> think. If you know actual application programs to use them, please
>> inform them to me. But I know discussions about the population of
>> these APIs are not a good direction in this case.
>
> Right, it's not the only indication.
>
>> My main intention of this patchset is to add the new APIs. These APIs
>> are completely upper compatible to the old APIs, and have benefits I
>> described. In fact, deleting public APIs is not preferable, but
>> deprecating them and still maintaining sometimes has advantages to
>> motivate users to start using the new ones. For this purpose, I think
>> it better to add 'deprecated' comment into documentations of old APIs.
>
> It depends.  The deprecation and the recommendation are different
> behavior.  The former is needed only when the old API is really
> harmful or doesn't work any longer.  In such a case, giving a link
> warning is helpful so that user can know of it.
>
> Meanwhile, the latter is done everywhere.  It's a programming
> practice, and let users choose a better one.  That being said, it'd be
> good to add recommendation for a better API in the documentation. But
> it's never meant to deprecate some old API function.
>
> Again, deprecation doesn't help much from maintenance POV.  I can
> judge it from my long experiences in both upstream maintainer and
> downstream packager sides.  It may help hardening some serious errors
> if the old API were really bad.  But that's all.

OK. I respect your long work for Linux system and can agree with the 
judgment. Let me drop this patch from patchset I'll post tonight. In the 
patchset, some comments are newly added to indicate the recommendation.

And how about the other parts? Are there any inappropriate codes or 
comments?


Thanks

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] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-15  6:57         ` Takashi Sakamoto
@ 2016-06-15  6:59           ` Takashi Iwai
  2016-06-15  7:23             ` Takashi Sakamoto
  0 siblings, 1 reply; 17+ messages in thread
From: Takashi Iwai @ 2016-06-15  6:59 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, clemens, ffado-devel

On Wed, 15 Jun 2016 08:57:09 +0200,
Takashi Sakamoto wrote:
> 
> On 2016年06月14日 20:46, Takashi Iwai wrote:
> > On Tue, 14 Jun 2016 13:25:16 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> Hi,
> >>
> >> On Jun 13 2016 21:51, Takashi Iwai wrote:
> >>> On Sun, 12 Jun 2016 10:16:07 +0200,
> >>> Takashi Sakamoto wrote:
> >>>>
> >>>> When comparing old APIs (to add a single element) with new APIs (to add
> >>>> an element set), the latter has an benefit to get full identical
> >>>> information for a first element in the element set. Furthermore, in
> >>>> previous commit, the old APIs become simple wrappers to the new APIs.
> >>>> Therefore, there's few intentions to use the old APIs.
> >>>>
> >>>> This commit deprecates the old APIs to encourage the usage of new APIs.
> >>>
> >>> In general, it's a bad idea to deprecate an API that has been actually
> >>> used, and even a worse idea to give a link warning.  We've done
> >>> deprecation for some API functions in the past, and it wasn't a really
> >>> smart move.  But it was still justified that they were really unused
> >>> API functions.  In this case, however, it's commonly used API.  That's
> >>> a big difference.
> >>>
> >>> I know several system libraries like Gtk+ often deprecating API
> >>> functions.  But it's more annoying than useful for developers and
> >>> users.  Unless you are masochistic, the likely first reaction after
> >>> seeing such a warning is: "upstream sucks again".
> >>
> >> I just added the link_warning() by immitating these old APIs such as
> >> snd_pcm_hwsync(). In short, I assumed that it's a fashion of this
> >> userspace library to use compiler/linker functionalities even if it's
> >> against usual ways to maintain APIs in userspace libraries.
> >
> > Yes, we have had such things, and as mentioned, it was a bad idea,
> > after all.  The deprecation doesn't help maintaining the stuff.
> > You don't need to follow our own bad attitude again.
> >
> >> (I'm not so strange developer, just foreigner to this project without
> >> enough documentations.)
> >>
> >> For the deprecation, I basically agree with you. But there might be
> >> exaggeration about usage of these APIs. They're rarely used, I
> >> think. If you know actual application programs to use them, please
> >> inform them to me. But I know discussions about the population of
> >> these APIs are not a good direction in this case.
> >
> > Right, it's not the only indication.
> >
> >> My main intention of this patchset is to add the new APIs. These APIs
> >> are completely upper compatible to the old APIs, and have benefits I
> >> described. In fact, deleting public APIs is not preferable, but
> >> deprecating them and still maintaining sometimes has advantages to
> >> motivate users to start using the new ones. For this purpose, I think
> >> it better to add 'deprecated' comment into documentations of old APIs.
> >
> > It depends.  The deprecation and the recommendation are different
> > behavior.  The former is needed only when the old API is really
> > harmful or doesn't work any longer.  In such a case, giving a link
> > warning is helpful so that user can know of it.
> >
> > Meanwhile, the latter is done everywhere.  It's a programming
> > practice, and let users choose a better one.  That being said, it'd be
> > good to add recommendation for a better API in the documentation. But
> > it's never meant to deprecate some old API function.
> >
> > Again, deprecation doesn't help much from maintenance POV.  I can
> > judge it from my long experiences in both upstream maintainer and
> > downstream packager sides.  It may help hardening some serious errors
> > if the old API were really bad.  But that's all.
> 
> OK. I respect your long work for Linux system and can agree with the
> judgment. Let me drop this patch from patchset I'll post tonight. In
> the patchset, some comments are newly added to indicate the
> recommendation.

Great, that's appreciated.

> And how about the other parts? Are there any inappropriate codes or
> comments?

They are good.  Let's keep them.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element
  2016-06-15  6:59           ` Takashi Iwai
@ 2016-06-15  7:23             ` Takashi Sakamoto
  0 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2016-06-15  7:23 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, clemens, ffado-devel

On 2016年06月15日 15:59, Takashi Iwai wrote:
> On Wed, 15 Jun 2016 08:57:09 +0200,
> Takashi Sakamoto wrote:
>>
>> On 2016年06月14日 20:46, Takashi Iwai wrote:
>>> On Tue, 14 Jun 2016 13:25:16 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Jun 13 2016 21:51, Takashi Iwai wrote:
>>>>> On Sun, 12 Jun 2016 10:16:07 +0200,
>>>>> Takashi Sakamoto wrote:
>>>>>>
>>>>>> When comparing old APIs (to add a single element) with new APIs (to add
>>>>>> an element set), the latter has an benefit to get full identical
>>>>>> information for a first element in the element set. Furthermore, in
>>>>>> previous commit, the old APIs become simple wrappers to the new APIs.
>>>>>> Therefore, there's few intentions to use the old APIs.
>>>>>>
>>>>>> This commit deprecates the old APIs to encourage the usage of new APIs.
>>>>>
>>>>> In general, it's a bad idea to deprecate an API that has been actually
>>>>> used, and even a worse idea to give a link warning.  We've done
>>>>> deprecation for some API functions in the past, and it wasn't a really
>>>>> smart move.  But it was still justified that they were really unused
>>>>> API functions.  In this case, however, it's commonly used API.  That's
>>>>> a big difference.
>>>>>
>>>>> I know several system libraries like Gtk+ often deprecating API
>>>>> functions.  But it's more annoying than useful for developers and
>>>>> users.  Unless you are masochistic, the likely first reaction after
>>>>> seeing such a warning is: "upstream sucks again".
>>>>
>>>> I just added the link_warning() by immitating these old APIs such as
>>>> snd_pcm_hwsync(). In short, I assumed that it's a fashion of this
>>>> userspace library to use compiler/linker functionalities even if it's
>>>> against usual ways to maintain APIs in userspace libraries.
>>>
>>> Yes, we have had such things, and as mentioned, it was a bad idea,
>>> after all.  The deprecation doesn't help maintaining the stuff.
>>> You don't need to follow our own bad attitude again.
>>>
>>>> (I'm not so strange developer, just foreigner to this project without
>>>> enough documentations.)
>>>>
>>>> For the deprecation, I basically agree with you. But there might be
>>>> exaggeration about usage of these APIs. They're rarely used, I
>>>> think. If you know actual application programs to use them, please
>>>> inform them to me. But I know discussions about the population of
>>>> these APIs are not a good direction in this case.
>>>
>>> Right, it's not the only indication.
>>>
>>>> My main intention of this patchset is to add the new APIs. These APIs
>>>> are completely upper compatible to the old APIs, and have benefits I
>>>> described. In fact, deleting public APIs is not preferable, but
>>>> deprecating them and still maintaining sometimes has advantages to
>>>> motivate users to start using the new ones. For this purpose, I think
>>>> it better to add 'deprecated' comment into documentations of old APIs.
>>>
>>> It depends.  The deprecation and the recommendation are different
>>> behavior.  The former is needed only when the old API is really
>>> harmful or doesn't work any longer.  In such a case, giving a link
>>> warning is helpful so that user can know of it.
>>>
>>> Meanwhile, the latter is done everywhere.  It's a programming
>>> practice, and let users choose a better one.  That being said, it'd be
>>> good to add recommendation for a better API in the documentation. But
>>> it's never meant to deprecate some old API function.
>>>
>>> Again, deprecation doesn't help much from maintenance POV.  I can
>>> judge it from my long experiences in both upstream maintainer and
>>> downstream packager sides.  It may help hardening some serious errors
>>> if the old API were really bad.  But that's all.
>>
>> OK. I respect your long work for Linux system and can agree with the
>> judgment. Let me drop this patch from patchset I'll post tonight. In
>> the patchset, some comments are newly added to indicate the
>> recommendation.
>
> Great, that's appreciated.
>
>> And how about the other parts? Are there any inappropriate codes or
>> comments?
>
> They are good.  Let's keep them.

Glad ;)


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] 17+ messages in thread

end of thread, other threads:[~2016-06-15  7:23 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-12  8:16 [alsa-lib][PATCH 00/10 v2] ctl: add APIs for control element set Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 01/10] ctl: add an overview for design of ALSA control interface Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 02/10] ctl: improve comments for handling element data Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 03/10] ctl: add functions to add an element set Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 04/10] ctl: improve comments for API to add an element of IEC958 type Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 05/10] ctl: change former APIs as wrapper functions of element set APIs Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 06/10] ctl: deprecate APIs to add an element set with a single element Takashi Sakamoto
2016-06-13 12:51   ` Takashi Iwai
2016-06-14 11:25     ` Takashi Sakamoto
2016-06-14 11:46       ` Takashi Iwai
2016-06-15  6:57         ` Takashi Sakamoto
2016-06-15  6:59           ` Takashi Iwai
2016-06-15  7:23             ` Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 07/10] pcm: use new APIs to add a control element set for softvol plugin Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 08/10] ctl: add explaination about threshold level feature Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 09/10] ctl: improve API documentation for threshold level operations Takashi Sakamoto
2016-06-12  8:16 ` [PATCH 10/10] ctl: add test program for control element set 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.