[U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/05/2014 10:09 AM, Simon Glass wrote:
> This is an implementation of GPIOs for Tegra that uses driver model. It has
> been tested on trimslice and also using the new iotrace feature.
> 
> The implementation uses a top-level GPIO device (which has no actual GPIOS).
> Under this all the banks are created as separate GPIO devices.

I don't think that dividing the GPIOs into banks is appropriate. While
the textual naming of Tegra GPIOs is based on the bank concept, I think
that division should only apply to the textual naming, and not any data
structures or device registration, etc. In fact, I often refer to Tegra
GPIOs using their integer ID rather than their name, and dividing the
controller into separate banks probably technically disallows this,
since there would be no architected guarantee that the numbering of the
banks would be sequential. Contiguous numbering of all GPIOs within the
controller is also useful for correlation with pinmux numbering of pins.

...
> Since driver model is not yet available before relocation, or in SPL, a
> special function is provided for seaboard's SPL code.

Perhaps we should just remove that code from Seaboard, since sharing the
UART/SPI pins really isn't a useful feature.

Still, we will need to maintain some low-level APIs so that the SPL can
initialize GPIOs at boot, since doing so is part of Tegra's HW-defined
pinmux initialization mechanism. For reference, see the series I posted:

https://www.mail-archive.com/u-boot at lists.denx.de/msg136607.html

In particular:
ARM: tegra: add GPIO initialization table function
ARM: tegra: make use of GPIO init table on Jetson TK1

> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c

> +struct tegra_gpio_platdata {
> +	struct gpio_ctlr_bank *bank;
> +	const char *port_name;	/* Name of port, e.g. "B" */
> +	int base_port;		/* Port number for this port (0, 1,.., n-1) */
> +};

I'm not sure what "platdata" means. Would "tegra_gpio_bank" be a better
name?

Oh, looking later in the patch, this seems to be about passing data to
probe(). It would be better to get rid of the bank concept, have a
single device, and just pass the GPIO count or SoC type to probe.

> +/* Information about each port at run-time */
> +struct tegra_port_info {
> +	char label[TEGRA_GPIOS_PER_PORT][GPIO_NAME_SIZE];
> +	struct gpio_ctlr_bank *bank;
> +	int base_port;		/* Port number for this port (0, 1,.., n-1) */
> +};

It seems odd to have two data-structures with Tegra-specific data
related to a struct gpio_ctrl_bank. Perhaps these can be combined into one?

> +#define GPIO_NUM(state, offset) \
> +	(((state)->base_port * TEGRA_GPIOS_PER_PORT) + (offset))

It's not clear whether "state" is supposed to be a "tegra_gpio_platdata"
or a "tegra_port_info". I guess the macro works with either, but that
seems a bit dangerous.

>  /* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
> +static int get_config(struct tegra_port_info *state, int offset)

Hmm. I guess the name "state" for the GPIO_NUM() macro parameter comes
from the fact that the functions in this file have a parameter named
"state". That seems nasty; it'd be better to name that port_info so it
represents that type it contains. Still, that's a pre-existing issue
unrelated to this patch.

That said, naming a macro parameter "state" and using it in functions
that typically have a parameter named "state" seems dangerous. Perhaps
prefix and suffix the macro parameter name with _ to make the
distinction obvious - i.e.. _state_. That's typical good practice for
macros.

> +static int tegra_gpio_get_state(struct device *dev, unsigned int offset,
> +				char *buf, int bufsize)

> +	is_gpio = get_config(state, offset);		/* GPIO, not SFPIO */

Typo in SFIO there.

> +	size = snprintf(buf, bufsize, "%s%d: ",
> +			uc_priv->bank_name ? uc_priv->bank_name : "", offset);
> +	buf += size;
> +	bufsize -= size;

size can be larger than bufsize if the string would have overflowed. Do
the later calls to snprintf() handle negative size arguments correctly?

> +static char *gpio_port_name(int base_port)
>  {
> +	char *name, *s;
> +
> +	name = malloc(3);

It seems like this should be statically allocated (e.g. as part of *plat
in gpio_tegra_bind() or something?). Still, if we stop splitting things
into banks, we can completely get rid of this.

> +static int gpio_tegra_bind(struct device *parent)

> +	/*
> +	 * This driver does not make use of interrupts, other than to figure
> +	 * out the number of GPIO banks
> +	 */
> +	if (!fdt_getprop(gd->fdt_blob, parent->of_offset, "interrupts", &len))
> +		return -EINVAL;
> +	bank_count = len / 3 / sizeof(u32);
> +	ctlr = (struct gpio_ctlr *)fdtdec_get_addr(gd->fdt_blob,
> +						   parent->of_offset, "reg");

It's dangerous to assume that #interrupt-cells of the GIC is 3. The GIC
driver is the only thing that can define/parse GIC IRQ specifiers...

> diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h

>  #define CONFIG_DM
> +#define CONFIG_DM_GPIO
> +#define CONFIG_CMD_DM

I think the second of those lines should be part of patch 12/13
shouldn't it?