From: Peter Korsgaard <jacmet@sunsite•dk>
To: avorontsov@ru•mvista.com
Cc: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings
Date: Mon, 22 Sep 2008 16:19:21 +0200 [thread overview]
Message-ID: <87ej3ctg9i.fsf@macbook.be.48ers.dk> (raw)
In-Reply-To: <20080922133254.GA25776@oksana.dev.rtsoft.ru> (Anton Vorontsov's message of "Mon\, 22 Sep 2008 17\:32\:54 +0400")
>>>>> "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
prev parent reply other threads:[~2008-09-22 14:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-09-22 6:32 [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings Peter Korsgaard
2008-09-22 13:32 ` Anton Vorontsov
2008-09-22 14:19 ` Peter Korsgaard [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ej3ctg9i.fsf@macbook.be.48ers.dk \
--to=jacmet@sunsite$(echo .)dk \
--cc=avorontsov@ru$(echo .)mvista.com \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox