[U-Boot] [PATCH] dm:gpio:mxc get configuration from dtb

[Peng Fan]

Hi Simon,

On 1/20/2015 3:45 AM, Simon Glass wrote:
> Hi Peng.
>
> On 18 January 2015 at 23:11, Peng Fan <Peng.Fan at freescale.com> wrote:
>> This patch supports getting gpios' configuration from dtb.
>> CONFIG_OF_CONTROL is used to indicated which part is for device tree,
>> and which is not.
>>
>> This patch is already tested on mx6sxsabresd board. 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`
>>
> Sorry I am going to repeat some of Igor's comments...
I've seen Igor's comments.I'll address them.
>
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>> ---
>>   drivers/gpio/mxc_gpio.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index 8bb9e39..8603068 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -14,6 +14,10 @@
>>   #include <asm/arch/imx-regs.h>
>>   #include <asm/gpio.h>
>>   #include <asm/io.h>
>> +#ifdef CONFIG_OF_CONTROL
>> +#include <fdtdec.h>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +#endif
> Hopefully #ifdef not needed.
Ok. I'll try to remove them in v2 patch.
>
>>   enum mxc_gpio_direction {
>>          MXC_GPIO_DIRECTION_IN,
>> @@ -258,6 +262,7 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>          .get_function           = mxc_gpio_get_function,
>>   };
>>
>> +#ifndef CONFIG_OF_CONTROL
>>   static const struct mxc_gpio_plat mxc_plat[] = {
>>          { (struct gpio_regs *)GPIO1_BASE_ADDR },
>>          { (struct gpio_regs *)GPIO2_BASE_ADDR },
>> @@ -274,6 +279,7 @@ static const struct mxc_gpio_plat mxc_plat[] = {
>>          { (struct gpio_regs *)GPIO7_BASE_ADDR },
>>   #endif
> Same here.
>
>>   };
>> +#endif
>>
>>   static int mxc_gpio_probe(struct udevice *dev)
>>   {
>> @@ -283,7 +289,19 @@ static int mxc_gpio_probe(struct udevice *dev)
>>          int banknum;
>>          char name[18], *str;
>>
>> +#ifdef CONFIG_OF_CONTROL
>> +       /*
>> +        * In dts file add:
>> +        * aliases {
>> +        *      gpio0 = &gpio1;
>> +        *      gpio1 = &gpio2;
>> +        *      .....
>> +        * };
>> +        * Then set banknum accoring dev's seq number. */
>> +       banknum = dev->seq;
>> +#else
>>          banknum = plat - mxc_plat;
>> +#endif
>>          sprintf(name, "GPIO%d_", banknum + 1);
>>          str = strdup(name);
>>          if (!str)
>> @@ -295,14 +313,71 @@ static int mxc_gpio_probe(struct udevice *dev)
>>          return 0;
>>   }
>>
>> +#ifdef CONFIG_OF_CONTROL
>> +static int mxc_gpio_bind(struct udevice *device)
>> +{
>> +       struct mxc_gpio_plat *plat = device->platdata;
>> +       struct gpio_regs *ctrl;
>> +
>> +       if (plat)
>> +               return 0;
>> +       /*
>> +        * In the dts file, gpiox bank are as following:
>> +        *      gpio1: gpio at 0209c000 {
>> +        *              compatible = "fsl,imx6q-gpio", "fsl,imx35-gpio";
>> +        *              reg = <0x0209c000 0x4000>;
>> +        *              interrupts = <0 66 0x04 0 67 0x04>;
>> +        *              gpio-controller;
>> +        *              #gpio-cells = <2>;
>> +        *              interrupt-controller;
>> +        *              #interrupt-cells = <2>;
>> +        *      };
>> +        *
>> +        *      gpio2: gpio at 020a0000 {
>> +        *              compatible = "fsl,imx6q-gpio", "fsl,imx35-gpio";
>> +        *              reg = <0x020a0000 0x4000>;
>> +        *              interrupts = <0 68 0x04 0 69 0x04>;
>> +        *              gpio-controller;
>> +        *              #gpio-cells = <2>;
>> +        *              interrupt-controller;
>> +        *              #interrupt-cells = <2>;
>> +        *      };
>> +        *
>> +        * gpio1 is the 1st bank, gpio2 is the 2nd bank and gpio3 ....
>> +        */
>> +
>> +       ctrl = (struct gpio_regs *)fdtdec_get_addr(gd->fdt_blob,
>> +                                                  device->of_offset, "reg");
>> +       plat = calloc(1, sizeof(*plat));
>> +       if (!plat)
>> +               return -ENOMEM;
>> +
>> +       plat->regs = ctrl;
>> +
>> +       device->platdata = plat;
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct udevice_id mxc_gpio_ids[] = {
>> +       { .compatible = "fsl,imx35-gpio" },
>> +       { }
>> +};
>> +#endif
>> +
>>   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),
>> +#ifdef CONFIG_OF_CONTROL
>> +       .of_match = mxc_gpio_ids,
>> +       .bind   = mxc_gpio_bind,
>> +#endif
>>   };
>>
>> +#ifndef CONFIG_OF_CONTROL
>>   U_BOOT_DEVICES(mxc_gpios) = {
>>          { "gpio_mxc", &mxc_plat[0] },
>>          { "gpio_mxc", &mxc_plat[1] },
>> @@ -320,3 +395,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>   #endif
>>   };
>>   #endif
>> +#endif
> Overall I wonder why you don't just convert the boards to device tree?
> It might be more work, but it would be a lot cleaner.
Yeah. Agree. Converting the boards to device tree may require lots of 
change, I am just hacking this in my side for personal interests.

Anyway, I'll try to address you all comments to make it better.
>
> Regards,
> Simon
Regards,
Peng.