Messages in this thread | | | From | "germano.barreiro" <> | Subject | Re: patch for sysfs in the cyclades driver | Date | Mon, 01 Nov 2004 14:01:30 -0300 (EST) |
| |
Hi Greg
Firstly, very thanks again you for your comments. I think I understood them and I will check the driver you pointed as an example.
Kind regards, Germano
Cópia Greg KH <greg@kroah.com>:
> 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/
| |