>>>>> "Anton" == Anton Vorontsov <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Anton> Looks good to me, thanks. Anton> Few comments below, might want to consider some of them if you want to. >> + gpio1: [EMAIL PROTECTED] { >> + #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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev