[PATCH] cxacru: Cleanup sysfs attribute code

[PATCH] cxacru: Cleanup sysfs attribute code

[Simon Arlott]

This changes the format of unknown status values to be less verbose and 
uses an array instead of several different snprintf calls. Since only 
enum values are assigned to it, poll_state is changed from int to enum. 
Use abs() for dB values instead of two almost identical return lines.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Duncan Sands <duncan.sands@math.u-psud.fr>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Patch depends on existing cxacru patches in -rc7-mm.

On 27/02/07 20:26, Andrew Morton wrote:
>> +static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
>> +{
>> +	switch (value) {
>> +	case 0: return 0;
>> +	case 1: return snprintf(buf, PAGE_SIZE, "ANSI T1.413\n");
>> +	case 2: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.1 (G.DMT)\n");
>> +	case 3: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.2 (G.LITE)\n");
>> +	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
>> +	}
>> +}
>  
> It is conventional to do
> 
> 	switch (a) {
> 	case 0:
> 		stuff();
> 		break;
> 	case 1:
> 		stuff();
> 	}
> 
> But all the above could be array-driven anyway.

Is the code below close to what you meant?

static char *str[] = { NULL, "not connected", "connected", "lost" };
if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
	return snprintf(buf, PAGE_SIZE, "%u\n", value);
return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);

On 21/04/07 18:56, Duncan Sands wrote:
>> +named device points to the USB interface device's directory which contains
>> +several sysfs attribute files for retriving device statistics:
> retrieving

Thanks.

 drivers/usb/atm/cxacru.c |   73 +++++++++++++++++++--------------------------
 1 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 0c47755..1c5d6de 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -171,7 +171,7 @@ struct cxacru_data {
 	struct delayed_work poll_work;
 	u32 card_info[CXINF_MAX];
 	struct mutex poll_state_serialize;
-	int poll_state;
+	enum cxacru_poll_state poll_state;
 
 	/* contol handles */
 	struct mutex cm_serialize;
@@ -226,58 +226,48 @@ static ssize_t cxacru_sysfs_showattr_s8(s8 value, char *buf)
 
 static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf)
 {
-	if (unlikely(value < 0)) {
-		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
-						value / 100, -value % 100);
-	} else {
-		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
-						value / 100, value % 100);
-	}
+	return snprintf(buf, PAGE_SIZE, "%u.%02u\n",
+					value / 100, abs(value) % 100);
 }
 
 static ssize_t cxacru_sysfs_showattr_bool(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "no\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "yes\n");
-	default: return 0;
-	}
+	static char *str[] = { "no", "yes" };
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_LINK(u32 value, char *buf)
 {
-	switch (value) {
-	case 1: return snprintf(buf, PAGE_SIZE, "not connected\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "connected\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "lost\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { NULL, "not connected", "connected", "lost" };
+	if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_LINE(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "down\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "attempting to activate\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "training\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "channel analysis\n");
-	case 4: return snprintf(buf, PAGE_SIZE, "exchange\n");
-	case 5: return snprintf(buf, PAGE_SIZE, "up\n");
-	case 6: return snprintf(buf, PAGE_SIZE, "waiting\n");
-	case 7: return snprintf(buf, PAGE_SIZE, "initialising\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { "down", "attempting to activate",
+		"training", "channel analysis", "exchange", "up",
+		"waiting", "initialising"
+	};
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return 0;
-	case 1: return snprintf(buf, PAGE_SIZE, "ANSI T1.413\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.1 (G.DMT)\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.2 (G.LITE)\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = {
+			NULL,
+			"ANSI T1.413",
+			"ITU-T G.992.1 (G.DMT)",
+			"ITU-T G.992.2 (G.LITE)"
+	};
+	if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 /*
@@ -308,11 +298,10 @@ static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 	u32 value = instance->card_info[CXINF_LINE_STARTABLE];
 
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "running\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "stopped\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { "running", "stopped" };
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
-- 
1.5.0.1

-- 
Simon Arlott

Re: [PATCH] cxacru: Cleanup sysfs attribute code

[Duncan Sands]

Hi Simon,

>  static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf)
>  {
> -	if (unlikely(value < 0)) {
> -		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
> -						value / 100, -value % 100);
> -	} else {
> -		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
> -						value / 100, value % 100);
> -	}
> +	return snprintf(buf, PAGE_SIZE, "%u.%02u\n",
> +					value / 100, abs(value) % 100);

shouldn't this be "%d.%02u\n"?

> +	static char *str[] = { "no", "yes" };
...
> +	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);

This could be
	static char *str[] = { "no\n", "yes\n" };
...
	return snprintf(buf, PAGE_SIZE, str[value]);
I'm not saying that it's better, just a remark!

Ciao,

Duncan.

Re: [PATCH] cxacru: Cleanup sysfs attribute code

[Simon Arlott]

On Wed, April 25, 2007 08:19, Duncan Sands wrote:
> Hi Simon,
>
>>  static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf)
>>  {
>> -	if (unlikely(value < 0)) {
>> -		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
>> -						value / 100, -value % 100);
>> -	} else {
>> -		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
>> -						value / 100, value % 100);
>> -	}
>> +	return snprintf(buf, PAGE_SIZE, "%u.%02u\n",
>> +					value / 100, abs(value) % 100);
>
> shouldn't this be "%d.%02u\n"?

Yes, I'll send an updated patch this evening, unless Andrew adds the change himself by then.

>> +	static char *str[] = { "no", "yes" };
> ...
>> +	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
>
> This could be
> 	static char *str[] = { "no\n", "yes\n" };
> ...
> 	return snprintf(buf, PAGE_SIZE, str[value]);
> I'm not saying that it's better, just a remark!

The \ns in the larger arrays were making them less readable.

-- 
Simon Arlott

[PATCH (rev 2)] cxacru: Cleanup sysfs attribute code

[Simon Arlott]

This changes the format of unknown status values to be less verbose and 
uses an array instead of several different snprintf calls. Since only 
enum values are assigned to it, poll_state is changed from int to enum. 
Use abs() for dB values instead of two almost identical return lines.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Duncan Sands <duncan.sands@math.u-psud.fr>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Patch depends on existing cxacru patches in -rc7-mm.

On 25/04/07 12:18, Simon Arlott wrote:
> On Wed, April 25, 2007 08:19, Duncan Sands wrote:
>>>  static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf)
>>>  {
>>> +	return snprintf(buf, PAGE_SIZE, "%u.%02u\n",
>>> +					value / 100, abs(value) % 100);
>> shouldn't this be "%d.%02u\n"?

I've updated the patch to fix this... I wasn't thinking properly when 
I changed that to %u.

 drivers/usb/atm/cxacru.c |   73 +++++++++++++++++++--------------------------
 1 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 0c47755..d53de3c 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -171,7 +171,7 @@ struct cxacru_data {
 	struct delayed_work poll_work;
 	u32 card_info[CXINF_MAX];
 	struct mutex poll_state_serialize;
-	int poll_state;
+	enum cxacru_poll_state poll_state;
 
 	/* contol handles */
 	struct mutex cm_serialize;
@@ -226,58 +226,48 @@ static ssize_t cxacru_sysfs_showattr_s8(s8 value, char *buf)
 
 static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf)
 {
-	if (unlikely(value < 0)) {
-		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
-						value / 100, -value % 100);
-	} else {
-		return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
-						value / 100, value % 100);
-	}
+	return snprintf(buf, PAGE_SIZE, "%d.%02u\n",
+					value / 100, abs(value) % 100);
 }
 
 static ssize_t cxacru_sysfs_showattr_bool(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "no\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "yes\n");
-	default: return 0;
-	}
+	static char *str[] = { "no", "yes" };
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_LINK(u32 value, char *buf)
 {
-	switch (value) {
-	case 1: return snprintf(buf, PAGE_SIZE, "not connected\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "connected\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "lost\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { NULL, "not connected", "connected", "lost" };
+	if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_LINE(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "down\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "attempting to activate\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "training\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "channel analysis\n");
-	case 4: return snprintf(buf, PAGE_SIZE, "exchange\n");
-	case 5: return snprintf(buf, PAGE_SIZE, "up\n");
-	case 6: return snprintf(buf, PAGE_SIZE, "waiting\n");
-	case 7: return snprintf(buf, PAGE_SIZE, "initialising\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { "down", "attempting to activate",
+		"training", "channel analysis", "exchange", "up",
+		"waiting", "initialising"
+	};
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_showattr_MODU(u32 value, char *buf)
 {
-	switch (value) {
-	case 0: return 0;
-	case 1: return snprintf(buf, PAGE_SIZE, "ANSI T1.413\n");
-	case 2: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.1 (G.DMT)\n");
-	case 3: return snprintf(buf, PAGE_SIZE, "ITU-T G.992.2 (G.LITE)\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = {
+			NULL,
+			"ANSI T1.413",
+			"ITU-T G.992.1 (G.DMT)",
+			"ITU-T G.992.2 (G.LITE)"
+	};
+	if (unlikely(value >= ARRAY_SIZE(str) || str[value] == NULL))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 /*
@@ -308,11 +298,10 @@ static ssize_t cxacru_sysfs_show_adsl_state(struct device *dev,
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 	u32 value = instance->card_info[CXINF_LINE_STARTABLE];
 
-	switch (value) {
-	case 0: return snprintf(buf, PAGE_SIZE, "running\n");
-	case 1: return snprintf(buf, PAGE_SIZE, "stopped\n");
-	default: return snprintf(buf, PAGE_SIZE, "unknown (%u)\n", value);
-	}
+	static char *str[] = { "running", "stopped" };
+	if (unlikely(value >= ARRAY_SIZE(str)))
+		return snprintf(buf, PAGE_SIZE, "%u\n", value);
+	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
 }
 
 static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev,
-- 
1.5.0.1

-- 
Simon Arlott

Re: [PATCH (rev 2)] cxacru: Cleanup sysfs attribute code

[Duncan Sands]

On Wednesday 25 April 2007 20:41:20 Simon Arlott wrote:
> This changes the format of unknown status values to be less verbose and 
> uses an array instead of several different snprintf calls. Since only 
> enum values are assigned to it, poll_state is changed from int to enum. 
> Use abs() for dB values instead of two almost identical return lines.
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Duncan Sands <duncan.sands@math.u-psud.fr>

Acked-by: Duncan Sands <baldrick@free.fr>

> Cc: Andrew Morton <akpm@linux-foundation.org>

Re: [PATCH (rev 2)] cxacru: Cleanup sysfs attribute code

[Andrew Morton]

On Wed, 25 Apr 2007 19:41:20 +0100 Simon Arlott <simon@fire.lp0.eu> wrote:

> This changes the format of unknown status values to be less verbose and 
> uses an array instead of several different snprintf calls. Since only 
> enum values are assigned to it, poll_state is changed from int to enum. 
> Use abs() for dB values instead of two almost identical return lines.
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Duncan Sands <duncan.sands@math.u-psud.fr>
> Cc: Andrew Morton <akpm@linux-foundation.org>
>
> ...
>  
>  static ssize_t cxacru_sysfs_showattr_bool(u32 value, char *buf)
>  {
> -	switch (value) {
> -	case 0: return snprintf(buf, PAGE_SIZE, "no\n");
> -	case 1: return snprintf(buf, PAGE_SIZE, "yes\n");
> -	default: return 0;
> -	}
> +	static char *str[] = { "no", "yes" };
> +	if (unlikely(value >= ARRAY_SIZE(str)))
> +		return snprintf(buf, PAGE_SIZE, "%u\n", value);
> +	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
>  }

Should a bool be displayed as "true" or "false"?

>  static ssize_t cxacru_sysfs_showattr_LINK(u32 value, char *buf)
>  {
>
> ...
>
> +		return snprintf(buf, PAGE_SIZE, "%u\n", value);
>  }

To be completely pedantic: we shouldn't be printing u32's with %u.  Because
%u assumes that u32 is implemented as unsigned int.  Only it's an opaque
type and we don't know what actual C type the architecture chose to use.

It happens to work OK on all architectures and I expect it always will, so
no change is needed, but there you have it.

u64's and %llu _are_ incompatible on some architectures and I get to fix
that about 1000000 times.

Re: [PATCH (rev 2)] cxacru: Cleanup sysfs attribute code

[Simon Arlott]

On 28/04/07 08:50, Andrew Morton wrote:
> On Wed, 25 Apr 2007 19:41:20 +0100 Simon Arlott <simon@fire.lp0.eu> wrote:
>> This changes the format of unknown status values to be less verbose and 
>> uses an array instead of several different snprintf calls. Since only 
>> enum values are assigned to it, poll_state is changed from int to enum. 
>> Use abs() for dB values instead of two almost identical return lines.
>> ...
>> +	static char *str[] = { "no", "yes" };
>> +	if (unlikely(value >= ARRAY_SIZE(str)))
>> +		return snprintf(buf, PAGE_SIZE, "%u\n", value);
>> +	return snprintf(buf, PAGE_SIZE, "%s\n", str[value]);
> 
> Should a bool be displayed as "true" or "false"?

The only attribute that uses this is line_startable... I suppose 
it could return true/false - I just happened to use yes/no.

>> +		return snprintf(buf, PAGE_SIZE, "%u\n", value);
> 
> To be completely pedantic: we shouldn't be printing u32's with %u.  Because
> %u assumes that u32 is implemented as unsigned int.  Only it's an opaque
> type and we don't know what actual C type the architecture chose to use.
> 
> It happens to work OK on all architectures and I expect it always will, so
> no change is needed, but there you have it.

Ok.

> u64's and %llu _are_ incompatible on some architectures and I get to fix
> that about 1000000 times.

-- 
Simon Arlott