RFC: minimalistic TLV implementation

RFC: minimalistic TLV implementation

[Jaroslav Kysela]

Hi,

	I've created a minimalistic TLV implemetation for the driver side. 
Please, review and comment it. We can add more TLV funcionality in future.
It is essential that user space will handle SNDRV_CTL_TLVT_LEVEL type 
immediately (altough it is not used in the kernel space now), otherwise we 
cannot group and level TLVs in future.

						Jaroslav

diff -r 96e63842ba5d core/control.c
--- a/core/control.c	Wed May 31 11:55:17 2006 +0200
+++ b/core/control.c	Wed May 31 15:02:38 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 = ncontrol->tlv;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
 	return snd_ctl_new(&kctl, access);
@@ -1067,6 +1068,44 @@ static int snd_ctl_subscribe_events(stru
 	return 0;
 }
 
+static int snd_ctl_tlv(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_to_user(&tlv, _tlv, sizeof(tlv)))
+		return -EFAULT;
+        if (tlv.length < sizeof(unsigned int) * 3)
+                return -EINVAL;
+        if (tlv.flags & SNDRV_CTL_TLV_NUMID) {
+        	down_read(&card->controls_rwsem);
+                kctl = snd_ctl_find_numid(card, tlv.value);
+                if (kctl == NULL) {
+                        err = -ENOENT;
+                        goto __kctl_end;
+                }
+                if (kctl->tlv == NULL) {
+                        err = -EIO;
+                        goto __kctl_end;
+                }
+                len = *((unsigned int *)kctl->tlv);
+                if (tlv.length < len) {
+                        err = -ENOMEM;
+                        goto __kctl_end;
+                }
+        	if (copy_to_user(tlv.ptr, kctl->tlv, len))
+        		err = -EFAULT;
+              __kctl_end:
+        	up_read(&card->controls_rwsem);
+        } else {
+                /* not implemented */
+                err = -EINVAL;
+        }
+        return err;
+}
+
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct snd_ctl_file *ctl;
@@ -1086,11 +1125,11 @@ static long snd_ctl_ioctl(struct file *f
 	case SNDRV_CTL_IOCTL_CARD_INFO:
 		return snd_ctl_card_info(card, ctl, cmd, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST:
-		return snd_ctl_elem_list(ctl->card, argp);
+		return snd_ctl_elem_list(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
-		return snd_ctl_elem_read_user(ctl->card, argp);
+		return snd_ctl_elem_read_user(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE:
 		return snd_ctl_elem_write_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LOCK:
@@ -1105,6 +1144,8 @@ static long snd_ctl_ioctl(struct file *f
 		return snd_ctl_elem_remove(ctl, argp);
 	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
 		return snd_ctl_subscribe_events(ctl, ip);
+        case SNDRV_CTL_IOCTL_TLV:
+                return snd_ctl_tlv(card, argp);
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
diff -r 96e63842ba5d include/asound.h
--- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/asound.h	Wed May 31 15:02:38 2006 +0200
@@ -818,6 +818,26 @@ struct snd_ctl_elem_value {
         unsigned char reserved[128-sizeof(struct timespec)];
 };
 
+#define SNDRV_CTL_TLV_NUMID	(1<<0)	/* value is numid, otherwise request global TLV info */
+
+#define SNDRV_CTL_TLVT_LEVEL	0	/* one level down - group of TLVs */
+#define SNDRV_CTL_TLVT_DB_SCALE	1	/* dB scale */
+
+struct snd_ctl_tlv {
+        unsigned int flags;
+        unsigned int length;
+        unsigned int value;
+        /*
+         * TLV structure is:
+         *   unsigned int length
+         *   unsigned int type	- see SNDRV_CTL_TLVT_*
+         *   .... data aligned to sizeof(unsigned int), use
+         *        block_length = (length + (sizeof(unsigned int) - 1)) &
+         *                       ~(sizeof(unsigned int) - 1)) ....
+         */
+        void *ptr;
+};
+
 enum {
 	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
 	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
@@ -831,6 +851,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_ADD = _IOWR('U', 0x17, struct snd_ctl_elem_info),
 	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 = _IOWR('U', 0x1a, 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 96e63842ba5d include/control.h
--- a/include/control.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/control.h	Wed May 31 15:02:38 2006 +0200
@@ -42,6 +42,7 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 };
 
@@ -58,6 +59,7 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 	void *private_data;
 	void (*private_free)(struct snd_kcontrol *kcontrol);
diff -r 96e63842ba5d pci/ca0106/ca0106_mixer.c
--- a/pci/ca0106/ca0106_mixer.c	Wed May 31 11:55:17 2006 +0200
+++ b/pci/ca0106/ca0106_mixer.c	Wed May 31 15:02:38 2006 +0200
@@ -70,8 +70,11 @@
 #include <sound/pcm.h>
 #include <sound/ac97_codec.h>
 #include <sound/info.h>
+#include <sound/tlv.h>
 
 #include "ca0106.h"
+
+static DECLARE_TLV_DB_SCALE(snd_ca0106_db_scale, -5150, 75, 1);
 
 static int snd_ca0106_shared_spdif_info(struct snd_kcontrol *kcontrol,
 					struct snd_ctl_elem_info *uinfo)
@@ -472,6 +475,7 @@ static int snd_ca0106_i2c_volume_put(str
 	.info =	 snd_ca0106_volume_info,			\
 	.get =   snd_ca0106_volume_get,				\
 	.put =   snd_ca0106_volume_put,				\
+	.tlv =	 snd_ca0106_db_scale,				\
 	.private_value = ((chid) << 8) | (reg)			\
 }
 
diff -r 96e63842ba5d include/tlv.h
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/include/tlv.h	Wed May 31 15:02:38 2006 +0200
@@ -0,0 +1,31 @@
+#ifndef __SOUND_TLV_H
+#define __SOUND_TLV_H
+
+/*
+ *  Advanced Linux Sound Architecture - ALSA - Driver
+ *  Copyright (c) 2006 by Jaroslav Kysela <perex@suse.cz>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#define DECLARE_TLV_DB_SCALE(name, min, step, mute) \
+unsigned int name[] = { \
+        4 * sizeof(unsigned int), SNDRV_CTL_TLVT_DB_SCALE, \
+        (min), (step & 0xffff) | (mute ? 0x10000 : 0) \
+}
+
+#endif /* __SOUND_TLV_H */

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

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Wed, 31 May 2006 15:10:21 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> Hi,
> 
> 	I've created a minimalistic TLV implemetation for the driver side. 
> Please, review and comment it. We can add more TLV funcionality in future.
> It is essential that user space will handle SNDRV_CTL_TLVT_LEVEL type 
> immediately (altough it is not used in the kernel space now), otherwise we 
> cannot group and level TLVs in future.

Please never ever include pointers in the ioctl struct.
That's to be avoided as much as possible.


Takashi

> 
> 						Jaroslav
> 
> diff -r 96e63842ba5d core/control.c
> --- a/core/control.c	Wed May 31 11:55:17 2006 +0200
> +++ b/core/control.c	Wed May 31 15:02:38 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 = ncontrol->tlv;
>  	kctl.private_value = ncontrol->private_value;
>  	kctl.private_data = private_data;
>  	return snd_ctl_new(&kctl, access);
> @@ -1067,6 +1068,44 @@ static int snd_ctl_subscribe_events(stru
>  	return 0;
>  }
>  
> +static int snd_ctl_tlv(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_to_user(&tlv, _tlv, sizeof(tlv)))
> +		return -EFAULT;
> +        if (tlv.length < sizeof(unsigned int) * 3)
> +                return -EINVAL;
> +        if (tlv.flags & SNDRV_CTL_TLV_NUMID) {
> +        	down_read(&card->controls_rwsem);
> +                kctl = snd_ctl_find_numid(card, tlv.value);
> +                if (kctl == NULL) {
> +                        err = -ENOENT;
> +                        goto __kctl_end;
> +                }
> +                if (kctl->tlv == NULL) {
> +                        err = -EIO;
> +                        goto __kctl_end;
> +                }
> +                len = *((unsigned int *)kctl->tlv);
> +                if (tlv.length < len) {
> +                        err = -ENOMEM;
> +                        goto __kctl_end;
> +                }
> +        	if (copy_to_user(tlv.ptr, kctl->tlv, len))
> +        		err = -EFAULT;
> +              __kctl_end:
> +        	up_read(&card->controls_rwsem);
> +        } else {
> +                /* not implemented */
> +                err = -EINVAL;
> +        }
> +        return err;
> +}
> +
>  static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct snd_ctl_file *ctl;
> @@ -1086,11 +1125,11 @@ static long snd_ctl_ioctl(struct file *f
>  	case SNDRV_CTL_IOCTL_CARD_INFO:
>  		return snd_ctl_card_info(card, ctl, cmd, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_LIST:
> -		return snd_ctl_elem_list(ctl->card, argp);
> +		return snd_ctl_elem_list(card, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_INFO:
>  		return snd_ctl_elem_info_user(ctl, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_READ:
> -		return snd_ctl_elem_read_user(ctl->card, argp);
> +		return snd_ctl_elem_read_user(card, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_WRITE:
>  		return snd_ctl_elem_write_user(ctl, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_LOCK:
> @@ -1105,6 +1144,8 @@ static long snd_ctl_ioctl(struct file *f
>  		return snd_ctl_elem_remove(ctl, argp);
>  	case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
>  		return snd_ctl_subscribe_events(ctl, ip);
> +        case SNDRV_CTL_IOCTL_TLV:
> +                return snd_ctl_tlv(card, argp);
>  	case SNDRV_CTL_IOCTL_POWER:
>  		return -ENOPROTOOPT;
>  	case SNDRV_CTL_IOCTL_POWER_STATE:
> diff -r 96e63842ba5d include/asound.h
> --- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
> +++ b/include/asound.h	Wed May 31 15:02:38 2006 +0200
> @@ -818,6 +818,26 @@ struct snd_ctl_elem_value {
>          unsigned char reserved[128-sizeof(struct timespec)];
>  };
>  
> +#define SNDRV_CTL_TLV_NUMID	(1<<0)	/* value is numid, otherwise request global TLV info */
> +
> +#define SNDRV_CTL_TLVT_LEVEL	0	/* one level down - group of TLVs */
> +#define SNDRV_CTL_TLVT_DB_SCALE	1	/* dB scale */
> +
> +struct snd_ctl_tlv {
> +        unsigned int flags;
> +        unsigned int length;
> +        unsigned int value;
> +        /*
> +         * TLV structure is:
> +         *   unsigned int length
> +         *   unsigned int type	- see SNDRV_CTL_TLVT_*
> +         *   .... data aligned to sizeof(unsigned int), use
> +         *        block_length = (length + (sizeof(unsigned int) - 1)) &
> +         *                       ~(sizeof(unsigned int) - 1)) ....
> +         */
> +        void *ptr;
> +};
> +
>  enum {
>  	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
>  	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
> @@ -831,6 +851,7 @@ enum {
>  	SNDRV_CTL_IOCTL_ELEM_ADD = _IOWR('U', 0x17, struct snd_ctl_elem_info),
>  	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 = _IOWR('U', 0x1a, 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 96e63842ba5d include/control.h
> --- a/include/control.h	Wed May 31 11:55:17 2006 +0200
> +++ b/include/control.h	Wed May 31 15:02:38 2006 +0200
> @@ -42,6 +42,7 @@ struct snd_kcontrol_new {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> +	unsigned int *tlv;
>  	unsigned long private_value;
>  };
>  
> @@ -58,6 +59,7 @@ struct snd_kcontrol {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> +	unsigned int *tlv;
>  	unsigned long private_value;
>  	void *private_data;
>  	void (*private_free)(struct snd_kcontrol *kcontrol);
> diff -r 96e63842ba5d pci/ca0106/ca0106_mixer.c
> --- a/pci/ca0106/ca0106_mixer.c	Wed May 31 11:55:17 2006 +0200
> +++ b/pci/ca0106/ca0106_mixer.c	Wed May 31 15:02:38 2006 +0200
> @@ -70,8 +70,11 @@
>  #include <sound/pcm.h>
>  #include <sound/ac97_codec.h>
>  #include <sound/info.h>
> +#include <sound/tlv.h>
>  
>  #include "ca0106.h"
> +
> +static DECLARE_TLV_DB_SCALE(snd_ca0106_db_scale, -5150, 75, 1);
>  
>  static int snd_ca0106_shared_spdif_info(struct snd_kcontrol *kcontrol,
>  					struct snd_ctl_elem_info *uinfo)
> @@ -472,6 +475,7 @@ static int snd_ca0106_i2c_volume_put(str
>  	.info =	 snd_ca0106_volume_info,			\
>  	.get =   snd_ca0106_volume_get,				\
>  	.put =   snd_ca0106_volume_put,				\
> +	.tlv =	 snd_ca0106_db_scale,				\
>  	.private_value = ((chid) << 8) | (reg)			\
>  }
>  
> diff -r 96e63842ba5d include/tlv.h
> --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
> +++ b/include/tlv.h	Wed May 31 15:02:38 2006 +0200
> @@ -0,0 +1,31 @@
> +#ifndef __SOUND_TLV_H
> +#define __SOUND_TLV_H
> +
> +/*
> + *  Advanced Linux Sound Architecture - ALSA - Driver
> + *  Copyright (c) 2006 by Jaroslav Kysela <perex@suse.cz>
> + *
> + *
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> + *
> + */
> +
> +#define DECLARE_TLV_DB_SCALE(name, min, step, mute) \
> +unsigned int name[] = { \
> +        4 * sizeof(unsigned int), SNDRV_CTL_TLVT_DB_SCALE, \
> +        (min), (step & 0xffff) | (mute ? 0x10000 : 0) \
> +}
> +
> +#endif /* __SOUND_TLV_H */
> 
> -----
> Jaroslav Kysela <perex@suse.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, SUSE Labs
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/alsa-devel
> 

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1500 bytes --]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> At Wed, 31 May 2006 15:10:21 +0200 (CEST),
> Jaroslav Kysela wrote:
> > 
> > Hi,
> > 
> > 	I've created a minimalistic TLV implemetation for the driver side. 
> > Please, review and comment it. We can add more TLV funcionality in future.
> > It is essential that user space will handle SNDRV_CTL_TLVT_LEVEL type 
> > immediately (altough it is not used in the kernel space now), otherwise we 
> > cannot group and level TLVs in future.
> 
> Please never ever include pointers in the ioctl struct.
> That's to be avoided as much as possible.

> > +struct snd_ctl_tlv {
> > +        unsigned int flags;
> > +        unsigned int length;
> > +        unsigned int value;
> > +        /*
> > +         * TLV structure is:
> > +         *   unsigned int length
> > +         *   unsigned int type	- see SNDRV_CTL_TLVT_*
> > +         *   .... data aligned to sizeof(unsigned int), use
> > +         *        block_length = (length + (sizeof(unsigned int) - 1)) &
> > +         *                       ~(sizeof(unsigned int) - 1)) ....
> > +         */
> > +        void *ptr;
> > +};

So what you suggest? Something like 'struct dm_ioctl' 
(include/linux/dm-ioctl.h) does? Data are stored after ioctl struct.
It's nearly similar.

						Jaroslav

-----
InterNet přes WIFI bez limitu dat, České Budějovice a okolí
Pokryté lokality: Šindlovy Dvory, Mokré, Litvínovice, Pekárenská, Srubec
Perex @ InterNet <internet@perex.cz>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 15:12:05 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 1 Jun 2006, Takashi Iwai wrote:
> 
> > At Wed, 31 May 2006 15:10:21 +0200 (CEST),
> > Jaroslav Kysela wrote:
> > > 
> > > Hi,
> > > 
> > > 	I've created a minimalistic TLV implemetation for the driver side. 
> > > Please, review and comment it. We can add more TLV funcionality in future.
> > > It is essential that user space will handle SNDRV_CTL_TLVT_LEVEL type 
> > > immediately (altough it is not used in the kernel space now), otherwise we 
> > > cannot group and level TLVs in future.
> > 
> > Please never ever include pointers in the ioctl struct.
> > That's to be avoided as much as possible.
> 
> > > +struct snd_ctl_tlv {
> > > +        unsigned int flags;
> > > +        unsigned int length;
> > > +        unsigned int value;
> > > +        /*
> > > +         * TLV structure is:
> > > +         *   unsigned int length
> > > +         *   unsigned int type	- see SNDRV_CTL_TLVT_*
> > > +         *   .... data aligned to sizeof(unsigned int), use
> > > +         *        block_length = (length + (sizeof(unsigned int) - 1)) &
> > > +         *                       ~(sizeof(unsigned int) - 1)) ....
> > > +         */
> > > +        void *ptr;
> > > +};
> 
> So what you suggest? Something like 'struct dm_ioctl' 
> (include/linux/dm-ioctl.h) does? Data are stored after ioctl struct.
> It's nearly similar.

Yes.  It's similar but a really big difference if you consider about
32/64bit compatibility.

Also, the order of "TLV" is type-length-value.


Takashi

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> Yes.  It's similar but a really big difference if you consider about
> 32/64bit compatibility.
> 
> Also, the order of "TLV" is type-length-value.

Ok, the second version of my patch. Note: in the ca0106 driver, only
three lines (if I don't count one empty line) are required to add
the dB scale information.

					Jaroslav

diff -r 96e63842ba5d core/control.c
--- a/core/control.c	Wed May 31 11:55:17 2006 +0200
+++ b/core/control.c	Thu Jun  1 16:04:37 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 = ncontrol->tlv;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
 	return snd_ctl_new(&kctl, access);
@@ -1067,6 +1068,46 @@ 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)
+{
+	struct snd_ctl_tlv tlv;
+	struct snd_kcontrol *kctl;
+	unsigned int len;
+	int err = 0;
+
+	if (copy_to_user(&tlv, _tlv, sizeof(tlv)))
+		return -EFAULT;
+        if (tlv.length < sizeof(unsigned int) * 3)
+                return -EINVAL;
+        if (tlv.flags & SNDRV_CTL_TLV_NUMID) {
+        	down_read(&card->controls_rwsem);
+                kctl = snd_ctl_find_numid(card, tlv.ident);
+                if (kctl == NULL) {
+                        err = -ENOENT;
+                        goto __kctl_end;
+                }
+                if (kctl->tlv == NULL) {
+                        err = -EIO;
+                        goto __kctl_end;
+                }
+                len = *(((unsigned int *)kctl->tlv) + 1) +
+                      2 * sizeof(unsigned int);
+                if (tlv.length < len) {
+                        err = -ENOMEM;
+                        goto __kctl_end;
+                }
+        	if (copy_to_user((unsigned char *)_tlv + tlv.data_offset,
+        	                 kctl->tlv, len))
+        		err = -EFAULT;
+              __kctl_end:
+        	up_read(&card->controls_rwsem);
+        } else {
+                /* not implemented */
+                err = -EINVAL;
+        }
+        return err;
+}
+
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct snd_ctl_file *ctl;
@@ -1086,11 +1127,11 @@ static long snd_ctl_ioctl(struct file *f
 	case SNDRV_CTL_IOCTL_CARD_INFO:
 		return snd_ctl_card_info(card, ctl, cmd, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST:
-		return snd_ctl_elem_list(ctl->card, argp);
+		return snd_ctl_elem_list(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
-		return snd_ctl_elem_read_user(ctl->card, argp);
+		return snd_ctl_elem_read_user(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE:
 		return snd_ctl_elem_write_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LOCK:
@@ -1105,6 +1146,8 @@ static long snd_ctl_ioctl(struct file *f
 		return snd_ctl_elem_remove(ctl, argp);
 	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);
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
diff -r 96e63842ba5d include/asound.h
--- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/asound.h	Thu Jun  1 16:04:37 2006 +0200
@@ -818,6 +818,26 @@ struct snd_ctl_elem_value {
         unsigned char reserved[128-sizeof(struct timespec)];
 };
 
+#define SNDRV_CTL_TLV_NUMID	(1<<0)	/* ident is numid, otherwise request global TLV info */
+
+#define SNDRV_CTL_TLVT_LEVEL	0	/* one level down - group of TLVs */
+#define SNDRV_CTL_TLVT_DB_SCALE	1	/* dB scale */
+
+struct snd_ctl_tlv {
+        unsigned int flags;
+        unsigned int length;
+        unsigned int ident;
+        /*
+         * TLV structure is:
+         *   unsigned int type	- see SNDRV_CTL_TLVT_*
+         *   unsigned int length
+         *   .... data aligned to sizeof(unsigned int), use
+         *        block_length = (length + (sizeof(unsigned int) - 1)) &
+         *                       ~(sizeof(unsigned int) - 1)) ....
+         */
+        unsigned int data_offset;	/* offset to data in bytes behind ioctl structure */
+};
+
 enum {
 	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
 	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
@@ -831,6 +851,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_ADD = _IOWR('U', 0x17, struct snd_ctl_elem_info),
 	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_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 96e63842ba5d include/control.h
--- a/include/control.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/control.h	Thu Jun  1 16:04:37 2006 +0200
@@ -42,6 +42,7 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 };
 
@@ -58,6 +59,7 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 	void *private_data;
 	void (*private_free)(struct snd_kcontrol *kcontrol);
diff -r 96e63842ba5d pci/ca0106/ca0106_mixer.c
--- a/pci/ca0106/ca0106_mixer.c	Wed May 31 11:55:17 2006 +0200
+++ b/pci/ca0106/ca0106_mixer.c	Thu Jun  1 16:04:37 2006 +0200
@@ -70,8 +70,11 @@
 #include <sound/pcm.h>
 #include <sound/ac97_codec.h>
 #include <sound/info.h>
+#include <sound/tlv.h>
 
 #include "ca0106.h"
+
+static DECLARE_TLV_DB_SCALE(snd_ca0106_db_scale, -5150, 75, 1);
 
 static int snd_ca0106_shared_spdif_info(struct snd_kcontrol *kcontrol,
 					struct snd_ctl_elem_info *uinfo)
@@ -472,6 +475,7 @@ static int snd_ca0106_i2c_volume_put(str
 	.info =	 snd_ca0106_volume_info,			\
 	.get =   snd_ca0106_volume_get,				\
 	.put =   snd_ca0106_volume_put,				\
+	.tlv =	 snd_ca0106_db_scale,				\
 	.private_value = ((chid) << 8) | (reg)			\
 }
 
diff -r 96e63842ba5d include/tlv.h
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/include/tlv.h	Thu Jun  1 16:04:37 2006 +0200
@@ -0,0 +1,31 @@
+#ifndef __SOUND_TLV_H
+#define __SOUND_TLV_H
+
+/*
+ *  Advanced Linux Sound Architecture - ALSA - Driver
+ *  Copyright (c) 2006 by Jaroslav Kysela <perex@suse.cz>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#define DECLARE_TLV_DB_SCALE(name, min, step, mute) \
+unsigned int name[] = { \
+        SNDRV_CTL_TLVT_DB_SCALE, 2 * sizeof(unsigned int), \
+        (min), ((step) & 0xffff) | ((mute) ? 0x10000 : 0) \
+}
+
+#endif /* __SOUND_TLV_H */

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

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 16:08:13 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 1 Jun 2006, Takashi Iwai wrote:
> 
> > Yes.  It's similar but a really big difference if you consider about
> > 32/64bit compatibility.
> > 
> > Also, the order of "TLV" is type-length-value.
> 
> Ok, the second version of my patch. Note: in the ca0106 driver, only
> three lines (if I don't count one empty line) are required to add
> the dB scale information.

The existence of flag makes the things complicated.
I prefer removing flag and make the ioctl numid-specific.
Also, I don't think data_offset is necessary in the practical use.

struct snd_ctl_tlv {
	u32 length;
	u32 numid;
	/* TLV data is stored in the following like the format below */
};

/* TLV data:
   u32 type;
   u32 length;
   u32 values[];
    length is always aligned to 4 bytes
 */


> 
> 					Jaroslav
> 
> diff -r 96e63842ba5d core/control.c
> --- a/core/control.c	Wed May 31 11:55:17 2006 +0200
> +++ b/core/control.c	Thu Jun  1 16:04:37 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 = ncontrol->tlv;
>  	kctl.private_value = ncontrol->private_value;
>  	kctl.private_data = private_data;
>  	return snd_ctl_new(&kctl, access);
> @@ -1067,6 +1068,46 @@ 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)

Too long line :)

> +{
> +	struct snd_ctl_tlv tlv;
> +	struct snd_kcontrol *kctl;
> +	unsigned int len;
> +	int err = 0;
> +
> +	if (copy_to_user(&tlv, _tlv, sizeof(tlv)))

copy_from_user()

> +		return -EFAULT;
> +        if (tlv.length < sizeof(unsigned int) * 3)
> +                return -EINVAL;
> +        if (tlv.flags & SNDRV_CTL_TLV_NUMID) {
> +        	down_read(&card->controls_rwsem);
> +                kctl = snd_ctl_find_numid(card, tlv.ident);
> +                if (kctl == NULL) {
> +                        err = -ENOENT;
> +                        goto __kctl_end;
> +                }
> +                if (kctl->tlv == NULL) {
> +                        err = -EIO;

-ENXIO sounds better to me.

> +                        goto __kctl_end;
> +                }
> +                len = *(((unsigned int *)kctl->tlv) + 1) +
> +                      2 * sizeof(unsigned int);

len = kctl->tlv[1] + 4;

> +                if (tlv.length < len) {
> +                        err = -ENOMEM;
> +                        goto __kctl_end;
> +                }
> +        	if (copy_to_user((unsigned char *)_tlv + tlv.data_offset,
> +        	                 kctl->tlv, len))
> +        		err = -EFAULT;
> +              __kctl_end:
> +        	up_read(&card->controls_rwsem);
> +        } else {
> +                /* not implemented */
> +                err = -EINVAL;
> +        }
> +        return err;
> +}
> +
>  static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct snd_ctl_file *ctl;
> @@ -1086,11 +1127,11 @@ static long snd_ctl_ioctl(struct file *f
>  	case SNDRV_CTL_IOCTL_CARD_INFO:
>  		return snd_ctl_card_info(card, ctl, cmd, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_LIST:
> -		return snd_ctl_elem_list(ctl->card, argp);
> +		return snd_ctl_elem_list(card, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_INFO:
>  		return snd_ctl_elem_info_user(ctl, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_READ:
> -		return snd_ctl_elem_read_user(ctl->card, argp);
> +		return snd_ctl_elem_read_user(card, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_WRITE:
>  		return snd_ctl_elem_write_user(ctl, argp);
>  	case SNDRV_CTL_IOCTL_ELEM_LOCK:
> @@ -1105,6 +1146,8 @@ static long snd_ctl_ioctl(struct file *f
>  		return snd_ctl_elem_remove(ctl, argp);
>  	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);
>  	case SNDRV_CTL_IOCTL_POWER:
>  		return -ENOPROTOOPT;
>  	case SNDRV_CTL_IOCTL_POWER_STATE:
> diff -r 96e63842ba5d include/asound.h
> --- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
> +++ b/include/asound.h	Thu Jun  1 16:04:37 2006 +0200
> @@ -818,6 +818,26 @@ struct snd_ctl_elem_value {
>          unsigned char reserved[128-sizeof(struct timespec)];
>  };
>  
> +#define SNDRV_CTL_TLV_NUMID	(1<<0)	/* ident is numid, otherwise request global TLV info */
> +
> +#define SNDRV_CTL_TLVT_LEVEL	0	/* one level down - group of TLVs */

CONTAINER?

> +#define SNDRV_CTL_TLVT_DB_SCALE	1	/* dB scale */
> +
> +struct snd_ctl_tlv {
> +        unsigned int flags;
> +        unsigned int length;
> +        unsigned int ident;
> +        /*
> +         * TLV structure is:
> +         *   unsigned int type	- see SNDRV_CTL_TLVT_*
> +         *   unsigned int length
> +         *   .... data aligned to sizeof(unsigned int), use
> +         *        block_length = (length + (sizeof(unsigned int) - 1)) &
> +         *                       ~(sizeof(unsigned int) - 1)) ....
> +         */
> +        unsigned int data_offset;	/* offset to data in bytes behind ioctl structure */
> +};
> +
>  enum {
>  	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
>  	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
> @@ -831,6 +851,7 @@ enum {
>  	SNDRV_CTL_IOCTL_ELEM_ADD = _IOWR('U', 0x17, struct snd_ctl_elem_info),
>  	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_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 96e63842ba5d include/control.h
> --- a/include/control.h	Wed May 31 11:55:17 2006 +0200
> +++ b/include/control.h	Thu Jun  1 16:04:37 2006 +0200
> @@ -42,6 +42,7 @@ struct snd_kcontrol_new {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> +	unsigned int *tlv;
>  	unsigned long private_value;
>  };
>  
> @@ -58,6 +59,7 @@ struct snd_kcontrol {
>  	snd_kcontrol_info_t *info;
>  	snd_kcontrol_get_t *get;
>  	snd_kcontrol_put_t *put;
> +	unsigned int *tlv;
>  	unsigned long private_value;
>  	void *private_data;
>  	void (*private_free)(struct snd_kcontrol *kcontrol);
> diff -r 96e63842ba5d pci/ca0106/ca0106_mixer.c
> --- a/pci/ca0106/ca0106_mixer.c	Wed May 31 11:55:17 2006 +0200
> +++ b/pci/ca0106/ca0106_mixer.c	Thu Jun  1 16:04:37 2006 +0200
> @@ -70,8 +70,11 @@
>  #include <sound/pcm.h>
>  #include <sound/ac97_codec.h>
>  #include <sound/info.h>
> +#include <sound/tlv.h>
>  
>  #include "ca0106.h"
> +
> +static DECLARE_TLV_DB_SCALE(snd_ca0106_db_scale, -5150, 75, 1);
>  
>  static int snd_ca0106_shared_spdif_info(struct snd_kcontrol *kcontrol,
>  					struct snd_ctl_elem_info *uinfo)
> @@ -472,6 +475,7 @@ static int snd_ca0106_i2c_volume_put(str
>  	.info =	 snd_ca0106_volume_info,			\
>  	.get =   snd_ca0106_volume_get,				\
>  	.put =   snd_ca0106_volume_put,				\
> +	.tlv =	 snd_ca0106_db_scale,				\
>  	.private_value = ((chid) << 8) | (reg)			\
>  }
>  
> diff -r 96e63842ba5d include/tlv.h
> --- /dev/null	Thu Jan  1 00:00:00 1970 +0000
> +++ b/include/tlv.h	Thu Jun  1 16:04:37 2006 +0200
> @@ -0,0 +1,31 @@
> +#ifndef __SOUND_TLV_H
> +#define __SOUND_TLV_H
> +
> +/*
> + *  Advanced Linux Sound Architecture - ALSA - Driver
> + *  Copyright (c) 2006 by Jaroslav Kysela <perex@suse.cz>
> + *
> + *
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> + *
> + */
> +
> +#define DECLARE_TLV_DB_SCALE(name, min, step, mute) \
> +unsigned int name[] = { \
> +        SNDRV_CTL_TLVT_DB_SCALE, 2 * sizeof(unsigned int), \
> +        (min), ((step) & 0xffff) | ((mute) ? 0x10000 : 0) \
> +}
> +
> +#endif /* __SOUND_TLV_H */

Should be in control.h if we define TLVT_DB_SCALE there.
Or, all tlv type and data definition should go to tlv.h.


Takashi

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1252 bytes --]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> At Thu, 1 Jun 2006 16:08:13 +0200 (CEST),
> Jaroslav Kysela wrote:
> > 
> > On Thu, 1 Jun 2006, Takashi Iwai wrote:
> > 
> > > Yes.  It's similar but a really big difference if you consider about
> > > 32/64bit compatibility.
> > > 
> > > Also, the order of "TLV" is type-length-value.
> > 
> > Ok, the second version of my patch. Note: in the ca0106 driver, only
> > three lines (if I don't count one empty line) are required to add
> > the dB scale information.
> 
> The existence of flag makes the things complicated.
> I prefer removing flag and make the ioctl numid-specific.

It might be. It's really small work for the user space and we may use
flags in future to determine ioctl version or some other extension at the
ioctl level.

> Also, I don't think data_offset is necessary in the practical use.

To separate ioctl and data in the user space? But it might be that 
the 32bit->64bit conversion layer won't handle the non-continuous memory 
in a right way. I'll remove it.

					Jaroslav

-----
InterNet přes WIFI bez limitu dat, České Budějovice a okolí
Pokryté lokality: Šindlovy Dvory, Mokré, Litvínovice, Pekárenská, Srubec
Perex @ InterNet <internet@perex.cz>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 17:00:30 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 1 Jun 2006, Takashi Iwai wrote:
> 
> > At Thu, 1 Jun 2006 16:08:13 +0200 (CEST),
> > Jaroslav Kysela wrote:
> > > 
> > > On Thu, 1 Jun 2006, Takashi Iwai wrote:
> > > 
> > > > Yes.  It's similar but a really big difference if you consider about
> > > > 32/64bit compatibility.
> > > > 
> > > > Also, the order of "TLV" is type-length-value.
> > > 
> > > Ok, the second version of my patch. Note: in the ca0106 driver, only
> > > three lines (if I don't count one empty line) are required to add
> > > the dB scale information.
> > 
> > The existence of flag makes the things complicated.
> > I prefer removing flag and make the ioctl numid-specific.
> 
> It might be. It's really small work for the user space and we may use
> flags in future to determine ioctl version or some other extension at the
> ioctl level.

Well, but this ioctl is damn simple.  Just query a control element for
its TLV data.

It's supposed to be the one containing all.  If we need a filter of
returned data, the query header should contain a type field rather
than flags.


Takashi

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> Well, but this ioctl is damn simple.  Just query a control element for
> its TLV data.
> 
> It's supposed to be the one containing all.  If we need a filter of
> returned data, the query header should contain a type field rather
> than flags.

Sure, but this ioctl can handle global data (not numid dependant) as well. 
If we decide to have two ioctls then it's also ok to me.

						Jaroslav

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

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 17:16:13 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 1 Jun 2006, Takashi Iwai wrote:
> 
> > Well, but this ioctl is damn simple.  Just query a control element for
> > its TLV data.
> > 
> > It's supposed to be the one containing all.  If we need a filter of
> > returned data, the query header should contain a type field rather
> > than flags.
> 
> Sure, but this ioctl can handle global data (not numid dependant) as well. 
> If we decide to have two ioctls then it's also ok to me.

Yeah, I prefer two ioctls now, too.
(But we don't need to add the global ioctl yet unless we really need
 it...)


Takashi

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> Yeah, I prefer two ioctls now, too.
> (But we don't need to add the global ioctl yet unless we really need
>  it...)

Fine. Here is the third version of patch.

						Jaroslav

diff -r 96e63842ba5d core/control.c
--- a/core/control.c	Wed May 31 11:55:17 2006 +0200
+++ b/core/control.c	Thu Jun  1 17:25:38 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 = ncontrol->tlv;
 	kctl.private_value = ncontrol->private_value;
 	kctl.private_data = private_data;
 	return snd_ctl_new(&kctl, access);
@@ -1067,6 +1068,40 @@ 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)
+{
+	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;
+}
+
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct snd_ctl_file *ctl;
@@ -1086,11 +1121,11 @@ static long snd_ctl_ioctl(struct file *f
 	case SNDRV_CTL_IOCTL_CARD_INFO:
 		return snd_ctl_card_info(card, ctl, cmd, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LIST:
-		return snd_ctl_elem_list(ctl->card, argp);
+		return snd_ctl_elem_list(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_INFO:
 		return snd_ctl_elem_info_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_READ:
-		return snd_ctl_elem_read_user(ctl->card, argp);
+		return snd_ctl_elem_read_user(card, argp);
 	case SNDRV_CTL_IOCTL_ELEM_WRITE:
 		return snd_ctl_elem_write_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_ELEM_LOCK:
@@ -1105,6 +1140,8 @@ static long snd_ctl_ioctl(struct file *f
 		return snd_ctl_elem_remove(ctl, argp);
 	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);
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
diff -r 96e63842ba5d include/asound.h
--- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/asound.h	Thu Jun  1 17:25:38 2006 +0200
@@ -688,7 +688,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 3)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 4)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */
@@ -818,6 +818,20 @@ struct snd_ctl_elem_value {
         unsigned char reserved[128-sizeof(struct timespec)];
 };
 
+struct snd_ctl_tlv {
+        unsigned int numid;	/* control element numeric identification */
+        unsigned int length;	/* in bytes aligned to 4 */
+        /*
+         * TLV structure is right behind the struct snd_ctl_tlv:
+         *   unsigned int type	- see SNDRV_CTL_TLVT_*
+         *   unsigned int length
+         *   .... data aligned to sizeof(unsigned int), use
+         *        block_length = (length + (sizeof(unsigned int) - 1)) &
+         *                       ~(sizeof(unsigned int) - 1)) ....
+         */
+        unsigned int tlv[0];
+};
+
 enum {
 	SNDRV_CTL_IOCTL_PVERSION = _IOR('U', 0x00, int),
 	SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
@@ -831,6 +845,7 @@ enum {
 	SNDRV_CTL_IOCTL_ELEM_ADD = _IOWR('U', 0x17, struct snd_ctl_elem_info),
 	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_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 96e63842ba5d include/control.h
--- a/include/control.h	Wed May 31 11:55:17 2006 +0200
+++ b/include/control.h	Thu Jun  1 17:25:38 2006 +0200
@@ -42,6 +42,7 @@ struct snd_kcontrol_new {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 };
 
@@ -58,6 +59,7 @@ struct snd_kcontrol {
 	snd_kcontrol_info_t *info;
 	snd_kcontrol_get_t *get;
 	snd_kcontrol_put_t *put;
+	unsigned int *tlv;
 	unsigned long private_value;
 	void *private_data;
 	void (*private_free)(struct snd_kcontrol *kcontrol);
diff -r 96e63842ba5d pci/ca0106/ca0106_mixer.c
--- a/pci/ca0106/ca0106_mixer.c	Wed May 31 11:55:17 2006 +0200
+++ b/pci/ca0106/ca0106_mixer.c	Thu Jun  1 17:25:38 2006 +0200
@@ -70,8 +70,11 @@
 #include <sound/pcm.h>
 #include <sound/ac97_codec.h>
 #include <sound/info.h>
+#include <sound/tlv.h>
 
 #include "ca0106.h"
+
+static DECLARE_TLV_DB_SCALE(snd_ca0106_db_scale, -5150, 75, 1);
 
 static int snd_ca0106_shared_spdif_info(struct snd_kcontrol *kcontrol,
 					struct snd_ctl_elem_info *uinfo)
@@ -472,6 +475,7 @@ static int snd_ca0106_i2c_volume_put(str
 	.info =	 snd_ca0106_volume_info,			\
 	.get =   snd_ca0106_volume_get,				\
 	.put =   snd_ca0106_volume_put,				\
+	.tlv =	 snd_ca0106_db_scale,				\
 	.private_value = ((chid) << 8) | (reg)			\
 }
 
diff -r 96e63842ba5d include/tlv.h
--- /dev/null	Thu Jan  1 00:00:00 1970 +0000
+++ b/include/tlv.h	Thu Jun  1 17:25:38 2006 +0200
@@ -0,0 +1,34 @@
+#ifndef __SOUND_TLV_H
+#define __SOUND_TLV_H
+
+/*
+ *  Advanced Linux Sound Architecture - ALSA - Driver
+ *  Copyright (c) 2006 by Jaroslav Kysela <perex@suse.cz>
+ *
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#define SNDRV_CTL_TLVT_CONTAINER 0	/* one level down - group of TLVs */
+#define SNDRV_CTL_TLVT_DB_SCALE	1       /* dB scale */
+
+#define DECLARE_TLV_DB_SCALE(name, min, step, mute) \
+unsigned int name[] = { \
+        SNDRV_CTL_TLVT_DB_SCALE, 2 * sizeof(unsigned int), \
+        (min), ((step) & 0xffff) | ((mute) ? 0x10000 : 0) \
+}
+
+#endif /* __SOUND_TLV_H */

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

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 17:28:41 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> diff -r 96e63842ba5d include/asound.h
> --- a/include/asound.h	Wed May 31 11:55:17 2006 +0200
> +++ b/include/asound.h	Thu Jun  1 17:25:38 2006 +0200
> @@ -688,7 +688,7 @@ struct snd_timer_tread {
>   *                                                                          *
>   ****************************************************************************/
>  
> -#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 3)
> +#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 4)
>  
>  struct snd_ctl_card_info {
>  	int card;			/* card number */
> @@ -818,6 +818,20 @@ struct snd_ctl_elem_value {
>          unsigned char reserved[128-sizeof(struct timespec)];
>  };
>  
> +struct snd_ctl_tlv {
> +        unsigned int numid;	/* control element numeric identification */
> +        unsigned int length;	/* in bytes aligned to 4 */
> +        /*
> +         * TLV structure is right behind the struct snd_ctl_tlv:
> +         *   unsigned int type	- see SNDRV_CTL_TLVT_*
> +         *   unsigned int length
> +         *   .... data aligned to sizeof(unsigned int), use
> +         *        block_length = (length + (sizeof(unsigned int) - 1)) &
> +         *                       ~(sizeof(unsigned int) - 1)) ....
> +         */

I'd put this detailed comment in tlv.h, and here shows a short comment
like:
"TLV data is put right behind the struct snd_ctl_tlv.
 See tlv.h for definition of TLV data".


Takashi

Re: RFC: minimalistic TLV implementation

[Jaroslav Kysela]

On Thu, 1 Jun 2006, Takashi Iwai wrote:

> I'd put this detailed comment in tlv.h, and here shows a short comment

Done. I've commited the patch to HG.

					Jaroslav

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

Re: RFC: minimalistic TLV implementation

[Takashi Iwai]

At Thu, 1 Jun 2006 18:36:15 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Thu, 1 Jun 2006, Takashi Iwai wrote:
> 
> > I'd put this detailed comment in tlv.h, and here shows a short comment
> 
> Done. I've commited the patch to HG.

What about the alsa-lib part?  Did you already work on it?


Takashi