[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support
Simon Glass
sjg at chromium.org
Mon Jan 26 14:38:12 CET 2015
Hi Peng,
On 24 January 2015 at 07:34, Peng Fan <B51431 at freescale.com> wrote:
> Hi Simon,
>
>
> On 1/23/2015 5:26 AM, Simon Glass wrote:
>>
>> Hi Peng,
>>
>> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan at freescale.com> 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 at freescale.com>
>>> ---
>>> 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)
>>
>> s/device/dev/ as that is the convention.
>
> Will fix this.
>>
>>
>>> +{
>>> + struct mxc_gpio_plat *plat = device->platdata;
>>> + struct gpio_regs *regs;
>>> +
>>> + if (plat)
>>> + return 0;
>>
>> Please add a comment as to why.
>
> Ok.
>>
>>
>>> +
>>> + regs = dev_get_addr(device);
>>> + if (!regs)
>>> + return -ENXIO;
>>
>> -ENODEV I think?
>
> Yeah. Right.
>>>
>>> +
>>> + plat = calloc(1, sizeof(*plat));
>>> + if (!plat)
>>> + return -ENOMEM;
>>
>> Can you use the auto-alloc feature instead? Trying to keep memory
>> allocations out of drivers unless it is for buffers, etc.
>
> I want the DM code can support DT and no DT. To no DT, platdata is defined
> in U_BOOT_DEVICES.
> If using auto-alloc feature, but DT is not supported, is it conflict with
> platdata in U_BOOT_DEVICES?
Yes it will. But please add a TODO in the code to remove this when
every board is converted to driver model and you don't need this.
>>
>>
>>> +
>>> + plat->regs = regs;
>>> + plat->bank_index = device->req_seq;
>>
>> Why store this? You can access it anytime using the device pointer.
>
> To no DT, bank_index is statically intialized in mxc_plat array. I do not
> want to introudce `#ifdef CONFIG_OF_CONTROL` in probe function and introudce
> `if (dev->of_offset == -1)`, so
> store it to bank_index.
> To no DT, `if(plat) return 0;` will return. So plat->bank_index =
> device->req_seq will only effects for DT.
> Just want to support DT and no DT.
OK I think I understand.
>>
>>
>>> + 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,
>>
>> Masahiro added a function for this.:
>>
>> .of_match = of_match_ptr(mxc_gpio_ids),
>>
>> But I'm not completely sure if this is wanted, since you include this
>> information even when not using device tree.
>
> Thanks,I'll try this. I am not sure whether using of_match_ptr will make
> compiler complain mxc_gpio_ids `defined but not used` for no DT, since
> `#ifdef xx` is not recommended to compiled out `mxc_gpio_ids` for no DT.
Yes it will. What you have is OK.
>>
>>
>>> + .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
>>
>> Can we remove the casts by changing the type to ulong?
>
> Hmm, this patch is just to make this driver can support DT. This will
> introduce more change. Also, changing the type to ulong will change
> mxc_gpio_plat struct.
> If change the type to ulong, functions in this driver that uses platdata
> will casts it to `struct gpio_regs *`. So i'd like not to remove the casts,
> since remove them in mxc_plat will introduce casts in other functions which
> use the platdata.
OK. Something to think about later. In general a long line of casts
indicates something should be fixed!
Regards,
Simon
More information about the U-Boot
mailing list