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@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? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
