Re: PPC440EPx gpio driver

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Thu, Oct 09, 2008 at 08:46:44PM +0200, Stefan Roese wrote:
[...]
> > +struct ppc4xx_gpio_chip {
> > +	struct of_mm_gpio_chip mm_gc;
> > +	spinlock_t lock;
> > +	u32 shadow_or;
> > +	u32 shadow_tcr;
> > +	u32 shadow_osrl;
> > +	u32 shadow_osrh;
> > +	u32 shadow_tsrl;
> > +	u32 shadow_tsrh;
> > +	u32 shadow_odr;
> > +};
> 
> Do we really need all those shadow registers? Access to the "real" registers 
> is done while holding the spinlock.

The shadowed registers aren't about holding a spinlock, but are
about open drain pins and the fact that some GPIO controllers are
using single "data" register for "read state", "set state" and
"clear state" GPIO operations.

See Grant's reply:

http://ozlabs.org/pipermail/linuxppc-dev/2008-January/050826.html

(I didn't look for the hardware details of this driver, so I don't
know if the driver really needs to shadow the registers.)

[...]
> > +	for_each_compatible_node(np, NULL, "amcc,ppc4xx-gpio") {
> > +		int ret;
> > +		struct ppc4xx_gpio_chip *ppc4xx_gc;
> > +		struct of_mm_gpio_chip *mm_gc;
> > +		struct of_gpio_chip *of_gc;
> > +		struct gpio_chip *gc;
> > +
> > +		ppc4xx_gc = kzalloc(sizeof(*ppc4xx_gc), GFP_KERNEL);
> > +		if (!ppc4xx_gc) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +
> > +		spin_lock_init(&ppc4xx_gc->lock);
> > +
> > +		mm_gc = &ppc4xx_gc->mm_gc;
> > +		of_gc = &mm_gc->of_gc;
> > +		gc = &of_gc->gc;
> > +
> > +		mm_gc->save_regs = ppc4xx_gpio_save_regs;
> > +		of_gc->gpio_cells = 2;
> > +		gc->ngpio = 32;
> > +		gc->direction_input = ppc4xx_gpio_dir_in;
> > +		gc->direction_output = ppc4xx_gpio_dir_out;
> > +		gc->get = ppc4xx_gpio_get;
> > +		gc->set = ppc4xx_gpio_set;
> > +
> > +		ret = of_mm_gpiochip_add(np, mm_gc);
> > +		if (ret)
> > +			goto err;
> > +		continue;
> > +err:
> > +		pr_err("%s: registration failed with status %d\n",
> > +		       np->full_name, ret);
> > +		kfree(ppc4xx_gc);
> 
> This probably crashes if the kzalloc() fails above.

kfree(NULL); is defined to be a no-op.


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2