Hi Igor, On 15 September 2014 11:13, Igor Grinberg <[email protected]> wrote: > Hi Simon, > > On 09/15/14 15:57, Simon Glass wrote: >> GPIOs should be requested before use. Without this, driver model will not >> permit the GPIO to be used. > > Right. That should have been done from the start... Sorry for that... > A question below though.. > >> >> Signed-off-by: Simon Glass <[email protected]> >> --- >> >> arch/arm/imx-common/i2c-mxv7.c | 14 ++++++++++++++ >> board/compulab/cm_fx6/cm_fx6.c | 9 +++++++++ >> board/compulab/cm_fx6/common.c | 3 +++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c >> index a580873..7cea024 100644 >> --- a/arch/arm/imx-common/i2c-mxv7.c >> +++ b/arch/arm/imx-common/i2c-mxv7.c >> @@ -4,6 +4,7 @@ >> * SPDX-License-Identifier: GPL-2.0+ >> */ >> #include <common.h> >> +#include <malloc.h> >> #include <asm/arch/clock.h> >> #include <asm/arch/imx-regs.h> >> #include <asm/errno.h> >> @@ -72,8 +73,21 @@ static void * const i2c_bases[] = { >> void setup_i2c(unsigned i2c_index, int speed, int slave_addr, >> struct i2c_pads_info *p) >> { >> + char *name; >> + >> if (i2c_index >= ARRAY_SIZE(i2c_bases)) >> return; >> + >> + name = malloc(9); >> + assert(name); >> + sprintf(name, "i2c_sda%d", i2c_index); >> + gpio_request(p->sda.gp, name); >> + >> + name = malloc(9); >> + assert(name); >> + sprintf(name, "i2c_scl%d", i2c_index); >> + gpio_request(p->scl.gp, name); >> + >> /* Enable i2c clock */ >> enable_i2c_clk(1, i2c_index); >> /* Make sure bus is idle */ >> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c >> index fdb8ebf..80a123d 100644 >> --- a/board/compulab/cm_fx6/cm_fx6.c >> +++ b/board/compulab/cm_fx6/cm_fx6.c >> @@ -71,8 +71,15 @@ static iomux_v3_cfg_t const sata_pads[] = { >> >> static void cm_fx6_setup_issd(void) >> { >> + int i; >> + >> SETUP_IOMUX_PADS(sata_pads); >> + >> + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) >> + gpio_request(cm_fx6_issd_gpios[i], "sata"); >> + >> /* Make sure this gpio has logical 0 value */ >> + gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int"); >> gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); >> udelay(100); >> >> @@ -167,6 +174,7 @@ static int cm_fx6_usb_hub_reset(void) >> } >> >> SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL)); >> + gpio_request(CM_FX6_USB_HUB_RST, "usb_hub_rst"); >> gpio_direction_output(CM_FX6_USB_HUB_RST, 0); >> udelay(10); >> gpio_direction_output(CM_FX6_USB_HUB_RST, 1); >> @@ -324,6 +332,7 @@ int board_eth_init(bd_t *bis) >> >> SETUP_IOMUX_PADS(enet_pads); >> /* phy reset */ >> + gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); >> gpio_direction_output(CM_FX6_ENET_NRST, 0); >> udelay(500); >> gpio_set_value(CM_FX6_ENET_NRST, 1); >> diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c >> index 1f39679..562313b 100644 >> --- a/board/compulab/cm_fx6/common.c >> +++ b/board/compulab/cm_fx6/common.c >> @@ -79,6 +79,9 @@ void cm_fx6_set_ecspi_iomux(void) >> >> int board_spi_cs_gpio(unsigned bus, unsigned cs) >> { >> +#ifndef CONFIG_SPL_BUILD >> + gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); >> +#endif >> return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; >> } >> #endif >> > > In all the above gpio_request() calls, I think we should check for > the return value. > Because after patch 8 in the series it can fail if if someone > double requests the same gpio.
That's true, although for a particular board you presumably know what you are doing. The problem happens more when we move this sort of thing to drivers, and there is a conflict. In some cases there is not way to report an error (void functions), and in others it needs additional plumbing. But I agree we should start to fix this sort of thing. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
