Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

[Peter Korsgaard <jacmet@sunsite.dk>]

>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes:

 Anton> On Mon, Sep 22, 2008 at 08:32:34AM +0200, Peter Korsgaard wrote:
 >> Structured similar to the existing QE GPIO support.
 >> 
 >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

 Anton> Looks good to me, thanks.

 Anton> Few comments below, might want to consider some of them if you want to.

 >> +	gpio1: gpio-controller@c00 {
 >> +		#gpio-cells = <2>;
 >> +		compatible = "fsl,mpc8347-gpio, fsl,mpc8349-gpio";

 Anton> Some quotes are missing. Should be "fsl,mpc8347-gpio",
 Anton> "fsl,mpc8349-gpio";

Argh, sorry about that.

 >> +#define GPIO_DIR		0x00
 >> +#define GPIO_ODR		0x04
 >> +#define GPIO_DAT		0x08
 >> +#define GPIO_IER		0x0c
 >> +#define GPIO_IMR		0x10
 >> +#define GPIO_ICR		0x14

 Anton> This is better described in a struct. Will save few characters,
 Anton> and just looks nicer. That is,

I think that's basically a question about taste. Some people like
structs, some don't - I prefer the defines as it's very easy to see
exactly what addresses gets used.

 >> +struct mpc8xxx_gpio_chip {
 >> +	struct of_mm_gpio_chip mm_gc;
 >> +	spinlock_t lock;
 >> +
 >> +	/* shadowed data register to be able to clear/set output pins in
 >> +	   open drain mode safely */

 Anton> Why not a canonical comment?

 Anton> /*
 Anton>  * Multi-line
 Anton>  * comment.
 Anton>  */

Fine with me. It started as a single line comment that I later
expanded on.

 >> +static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 >> +{
 >> +	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
 >> +
 >> +	return !!(in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio));

 Anton> No need for !!. gpio api spec says that you may return any
 Anton> value != 0 for the logical 1. Negative values are ok.

True.

 >> +static int mpc8xxx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 >> +{
 >> +	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
 >> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
 >> +	unsigned long flags;
 >> +
 >> +	spin_lock_irqsave(&mpc8xxx_gc->lock, flags);
 >> +
 >> +	out_be32(mm->regs + GPIO_DIR,
 >> +		 in_be32(mm->regs + GPIO_DIR) & ~mpc8xxx_gpio2mask(gpio));

 Anton> Would look better if you'd use clrbits32().

Ok.

 >> +static int __init mpc8xxx_add_controller(struct device_node *np)
 >> +{

 Anton> You don't check return value for this function. `void' return type
 Anton> would work.

Yes, I kept it as we already keep track of the return value within the
function (for error reporting) - I'll get rid of it.

I'll send an updated patch shortly.

-- 
Bye, Peter Korsgaard