Messages in this thread | | | Date | Fri, 29 Oct 2004 21:40:56 -0700 | From | Greg KH <> | Subject | Re: patch for sysfs in the cyclades driver |
| |
On Thu, Oct 28, 2004 at 04:56:30PM -0200, Germano Barreiro wrote: > Hi > > This patch implements the sysfs support in the cyclades async driver, > which is needed by the Cyclades' CyMonitor application. It is based in > the last one sent (see copied message below), but implements the changes > asked for that patch by Greg (the array of attributes was eliminated and > now there is only one file for each value to be exported). > I hope it is ok to be applied now, but if more changes are needed I > would be pleased to listen about them. By the way, I'm grateful to > Marcelo for taking time to examining many "middle" versions of this > patch, and also to Greg for his comments to the last patch.
Close, it's getting better, but you still have a ways to go...
> --- linux-2.6.10rc1.orig/drivers/char/cyclades.c 2004-10-25 16:40:00.000000000 -0200 > +++ linux-2.6.10rc1/drivers/char/cyclades.c 2004-10-26 17:13:20.000000000 -0200 > @@ -646,6 +646,7 @@ static char rcsid[] = > #include <linux/string.h> > #include <linux/fcntl.h> > #include <linux/ptrace.h> > +#include <linux/device.h> > #include <linux/cyclades.h> > #include <linux/mm.h> > #include <linux/ioport.h> > @@ -700,8 +701,36 @@ static void cy_send_xchar (struct tty_st > > #define JIFFIES_DIFF(n, j) ((j) - (n)) > > +static char _version[150]; > static struct tty_driver *cy_serial_driver; > > + > +const static char sysufixes[18][10]={"stat","card","baud","flow","rxfl","txfl","dcd","dsr","cts", > + "rts","dtr","rx","tx","bdrx","bdtx","par","stop","chlen"};
Ick, this isn't needed.
> +ssize_t (*showfunctions[])(struct device *, char *)={ > + show_sys_stat, > + show_sys_card, > + show_sys_baud, > + show_sys_flow, > + show_sys_rxfl, > + show_sys_txfl, > + show_sys_dcd, > + show_sys_dsr, > + show_sys_cts, > + show_sys_rts, > + show_sys_dtr, > + show_sys_rx, > + show_sys_tx, > + show_sys_bdrx, > + show_sys_bdtx, > + show_sys_par, > + show_sys_stop, > + show_sys_chlen > +};
Why not just do as the i2c chip drivers do? Use a macro for your show functions, it's much simpler.
> + switch(whatinfo){ > + > + case STAT: //open/closed
Break this big switch statement up into the individual show functions. Hm, ok, you can't use a macro for them then, sorry. But it should be simpler.
> + if ( tty == 0 ) > + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_close); > + else > + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_open);
No, don't put the %s: stuff in there. The user read from the "stat" file. They know what the value should be, you don't have to remind them again :)
thanks,
greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |