Re: [PATCH v4] powerpc/5200: Rework GPT driver to also be an IRQ controller

[Grant Likely <grant.likely@secretlab.ca>]

Hey Wolfram, thanks for the review.

On Wed, Feb 4, 2009 at 12:55 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
> I have been told to move such a changelog below the dashed lines,
> because it is not so important for the git-history.

This is only in my email for review purposes.  It won't be in the git commit.

>> +     status = in_be32(&gpt->regs->status) | MPC52xx_GPT_STATUS_IRQMASK;
>
> Like I mentioned in my first review, the following if-clause is always
> true. You probably mean "& MPC52xx_GPT_STATUS_IRQMASK"?

You're right.  I missed this from your first review.  Fixed.

>> +/* ---------------------------------------------------------------------
>> + * GPIOLIB hooks
>> + */
>> +#if defined(CONFIG_GPIOLIB)
>> +static inline struct mpc52xx_gpt_priv *gc_to_mpc52xx_gpt(struct gpio_chip *gc)
>> +{
>> +     return container_of(to_of_gpio_chip(gc), struct mpc52xx_gpt_priv,of_gc);
>
> Space after ','. As it is now, this is a checkpatch-error. Lines over 80
> chars are a warning.

This is on purpose as I described earlier.  Checkpatch is not God, and
the cure is worse than the disease.  (Granted, the disease is
equivalent to a running nose, and the cure to cheap kleenex, but hey,
it's still my decision).

>> +#else /* defined(CONFIG_SYSFS) */
>> +static void mpc52xx_gpt_create_attribs(struct mpc52xx_gpt_priv *) { return 0; }
>> +#endif /* defined(CONFIG_SYSFS) */
>
> Hmmm, I still have problems with this sysfs-entry. If you say that this
> is good for debugging, then please make it #DEBUG at least. Imagine
> every driver doing such things, this adds bloat. I'd still rather
> suggest using
>
> ~ memedit /dev/mem
> -> map <MBAR> 0x4000
> -> md 0x600
>
> which does the same and even allows for more (writing for example).

I understand your argument, but I'm still going to leave it in and
leave it enabled for now.  I will consider removing it after a release
or too.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.