Re: [PATCH 1/2] Add c2 port support.

[Andrew Morton]

On Wed, 22 Oct 2008 21:52:46 +0200
Rodolfo Giometti <giometti@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/