[U-Boot] [PATCH 3/7] mpc85xx: Add inline GPIO acessor functions

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1298311199-18775-4-git-send-email-Kyle.D.Moffett at boeing.com> you wrote:
> To ease the implementation of other MPC85xx board ports, several common
> GPIO helpers are added to <asm/mpc85xx_gpio.h>.

In which way is this specific to 85xx?  Why not make available on a
wider base?

> To assist other board ports, a small set of wrappers are used which
> provides a standard gpio_request() interface around the MPC85xx-specific
> functions.  This can be enabled with CONFIG_MPC85XX_GENERIC_GPIO

How similar is this interface with other "generic" GPIO interfaces we
have here in U-Boot (and in Linux) ?

> +static inline void mpc85xx_gpio_set(unsigned int mask,
> +		unsigned int dir, unsigned int val)
> +{
> +	volatile ccsr_gpio_t *gpio;
> +
> +	/* First mask off the unwanted parts of "dir" and "val" */
> +	dir &= mask;
> +	val &= mask;
> +
> +	/* Now read in the values we're supposed to preserve */
> +	gpio = (void *)(CONFIG_SYS_MPC85xx_GPIO_ADDR + 0xc00);
> +	dir |= (in_be32(&gpio->gpdir) & ~mask);
> +	val |= (in_be32(&gpio->gpdat) & ~mask);
> +
> +	/* Now write out the new values, writing the direction first */
> +	out_be32(&gpio->gpdir, dir);

This way work, but quite often is wrong.  If you set the direction
first for an output, you start driving the old value, before you set
the correct one.  This results in a short pulse that may cause
negative side effects.

Don't!

> +	asm("sync; isync":::"memory");

Why would that be needed? out_be32() is supposed to provide sufficient
memory barriers.

> +static inline unsigned int mpc85xx_gpio_get(unsigned int mask)
> +{
> +	volatile ccsr_gpio_t *gpio;

Why would this volatile be needed here?  [Please fix globally.]

> +#ifdef CONFIG_MPC85XX_GENERIC_GPIO
> +static inline int gpio_request(unsigned gpio, const char *label)
> +{
> +	/* Do nothing */
> +	(void)gpio;
> +	(void)label;

NAK.  Please don't do that. Fix globally.

> +static inline int gpio_direction_input(unsigned gpio)
> +{
> +	mpc85xx_gpio_set_in(1U << gpio);
> +	return 0;
> +}

Why is this function not void when it cannot return any usefult return
code anyway?

> +static inline int gpio_direction_output(unsigned gpio, int value)
> +{
> +	mpc85xx_gpio_set_low(1U << gpio);
> +	return 0;
> +}

Ditto.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Cleverness and Style have no place in getting a project completed.
                                                  -- Tom Christiansen