Messages in this thread | | | Date | Sat, 2 Aug 2008 07:45:11 +0400 | From | Alexey Dobriyan <> | Subject | Re: [PATCH 01/13] proc tty: introduce ->proc_fops |
| |
On Tue, Jul 29, 2008 at 10:53:50AM +0100, Alan Cox wrote: > On Tue, 29 Jul 2008 05:29:08 +0400 > Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > Add struct tty_operations:proc_fops . > > > > The intent is gradual switch of TTY drivers from ->read_proc usage. > > proc entries are created with proc_create_data() which even fixes > > early-read races. > > > > Eventually ->read_proc code will be removed from TTY code, thus helping > > remove ->read_proc from whole proc code. > > I'm going to NAK this but not because I think the concept is wrong. I > think the way its been done is perhaps wrong. > > You've added ifdefs to a lot of drivers and more basically duplicate > code.
It's boileplate code which is hard to screwup, unlike boilerplate at the end of every ->read_proc hook.
> Is there a reason you can't keep the ->read_proc method in the tty > code but as a function called by a single instance of proc_fops and > seq_file methods for the whole tty driver layer. > > Ie have a single tty seq_file method that calls driver->ops->read_proc as > the seqfile iterator ?
I think it should be struct tty_operations::proc_fops .
In fact, posted conversions are bit incorrect.
Let's look at cyclades. It prints banner, then information about each card. The very right way to print it via seq_files is implement struct seq_operations::start, next, stop, show.
If TTY driver prints something simple, just one show hook is enough. If TTY driver wants something more complex, right way start to be seq_operations and one hook is not enough anymore.
But I don't really care about this exact issue, as long as ->read_proc TTY hook is gone as ->read_proc proc hook.
> > int count, int *eof, void *data); > > +#ifdef CONFIG_PROC_FS > > + const struct file_operations *proc_fops; > > +#endif > > I'd prefer you didn't put variables in the middle of the methods. Also > the ifdef isn't needed. It's not worth one pointer to create a load of > ifdefs
OK.
| |