Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

[James Courtier-Dutton]

Jaroslav Kysela wrote:
> changeset:   4278:d194ae01b0ba9d5614ff29a82f4aacae9edd1212
> tag:         tip
> user:        perex
> date:        Thu Jun  1 18:34:01 2006 +0200
> files:       core/control.c include/asound.h include/control.h include/tlv.h pci/ca0106/ca0106_mixer.c
> description:
> Control API - TLV implementation for additional information like dB scale
> 
> This patch implements a TLV mechanism to transfer an additional information
> like dB scale to the user space. The types might be extended in future.
> 
> Acked-by: Takashi Iwai <tiwai@suse.de>
> 
> 
> +struct snd_ctl_tlv {
> +        unsigned int numid;	/* control element numeric identification */
> +        unsigned int length;	/* in bytes aligned to 4 */
> +        unsigned int tlv[0];	/* first TLV */
> +};
> +

> +static int snd_ctl_tlv_read(struct snd_card *card,
> +                            struct snd_ctl_tlv __user *_tlv)
> +{
> +	struct snd_ctl_tlv tlv;
> +	struct snd_kcontrol *kctl;
> +	unsigned int len;
> +	int err = 0;
> +
> +	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
> +		return -EFAULT;
> +        if (tlv.length < sizeof(unsigned int) * 3)
> +                return -EINVAL;
> +        down_read(&card->controls_rwsem);
> +        kctl = snd_ctl_find_numid(card, tlv.numid);
> +        if (kctl == NULL) {
> +                err = -ENOENT;
> +                goto __kctl_end;
> +        }
> +        if (kctl->tlv == NULL) {
> +                err = -ENXIO;
> +                goto __kctl_end;
> +        }
> +        len = kctl->tlv[1] + 2 * sizeof(unsigned int);
> +        if (tlv.length < len) {
> +                err = -ENOMEM;
> +                goto __kctl_end;
> +        }
> +        if (copy_to_user(_tlv->tlv, kctl->tlv, len))
> +        	err = -EFAULT;
> +      __kctl_end:
> +        up_read(&card->controls_rwsem);
> +        return err;
> +}

I have been away for the last week, so I was not able to comment on this
commit.

How do you intend to expand this for requests for anything apart from
db_scale? For example, how could we get it to return all the current
"info" (min,max etc.) information instead of the db_scale? Or, are you
keeping it simple, and make a general request for info from the numid,
and receive a group of TLVs back for all the different information that
the numid might contain? I.e. one gets the entire group of information,
regardless of what is actually needed.

My intention was to place a TLV or multiple TLVs in the request to
select the information needed, and then receive the TLVs in return.

For example, audio professionals don't like any volume steps at all.
They like continuous volume gain adjustment. For example, they request
+1dB up, the software then gradually increases the actual gain in really
small steps until a +1dB is reached. This permits totally click free
volume adjustments. This is all achievable on any sound cards with DSP
based volume control. (i.e. not cheep hardware based volume control with
defined steps.). To achieve this, one would pass the raw linear gain
values from the DSP control (.e.g. 32bit values), and then use some more
complicated userland (alsa-lib), possibly floating point, conversion
function to convert it to dB gain levels (e.g.linear to log conversion).
In this case, one might want the raw linear gain via the
snd_ctl_tlv_read IOCTL, instead of the stepwise value currently returned
using the current snd_ca0106_volume_get API. In which case, one would
need a TLV in the request, to differentiate it from the db_scale request.

What I am trying to say, is that your "simplification" of the API, has
resulted in this sort of functionality being excluded, so I would need
to implement yet another IOCTL to support it! My whole point of using
the full request/response TLV api, was to not restrict us. I was also
intending to maybe use the TLVs to set values, as well as just get
values. For example, another way to implement the "continuous gain"
feature would be to use the TLV api to set a flag, so that the 32bit
values are returned via the current snd_ca0106_volume_get(), but only
for this particular mixer open/close cycle, for backward compatibility
with the current api.

Do you prefer separate new IOCTLs for all these features?

James

Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

[Takashi Iwai]

At Sat, 10 Jun 2006 10:24:26 +0100,
James Courtier-Dutton wrote:
> 
> Jaroslav Kysela wrote:
> > changeset:   4278:d194ae01b0ba9d5614ff29a82f4aacae9edd1212
> > tag:         tip
> > user:        perex
> > date:        Thu Jun  1 18:34:01 2006 +0200
> > files:       core/control.c include/asound.h include/control.h include/tlv.h pci/ca0106/ca0106_mixer.c
> > description:
> > Control API - TLV implementation for additional information like dB scale
> > 
> > This patch implements a TLV mechanism to transfer an additional information
> > like dB scale to the user space. The types might be extended in future.
> > 
> > Acked-by: Takashi Iwai <tiwai@suse.de>
> > 
> > 
> > +struct snd_ctl_tlv {
> > +        unsigned int numid;	/* control element numeric identification */
> > +        unsigned int length;	/* in bytes aligned to 4 */
> > +        unsigned int tlv[0];	/* first TLV */
> > +};
> > +
> 
> > +static int snd_ctl_tlv_read(struct snd_card *card,
> > +                            struct snd_ctl_tlv __user *_tlv)
> > +{
> > +	struct snd_ctl_tlv tlv;
> > +	struct snd_kcontrol *kctl;
> > +	unsigned int len;
> > +	int err = 0;
> > +
> > +	if (copy_from_user(&tlv, _tlv, sizeof(tlv)))
> > +		return -EFAULT;
> > +        if (tlv.length < sizeof(unsigned int) * 3)
> > +                return -EINVAL;
> > +        down_read(&card->controls_rwsem);
> > +        kctl = snd_ctl_find_numid(card, tlv.numid);
> > +        if (kctl == NULL) {
> > +                err = -ENOENT;
> > +                goto __kctl_end;
> > +        }
> > +        if (kctl->tlv == NULL) {
> > +                err = -ENXIO;
> > +                goto __kctl_end;
> > +        }
> > +        len = kctl->tlv[1] + 2 * sizeof(unsigned int);
> > +        if (tlv.length < len) {
> > +                err = -ENOMEM;
> > +                goto __kctl_end;
> > +        }
> > +        if (copy_to_user(_tlv->tlv, kctl->tlv, len))
> > +        	err = -EFAULT;
> > +      __kctl_end:
> > +        up_read(&card->controls_rwsem);
> > +        return err;
> > +}
> 
> I have been away for the last week, so I was not able to comment on this
> commit.

I also didn't expect that the patch was merged so quickly without
testing...

> How do you intend to expand this for requests for anything apart from
> db_scale? For example, how could we get it to return all the current
> "info" (min,max etc.) information instead of the db_scale? Or, are you
> keeping it simple, and make a general request for info from the numid,
> and receive a group of TLVs back for all the different information that
> the numid might contain? I.e. one gets the entire group of information,
> regardless of what is actually needed.

I understand so.  Get TLV composites and parse them in alsa-lib.

> My intention was to place a TLV or multiple TLVs in the request to
> select the information needed, and then receive the TLVs in return.
> 
> For example, audio professionals don't like any volume steps at all.
> They like continuous volume gain adjustment. For example, they request
> +1dB up, the software then gradually increases the actual gain in really
> small steps until a +1dB is reached. This permits totally click free
> volume adjustments. This is all achievable on any sound cards with DSP
> based volume control. (i.e. not cheep hardware based volume control with
> defined steps.). To achieve this, one would pass the raw linear gain
> values from the DSP control (.e.g. 32bit values), and then use some more
> complicated userland (alsa-lib), possibly floating point, conversion
> function to convert it to dB gain levels (e.g.linear to log conversion).
> In this case, one might want the raw linear gain via the
> snd_ctl_tlv_read IOCTL, instead of the stepwise value currently returned
> using the current snd_ca0106_volume_get API. In which case, one would
> need a TLV in the request, to differentiate it from the db_scale request.

Until now, such a hardware-specific thing was supposed to be processed
over hwdep device.  Maybe implementing a new ioctl in control API
might make the alsa-lib code a bit smaller.

> What I am trying to say, is that your "simplification" of the API, has
> resulted in this sort of functionality being excluded, so I would need
> to implement yet another IOCTL to support it! My whole point of using
> the full request/response TLV api, was to not restrict us. I was also
> intending to maybe use the TLVs to set values, as well as just get
> values. For example, another way to implement the "continuous gain"
> feature would be to use the TLV api to set a flag, so that the 32bit
> values are returned via the current snd_ca0106_volume_get(), but only
> for this particular mixer open/close cycle, for backward compatibility
> with the current api.
> 
> Do you prefer separate new IOCTLs for all these features?

We can modify the implementation.  For example, the patch like one
below.  tlv_rw callback is completely free for own implementation.
Each callback is in change of reading/writing the given user-space
pointer arbitrarily.


Takashi


diff -r 08577d0b45ef core/control.c
--- a/core/control.c	Tue Jun 13 12:01:14 2006 +0200
+++ b/core/control.c	Wed Jun 14 16:09:50 2006 +0200
@@ -241,6 +241,7 @@ struct snd_kcontrol *snd_ctl_new1(const 
 	kctl.info = ncontrol->info;
 	kctl.get = ncontrol->get;
 	kctl.put = ncontrol->put;
+	kctl.tlv_rw = ncontrol->tlv_rw;
 	kctl.tlv = ncontrol->tlv;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
@@ -1086,17 +1087,21 @@ static int snd_ctl_tlv_read(struct snd_c
                 err = -ENOENT;
                 goto __kctl_end;
         }
-        if (kctl->tlv == NULL) {
-                err = -ENXIO;
-                goto __kctl_end;
-        }
-        len = kctl->tlv[1] + 2 * sizeof(unsigned int);
-        if (tlv.length < len) {
-                err = -ENOMEM;
-                goto __kctl_end;
-        }
-        if (copy_to_user(_tlv->tlv, kctl->tlv, len))
-        	err = -EFAULT;
+	if (kctl->tlv_rw)
+		err = kctl->tlv_rw(kctl, tlv.length, _tlv->tlv); 
+	else {
+		if (kctl->tlv == NULL) {
+			err = -ENXIO;
+			goto __kctl_end;
+		}
+		len = kctl->tlv[1] + 2 * sizeof(unsigned int);
+		if (tlv.length < len) {
+			err = -ENOMEM;
+			goto __kctl_end;
+		}
+		if (copy_to_user(_tlv->tlv, kctl->tlv, len))
+			err = -EFAULT;
+	}
       __kctl_end:
         up_read(&card->controls_rwsem);
         return err;
diff -r 08577d0b45ef include/control.h
--- a/include/control.h	Tue Jun 13 12:01:14 2006 +0200
+++ b/include/control.h	Wed Jun 14 16:09:50 2006 +0200
@@ -30,6 +30,10 @@ typedef int (snd_kcontrol_info_t) (struc
 typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
 typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
 typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
+typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
+				    unsigned int size,
+				    unsigned int __user *tlv);
+
 
 struct snd_kcontrol_new {
 	snd_ctl_elem_iface_t iface;	/* interface identifier */
@@ -42,6 +46,7 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	snd_kcontrol_tlv_rw_t *tlv_rw;
 	unsigned int *tlv;
 	unsigned long private_value;
 };
@@ -59,6 +64,7 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	snd_kcontrol_tlv_rw_t *tlv_rw;
 	unsigned int *tlv;
 	unsigned long private_value;
 	void *private_data;

Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

[Takashi Iwai]

At Wed, 14 Jun 2006 16:14:17 +0200,
I wrote:
> 
> > What I am trying to say, is that your "simplification" of the API, has
> > resulted in this sort of functionality being excluded, so I would need
> > to implement yet another IOCTL to support it! My whole point of using
> > the full request/response TLV api, was to not restrict us. I was also
> > intending to maybe use the TLVs to set values, as well as just get
> > values. For example, another way to implement the "continuous gain"
> > feature would be to use the TLV api to set a flag, so that the 32bit
> > values are returned via the current snd_ca0106_volume_get(), but only
> > for this particular mixer open/close cycle, for backward compatibility
> > with the current api.
> > 
> > Do you prefer separate new IOCTLs for all these features?
> 
> We can modify the implementation.  For example, the patch like one
> below.  tlv_rw callback is completely free for own implementation.
> Each callback is in change of reading/writing the given user-space
> pointer arbitrarily.

After reconsideration, I found it's better to split the ioctl.
It's not nice that the caller has no idea whether the driver actually
reads the value or not.

Still I feel that the implementation of the callback is OK -- keep the
core stuff as small and simple as possible, and let callbacks parse
the user-pointer.


Takashi

Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

[Jaroslav Kysela]

On Fri, 16 Jun 2006, Takashi Iwai wrote:

> At Wed, 14 Jun 2006 16:14:17 +0200,
> I wrote:
> > 
> > > What I am trying to say, is that your "simplification" of the API, has
> > > resulted in this sort of functionality being excluded, so I would need
> > > to implement yet another IOCTL to support it! My whole point of using
> > > the full request/response TLV api, was to not restrict us. I was also
> > > intending to maybe use the TLVs to set values, as well as just get
> > > values. For example, another way to implement the "continuous gain"
> > > feature would be to use the TLV api to set a flag, so that the 32bit
> > > values are returned via the current snd_ca0106_volume_get(), but only
> > > for this particular mixer open/close cycle, for backward compatibility
> > > with the current api.
> > > 
> > > Do you prefer separate new IOCTLs for all these features?
> > 
> > We can modify the implementation.  For example, the patch like one
> > below.  tlv_rw callback is completely free for own implementation.
> > Each callback is in change of reading/writing the given user-space
> > pointer arbitrarily.
> 
> After reconsideration, I found it's better to split the ioctl.
> It's not nice that the caller has no idea whether the driver actually
> reads the value or not.

Yes, r/w ops should be splited (it's the reason why I used TLV_READ for 
ioctl).

> Still I feel that the implementation of the callback is OK -- keep the
> core stuff as small and simple as possible, and let callbacks parse
> the user-pointer.

I have some cleanup in my tree to reduce memory usage. See bellow. Also, 
while implementing, I realized that it might be worth to add 
SNDRV_CTL_ELEM_ACCESS_TLV_READ and SNDRV_CTL_ELEM_ACCESS_TLV_WRITE flags 
for control elements. Comments?

					Jaroslav

diff -r 08577d0b45ef core/control.c
--- a/core/control.c	Tue Jun 13 12:01:14 2006 +0200
+++ b/core/control.c	Fri Jun 16 12:57:54 2006 +0200
@@ -241,7 +241,7 @@ struct snd_kcontrol *snd_ctl_new1(const 
 	kctl.info = ncontrol->info;
 	kctl.get = ncontrol->get;
 	kctl.put = ncontrol->put;
-	kctl.tlv = ncontrol->tlv;
+	kctl.tlv.p = ncontrol->tlv.p;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
 	return snd_ctl_new(&kctl, access);
@@ -1068,8 +1068,9 @@ static int snd_ctl_subscribe_events(stru
 	return 0;
 }
 
-static int snd_ctl_tlv_read(struct snd_card *card,
-                            struct snd_ctl_tlv __user *_tlv)
+static int snd_ctl_tlv_ioctl(struct snd_card *card,
+                             struct snd_ctl_tlv __user *_tlv,
+                             int write_flag)
 {
 	struct snd_ctl_tlv tlv;
 	struct snd_kcontrol *kctl;
@@ -1086,17 +1087,26 @@ static int snd_ctl_tlv_read(struct snd_c
                 err = -ENOENT;
                 goto __kctl_end;
         }
-        if (kctl->tlv == NULL) {
-                err = -ENXIO;
-                goto __kctl_end;
-        }
-        len = kctl->tlv[1] + 2 * sizeof(unsigned int);
-        if (tlv.length < len) {
-                err = -ENOMEM;
-                goto __kctl_end;
-        }
-        if (copy_to_user(_tlv->tlv, kctl->tlv, len))
-        	err = -EFAULT;
+        if (kctl->tlv.p == NULL) {
+		err = -ENXIO;
+		goto __kctl_end;
+	}
+	if (kctl->vd[tlv.numid - kctl->id.numid].access &
+	    SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+		err = kctl->tlv.c(kctl, write_flag, tlv.length, _tlv->tlv); 
+	} else {
+	        if (write_flag) {
+	                err = -ENXIO;
+                        goto __kctl_end;
+                }
+		len = kctl->tlv.p[1] + 2 * sizeof(unsigned int);
+		if (tlv.length < len) {
+			err = -ENOMEM;
+			goto __kctl_end;
+		}
+		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
+			err = -EFAULT;
+	}
       __kctl_end:
         up_read(&card->controls_rwsem);
         return err;
@@ -1141,7 +1151,9 @@ static long snd_ctl_ioctl(struct file *f
 	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
 		return snd_ctl_subscribe_events(ctl, ip);
         case SNDRV_CTL_IOCTL_TLV_READ:
-                return snd_ctl_tlv_read(card, argp);
+                return snd_ctl_tlv_ioctl(card, argp, 0);
+        case SNDRV_CTL_IOCTL_TLV_WRITE:
+                return snd_ctl_tlv_ioctl(card, argp, 1);
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
diff -r 08577d0b45ef include/asound.h
--- a/include/asound.h	Tue Jun 13 12:01:14 2006 +0200
+++ b/include/asound.h	Fri Jun 16 12:57:54 2006 +0200
@@ -731,6 +731,7 @@ typedef int __bitwise snd_ctl_elem_iface
 #define SNDRV_CTL_ELEM_ACCESS_INACTIVE		(1<<8)	/* control does actually nothing, but may be updated */
 #define SNDRV_CTL_ELEM_ACCESS_LOCK		(1<<9)	/* write lock */
 #define SNDRV_CTL_ELEM_ACCESS_OWNER		(1<<10)	/* write lock owner */
+#define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK	(1<<28)	/* kernel use a TLV callback */ 
 #define SNDRV_CTL_ELEM_ACCESS_USER		(1<<29) /* user space element */
 #define SNDRV_CTL_ELEM_ACCESS_DINDIRECT		(1<<30)	/* indirect access for matrix dimensions in the info structure */
 #define SNDRV_CTL_ELEM_ACCESS_INDIRECT		(1<<31)	/* indirect access for element value in the value structure */
@@ -838,6 +839,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_REPLACE = _IOWR('U', 0x18, struct snd_ctl_elem_info),
 	SNDRV_CTL_IOCTL_ELEM_REMOVE = _IOWR('U', 0x19, struct snd_ctl_elem_id),
 	SNDRV_CTL_IOCTL_TLV_READ = _IOWR('U', 0x1a, struct snd_ctl_tlv),
+	SNDRV_CTL_IOCTL_TLV_WRITE = _IOWR('U', 0x1b, struct snd_ctl_tlv),
 	SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE = _IOWR('U', 0x20, int),
 	SNDRV_CTL_IOCTL_HWDEP_INFO = _IOR('U', 0x21, struct snd_hwdep_info),
 	SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE = _IOR('U', 0x30, int),
diff -r 08577d0b45ef include/control.h
--- a/include/control.h	Tue Jun 13 12:01:14 2006 +0200
+++ b/include/control.h	Fri Jun 16 12:57:54 2006 +0200
@@ -30,6 +30,11 @@ typedef int (snd_kcontrol_info_t) (struc
 typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
 typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
 typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
+typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
+				    int write_flag,
+				    unsigned int size,
+				    unsigned int __user *tlv);
+
 
 struct snd_kcontrol_new {
 	snd_ctl_elem_iface_t iface;	/* interface identifier */
@@ -42,7 +47,10 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
-	unsigned int *tlv;
+	union {
+        	snd_kcontrol_tlv_rw_t *c;
+        	unsigned int *p;
+        } tlv;
 	unsigned long private_value;
 };
 
@@ -59,7 +67,11 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
-	unsigned int *tlv;
+	snd_kcontrol_tlv_rw_t *tlv_rw;
+	union {
+	        snd_kcontrol_tlv_rw_t *c;
+        	unsigned int *p;
+        } tlv;
 	unsigned long private_value;
 	void *private_data;
 	void (*private_free)(struct snd_kcontrol *kcontrol);

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs

Re: [alsa-cvslog] alsa-kernel: Control API - TLV implementation for additional information like dB scale

[Takashi Iwai]

At Fri, 16 Jun 2006 13:00:57 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 16 Jun 2006, Takashi Iwai wrote:
> 
> > At Wed, 14 Jun 2006 16:14:17 +0200,
> > I wrote:
> > > 
> > > > What I am trying to say, is that your "simplification" of the API, has
> > > > resulted in this sort of functionality being excluded, so I would need
> > > > to implement yet another IOCTL to support it! My whole point of using
> > > > the full request/response TLV api, was to not restrict us. I was also
> > > > intending to maybe use the TLVs to set values, as well as just get
> > > > values. For example, another way to implement the "continuous gain"
> > > > feature would be to use the TLV api to set a flag, so that the 32bit
> > > > values are returned via the current snd_ca0106_volume_get(), but only
> > > > for this particular mixer open/close cycle, for backward compatibility
> > > > with the current api.
> > > > 
> > > > Do you prefer separate new IOCTLs for all these features?
> > > 
> > > We can modify the implementation.  For example, the patch like one
> > > below.  tlv_rw callback is completely free for own implementation.
> > > Each callback is in change of reading/writing the given user-space
> > > pointer arbitrarily.
> > 
> > After reconsideration, I found it's better to split the ioctl.
> > It's not nice that the caller has no idea whether the driver actually
> > reads the value or not.
> 
> Yes, r/w ops should be splited (it's the reason why I used TLV_READ for 
> ioctl).
> 
> > Still I feel that the implementation of the callback is OK -- keep the
> > core stuff as small and simple as possible, and let callbacks parse
> > the user-pointer.
> 
> I have some cleanup in my tree to reduce memory usage. See bellow. Also, 
> while implementing, I realized that it might be worth to add 
> SNDRV_CTL_ELEM_ACCESS_TLV_READ and SNDRV_CTL_ELEM_ACCESS_TLV_WRITE flags 
> for control elements. Comments?

Looks almost OK but white space problems.

Anyway, let's wait for the consensus.


Takashi

> 
> 					Jaroslav
> 
> diff -r 08577d0b45ef core/control.c
> --- a/core/control.c	Tue Jun 13 12:01:14 2006 +0200
> +++ b/core/control.c	Fri Jun 16 12:57:54 2006 +0200
> @@ -241,7 +241,7 @@ struct snd_kcontrol *snd_ctl_new1(const 
>  	kctl.info = ncontrol->info;
>  	kctl.get = ncontrol->get;
>  	kctl.put = ncontrol->put;
> -	kctl.tlv = ncontrol->tlv;
> +	kctl.tlv.p = ncontrol->tlv.p;
>  	kctl.private_value = ncontrol->private_value;
>  	kctl.private_data = private_data;
>  	return snd_ctl_new(&kctl, access);
> @@ -1068,8 +1068,9 @@ static int snd_ctl_subscribe_events(stru
>  	return 0;
>  }
>  
> -static int snd_ctl_tlv_read(struct snd_card *card,
> -                            struct snd_ctl_tlv __user *_tlv)
> +static int snd_ctl_tlv_ioctl(struct snd_card *card,
> +                             struct snd_ctl_tlv __user *_tlv,
> +                             int write_flag)
>  {
>  	struct snd_ctl_tlv tlv;
>  	struct snd_kcontrol *kctl;
> @@ -1086,17 +1087,26 @@ static int snd_ctl_tlv_read(struct snd_c
>                  err = -ENOENT;
>                  goto __kctl_end;
>          }
> -        if (kctl->tlv == NULL) {
> -                err = -ENXIO;
> -                goto __kctl_end;
> -        }
> -        len = kctl->tlv[1] + 2 * sizeof(unsigned int);
> -        if (tlv.length < len) {
> -                err = -ENOMEM;
> -                goto __kctl_end;
> -        }
> -        if (copy_to_user(_tlv->tlv, kctl->tlv, len))
> -        	err = -EFAULT;
> +        if (kctl->tlv.p == NULL) {
> +		err = -ENXIO;
> +		goto __kctl_end;
> +	}
> +	if (kctl->vd[tlv.numid - kctl->id.numid].access &
> +	    SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> +		err = kctl->tlv.c(kctl, write_flag, tlv.length, _tlv->tlv); 
> +	} else {
> +	        if (write_flag) {
> +	                err = -ENXIO;
> +                        goto __kctl_end;
> +                }
> +		len = kctl->tlv.p[1] + 2 * sizeof(unsigned int);
> +		if (tlv.length < len) {
> +			err = -ENOMEM;
> +			goto __kctl_end;
> +		}
> +		if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
> +			err = -EFAULT;
> +	}
>        __kctl_end:
>          up_read(&card->controls_rwsem);
>          return err;
> @@ -1141,7 +1151,9 @@ static long snd_ctl_ioctl(struct file *f
>  	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
>  		return snd_ctl_subscribe_events(ctl, ip);
>          case SNDRV_CTL_IOCTL_TLV_READ:
> -                return snd_ctl_tlv_read(card, argp);
> +                return snd_ctl_tlv_ioctl(card, argp, 0);
> +        case SNDRV_CTL_IOCTL_TLV_WRITE:
> +                return snd_ctl_tlv_ioctl(card, argp, 1);
>  	case SNDRV_CTL_IOCTL_POWER:
>  		return -ENOPROTOOPT;
>  	case SNDRV_CTL_IOCTL_POWER_STATE:
> diff -r 08577d0b45ef include/asound.h
> --- a/include/asound.h	Tue Jun 13 12:01:14 2006 +0200
> +++ b/include/asound.h	Fri Jun 16 12:57:54 2006 +0200
> @@ -731,6 +731,7 @@ typedef int __bitwise snd_ctl_elem_iface
>  #define SNDRV_CTL_ELEM_ACCESS_INACTIVE		(1<<8)	/* control does actually nothing, but may be updated */
>  #define SNDRV_CTL_ELEM_ACCESS_LOCK		(1<<9)	/* write lock */
>  #define SNDRV_CTL_ELEM_ACCESS_OWNER		(1<<10)	/* write lock owner */
> +#define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK	(1<<28)	/* kernel use a TLV callback */ 
>  #define SNDRV_CTL_ELEM_ACCESS_USER		(1<<29) /* user space element */
>  #define SNDRV_CTL_ELEM_ACCESS_DINDIRECT		(1<<30)	/* indirect access for matrix dimensions in the info structure */
>  #define SNDRV_CTL_ELEM_ACCESS_INDIRECT		(1<<31)	/* indirect access for element value in the value structure */
> @@ -838,6 +839,7 @@ enum {
>  	SNDRV_CTL_IOCTL_ELEM_REPLACE = _IOWR('U', 0x18, struct snd_ctl_elem_info),
>  	SNDRV_CTL_IOCTL_ELEM_REMOVE = _IOWR('U', 0x19, struct snd_ctl_elem_id),
>  	SNDRV_CTL_IOCTL_TLV_READ = _IOWR('U', 0x1a, struct snd_ctl_tlv),
> +	SNDRV_CTL_IOCTL_TLV_WRITE = _IOWR('U', 0x1b, struct snd_ctl_tlv),
>  	SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE = _IOWR('U', 0x20, int),
>  	SNDRV_CTL_IOCTL_HWDEP_INFO = _IOR('U', 0x21, struct snd_hwdep_info),
>  	SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE = _IOR('U', 0x30, int),
> diff -r 08577d0b45ef include/control.h
> --- a/include/control.h	Tue Jun 13 12:01:14 2006 +0200
> +++ b/include/control.h	Fri Jun 16 12:57:54 2006 +0200
> @@ -30,6 +30,11 @@ typedef int (snd_kcontrol_info_t) (struc
>  typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
>  typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
>  typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
> +typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
> +				    int write_flag,
> +				    unsigned int size,
> +				    unsigned int __user *tlv);
> +
>  
>  struct snd_kcontrol_new {
>  	snd_ctl_elem_iface_t iface;	/* interface identifier */
> @@ -42,7 +47,10 @@ struct snd_kcontrol_new {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> -	unsigned int *tlv;
> +	union {
> +        	snd_kcontrol_tlv_rw_t *c;
> +        	unsigned int *p;
> +        } tlv;
>  	unsigned long private_value;
>  };
>  
> @@ -59,7 +67,11 @@ struct snd_kcontrol {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> -	unsigned int *tlv;
> +	snd_kcontrol_tlv_rw_t *tlv_rw;
> +	union {
> +	        snd_kcontrol_tlv_rw_t *c;
> +        	unsigned int *p;
> +        } tlv;
>  	unsigned long private_value;
>  	void *private_data;
>  	void (*private_free)(struct snd_kcontrol *kcontrol);
> 
> -----
> Jaroslav Kysela <perex@suse.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, SUSE Labs
>