[U-Boot] [PATCH v4 6/9] dm: imx: Use gpio_request() to request GPIOs

[Simon Glass]

Hi Nikita,

On 2 October 2014 08:18, Nikita Kiryanov <nikita at 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 at 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