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