On Fri, Mar 04, 2005 at 04:08:17PM -0500, Wen Xiong wrote:
> +int get_jsm_board_number(void)
> +{
> + struct list_head *tmp;
> + struct jsm_board *cur_board_entry;
> + int adapter_count = 0;
> + u64 lock_flags;
> +
> + spin_lock_irqsave(&jsm_board_head_lock, lock_flags);
> + list_for_each(tmp, &jsm_board_head) {
> + cur_board_entry =
> + list_entry(tmp, struct jsm_board,
> + jsm_board_entry);
> + adapter_count++;
> + }
> + spin_unlock_irqrestore(&jsm_board_head_lock, lock_flags);
> +
> + return adapter_count;
> +}
Should this be static?
And it's returning the number of boards, not the current board number,
right?
And you have a indenting error in the list_for_each() section...
> +static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", "jsm:
> 1.1-1-INKERNEL");
Shouldn't that value also be in MODULE_VERSION()? And if so, it should
be a #define somewhere.
Also, don't put "jsm:" in your sysfs files, the file name describes what
the value should be. That goes for a lot of your sysfs files.
> +static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "0x%x\n", debug);
> +}
"debug" is not a nice variable to have in the global namespace :(
Also, why not just make this a module paramater, that way it can be
modified through that interface, and you don't have to create your own?
> +#define JSM_VERIFY_BOARD(p, bd) \
> + if (!p) \
> + return 0; \
> + bd = (struct jsm_board *)dev_get_drvdata(p); \
Cast is not needed.
> + if (!bd) \
> + return 0; \
> + if (bd->state != BOARD_READY) \
> + return 0; \
Don't break out of functions from within a macro. Will cause headaches
for people reviewing your code in the future.
And shouldn't you be returning an error if one of these checks fail?
> +static ssize_t jsm_ports_state_show(struct device *p, char *buf)
> +{
> + struct jsm_board *bd;
> + int count = 0;
> + int i = 0;
> +
> + JSM_VERIFY_BOARD(p, bd);
> +
> + for (i = 0; i < bd->nasync; i++) {
> + count += snprintf(buf + count, PAGE_SIZE - count,
> + "%d %s\n", bd->channels[i]->ch_portnum,
> + bd->channels[i]->ch_open_count ? "Open" : "Closed");
> + }
> + return count;
No, make this a per-port value. You are showing more than one value in
a single file. You do this for a few other sysfs files :(
And who cares about this value?
> +static ssize_t jsm_ports_baud_show(struct device *p, char *buf)
> +{
> + struct jsm_board *bd;
> + int count = 0;
> + int i = 0;
> +
> + JSM_VERIFY_BOARD(p, bd);
> +
> + for (i = 0; i < bd->nasync; i++) {
> + count += snprintf(buf + count, PAGE_SIZE - count,
> + "%d %d\n", bd->channels[i]->ch_portnum,
> bd->channels[i]->ch_old_baud);
> + }
> + return count;
> +}
> +static DEVICE_ATTR(ports_baud, S_IRUSR, jsm_ports_baud_show, NULL);
What's wrong with the standard tty ioctls that return this value, and
the other values you are exporting through sysfs? What is all of this
data needed for?
> +#define JSM_VERIFY_CHANNEL(p, ch) \
> + if (!p) \
> + return 0; \
> + ch = (struct jsm_channel *)class_get_devdata(p);\
> + if (!ch) \
> + return 0; \
> + if (ch->ch_bd->state != BOARD_READY) \
> + return 0; \
Again, don't put return in a macro, and return an error if there really
is one.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
