Hi Nikita,
On 2 October 2014 08:18, Nikita Kiryanov <nik...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
