| Message ID | 1412215048-565-7-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
This series splits the patch "dm: imx: Use gpio_request() to request GPIOs" into two patches, one for the i2c-mxv7.c stuff retaining Simon Glass as the author, and the other a rework of cm_fx6 subsystem init functions to accomodate calls to gpio_request(). Cc: Igor Grinberg <grinberg@compulab.co.il> Cc: Simon Glass <sjg@chromium.org> Nikita Kiryanov (1): arm: mx6: cm_fx6: use gpio request Simon Glass (1): dm: imx: i2c: Use gpio_request() to request GPIOs arch/arm/imx-common/i2c-mxv7.c | 25 ++++++++ board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------ 2 files changed, 119 insertions(+), 39 deletions(-)
Hi Simon, On 02/10/14 04:57, Simon Glass wrote: > GPIOs should be requested before use. Without this, driver model will not > permit the GPIO to be used. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v4: > - Adjust error checking to permit calling gpio_request() multiple times > - Avoid doing low-level SATA init multiple times > - Move SATA changes into this patch > > Changes in v3: > - Add a check for the Ethernet gpio_request() also > - Add a comment for the CONFIG_SPL_BUILD #ifdef > - Just warn when one of the board init stages fails > > Changes in v2: > - Check return values of gpio_request() > > arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++ > board/compulab/cm_fx6/cm_fx6.c | 61 ++++++++++++++++++++++++++++++++---------- > board/compulab/cm_fx6/common.c | 14 +++++++++- > 3 files changed, 84 insertions(+), 15 deletions(-) > > diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c > index 70cff5c..aaf6936 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,10 +73,26 @@ static void * const i2c_bases[] = { > int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > struct i2c_pads_info *p) > { > + char *name1, *name2; > int ret; > > if (i2c_index >= ARRAY_SIZE(i2c_bases)) > return -EINVAL; > + > + name1 = malloc(9); > + name2 = malloc(9); > + if (!name1 || !name2) > + return -ENOMEM; You have a memory leak here if name1 is allocated but name2 is not. > + sprintf(name1, "i2c_sda%d", i2c_index); > + sprintf(name2, "i2c_scl%d", i2c_index); > + ret = gpio_request(p->sda.gp, name1); > + if (ret) > + goto err_req1; > + > + ret = gpio_request(p->scl.gp, name2); > + if (ret) > + goto err_req2; > + > /* Enable i2c clock */ > ret = enable_i2c_clk(1, i2c_index); > if (ret) > @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > > err_idle: > err_clk: > + gpio_free(p->scl.gp); > +err_req2: > + gpio_free(p->sda.gp); > +err_req1: > + free(name1); > + free(name2); > + > return ret; > } > diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c > index 9c6e686..53681d4 100644 > --- a/board/compulab/cm_fx6/cm_fx6.c > +++ b/board/compulab/cm_fx6/cm_fx6.c While this patch addresses the errors I mentioned in V3, I think that the amount of additional checks this required demonstrates that these init functions (which can be called multiple times) are not the best place to do gpio requests. I'm open to the idea that these requests will be handled by the respective drivers (where applicable), but until that functionality is implemented I think it's best to do them in board_init(). I'm attaching 2 patches, which split this patch into two, one for i2c-mxv7.c, and the other for cm_fx6.c.
Hi Nikita, On 2 October 2014 08:18, Nikita Kiryanov <nikita@compulab.co.il> wrote: > Hi Simon, > > > On 02/10/14 04:57, Simon Glass wrote: >> >> GPIOs should be requested before use. Without this, driver model will not >> permit the GPIO to be used. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v4: >> - Adjust error checking to permit calling gpio_request() multiple times >> - Avoid doing low-level SATA init multiple times >> - Move SATA changes into this patch >> >> Changes in v3: >> - Add a check for the Ethernet gpio_request() also >> - Add a comment for the CONFIG_SPL_BUILD #ifdef >> - Just warn when one of the board init stages fails >> >> Changes in v2: >> - Check return values of gpio_request() >> >> arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++ >> board/compulab/cm_fx6/cm_fx6.c | 61 >> ++++++++++++++++++++++++++++++++---------- >> board/compulab/cm_fx6/common.c | 14 +++++++++- >> 3 files changed, 84 insertions(+), 15 deletions(-) >> >> diff --git a/arch/arm/imx-common/i2c-mxv7.c >> b/arch/arm/imx-common/i2c-mxv7.c >> index 70cff5c..aaf6936 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,10 +73,26 @@ static void * const i2c_bases[] = { >> int setup_i2c(unsigned i2c_index, int speed, int slave_addr, >> struct i2c_pads_info *p) >> { >> + char *name1, *name2; >> int ret; >> >> if (i2c_index >= ARRAY_SIZE(i2c_bases)) >> return -EINVAL; >> + >> + name1 = malloc(9); >> + name2 = malloc(9); >> + if (!name1 || !name2) >> + return -ENOMEM; > > > You have a memory leak here if name1 is allocated but name2 is not. Yes. Actually there is also a memory leak if the devices are removed (I'm not sure if I mentioned that somewhere, maybe on a Tegra thread). I'm planning to address both of these as part of the gpio_request() update, now that I think there are enough GPIO drivers to be reasonably confident of the best approach. > > >> + sprintf(name1, "i2c_sda%d", i2c_index); >> + sprintf(name2, "i2c_scl%d", i2c_index); >> + ret = gpio_request(p->sda.gp, name1); >> + if (ret) >> + goto err_req1; >> + >> + ret = gpio_request(p->scl.gp, name2); >> + if (ret) >> + goto err_req2; >> + >> /* Enable i2c clock */ >> ret = enable_i2c_clk(1, i2c_index); >> if (ret) >> @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int >> slave_addr, >> >> err_idle: >> err_clk: >> + gpio_free(p->scl.gp); >> +err_req2: >> + gpio_free(p->sda.gp); >> +err_req1: >> + free(name1); >> + free(name2); >> + >> return ret; >> } >> diff --git a/board/compulab/cm_fx6/cm_fx6.c >> b/board/compulab/cm_fx6/cm_fx6.c >> index 9c6e686..53681d4 100644 >> --- a/board/compulab/cm_fx6/cm_fx6.c >> +++ b/board/compulab/cm_fx6/cm_fx6.c > > > While this patch addresses the errors I mentioned in V3, I think that > the amount of additional checks this required demonstrates that these > init functions (which can be called multiple times) are not the best > place to do gpio requests. Well ignoring the names, there are just the two checks for gpio_request(), which you are going to need anyway I think. > > I'm open to the idea that these requests will be handled by the > respective drivers (where applicable), but until that functionality is > implemented I think it's best to do them in board_init(). I'm not convinced that swapping this back to the board, only to swap it back to the driver is a good plan. > > I'm attaching 2 patches, which split this patch into two, one for > i2c-mxv7.c, and the other for cm_fx6.c. OK will take a look, thanks. Regards, Simon
diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c index 70cff5c..aaf6936 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,10 +73,26 @@ static void * const i2c_bases[] = { int setup_i2c(unsigned i2c_index, int speed, int slave_addr, struct i2c_pads_info *p) { + char *name1, *name2; int ret; if (i2c_index >= ARRAY_SIZE(i2c_bases)) return -EINVAL; + + name1 = malloc(9); + name2 = malloc(9); + if (!name1 || !name2) + return -ENOMEM; + sprintf(name1, "i2c_sda%d", i2c_index); + sprintf(name2, "i2c_scl%d", i2c_index); + ret = gpio_request(p->sda.gp, name1); + if (ret) + goto err_req1; + + ret = gpio_request(p->scl.gp, name2); + if (ret) + goto err_req2; + /* Enable i2c clock */ ret = enable_i2c_clk(1, i2c_index); if (ret) @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, err_idle: err_clk: + gpio_free(p->scl.gp); +err_req2: + gpio_free(p->sda.gp); +err_req1: + free(name1); + free(name2); + return ret; } diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c index 9c6e686..53681d4 100644 --- a/board/compulab/cm_fx6/cm_fx6.c +++ b/board/compulab/cm_fx6/cm_fx6.c @@ -69,24 +69,52 @@ static iomux_v3_cfg_t const sata_pads[] = { IOMUX_PADS(PAD_EIM_BCLK__GPIO6_IO31 | MUX_PAD_CTRL(NO_PAD_CTRL)), }; -static void cm_fx6_setup_issd(void) +static int cm_fx6_setup_issd(void) { - SETUP_IOMUX_PADS(sata_pads); - /* Make sure this gpio has logical 0 value */ + static bool init_done = false; + int ret; + int i; + + if (!init_done) { + SETUP_IOMUX_PADS(sata_pads); + + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) { + ret = gpio_request(cm_fx6_issd_gpios[i], "sata"); + if (ret) + return ret; + } + + /* Make sure this gpio has logical 0 value */ + ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int"); + if (ret) + return ret; + init_done = true; + } + gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); udelay(100); cm_fx6_sata_power(0); mdelay(250); cm_fx6_sata_power(1); + + return 0; } #define CM_FX6_SATA_INIT_RETRIES 10 int sata_initialize(void) { - int err, i; + int err, i, ret; - cm_fx6_setup_issd(); + /* + * cm-fx6 may have iSSD not assembled and in this case it has + * bypasses for a (m)SATA socket on the baseboard. The socketed + * device is not controlled by those GPIOs. So just print a warning + * if the setup fails. + */ + ret = cm_fx6_setup_issd(); + if (ret) + printf("Warning: iSSD setup failed!\n"); for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) { err = setup_sata(); if (err) { @@ -183,9 +211,9 @@ static int cm_fx6_usb_hub_reset(void) int err; err = gpio_request(CM_FX6_USB_HUB_RST, "usb hub rst"); - if (err) { + if (err && err != -EBUSY) { printf("USB hub rst gpio request failed: %d\n", err); - return -1; + return err; } SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL)); @@ -199,13 +227,13 @@ static int cm_fx6_usb_hub_reset(void) static int cm_fx6_init_usb_otg(void) { - int ret; + int err; struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR; - ret = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr"); - if (ret) { - printf("USB OTG pwr gpio request failed: %d\n", ret); - return ret; + err = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr"); + if (err && err != -EBUSY) { + printf("err OTG pwr gpio request failed: %d\n", err); + return err; } SETUP_IOMUX_PAD(PAD_EIM_D22__GPIO3_IO22 | MUX_PAD_CTRL(NO_PAD_CTRL)); @@ -340,12 +368,17 @@ static int handle_mac_address(void) int board_eth_init(bd_t *bis) { - int res = handle_mac_address(); - if (res) + int err; + + err = handle_mac_address(); + if (err) puts("No MAC address found\n"); SETUP_IOMUX_PADS(enet_pads); /* phy reset */ + err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); + if (err) + printf("Etnernet NRST gpio request failed: %d\n", err); 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..22ace2f 100644 --- a/board/compulab/cm_fx6/common.c +++ b/board/compulab/cm_fx6/common.c @@ -79,6 +79,18 @@ void cm_fx6_set_ecspi_iomux(void) int board_spi_cs_gpio(unsigned bus, unsigned cs) { - return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; + if (bus != 0 || cs != 0) + return -EINVAL; + + /* DM does not support SPL yet and this function is not implemented */ +#ifndef CONFIG_SPL_BUILD + int ret; + + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); + if (ret && ret != -EBUSY) + return ret; +#endif + + return CM_FX6_ECSPI_BUS0_CS0; } #endif
GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v4: - Adjust error checking to permit calling gpio_request() multiple times - Avoid doing low-level SATA init multiple times - Move SATA changes into this patch Changes in v3: - Add a check for the Ethernet gpio_request() also - Add a comment for the CONFIG_SPL_BUILD #ifdef - Just warn when one of the board init stages fails Changes in v2: - Check return values of gpio_request() arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++ board/compulab/cm_fx6/cm_fx6.c | 61 ++++++++++++++++++++++++++++++++---------- board/compulab/cm_fx6/common.c | 14 +++++++++- 3 files changed, 84 insertions(+), 15 deletions(-)