Skip to contentSkip to conversation

staging/speakup/kobjects.c: Code improvement.

PATCH In review Raphael S. Carvalho 3 participants 3 messages Jun 22, 2013 – Jul 23, 2013 Staging

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging/speakup/kobjects.c: Code improvement.

Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param. I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".

spk_xlate isn't used anymore (in line 608), then there is no difference between using cp and buf in VAR_STRING case. Besides, buf is a const char and those changes remove one uneeded line.

I created the function spk_reset_default_value because it clarifies the code and allows code reusing.

---
 drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 33 deletions(-)
drivers/staging/speakup/kobjects.c +40 -33
index 943b6c1..4660ce3 100644
@@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
 }
 EXPORT_SYMBOL_GPL(spk_var_show);
 
+/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+					int *synth_default_value, int idx)
+{
+	struct st_var_header *param;
+
+	if (synth && synth_default_value) {
+		param = spk_var_header_by_name(header_name);
+		if (param)  {
+			spk_set_num_var(synth_default_value[idx],
+					param, E_NEW_DEFAULT);
+			spk_set_num_var(0, param, E_DEFAULT);
+			pr_info("%s reset to default value\n", param->name);
+		}
+	}
+}
+
 /*
  * This function is called when a user echos a value to one of the
  * variable parameters.
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
 		if (ret == -ERANGE) {
 			var_data = param->data;
 			pr_warn("value for %s out of range, expect %d to %d\n",
-				attr->attr.name,
+				param->name,
 				var_data->u.n.low, var_data->u.n.high);
 		}
+
+	       /*
+		* If voice was just changed, we might need to reset our default
+		* pitch and volume.
+		*/
+		if (param->var_id == VOICE) {
+			spk_reset_default_value("pitch", synth->default_pitch,
+				value);
+			spk_reset_default_value("vol", synth->default_vol,
+				value);
+		}
 		break;
 	case VAR_STRING:
-		len = strlen(buf);
-		if ((len >= 1) && (buf[len - 1] == '\n'))
+		len = strlen(cp);
+		if ((len >= 1) && (cp[len - 1] == '\n'))
 			--len;
-		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
-			++buf;
+		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+			++cp;
 			len -= 2;
 		}
-		cp = (char *) buf;
 		cp[len] = '\0';
-		ret = spk_set_string_var(buf, param, len);
+		ret = spk_set_string_var(cp, param, len);
 		if (ret == -E2BIG)
 			pr_warn("value too long for %s\n",
-					attr->attr.name);
+					param->name);
 		break;
 	default:
 		pr_warn("%s unknown type %d\n",
 			param->name, (int)param->var_type);
-	break;
-	}
-	/*
-	 * If voice was just changed, we might need to reset our default
-	 * pitch and volume.
-	 */
-	if (strcmp(attr->attr.name, "voice") == 0) {
-		if (synth && synth->default_pitch) {
-			param = spk_var_header_by_name("pitch");
-			if (param)  {
-				spk_set_num_var(synth->default_pitch[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-		if (synth && synth->default_vol) {
-			param = spk_var_header_by_name("vol");
-			if (param)  {
-				spk_set_num_var(synth->default_vol[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-	}
+		break;
+	}
 	spk_unlock(flags);
 
 	if (ret == -ERESTART)
-		pr_info("%s reset to default value\n", attr->attr.name);
+		pr_info("%s reset to default value\n", param->name);
 	return count;
 }
 EXPORT_SYMBOL_GPL(spk_var_store);
Signature · 1 line
1.7.2.5
Acked-by on v1 1/1
#1 lore raw
Quoted 9 lines · Raphael S. Carvalho, le Fri 21 Jun 2013 22:54:40 -0300, a écrit :

Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param. I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".

spk_xlate isn't used anymore (in line 608), then there is no difference between using cp and buf in VAR_STRING case. Besides, buf is a const char and those changes remove one uneeded line.

I created the function spk_reset_default_value because it clarifies the code and allows code reusing.

Quoted 115 lines
---
 drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 33 deletions(-)
drivers/staging/speakup/kobjects.c +40 -33
index 943b6c1..4660ce3 100644
@@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
 }
 EXPORT_SYMBOL_GPL(spk_var_show);
 
+/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+					int *synth_default_value, int idx)
+{
+	struct st_var_header *param;
+
+	if (synth && synth_default_value) {
+		param = spk_var_header_by_name(header_name);
+		if (param)  {
+			spk_set_num_var(synth_default_value[idx],
+					param, E_NEW_DEFAULT);
+			spk_set_num_var(0, param, E_DEFAULT);
+			pr_info("%s reset to default value\n", param->name);
+		}
+	}
+}
+
 /*
  * This function is called when a user echos a value to one of the
  * variable parameters.
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
 		if (ret == -ERANGE) {
 			var_data = param->data;
 			pr_warn("value for %s out of range, expect %d to %d\n",
-				attr->attr.name,
+				param->name,
 				var_data->u.n.low, var_data->u.n.high);
 		}
+
+	       /*
+		* If voice was just changed, we might need to reset our default
+		* pitch and volume.
+		*/
+		if (param->var_id == VOICE) {
+			spk_reset_default_value("pitch", synth->default_pitch,
+				value);
+			spk_reset_default_value("vol", synth->default_vol,
+				value);
+		}
 		break;
 	case VAR_STRING:
-		len = strlen(buf);
-		if ((len >= 1) && (buf[len - 1] == '\n'))
+		len = strlen(cp);
+		if ((len >= 1) && (cp[len - 1] == '\n'))
 			--len;
-		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
-			++buf;
+		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+			++cp;
 			len -= 2;
 		}
-		cp = (char *) buf;
 		cp[len] = '\0';
-		ret = spk_set_string_var(buf, param, len);
+		ret = spk_set_string_var(cp, param, len);
 		if (ret == -E2BIG)
 			pr_warn("value too long for %s\n",
-					attr->attr.name);
+					param->name);
 		break;
 	default:
 		pr_warn("%s unknown type %d\n",
 			param->name, (int)param->var_type);
-	break;
-	}
-	/*
-	 * If voice was just changed, we might need to reset our default
-	 * pitch and volume.
-	 */
-	if (strcmp(attr->attr.name, "voice") == 0) {
-		if (synth && synth->default_pitch) {
-			param = spk_var_header_by_name("pitch");
-			if (param)  {
-				spk_set_num_var(synth->default_pitch[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-		if (synth && synth->default_vol) {
-			param = spk_var_header_by_name("vol");
-			if (param)  {
-				spk_set_num_var(synth->default_vol[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-	}
+		break;
+	}
 	spk_unlock(flags);
 
 	if (ret == -ERESTART)
-		pr_info("%s reset to default value\n", attr->attr.name);
+		pr_info("%s reset to default value\n", param->name);
 	return count;
 }
 EXPORT_SYMBOL_GPL(spk_var_store);
Signature · 1 line
1.7.2.5
Signature · 6 lines
Samuel
$ du temp.iso 
2,0T    temp.iso
$ ls temp.iso -l
-r-xr-xr-x    1 samy     thibault      16E 2003-03-22 14:44 temp.iso*
 -+- je vous dirai pas la marque de mon disque dur, na :p -+- 
#2 lore raw
In reply to Raphael S. Carvalho
Quoted 9 lines · Raphael S. Carvalho

Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".

spk_xlate isn't used anymore (in line 608), then there is no difference between using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.

Quoted 8 lines

I created the function spk_reset_default_value because it clarifies the code and allows code reusing.

---
 drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 33 deletions(-)

This patch no longer applies to my tree, can you please refresh it against linux-next and resend, keeping Samuel's ack on it?

thanks,

greg k-h