[PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/control/ctlparse.c b/src/control/ctlparse.c
index 978977d..acaf734 100644
--- a/src/control/ctlparse.c
+++ b/src/control/ctlparse.c
@@ -59,10 +59,10 @@ static long get_integer(const char **ptr, long min, long max)
 		goto out;
 
 	s = p;
-	val = strtol(s, &p, 10);
+	val = strtol(s, &p, 0);
 	if (*p == '.') {
 		p++;
-		strtol(p, &p, 10);
+		strtol(p, &p, 0);
 	}
 	if (*p == '%') {
 		val = (long)convert_prange1(strtod(s, NULL), min, max);
@@ -87,10 +87,10 @@ static long long get_integer64(const char **ptr, long long min, long long max)
 		goto out;
 
 	s = p;
-	val = strtol(s, &p, 10);
+	val = strtol(s, &p, 0);
 	if (*p == '.') {
 		p++;
-		strtol(p, &p, 10);
+		strtol(p, &p, 0);
 	}
 	if (*p == '%') {
 		val = (long long)convert_prange1(strtod(s, NULL), min, max);
-- 
2.1.0

[PATCH - UCM 2/2] ucm: add binary configure file parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

with cset command, UCM set kcontrol parameters directly:
    cset "name='<KCONTROL_NAME>' 1,2<,3,...>"
This patch enables UCM to set kcontrol with parameters from
configure file:
    bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
where "bcsetf" is a newly added keyword alongside of "cset", to
indicate binary cset with file; and <path/to/file> is the
configure file storing parameters in bytes array, up to 512 Bytes
(the maxim value that struct snd_ctl_elem_value can hold).

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 37ae4c8..1496b22 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
 	return 0;
 }
 
+static int binary_file_parse(snd_ctl_elem_value_t *dst,
+			      const char *filepath)
+{
+	int err = 0;
+	FILE *in;
+	long len;
+	char *res;
+	unsigned int idx;
+
+	in = fopen(filepath, "r");
+	if (!in) {
+		err = -errno;
+		goto __fail;
+	}
+	fseek(in, 0L, SEEK_END);
+	len = ftell(in);
+	rewind(in);
+	if (len > 512)
+		len = 512;
+	res = calloc(1, (size_t)len);
+	if (res == NULL) {
+		err = -ENOMEM;
+		goto __fail_nomem;
+	}
+	fread(res, (size_t)len, 1, in);
+	for (idx = 0; idx < len; idx++)
+		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
+	free(res);
+      __fail_nomem:
+	fclose(in);
+      __fail:
+	return err;
+}
+
 extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
 					 const char *str,
 					 const char **ret_ptr);
 
-static int execute_cset(snd_ctl_t *ctl, const char *cset)
+static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
 {
 	const char *pos;
 	int err;
@@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
 	err = snd_ctl_elem_info(ctl, info);
 	if (err < 0)
 		goto __fail;
-	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
+	if (isbin)
+		err = binary_file_parse(value, pos);
+	else
+		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
 	if (err < 0)
 		goto __fail;
 	err = snd_ctl_elem_write(ctl, value);
@@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 				goto __fail_nomem;
 			break;
 		case SEQUENCE_ELEMENT_TYPE_CSET:
+		case SEQUENCE_ELEMENT_TYPE_BCSETF:
 			if (cdev == NULL) {
 				const char *cdev1 = NULL, *cdev2 = NULL;
 				err = get_value3(&cdev1, "PlaybackCTL",
@@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					goto __fail;
 				}
 			}
-			err = execute_cset(ctl, s->data.cset);
+			err = execute_cset(ctl, s->data.cset, s->isbin);
 			if (err < 0) {
 				uc_error("unable to execute cset '%s'\n", s->data.cset);
 				goto __fail;
diff --git a/src/ucm/parser.c b/src/ucm/parser.c
index d7517f6..686c883 100644
--- a/src/ucm/parser.c
+++ b/src/ucm/parser.c
@@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
 			continue;
 		}
 
+		if (strcmp(cmd, "bcsetf") == 0) {
+			curr->type = SEQUENCE_ELEMENT_TYPE_BCSETF;
+			curr->isbin = 1;
+			err = parse_string(n, &curr->data.cset);
+			if (err < 0) {
+				uc_error("error: bcsetf requires a string!");
+				return err;
+			}
+			continue;
+		}
+
 		if (strcmp(cmd, "usleep") == 0) {
 			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
 			err = snd_config_get_integer(n, &curr->data.sleep);
diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
index 87f14a2..80d7335 100644
--- a/src/ucm/ucm_local.h
+++ b/src/ucm/ucm_local.h
@@ -47,6 +47,7 @@
 #define SEQUENCE_ELEMENT_TYPE_CSET	2
 #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
 #define SEQUENCE_ELEMENT_TYPE_EXEC	4
+#define SEQUENCE_ELEMENT_TYPE_BCSETF	5
 
 struct ucm_value {
         struct list_head list;
@@ -63,6 +64,7 @@ struct sequence_element {
 		char *cset;
 		char *exec;
 	} data;
+	int isbin;          /* Indicate cset is binary array or ascii array */
 };
 
 /*
-- 
2.1.0

Re: [PATCH - UCM 2/2] ucm: add binary configure file parse

[Takashi Iwai]

At Tue, 13 Jan 2015 11:00:39 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> with cset command, UCM set kcontrol parameters directly:
>     cset "name='<KCONTROL_NAME>' 1,2<,3,...>"
> This patch enables UCM to set kcontrol with parameters from
> configure file:
>     bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
> where "bcsetf" is a newly added keyword alongside of "cset", to
> indicate binary cset with file; and <path/to/file> is the
> configure file storing parameters in bytes array, up to 512 Bytes
> (the maxim value that struct snd_ctl_elem_value can hold).

Why binary?  It's not portable.  You can't carry it to a different
architecture.


Takashi

> 
> Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 37ae4c8..1496b22 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
>  	return 0;
>  }
>  
> +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> +			      const char *filepath)
> +{
> +	int err = 0;
> +	FILE *in;
> +	long len;
> +	char *res;
> +	unsigned int idx;
> +
> +	in = fopen(filepath, "r");
> +	if (!in) {
> +		err = -errno;
> +		goto __fail;
> +	}
> +	fseek(in, 0L, SEEK_END);
> +	len = ftell(in);
> +	rewind(in);
> +	if (len > 512)
> +		len = 512;
> +	res = calloc(1, (size_t)len);
> +	if (res == NULL) {
> +		err = -ENOMEM;
> +		goto __fail_nomem;
> +	}
> +	fread(res, (size_t)len, 1, in);
> +	for (idx = 0; idx < len; idx++)
> +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> +	free(res);
> +      __fail_nomem:
> +	fclose(in);
> +      __fail:
> +	return err;
> +}
> +
>  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
>  					 const char *str,
>  					 const char **ret_ptr);
>  
> -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> +static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
>  {
>  	const char *pos;
>  	int err;
> @@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
>  	err = snd_ctl_elem_info(ctl, info);
>  	if (err < 0)
>  		goto __fail;
> -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> +	if (isbin)
> +		err = binary_file_parse(value, pos);
> +	else
> +		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
>  	if (err < 0)
>  		goto __fail;
>  	err = snd_ctl_elem_write(ctl, value);
> @@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  				goto __fail_nomem;
>  			break;
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
> +		case SEQUENCE_ELEMENT_TYPE_BCSETF:
>  			if (cdev == NULL) {
>  				const char *cdev1 = NULL, *cdev2 = NULL;
>  				err = get_value3(&cdev1, "PlaybackCTL",
> @@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					goto __fail;
>  				}
>  			}
> -			err = execute_cset(ctl, s->data.cset);
> +			err = execute_cset(ctl, s->data.cset, s->isbin);
>  			if (err < 0) {
>  				uc_error("unable to execute cset '%s'\n", s->data.cset);
>  				goto __fail;
> diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> index d7517f6..686c883 100644
> --- a/src/ucm/parser.c
> +++ b/src/ucm/parser.c
> @@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
>  			continue;
>  		}
>  
> +		if (strcmp(cmd, "bcsetf") == 0) {
> +			curr->type = SEQUENCE_ELEMENT_TYPE_BCSETF;
> +			curr->isbin = 1;
> +			err = parse_string(n, &curr->data.cset);
> +			if (err < 0) {
> +				uc_error("error: bcsetf requires a string!");
> +				return err;
> +			}
> +			continue;
> +		}
> +
>  		if (strcmp(cmd, "usleep") == 0) {
>  			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
>  			err = snd_config_get_integer(n, &curr->data.sleep);
> diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
> index 87f14a2..80d7335 100644
> --- a/src/ucm/ucm_local.h
> +++ b/src/ucm/ucm_local.h
> @@ -47,6 +47,7 @@
>  #define SEQUENCE_ELEMENT_TYPE_CSET	2
>  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
>  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> +#define SEQUENCE_ELEMENT_TYPE_BCSETF	5
>  
>  struct ucm_value {
>          struct list_head list;
> @@ -63,6 +64,7 @@ struct sequence_element {
>  		char *cset;
>  		char *exec;
>  	} data;
> +	int isbin;          /* Indicate cset is binary array or ascii array */
>  };
>  
>  /*
> -- 
> 2.1.0
> 

Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[Takashi Iwai]

At Tue, 13 Jan 2015 11:00:38 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>

Looks good to me.  Liam, any objection for this extension?


thanks,

Takashi

> 
> diff --git a/src/control/ctlparse.c b/src/control/ctlparse.c
> index 978977d..acaf734 100644
> --- a/src/control/ctlparse.c
> +++ b/src/control/ctlparse.c
> @@ -59,10 +59,10 @@ static long get_integer(const char **ptr, long min, long max)
>  		goto out;
>  
>  	s = p;
> -	val = strtol(s, &p, 10);
> +	val = strtol(s, &p, 0);
>  	if (*p == '.') {
>  		p++;
> -		strtol(p, &p, 10);
> +		strtol(p, &p, 0);
>  	}
>  	if (*p == '%') {
>  		val = (long)convert_prange1(strtod(s, NULL), min, max);
> @@ -87,10 +87,10 @@ static long long get_integer64(const char **ptr, long long min, long long max)
>  		goto out;
>  
>  	s = p;
> -	val = strtol(s, &p, 10);
> +	val = strtol(s, &p, 0);
>  	if (*p == '.') {
>  		p++;
> -		strtol(p, &p, 10);
> +		strtol(p, &p, 0);
>  	}
>  	if (*p == '%') {
>  		val = (long long)convert_prange1(strtod(s, NULL), min, max);
> -- 
> 2.1.0
> 

Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[Liam Girdwood]

On Tue, 2015-01-13 at 17:53 +0100, Takashi Iwai wrote:
> At Tue, 13 Jan 2015 11:00:38 +0800,
> han.lu@intel.com wrote:
> > 
> > From: "Lu, Han" <han.lu@intel.com>
> > 
> > Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> Looks good to me.  Liam, any objection for this extension?
> 

Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

> 
> thanks,
> 
> Takashi
> 
> > 
> > diff --git a/src/control/ctlparse.c b/src/control/ctlparse.c
> > index 978977d..acaf734 100644
> > --- a/src/control/ctlparse.c
> > +++ b/src/control/ctlparse.c
> > @@ -59,10 +59,10 @@ static long get_integer(const char **ptr, long min, long max)
> >  		goto out;
> >  
> >  	s = p;
> > -	val = strtol(s, &p, 10);
> > +	val = strtol(s, &p, 0);
> >  	if (*p == '.') {
> >  		p++;
> > -		strtol(p, &p, 10);
> > +		strtol(p, &p, 0);
> >  	}
> >  	if (*p == '%') {
> >  		val = (long)convert_prange1(strtod(s, NULL), min, max);
> > @@ -87,10 +87,10 @@ static long long get_integer64(const char **ptr, long long min, long long max)
> >  		goto out;
> >  
> >  	s = p;
> > -	val = strtol(s, &p, 10);
> > +	val = strtol(s, &p, 0);
> >  	if (*p == '.') {
> >  		p++;
> > -		strtol(p, &p, 10);
> > +		strtol(p, &p, 0);
> >  	}
> >  	if (*p == '%') {
> >  		val = (long long)convert_prange1(strtod(s, NULL), min, max);
> > -- 
> > 2.1.0
> > 

Re: [PATCH - UCM 2/2] ucm: add binary configure file parse

[Liam Girdwood]

On Tue, 2015-01-13 at 17:52 +0100, Takashi Iwai wrote:
> At Tue, 13 Jan 2015 11:00:39 +0800,
> han.lu@intel.com wrote:
> > 
> > From: "Lu, Han" <han.lu@intel.com>
> > 
> > with cset command, UCM set kcontrol parameters directly:
> >     cset "name='<KCONTROL_NAME>' 1,2<,3,...>"
> > This patch enables UCM to set kcontrol with parameters from
> > configure file:
> >     bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
> > where "bcsetf" is a newly added keyword alongside of "cset", to
> > indicate binary cset with file; and <path/to/file> is the
> > configure file storing parameters in bytes array, up to 512 Bytes
> > (the maxim value that struct snd_ctl_elem_value can hold).
> 
> Why binary?  It's not portable.  You can't carry it to a different
> architecture.
> 

The intention here is that the binary data is not meant for the host but
for audio DSPs so it's just passed by UCM/ALSA as raw data.

We do have some DSP processing algos that have tuning tools where the
tool will generate a binary file of coefficients (depending on various
physical device properties etc). These coefficient files can then be
uploaded to the DSP by UCM on use case change.

Liam

> 
> Takashi
> 
> > 
> > Signed-off-by: Lu, Han <han.lu@intel.com>
> > 
> > diff --git a/src/ucm/main.c b/src/ucm/main.c
> > index 37ae4c8..1496b22 100644
> > --- a/src/ucm/main.c
> > +++ b/src/ucm/main.c
> > @@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> >  	return 0;
> >  }
> >  
> > +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> > +			      const char *filepath)
> > +{
> > +	int err = 0;
> > +	FILE *in;
> > +	long len;
> > +	char *res;
> > +	unsigned int idx;
> > +
> > +	in = fopen(filepath, "r");
> > +	if (!in) {
> > +		err = -errno;
> > +		goto __fail;
> > +	}
> > +	fseek(in, 0L, SEEK_END);
> > +	len = ftell(in);
> > +	rewind(in);
> > +	if (len > 512)
> > +		len = 512;
> > +	res = calloc(1, (size_t)len);
> > +	if (res == NULL) {
> > +		err = -ENOMEM;
> > +		goto __fail_nomem;
> > +	}
> > +	fread(res, (size_t)len, 1, in);
> > +	for (idx = 0; idx < len; idx++)
> > +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> > +	free(res);
> > +      __fail_nomem:
> > +	fclose(in);
> > +      __fail:
> > +	return err;
> > +}
> > +
> >  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
> >  					 const char *str,
> >  					 const char **ret_ptr);
> >  
> > -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> > +static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
> >  {
> >  	const char *pos;
> >  	int err;
> > @@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
> >  	err = snd_ctl_elem_info(ctl, info);
> >  	if (err < 0)
> >  		goto __fail;
> > -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> > +	if (isbin)
> > +		err = binary_file_parse(value, pos);
> > +	else
> > +		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> >  	if (err < 0)
> >  		goto __fail;
> >  	err = snd_ctl_elem_write(ctl, value);
> > @@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> >  				goto __fail_nomem;
> >  			break;
> >  		case SEQUENCE_ELEMENT_TYPE_CSET:
> > +		case SEQUENCE_ELEMENT_TYPE_BCSETF:
> >  			if (cdev == NULL) {
> >  				const char *cdev1 = NULL, *cdev2 = NULL;
> >  				err = get_value3(&cdev1, "PlaybackCTL",
> > @@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> >  					goto __fail;
> >  				}
> >  			}
> > -			err = execute_cset(ctl, s->data.cset);
> > +			err = execute_cset(ctl, s->data.cset, s->isbin);
> >  			if (err < 0) {
> >  				uc_error("unable to execute cset '%s'\n", s->data.cset);
> >  				goto __fail;
> > diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> > index d7517f6..686c883 100644
> > --- a/src/ucm/parser.c
> > +++ b/src/ucm/parser.c
> > @@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
> >  			continue;
> >  		}
> >  
> > +		if (strcmp(cmd, "bcsetf") == 0) {
> > +			curr->type = SEQUENCE_ELEMENT_TYPE_BCSETF;
> > +			curr->isbin = 1;
> > +			err = parse_string(n, &curr->data.cset);
> > +			if (err < 0) {
> > +				uc_error("error: bcsetf requires a string!");
> > +				return err;
> > +			}
> > +			continue;
> > +		}
> > +
> >  		if (strcmp(cmd, "usleep") == 0) {
> >  			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
> >  			err = snd_config_get_integer(n, &curr->data.sleep);
> > diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
> > index 87f14a2..80d7335 100644
> > --- a/src/ucm/ucm_local.h
> > +++ b/src/ucm/ucm_local.h
> > @@ -47,6 +47,7 @@
> >  #define SEQUENCE_ELEMENT_TYPE_CSET	2
> >  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
> >  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> > +#define SEQUENCE_ELEMENT_TYPE_BCSETF	5
> >  
> >  struct ucm_value {
> >          struct list_head list;
> > @@ -63,6 +64,7 @@ struct sequence_element {
> >  		char *cset;
> >  		char *exec;
> >  	} data;
> > +	int isbin;          /* Indicate cset is binary array or ascii array */
> >  };
> >  
> >  /*
> > -- 
> > 2.1.0
> > 

Re: [PATCH - UCM 2/2] ucm: add binary configure file parse

[Takashi Iwai]

At Tue, 13 Jan 2015 18:26:18 +0000,
Liam Girdwood wrote:
> 
> On Tue, 2015-01-13 at 17:52 +0100, Takashi Iwai wrote:
> > At Tue, 13 Jan 2015 11:00:39 +0800,
> > han.lu@intel.com wrote:
> > > 
> > > From: "Lu, Han" <han.lu@intel.com>
> > > 
> > > with cset command, UCM set kcontrol parameters directly:
> > >     cset "name='<KCONTROL_NAME>' 1,2<,3,...>"
> > > This patch enables UCM to set kcontrol with parameters from
> > > configure file:
> > >     bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
> > > where "bcsetf" is a newly added keyword alongside of "cset", to
> > > indicate binary cset with file; and <path/to/file> is the
> > > configure file storing parameters in bytes array, up to 512 Bytes
> > > (the maxim value that struct snd_ctl_elem_value can hold).
> > 
> > Why binary?  It's not portable.  You can't carry it to a different
> > architecture.
> > 
> 
> The intention here is that the binary data is not meant for the host but
> for audio DSPs so it's just passed by UCM/ALSA as raw data.

In that case, we should limit to certain element data types.
Otherwise people would abuse it for passing data even to integer or
enum ctls.

And of course it'd be better to clarify the reason in the patch
description :)

BTW, I'm still not so convinced by bcsetf...  Can't it be more verbose
or readable?


thanks,

Takashi

> We do have some DSP processing algos that have tuning tools where the
> tool will generate a binary file of coefficients (depending on various
> physical device properties etc). These coefficient files can then be
> uploaded to the DSP by UCM on use case change.
> 
> Liam
> 
> > 
> > Takashi
> > 
> > > 
> > > Signed-off-by: Lu, Han <han.lu@intel.com>
> > > 
> > > diff --git a/src/ucm/main.c b/src/ucm/main.c
> > > index 37ae4c8..1496b22 100644
> > > --- a/src/ucm/main.c
> > > +++ b/src/ucm/main.c
> > > @@ -160,11 +160,45 @@ static int open_ctl(snd_use_case_mgr_t *uc_mgr,
> > >  	return 0;
> > >  }
> > >  
> > > +static int binary_file_parse(snd_ctl_elem_value_t *dst,
> > > +			      const char *filepath)
> > > +{
> > > +	int err = 0;
> > > +	FILE *in;
> > > +	long len;
> > > +	char *res;
> > > +	unsigned int idx;
> > > +
> > > +	in = fopen(filepath, "r");
> > > +	if (!in) {
> > > +		err = -errno;
> > > +		goto __fail;
> > > +	}
> > > +	fseek(in, 0L, SEEK_END);
> > > +	len = ftell(in);
> > > +	rewind(in);
> > > +	if (len > 512)
> > > +		len = 512;
> > > +	res = calloc(1, (size_t)len);
> > > +	if (res == NULL) {
> > > +		err = -ENOMEM;
> > > +		goto __fail_nomem;
> > > +	}
> > > +	fread(res, (size_t)len, 1, in);
> > > +	for (idx = 0; idx < len; idx++)
> > > +		snd_ctl_elem_value_set_byte(dst, idx, *(res + idx));
> > > +	free(res);
> > > +      __fail_nomem:
> > > +	fclose(in);
> > > +      __fail:
> > > +	return err;
> > > +}
> > > +
> > >  extern int __snd_ctl_ascii_elem_id_parse(snd_ctl_elem_id_t *dst,
> > >  					 const char *str,
> > >  					 const char **ret_ptr);
> > >  
> > > -static int execute_cset(snd_ctl_t *ctl, const char *cset)
> > > +static int execute_cset(snd_ctl_t *ctl, const char *cset, int isbin)
> > >  {
> > >  	const char *pos;
> > >  	int err;
> > > @@ -194,7 +228,10 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset)
> > >  	err = snd_ctl_elem_info(ctl, info);
> > >  	if (err < 0)
> > >  		goto __fail;
> > > -	err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> > > +	if (isbin)
> > > +		err = binary_file_parse(value, pos);
> > > +	else
> > > +		err = snd_ctl_ascii_value_parse(ctl, value, info, pos);
> > >  	if (err < 0)
> > >  		goto __fail;
> > >  	err = snd_ctl_elem_write(ctl, value);
> > > @@ -239,6 +276,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> > >  				goto __fail_nomem;
> > >  			break;
> > >  		case SEQUENCE_ELEMENT_TYPE_CSET:
> > > +		case SEQUENCE_ELEMENT_TYPE_BCSETF:
> > >  			if (cdev == NULL) {
> > >  				const char *cdev1 = NULL, *cdev2 = NULL;
> > >  				err = get_value3(&cdev1, "PlaybackCTL",
> > > @@ -274,7 +312,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> > >  					goto __fail;
> > >  				}
> > >  			}
> > > -			err = execute_cset(ctl, s->data.cset);
> > > +			err = execute_cset(ctl, s->data.cset, s->isbin);
> > >  			if (err < 0) {
> > >  				uc_error("unable to execute cset '%s'\n", s->data.cset);
> > >  				goto __fail;
> > > diff --git a/src/ucm/parser.c b/src/ucm/parser.c
> > > index d7517f6..686c883 100644
> > > --- a/src/ucm/parser.c
> > > +++ b/src/ucm/parser.c
> > > @@ -306,6 +306,17 @@ static int parse_sequence(snd_use_case_mgr_t *uc_mgr ATTRIBUTE_UNUSED,
> > >  			continue;
> > >  		}
> > >  
> > > +		if (strcmp(cmd, "bcsetf") == 0) {
> > > +			curr->type = SEQUENCE_ELEMENT_TYPE_BCSETF;
> > > +			curr->isbin = 1;
> > > +			err = parse_string(n, &curr->data.cset);
> > > +			if (err < 0) {
> > > +				uc_error("error: bcsetf requires a string!");
> > > +				return err;
> > > +			}
> > > +			continue;
> > > +		}
> > > +
> > >  		if (strcmp(cmd, "usleep") == 0) {
> > >  			curr->type = SEQUENCE_ELEMENT_TYPE_SLEEP;
> > >  			err = snd_config_get_integer(n, &curr->data.sleep);
> > > diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h
> > > index 87f14a2..80d7335 100644
> > > --- a/src/ucm/ucm_local.h
> > > +++ b/src/ucm/ucm_local.h
> > > @@ -47,6 +47,7 @@
> > >  #define SEQUENCE_ELEMENT_TYPE_CSET	2
> > >  #define SEQUENCE_ELEMENT_TYPE_SLEEP	3
> > >  #define SEQUENCE_ELEMENT_TYPE_EXEC	4
> > > +#define SEQUENCE_ELEMENT_TYPE_BCSETF	5
> > >  
> > >  struct ucm_value {
> > >          struct list_head list;
> > > @@ -63,6 +64,7 @@ struct sequence_element {
> > >  		char *cset;
> > >  		char *exec;
> > >  	} data;
> > > +	int isbin;          /* Indicate cset is binary array or ascii array */
> > >  };
> > >  
> > >  /*
> > > -- 
> > > 2.1.0
> > > 
> 
> 

Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[Takashi Iwai]

At Tue, 13 Jan 2015 17:53:14 +0100,
Takashi Iwai wrote:
> 
> At Tue, 13 Jan 2015 11:00:38 +0800,
> han.lu@intel.com wrote:
> > 
> > From: "Lu, Han" <han.lu@intel.com>
> > 
> > Signed-off-by: Lu, Han <han.lu@intel.com>
> 
> Looks good to me.  Liam, any objection for this extension?

Erm, sorry, I correct my statement: this is buggy.  Look at the code:

> > -	val = strtol(s, &p, 10);
> > +	val = strtol(s, &p, 0);
> >  	if (*p == '.') {
> >  		p++;
> > -		strtol(p, &p, 10);
> > +		strtol(p, &p, 0);

The second strtol() is for skipping the decimals.  So this has to be
10-based.  That is, use zero-base only for the first strtol().


Takashi

Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[Lu, Han]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, January 14, 2015 4:02 AM
> To: Takashi Iwai
> Cc: Lu, Han; Liam Girdwood; alsa-devel@alsa-project.org
> Subject: Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse
> 
> At Tue, 13 Jan 2015 17:53:14 +0100,
> Takashi Iwai wrote:
> >
> > At Tue, 13 Jan 2015 11:00:38 +0800,
> > han.lu@intel.com wrote:
> > >
> > > From: "Lu, Han" <han.lu@intel.com>
> > >
> > > Signed-off-by: Lu, Han <han.lu@intel.com>
> >
> > Looks good to me.  Liam, any objection for this extension?
> 
> Erm, sorry, I correct my statement: this is buggy.  Look at the code:
> 
> > > -	val = strtol(s, &p, 10);
> > > +	val = strtol(s, &p, 0);
> > >  	if (*p == '.') {
> > >  		p++;
> > > -		strtol(p, &p, 10);
> > > +		strtol(p, &p, 0);
> 
> The second strtol() is for skipping the decimals.  So this has to be 10-based.
> That is, use zero-base only for the first strtol().
> 
Yes. I thought of skipping hexadecimal, but string begin with ".0x" looks not reasonable, and string begin with ".0" will not be skipped as expected with this patch.
I have removed the second change and resend the patch. Please review, Thanks.

BR,
Han Lu
> 
> Takashi

Re: [PATCH - UCM 2/2] ucm: add binary configure file parse

[Liam Girdwood]

On Tue, 2015-01-13 at 20:50 +0100, Takashi Iwai wrote:
> At Tue, 13 Jan 2015 18:26:18 +0000,
> Liam Girdwood wrote:
> > 
> > On Tue, 2015-01-13 at 17:52 +0100, Takashi Iwai wrote:
> > > At Tue, 13 Jan 2015 11:00:39 +0800,
> > > han.lu@intel.com wrote:
> > > > 
> > > > From: "Lu, Han" <han.lu@intel.com>
> > > > 
> > > > with cset command, UCM set kcontrol parameters directly:
> > > >     cset "name='<KCONTROL_NAME>' 1,2<,3,...>"
> > > > This patch enables UCM to set kcontrol with parameters from
> > > > configure file:
> > > >     bcsetf "name='<KCONTROL_NAME>' <path/to/file>"
> > > > where "bcsetf" is a newly added keyword alongside of "cset", to
> > > > indicate binary cset with file; and <path/to/file> is the
> > > > configure file storing parameters in bytes array, up to 512 Bytes
> > > > (the maxim value that struct snd_ctl_elem_value can hold).
> > > 
> > > Why binary?  It's not portable.  You can't carry it to a different
> > > architecture.
> > > 
> > 
> > The intention here is that the binary data is not meant for the host but
> > for audio DSPs so it's just passed by UCM/ALSA as raw data.
> 
> In that case, we should limit to certain element data types.
> Otherwise people would abuse it for passing data even to integer or
> enum ctls.
> 
> And of course it'd be better to clarify the reason in the patch
> description :)
> 
> BTW, I'm still not so convinced by bcsetf...  Can't it be more verbose
> or readable?
> 

Yeah, we should probably make it more readable :)

Lu Han will probably have some naming suggestions shortly....

Liam

Re: [PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[Takashi Iwai]

At Wed, 14 Jan 2015 09:08:30 +0800,
han.lu@intel.com wrote:
> 
> From: "Lu, Han" <han.lu@intel.com>
> 
> Use zero-base for strtol(), so get_integer() and get_integer64()
> can parse decimal, octal and hexadecimal data from input string.
> 
> Signed-off-by: Lu, Han <han.lu@intel.com>

Applied, thanks.


Takashi

> 
> diff --git a/src/control/ctlparse.c b/src/control/ctlparse.c
> index 978977d..8d6c385 100644
> --- a/src/control/ctlparse.c
> +++ b/src/control/ctlparse.c
> @@ -59,7 +59,7 @@ static long get_integer(const char **ptr, long min, long max)
>  		goto out;
>  
>  	s = p;
> -	val = strtol(s, &p, 10);
> +	val = strtol(s, &p, 0);
>  	if (*p == '.') {
>  		p++;
>  		strtol(p, &p, 10);
> @@ -87,7 +87,7 @@ static long long get_integer64(const char **ptr, long long min, long long max)
>  		goto out;
>  
>  	s = p;
> -	val = strtol(s, &p, 10);
> +	val = strtol(s, &p, 0);
>  	if (*p == '.') {
>  		p++;
>  		strtol(p, &p, 10);
> -- 
> 1.9.1
> 

[PATCH - UCM 1/2] control: enable octal and hexadecimal parse

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Use zero-base for strtol(), so get_integer() and get_integer64()
can parse decimal, octal and hexadecimal data from input string.

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/src/control/ctlparse.c b/src/control/ctlparse.c
index 978977d..8d6c385 100644
--- a/src/control/ctlparse.c
+++ b/src/control/ctlparse.c
@@ -59,7 +59,7 @@ static long get_integer(const char **ptr, long min, long max)
 		goto out;
 
 	s = p;
-	val = strtol(s, &p, 10);
+	val = strtol(s, &p, 0);
 	if (*p == '.') {
 		p++;
 		strtol(p, &p, 10);
@@ -87,7 +87,7 @@ static long long get_integer64(const char **ptr, long long min, long long max)
 		goto out;
 
 	s = p;
-	val = strtol(s, &p, 10);
+	val = strtol(s, &p, 0);
 	if (*p == '.') {
 		p++;
 		strtol(p, &p, 10);
-- 
1.9.1