>>>>> "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

Reply via email to