Messages in this thread | | | Date | Wed, 22 Oct 2008 21:52:46 +0200 | From | Rodolfo Giometti <> | Subject | Re: [PATCH 1/2] Add c2 port support. |
| |
On Wed, Oct 22, 2008 at 11:11:45AM -0700, Andrew Morton wrote:
> No opinion yet - I haven't review them. > > <looks> > > <wonders why c2port_idr_lock and c2port_idr are kernel-wide symbols>
Fixed.
> <wonders what local_irq_disable() is supposed to achieve on SMP>
The SMP is achieved by mutex_lock(), the local_irq_disable() is used to avoid interructions in code execution. C2 port has very stringent timings.
> <spots an error-path memory leak in c2port_device_register> > > <spots a stray semicolon in c2port_device_register>
Can you please explain these two points?
> <suggests that update_lock be initialised at compile-time, not at runtime>
Fixed.
> > What I haven't yet got my head around is this: > > Currently this code supports only flash programming through sysfs > interface but extensions shoud be easy to add. > > is that really what we want to use sysfs for? As the prime interface > to a device driver (or is it a bus driver?) Didn't we used to use > device nodes for that sort of thing?
I decided to use this interface since the C2 ports are simple and very easy to manage with such files abstraction.
Whould you like I repropose the code with the fixed issues for your review?
Thanks for your suggestions,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com Linux Device Driver giometti@linux.it Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti
| |