| Message ID | 1410925884-8767-9-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On 09/17/14 06:51, 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 v2: > - Check return values of gpio_request() > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ > board/compulab/cm_fx6/common.c | 7 +++++++ > 3 files changed, 46 insertions(+) > > 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 62c625a..10e31b6 100644 > --- a/board/compulab/cm_fx6/cm_fx6.c > +++ b/board/compulab/cm_fx6/cm_fx6.c > @@ -71,8 +71,21 @@ static iomux_v3_cfg_t const sata_pads[] = { > > static int cm_fx6_setup_issd(void) > { > + int ret; > + int i; > + > 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; > gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); > udelay(100); > > @@ -182,6 +195,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"); This is confusing... This GPIO should be already requested several lines above. > gpio_direction_output(CM_FX6_USB_HUB_RST, 0); > udelay(10); > gpio_direction_output(CM_FX6_USB_HUB_RST, 1); > @@ -339,6 +353,7 @@ int board_eth_init(bd_t *bis) > > SETUP_IOMUX_PADS(enet_pads); > /* phy reset */ > + gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); Error check? This one also should not fail the boot process, just abort the Ethernet init. > 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..e4d7e2e 100644 > --- a/board/compulab/cm_fx6/common.c > +++ b/board/compulab/cm_fx6/common.c > @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > { > +#ifndef CONFIG_SPL_BUILD > + int ret; > + > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > + if (ret) > + return ret; > +#endif Is there a reason for excluding this request from spl builds? We do build with CONFIG_SPL_GPIO_SUPPORT and we don't have a problem with spl size. So, if there is no strong reason to exclude it from spl build, I'd like to remove the #ifndef. > return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; > } > #endif >
Hi Igor, On 17 September 2014 06:13, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi Simon, > > On 09/17/14 06:51, 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 v2: > > - Check return values of gpio_request() > > > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > > board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ > > board/compulab/cm_fx6/common.c | 7 +++++++ > > 3 files changed, 46 insertions(+) > > > > 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 62c625a..10e31b6 100644 > > --- a/board/compulab/cm_fx6/cm_fx6.c > > +++ b/board/compulab/cm_fx6/cm_fx6.c > > @@ -71,8 +71,21 @@ static iomux_v3_cfg_t const sata_pads[] = { > > > > static int cm_fx6_setup_issd(void) > > { > > + int ret; > > + int i; > > + > > 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; > > gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); > > udelay(100); > > > > @@ -182,6 +195,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"); > > This is confusing... This GPIO should be already requested > several lines above. > OK > > > gpio_direction_output(CM_FX6_USB_HUB_RST, 0); > > udelay(10); > > gpio_direction_output(CM_FX6_USB_HUB_RST, 1); > > @@ -339,6 +353,7 @@ int board_eth_init(bd_t *bis) > > > > SETUP_IOMUX_PADS(enet_pads); > > /* phy reset */ > > + gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); > > Error check? > This one also should not fail the boot process, just abort the > Ethernet init. > OK > > > 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..e4d7e2e 100644 > > --- a/board/compulab/cm_fx6/common.c > > +++ b/board/compulab/cm_fx6/common.c > > @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) > > > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > > { > > +#ifndef CONFIG_SPL_BUILD > > + int ret; > > + > > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > > + if (ret) > > + return ret; > > +#endif > > Is there a reason for excluding this request from spl builds? > We do build with CONFIG_SPL_GPIO_SUPPORT and we don't have a > problem with spl size. > So, if there is no strong reason to exclude it from spl build, > I'd like to remove the #ifndef. > It isn't supported by DM yet. > > > return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; > > } > > #endif > > > > -- > Regards, > Igor. > Regards, Simon
On 09/17/14 17:00, Simon Glass wrote: > Hi Igor, > > On 17 September 2014 06:13, Igor Grinberg <grinberg@compulab.co.il <mailto:grinberg@compulab.co.il>> wrote: > > Hi Simon, > > On 09/17/14 06:51, 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 <mailto:sjg@chromium.org>> > > --- > > > > Changes in v2: > > - Check return values of gpio_request() > > > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > > board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ > > board/compulab/cm_fx6/common.c | 7 +++++++ > > 3 files changed, 46 insertions(+) > > [...] > > diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c > > index 1f39679..e4d7e2e 100644 > > --- a/board/compulab/cm_fx6/common.c > > +++ b/board/compulab/cm_fx6/common.c > > @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) > > > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > > { > > +#ifndef CONFIG_SPL_BUILD > > + int ret; > > + > > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > > + if (ret) > > + return ret; > > +#endif > > Is there a reason for excluding this request from spl builds? > We do build with CONFIG_SPL_GPIO_SUPPORT and we don't have a > problem with spl size. > So, if there is no strong reason to exclude it from spl build, > I'd like to remove the #ifndef. > > > It isn't supported by DM yet. AFAICS, gpio_request() is available for SPL. So in such case the non-DM version will be used and I can't see any problem with this (unless it does not work for some reason). > > > > > return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; > > } > > #endif > > > > -- > Regards, > Igor. > > > Regards, > Simon >
Hi Igor, On 17 September 2014 08:31, Igor Grinberg <grinberg@compulab.co.il> wrote: > On 09/17/14 17:00, Simon Glass wrote: > > Hi Igor, > > > > On 17 September 2014 06:13, Igor Grinberg <grinberg@compulab.co.il > <mailto:grinberg@compulab.co.il>> wrote: > > > > Hi Simon, > > > > On 09/17/14 06:51, 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 <mailto: > sjg@chromium.org>> > > > --- > > > > > > Changes in v2: > > > - Check return values of gpio_request() > > > > > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > > > board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ > > > board/compulab/cm_fx6/common.c | 7 +++++++ > > > 3 files changed, 46 insertions(+) > > > > > [...] > > > > diff --git a/board/compulab/cm_fx6/common.c > b/board/compulab/cm_fx6/common.c > > > index 1f39679..e4d7e2e 100644 > > > --- a/board/compulab/cm_fx6/common.c > > > +++ b/board/compulab/cm_fx6/common.c > > > @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) > > > > > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > > > { > > > +#ifndef CONFIG_SPL_BUILD > > > + int ret; > > > + > > > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > > > + if (ret) > > > + return ret; > > > +#endif > > > > Is there a reason for excluding this request from spl builds? > > We do build with CONFIG_SPL_GPIO_SUPPORT and we don't have a > > problem with spl size. > > So, if there is no strong reason to exclude it from spl build, > > I'd like to remove the #ifndef. > > > > > > It isn't supported by DM yet. > > AFAICS, gpio_request() is available for SPL. > So in such case the non-DM version will be used and > I can't see any problem with this (unless it does not work for some > reason). > I suggest that if you wanted the gpio_request() in the non-DM case you would already have added it? I would prefer not to have an #ifdef CONFIG_DM_GPIO here also. Can we revisit when DM supports SPL? Regards, Simon
On 09/17/14 17:34, Simon Glass wrote: > Hi Igor, > > On 17 September 2014 08:31, Igor Grinberg <grinberg@compulab.co.il <mailto:grinberg@compulab.co.il>> wrote: > > On 09/17/14 17:00, Simon Glass wrote: > > Hi Igor, > > > > On 17 September 2014 06:13, Igor Grinberg <grinberg@compulab.co.il <mailto:grinberg@compulab.co.il> <mailto:grinberg@compulab.co.il <mailto:grinberg@compulab.co.il>>> wrote: > > > > Hi Simon, > > > > On 09/17/14 06:51, 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 <mailto:sjg@chromium.org> <mailto:sjg@chromium.org <mailto:sjg@chromium.org>>> > > > --- > > > > > > Changes in v2: > > > - Check return values of gpio_request() > > > > > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > > > board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ > > > board/compulab/cm_fx6/common.c | 7 +++++++ > > > 3 files changed, 46 insertions(+) > > > > > [...] > > > > diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c > > > index 1f39679..e4d7e2e 100644 > > > --- a/board/compulab/cm_fx6/common.c > > > +++ b/board/compulab/cm_fx6/common.c > > > @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) > > > > > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > > > { > > > +#ifndef CONFIG_SPL_BUILD > > > + int ret; > > > + > > > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > > > + if (ret) > > > + return ret; > > > +#endif > > > > Is there a reason for excluding this request from spl builds? > > We do build with CONFIG_SPL_GPIO_SUPPORT and we don't have a > > problem with spl size. > > So, if there is no strong reason to exclude it from spl build, > > I'd like to remove the #ifndef. > > > > > > It isn't supported by DM yet. > > AFAICS, gpio_request() is available for SPL. > So in such case the non-DM version will be used and > I can't see any problem with this (unless it does not work for some reason). > > > I suggest that if you wanted the gpio_request() in the non-DM case you would already have added it? I would thought so too... Unfortunately, it has slipped through... > > I would prefer not to have an #ifdef CONFIG_DM_GPIO here also. Can we revisit when DM supports SPL? Yes, no problem.
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 62c625a..10e31b6 100644 --- a/board/compulab/cm_fx6/cm_fx6.c +++ b/board/compulab/cm_fx6/cm_fx6.c @@ -71,8 +71,21 @@ static iomux_v3_cfg_t const sata_pads[] = { static int cm_fx6_setup_issd(void) { + int ret; + int i; + 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; gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); udelay(100); @@ -182,6 +195,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); @@ -339,6 +353,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..e4d7e2e 100644 --- a/board/compulab/cm_fx6/common.c +++ b/board/compulab/cm_fx6/common.c @@ -79,6 +79,13 @@ void cm_fx6_set_ecspi_iomux(void) int board_spi_cs_gpio(unsigned bus, unsigned cs) { +#ifndef CONFIG_SPL_BUILD + int ret; + + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); + if (ret) + return ret; +#endif return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; } #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 v2: - Check return values of gpio_request() arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ board/compulab/cm_fx6/cm_fx6.c | 15 +++++++++++++++ board/compulab/cm_fx6/common.c | 7 +++++++ 3 files changed, 46 insertions(+)