[PATCH] cxacru: Cleanup sysfs attribute code

Simon Arlott

author

2007-04-24 21:14 UTC #0 lore raw

PATCH 1/1 cxacru: Cleanup sysfs attribute code

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 <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Duncan Sands <[email protected]>

Cc Andrew Morton <[email protected]>

---
Patch depends on existing cxacru patches in -rc7-mm.

Quoted 21 lines · Andrew Morton

Review

Quoted 10 lines

(quoted diff) +10 -0

+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();
	}

Quoted 1 line

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]);

Quoted 3 lines · Duncan Sands

Review

Quoted 2 lines

(quoted diff) +2 -0

+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(-)

drivers/usb/atm/cxacru.c +0 -0

index 0c47755..1c5d6de 100644
@@ -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,

Signature · 4 lines

1.5.0.1

-- 
Simon Arlott

Duncan Sands

2007-04-25 07:19 UTC #1 lore raw

Hi Simon,

Review

Quoted 11 lines

static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf) {

(quoted diff) +2 -7

-	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"?

Quoted 1 line

(quoted diff) +1 -0

+	static char *str[] = { "no", "yes" };

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	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.

Simon Arlott

author

2007-04-25 11:18 UTC #2 lore raw

Quoted 15 lines · Duncan Sands

Hi Simon,
Review
Quoted 11 lines
static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf) {
(quoted diff) +2 -7
-	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.

Quoted 9 lines

Quoted 1 line
(quoted diff) +1 -0
+	static char *str[] = { "no", "yes" };
...
Review
Quoted 1 line
(quoted diff) +1 -0
+	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.

Signature · 1 line

Simon Arlott

Simon Arlott

author

2007-04-25 18:41 UTC #3 lore raw

PATCH 1/1 cxacru: Cleanup sysfs attribute code

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 <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Duncan Sands <[email protected]>

Cc Andrew Morton <[email protected]>

---
Patch depends on existing cxacru patches in -rc7-mm.

Quoted 6 lines · Simon Arlott

Quoted 5 lines · Duncan Sands
Review
Quoted 4 lines
static ssize_t cxacru_sysfs_showattr_dB(s16 value, char *buf) {
(quoted diff) +2 -0
+	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(-)

drivers/usb/atm/cxacru.c +0 -0

index 0c47755..d53de3c 100644
@@ -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,

Signature · 4 lines

1.5.0.1

-- 
Simon Arlott

Duncan Sands

Acked-by on v1 1/1

2007-04-25 19:33 UTC #4 lore raw

Quoted 8 lines · 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 <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Duncan Sands <[email protected]>

Acked-by Duncan Sands <[email protected]>

Quoted 1 line

Cc Andrew Morton <[email protected]>

Andrew Morton

2007-04-28 07:51 UTC #5 lore raw

In reply to Simon Arlott

Quoted 14 lines · On Wed, 25 Apr 2007 19:41:20 +0100 Simon Arlott <[email protected]> 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 <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Duncan Sands <[email protected]>
Cc Andrew Morton <[email protected]>
...
static ssize_t cxacru_sysfs_showattr_bool(u32 value, char *buf) {

Review

Quoted 10 lines

(quoted diff) +4 -5

-	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"?

Review

Quoted 7 lines

static ssize_t cxacru_sysfs_showattr_LINK(u32 value, char *buf) {
...
(quoted diff) +1 -0
+		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.

Simon Arlott

author

2007-04-28 08:59 UTC #6 lore raw

Quoted 9 lines · Andrew Morton

Review
Quoted 6 lines · On Wed, 25 Apr 2007 19:41:20 +0100 Simon Arlott <[email protected]> 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.
...
(quoted diff) +4 -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"?

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

Quoted 8 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+		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.

Quoted 2 lines

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

Signature · 1 line

Simon Arlott