[U-Boot] [PATCH v2] dm:gpio:mxc add DT support

[Peng Fan]

Hi, Igor

On 1/20/2015 10:39 PM, Igor Grinberg wrote:
> On 01/20/15 09:15, Peng Fan wrote:
>> This patch add DT support for mxc gpio driver.
>>
>> Include a bank_index entry in platdata. This can avoid using
>> `plat - mxc_plat` to calculate bank number. Also this can simplify code.
> Although, I don't insist, but I would recommend to split the patch into 2:
> the bank_index rework and the DT support addition.
Ok. I can split this in v3 patch.
>
>> There are two places still using CONFIG_OF_CONTROL macro, just to
>> shrink code size if only support DM but not support DT.
>> 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.
>> 2. add a function mxc_get_gpio_addr to get "reg" property if DT support.
>>     If no DT, this function just returns NULL.
> I have some comments on this one, please see below...
Reply below.
>
>> The following situations are tested:
>> 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 at freescale.com>
>> ---
>>
>> Changes v2:
>>   1. remove uneccessary #ifdef
>>   2. add more stuff in commit log
>>   3. include a new function mxc_get_gpio_addr to get register base.
>>      This function is different for DT and not DT, by `#ifdef`.
>>      If using one implementation for DT and not DT, final image will be big.
>>   4. include a new entry in platdata, named bank_index. it can simplify DT
>>      support. To no DT, bank_index is static initilized; to DT, bank_index
>>      is get from device's req_seq.
>>
>>   drivers/gpio/mxc_gpio.c | 89 +++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 71 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index 8bb9e39..5826620 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -23,6 +23,7 @@ enum mxc_gpio_direction {
>>   #define GPIO_PER_BANK			32
>>   
>>   struct mxc_gpio_plat {
>> +	int bank_index;
>>   	struct gpio_regs *regs;
>>   };
>>   
>> @@ -150,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;
>> @@ -258,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[] = {
>> -	{ (struct gpio_regs *)GPIO1_BASE_ADDR },
>> -	{ (struct gpio_regs *)GPIO2_BASE_ADDR },
>> -	{ (struct gpio_regs *)GPIO3_BASE_ADDR },
>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>> -		defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -	{ (struct gpio_regs *)GPIO4_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -	{ (struct gpio_regs *)GPIO5_BASE_ADDR },
>> -	{ (struct gpio_regs *)GPIO6_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
>> -#endif
>> -};
>> -
>>   static int mxc_gpio_probe(struct udevice *dev)
>>   {
>>   	struct mxc_bank_info *bank = dev_get_priv(dev);
>> @@ -283,7 +270,7 @@ static int mxc_gpio_probe(struct udevice *dev)
>>   	int banknum;
>>   	char name[18], *str;
>>   
>> -	banknum = plat - mxc_plat;
>> +	banknum = plat->bank_index;
>>   	sprintf(name, "GPIO%d_", banknum + 1);
>>   	str = strdup(name);
>>   	if (!str)
>> @@ -295,12 +282,77 @@ static int mxc_gpio_probe(struct udevice *dev)
>>   	return 0;
>>   }
>>   
>> +#ifdef CONFIG_OF_CONTROL
>> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
>> +{
>> +	fdt_addr_t addr;
>> +	addr = fdtdec_get_addr(gd->fdt_blob, device->of_offset, "reg");
>> +	if (addr == FDT_ADDR_T_NONE)
>> +		return NULL;
>> +	else
>> +		return (struct gpio_regs *)addr;
>> +}
>> +#else
>> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
>> +{
>> +	return NULL;
>> +}
>> +#endif
> In general, I'm fine with this concept, but I believe we should implement
> a stub for fdtdec_get_addr() function in the fdtdec.h (say just returning
> FDT_ADDR_T_NONE), as otherwise we might end up with multiple drivers
> implementing the same noop callback just to work around a poor fdtdec_*()
> interface.
I tried to implement a stub function in fdtdec.h like this:
__weak fdt_addr_t fdtdec_get_addr_wrap(xxxx)
{
     return FDT_ADDR_T_NONE;
}
And in driver code, implement non weak version as following:
#ifdef CONFIG_OF_CONTROL
fdt_addr_t fdtdec_get_addr_wrap(xxxx)
{
     ..........
}
#endif
But gcc complains about conficting types, since we have a weak 
implementation in header file and a strong implementation in c file. If 
the weak one is in fdtxx c file, no error, but i thinke this is not a 
good idea to put this in fdtxx c file. If we do not want DT, but only 
DM, DT code should not be compiled into the final image.

I tried another way, add the following piece code in 
driver/core/device.c and function prototype in device.h,
"
#ifdef CONFIG_OF_CONTROL
void *dev_reg_addr(struct udevice *dev)
{
     fdt_addr_t addr;

     addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg");
     if (addr == FDT_ADDR_T_NONE)
         return NULL;
     else
         return (void *)addr
}
#else
void *dev_reg_addr(struct udevice *dev)
{
     return NULL;
}
#endif
"
I think `#ifdef` is needed here. I think this way is better that put 
stub function in fdtdec.h. Using this way, the driver code can just `add 
= dev_reg_addr(device)` to get reg address.
Maybe the upper piece code should be put in a new file named 
device-util.c in directory device/core but not device.c?

Comments?

>> +
>> +static int mxc_gpio_bind(struct udevice *device)
>> +{
>> +	struct mxc_gpio_plat *plat = device->platdata;
>> +	struct gpio_regs *regs;
>> +
>> +	if (plat)
>> +		return 0;
>> +
>> +	regs = mxc_get_gpio_addr(device);
>> +	if (!regs)
>> +		return -ENXIO;
>> +
>> +	plat = calloc(1, sizeof(*plat));
>> +	if (!plat)
>> +		return -ENOMEM;
>> +
>> +	plat->regs = regs;
>> +	plat->bank_index = device->req_seq;
>> +	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) = {
>> @@ -320,3 +372,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>   #endif
>>   };
>>   #endif
>> +#endif
>>
Regards,
Peng.