Messages in this thread | | | Date | Wed, 22 Oct 2008 13:01:50 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 1/2] Add c2 port support. |
| |
On Wed, 22 Oct 2008 21:52:46 +0200 Rodolfo Giometti <giometti@enneenne.com> wrote:
> 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.
Please add code comments explaning this (unless I missed them).
> > <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?
> +struct c2port_device *c2port_device_register(char *name, > + struct c2port_ops *ops, void *devdata) > +{ > + struct c2port_device *c2dev; > + int id, ret; > + > + if (unlikely(!ops) || unlikely(!ops->access) || \ > + unlikely(!ops->c2d_dir) || unlikely(!ops->c2ck_set) || \ > + unlikely(!ops->c2d_get) || unlikely(!ops->c2d_set)) > + return ERR_PTR(-EINVAL); > + > + c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL); > + if (unlikely(!c2dev)) > + return ERR_PTR(-ENOMEM); > + > + ret = idr_pre_get(&c2port_idr, GFP_KERNEL); > + if (!ret) > + return ERR_PTR(-ENOMEM);
^^ leaks c2dev
> + spin_lock_irq(&c2port_idr_lock); > + ret = idr_get_new(&c2port_idr, c2dev, &id); > + spin_unlock_irq(&c2port_idr_lock); > + > + if (ret < 0) > + goto error_idr_get_new; > + c2dev->id = id; > + > + c2dev->dev = device_create(c2port_class, NULL, 0, c2dev, > + "c2port%d", id); > + if (unlikely(!c2dev->dev)) { > + ret = -ENOMEM; > + goto error_device_create; > + };
^
> > > <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.
well.. what _is_ the implemented interface? What alternatives were considered and why were they judged inappropriate? What interfaces do any similar in-tree drivers use?
> Whould you like I repropose the code with the fixed issues for your > review?
yup
| |