lkml.org 
[lkml]   [2008]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-10-22 22:05    [W:0.072 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site