[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Peng Fan <B51431@freescale.com>]

Hi, Igor

Reply below.

On 1/22/2015 4:38 PM, Igor Grinberg wrote:
> Hi Peng,
>
> On 01/22/15 03:06, Peng Fan wrote:
>> Hi Igor,
>>
>> Just kindly remind, did you miss this one? Since you ack the other patches in this patch set.
> Nope, I did not miss it.
> I just haven't looked at it yet...
>
>> On 1/21/2015 7:09 PM, Peng Fan wrote:
>>> This patch add DT support for mxc gpio driver.
>>>
>>> There are one place using CONFIG_OF_CONTROL macro.
>>> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>>>      platdata is alloced using calloc, so there is no need to use mxc_plat.
>>>
>>> The following situations are tested, and all work fine:
>>> 1. with DM, without DT
>>> 2. with DM and DT
>>> 3. without DM
>>> Since device tree has not been upstreamed, if want to test this patch.
>>> The followings need to be done.
>>>    + pieces of code does not gpio_request when using gpio_direction_xxx and
>>>      etc, need to request gpio.
>>>    + move the gpio settings from board_early_init_f to board_init
>>>    + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>>>    + Add device tree file and do related configuration in
>>>      `make ARCH=arm menuconfig`
>>> These will be done in future patches by step.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Besides the question below, looks good.
>
>>> ---
>>>    drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>>>    1 file changed, 52 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>>> index c52dd19..0766b78 100644
>>> --- a/drivers/gpio/mxc_gpio.c
>>> +++ b/drivers/gpio/mxc_gpio.c
>>> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>>>    #endif
>>>      #ifdef CONFIG_DM_GPIO
>>> +#include <fdtdec.h>
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>    static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>>>    {
>>>        u32 val;
>>> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>>        .get_function        = mxc_gpio_get_function,
>>>    };
>>>    -static const struct mxc_gpio_plat mxc_plat[] = {
>>> -    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> -    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> -    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>>> -        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> -    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> -#endif
>>> -};
>>> -
>>>    static int mxc_gpio_probe(struct udevice *dev)
>>>    {
>>>        struct mxc_bank_info *bank = dev_get_priv(dev);
>>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>>>        return 0;
>>>    }
>>>    +static int mxc_gpio_bind(struct udevice *device)
>>> +{
>>> +    struct mxc_gpio_plat *plat = device->platdata;
>>> +    struct gpio_regs *regs;
>>> +
>>> +    if (plat)
>>> +        return 0;
>>> +
>>> +    regs = dev_get_addr(device);
>>> +    if (!regs)
>>> +        return -ENXIO;
>>> +
>>> +    plat = calloc(1, sizeof(*plat));
>>> +    if (!plat)
>>> +        return -ENOMEM;
>>> +
>>> +    plat->regs = regs;
>>> +    plat->bank_index = device->req_seq;
> Can we assume that in mxc_gpio case, device->req_seq will never equal -1?
To NO DT situation, "if (plat) return0;" will directly return, because 
plat is staticlly intialized in source file. To DT,  in aliases, 
"gpio0=&gpio1" and etc should be added, to make
req_seq work. If "reg" property or "alises" are not provied, req_seq 
will be -1. Here I want aliases, because want bank_index from 0 to max 
bank, but this can not be guaranteed.

If reg property is not provided, dev_get_addr will return error. And 
plat->bank_index = device->req_seq will not be executed. If reg property 
is provided, but alises "gpiox=&gpioy" are not provied, there is not a 
good way to check req_seq exceeds max bank, since I do not want to 
include `#define xxMAX_BANK yy`. So I did not test this "req_seq < 0 " 
situation.
>
>>> +    device->platdata = plat;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct udevice_id mxc_gpio_ids[] = {
>>> +    { .compatible = "fsl,imx35-gpio" },
>>> +    { }
>>> +};
>>> +
>>>    U_BOOT_DRIVER(gpio_mxc) = {
>>>        .name    = "gpio_mxc",
>>>        .id    = UCLASS_GPIO,
>>>        .ops    = &gpio_mxc_ops,
>>>        .probe    = mxc_gpio_probe,
>>>        .priv_auto_alloc_size = sizeof(struct mxc_bank_info),
>>> +    .of_match = mxc_gpio_ids,
>>> +    .bind    = mxc_gpio_bind,
>>> +};
>>> +
>>> +#ifndef CONFIG_OF_CONTROL
>>> +static const struct mxc_gpio_plat mxc_plat[] = {
>>> +    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> +    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> +    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>>> +        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> +    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> +#endif
>>>    };
>>>      U_BOOT_DEVICES(mxc_gpios) = {
>>> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>>    #endif
>>>    };
>>>    #endif
>>> +#endif
>> Thanks,
>> Peng.
>>
Thanks,
Peng.