dm:gpio:mxc add DT support
Subsystems (2)
v3 cover dm:gpio:mxc add DT support
This patch set is to add DT support for mxc_gpio driver.
patch 1/4 and 2/4, a new dev_get_addr interface is abstracted to
improve driver who want to get device address.
patch 3/4, add a new bank_index entry in platdata to avoid plat - mxc_plat
pointer subtract usage.
patch 4/4, add compatible ids and implement bind function. Also commented
out U_BOOT_DEVICES and mxc_plat, since they are not needed if using DT.
Changes v3:
1. split bank_index patch
2. abstract dev_get_addr for driver
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.
Peng Fan (4):
dm: introduce dev_get_addr interface
dm: add dev_get_addr prototype
dm:gpio:mxc add a bank_index entry in platdata
dm:gpio:mxc add DT support
drivers/core/device.c | 19 +++++++++++++ drivers/gpio/mxc_gpio.c | 72 ++++++++++++++++++++++++++++++++++++------------- include/dm/device.h | 9 +++++++ 3 files changed, 82 insertions(+), 18 deletions(-)
Signature · 1 line
1.8.4
PATCH v3 1/4 dm: introduce dev_get_addr interface
Abstracting dev_get_addr can improve drivers that want to get device's address.
--- drivers/core/device.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
drivers/core/device.c +19 -0
@@ -12,6 +12,7 @@#include <common.h> #include <fdtdec.h> #include <malloc.h> +#include <libfdt.h> #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h>
@@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev) {return dev->of_id->data; } + +#ifdef CONFIG_OF_CONTROL +void *dev_get_addr (struct udevice *dev) +{ +fdt_addr_t addr; + + addr =fdtdec_get_addr (gd->fdt_blob, dev->of_offset,"reg" ); +if (addr ==FDT_ADDR_T_NONE ) +return NULL ; +else +return (void *)addr; +} +#else +void *dev_get_addr (struct udevice *dev) +{ +return NULL ; +} +#endif
Signature · 1 line
1.8.4
PATCH v3 1/4 dm: introduce dev_get_addr interface
Quoted 4 lines · Peng Fan
Abstracting
dev_get_addrcan improve drivers that want to get device's address.Signed-off-by Peng Fan <[email protected]>
Signature · 2 lines
Regards, Igor.
PATCH v3 1/4 dm: introduce dev_get_addr interface
In reply to Peng FanHi,
Quoted 21 lines · Peng Fan
Abstracting
dev_get_addrcan improve drivers that want to get device's address.Signed-off-by Peng Fan <[email protected]>--- drivers/core/device.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)drivers/core/device.c +1
-0@@ -12,6 +12,7 @@#include <common.h> #include <fdtdec.h> #include <malloc.h> +#include <libfdt.h> #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h>
drivers/core/device.c ulong dev_get_of_data(struct udevice *dev) · line 396
Quoted 6 lines
drivers/core/device.c +3
-0@@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev) {return dev->of_id->data; } + +#ifdef CONFIG_OF_CONTROL +void *dev_get_addr (struct udevice *dev)
My approach so far has been to use a ulong for the device address (e.g. in platform data) and only use a pointer when we know the type (e.g. struct disp_ctlr *), typically in driver-private data.
So do you think it would be better to return FDT_ADDR_T_NONE?
Quoted 16 lines
drivers/core/device.c +15
-1+{ +fdt_addr_t addr; + + addr =fdtdec_get_addr (gd->fdt_blob, dev->of_offset,"reg" ); +if (addr ==FDT_ADDR_T_NONE ) +return NULL ; +else +return (void *)addr; +} +#else +void *dev_get_addr (struct udevice *dev) +{ +return NULL ; +} +#endif --
Quoted 3 lines
1.8.4
Regards,
Simon
PATCH v3 1/4 dm: introduce dev_get_addr interface
Hi, Simon
Quoted 34 lines · Simon Glass
Hi,
Quoted 21 lines · Peng Fan
Abstracting
dev_get_addrcan improve drivers that want to get device's address.Signed-off-by Peng Fan <[email protected]>--- drivers/core/device.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)drivers/core/device.c +1
-0@@ -12,6 +12,7 @@#include <common.h> #include <fdtdec.h> #include <malloc.h> +#include <libfdt.h> #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h> drivers/core/device.c ulong dev_get_of_data(struct udevice *dev) · line 396
Quoted 6 lines
drivers/core/device.c +3
-0@@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev) {return dev->of_id->data; } + +#ifdef CONFIG_OF_CONTROL +void *dev_get_addr (struct udevice *dev)My approach so far has been to use a ulong for the device address (e.g. in platform data) and only use a pointer when we know the type (e.g. struct
disp_ctlr*), typically in driver-private data.
Quoted 1 line
So do you think it would be better to return
FDT_ADDR_T_NONE?
Sorry for the long time delay to reply.
Do you agree this way using ulong as the return type? "
#ifdef CONFIG_OF_CONTROL
unsigned long dev_get_addr(struct udevice *dev)
{
fdt_addr_t addr;
addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
return addr;
}
#else
unsigned long dev_get_addr(struct udevice *dev)
{
return FDT_ADDR_T_NONE;
}#endif
"
Is it better to move this piece of code to include/dm/device.h and using static inline prototype? or put them still in driver/core/device.c?
Quoted 13 lines
Quoted 13 lines
drivers/core/device.c +13
-0+{ +fdt_addr_t addr; + + addr =fdtdec_get_addr (gd->fdt_blob, dev->of_offset,"reg" ); +if (addr ==FDT_ADDR_T_NONE ) +return NULL ; +else +return (void *)addr; +} +#else +void *dev_get_addr (struct udevice *dev) +{ +return NULL ;
Quoted 8 lines
Quoted 6 lines
drivers/core/device.c +2
-1+} +#endif --1.8.4
Regards,
Simon
Thanks,
Peng.
PATCH v3 1/4 dm: introduce dev_get_addr interface
Hi Peng,
Quoted 58 lines · Peng Fan
Hi, Simon
Quoted 37 lines · Simon Glass
Hi,
Quoted 22 lines · Peng Fan
Abstracting
dev_get_addrcan improve drivers that want to get device's address.Signed-off-by Peng Fan <[email protected]>--- drivers/core/device.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)drivers/core/device.c +1
-0@@ -12,6 +12,7 @@#include <common.h> #include <fdtdec.h> #include <malloc.h> +#include <libfdt.h> #include <dm/device.h> #include <dm/device-internal.h> #include <dm/lists.h> drivers/core/device.c ulong dev_get_of_data(struct udevice *dev) · line 396
Quoted 6 lines
drivers/core/device.c +3
-0@@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev) {return dev->of_id->data; } + +#ifdef CONFIG_OF_CONTROL +void *dev_get_addr (struct udevice *dev)My approach so far has been to use a ulong for the device address (e.g. in platform data) and only use a pointer when we know the type (e.g. struct
disp_ctlr*), typically in driver-private data.Quoted 1 line
So do you think it would be better to return
FDT_ADDR_T_NONE?Sorry for the long time delay to reply.
Do you agree this way using ulong as the return type? "
#ifdefCONFIG_OF_CONTROL
unsigned longdev_get_addr(struct udevice *dev){ fdt_addr_t addr; addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg"); return addr; } #else unsigned long dev_get_addr(struct udevice *dev) { return FDT_ADDR_T_NONE; }
Quoted 2 lines
#endif
"
Sure, but can you return fdt_addr_t?
Quoted 2 lines
Is it better to move this piece of code to
include/dm/device.hand using static inline prototype? or put them still indriver/core/device.c?
I'd suggest:
- static inline in the header for the case where CONFIG_OF_CONTROL is not defined
- full implementation in device.c when that is defined
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/core/device.c +9
-0+{ +fdt_addr_t addr; + + addr =fdtdec_get_addr (gd->fdt_blob, dev->of_offset,"reg" ); +if (addr ==FDT_ADDR_T_NONE ) +return NULL ; +else +return (void *)addr; +}
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/core/device.c +6
-1+#else +void *dev_get_addr (struct udevice *dev) +{ +return NULL ; +} +#endif --1.8.4
Regards,
Simon
PATCH v3 2/4 dm: add dev_get_addr prototype
In reply to Peng Fan--- include/dm/device.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/dm/device.h +9 -0
@@ -322,4 +322,13 @@ int device_find_first_child(struct udevice *parent, struct udevice **devp);*/ int device_find_next_child (struct udevice **devp); + +/** +* dev_get_addr() - Get the reg property of a device +* +* @dev: Pointer to a device. Returns reg address, or NULL if no DT +* +* @return addr, or NULL if NO DT +*/ +void *dev_get_addr (struct udevice *dev);#endif
Signature · 1 line
1.8.4
PATCH v3 2/4 dm: add dev_get_addr prototype
Quoted 1 line · Peng Fan
Signed-off-by Peng Fan <[email protected]>
I think this should be a part of the first patch, anyway:
Quoted 23 lines
--- include/dm/device.h | 9 +++++++++ 1 file changed, 9 insertions(+)include/dm/device.h +9
-0@@ -322,4 +322,13 @@ int device_find_first_child(struct udevice *parent, struct udevice **devp);*/ int device_find_next_child (struct udevice **devp); + +/** +* dev_get_addr() - Get the reg property of a device +* +* @dev: Pointer to a device. Returns reg address, or NULL if no DT +* +* @return addr, or NULL if NO DT +*/ +void *dev_get_addr (struct udevice *dev);#endif
Signature · 2 lines
Regards, Igor.
PATCH v3 3/4 dm:gpio:mxc add a bank_index entry in platdata
In reply to Peng FanAdd a new entry in platdata structure and intialize bank_index in mxc_plat array.
This new entry can avoid using plat - mxc_plat by using plat->bank_index.
--- drivers/gpio/mxc_gpio.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/gpio/mxc_gpio.c +9 -8
@@ -23,6 +23,7 @@ enum mxc_gpio_direction {#define GPIO_PER_BANK 32struct mxc_gpio_plat { +int bank_index;struct gpio_regs *regs; };
@@ -259,19 +260,19 @@ static const struct dm_gpio_ops gpio_mxc_ops = { };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 }, + {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) - { (struct gpio_regs *)GPIO4_BASE_ADDR }, + {3 , (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 }, + {4 , (struct gpio_regs *)GPIO5_BASE_ADDR }, + {5 , (struct gpio_regs *)GPIO6_BASE_ADDR }, #endif #if defined(CONFIG_MX53) || defined(CONFIG_MX6) - { (struct gpio_regs *)GPIO7_BASE_ADDR }, + {6 , (struct gpio_regs *)GPIO7_BASE_ADDR }, #endif };
@@ -283,7 +284,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)
Signature · 1 line
1.8.4
PATCH v3 3/4 dm:gpio:mxc add a bank_index entry in platdata
Quoted 6 lines · Peng Fan
Add a new entry in platdata structure and intialize
bank_indexinmxc_platarray.
This new entry can avoid usingplat - mxc_platby usingplat->bank_index.Signed-off-by Peng Fan <[email protected]>
Signature · 2 lines
Regards, Igor.
PATCH v3 3/4 dm:gpio:mxc add a bank_index entry in platdata
In reply to Peng FanHi,
Quoted 36 lines · Peng Fan
Add a new entry in platdata structure and intialize
bank_indexinmxc_platarray.
This new entry can avoid usingplat - mxc_platby usingplat->bank_index.Signed-off-by Peng Fan <[email protected]>--- drivers/gpio/mxc_gpio.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)drivers/gpio/mxc_gpio.c +5
-4@@ -23,6 +23,7 @@ enum mxc_gpio_direction {#define GPIO_PER_BANK 32struct mxc_gpio_plat { +int bank_index;struct gpio_regs *regs; };@@ -259,19 +260,19 @@ static const struct dm_gpio_ops gpio_mxc_ops = { };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 }, + {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) - { (struct gpio_regs *)GPIO4_BASE_ADDR }, + { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
drivers/gpio/mxc_gpio.c static const struct dm_gpio_ops gpio_mxc_ops = { · line 275
Quoted 10 lines
drivers/gpio/mxc_gpio.c +3
-3#endif #if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6) - { (struct gpio_regs *)GPIO5_BASE_ADDR }, - { (struct gpio_regs *)GPIO6_BASE_ADDR }, + { 4, (struct gpio_regs *)GPIO5_BASE_ADDR }, + { 5, (struct gpio_regs *)GPIO6_BASE_ADDR }, #endif #if defined(CONFIG_MX53) || defined(CONFIG_MX6) - { (struct gpio_regs *)GPIO7_BASE_ADDR }, + { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
Is it possible to drop these casts? Or should that be another patch?
Quoted 13 lines
drivers/gpio/mxc_gpio.c +1
-2#endif };@@ -283,7 +284,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) --
Quoted 3 lines
1.8.4
Regards,
Simon
PATCH v3 4/4 dm:gpio:mxc add DT support
In reply to Peng FanThis 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.
--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)
drivers/gpio/mxc_gpio.c +52 -17
@@ -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; + 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
Signature · 1 line
1.8.4
PATCH v3 4/4 dm:gpio:mxc add DT support
Hi Igor,
Just kindly remind, did you miss this one? Since you ack the other patches in this patch set.
Quoted 125 lines · Peng Fan
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxxand etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_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 <[email protected]>--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +51
-17@@ -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; + 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) = {
drivers/gpio/mxc_gpio.c U_BOOT_DEVICES(mxc_gpios) = { · line 358
Quoted 4 lines
drivers/gpio/mxc_gpio.c +1
-0@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {#endif };#endif +#endif
Thanks,
Peng.
PATCH v3 4/4 dm:gpio:mxc add DT support
Hi Peng,
Quoted 3 lines · Peng Fan
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...
Quoted 22 lines · Peng Fan
Quoted 20 lines
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxxand etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_OF_CONTROL
+ Add device tree file and do related configuration in
make ARCH=arm menuconfig
These will be done in future patches by step.
Quoted 1 line
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>
Besides the question below, looks good.
Quoted 53 lines
Quoted 53 lines
--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +12
-16@@ -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)
Quoted 8 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 302
Quoted 8 lines
drivers/gpio/mxc_gpio.c +8
-0+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?
Quoted 46 lines
Quoted 39 lines
drivers/gpio/mxc_gpio.c +30
-0+ 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) = {drivers/gpio/mxc_gpio.c U_BOOT_DEVICES(mxc_gpios) = { · line 358
Quoted 4 lines
drivers/gpio/mxc_gpio.c +1
-0@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {#endif };#endif +#endif Thanks,
Peng.
Signature · 2 lines
Regards, Igor.
PATCH v3 4/4 dm:gpio:mxc add DT support
Hi, Igor
Reply below.
Quoted 87 lines · Igor Grinberg
Hi Peng,
Quoted 3 lines · Peng Fan
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...Quoted 21 lines · Peng Fan
Quoted 20 lines
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxxand etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_OF_CONTROL
+ Add device tree file and do related configuration in
make ARCH=arm menuconfig
These will be done in future patches by step.Quoted 1 line
Quoted 1 line
Signed-off-by Peng Fan <[email protected]>Besides the question below, looks good.
Quoted 54 lines
Quoted 54 lines
--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +13
-16@@ -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 ;
Quoted 8 lines
Quoted 7 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 302
Quoted 7 lines
drivers/gpio/mxc_gpio.c +7
-0+ + plat =calloc (1 ,sizeof (*plat)); +if (!plat) +return -ENOMEM ; + + plat->regs = regs; + plat->bank_index = device->req_seq;Can we assume that in
mxc_gpiocase,device->req_seqwill 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 makereq_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.
Quoted 39 lines
Quoted 39 lines
Quoted 39 lines
drivers/gpio/mxc_gpio.c +30
-0+ 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) = {
Quoted 8 lines
Quoted 8 lines
drivers/gpio/mxc_gpio.c U_BOOT_DEVICES(mxc_gpios) = { · line 358
Quoted 5 lines
drivers/gpio/mxc_gpio.c +1
-0@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {#endif };#endif +#endif Thanks,
Peng.
Thanks,
Peng.
PATCH v3 4/4 dm:gpio:mxc add DT support
In reply to Peng FanHi Peng,
Quoted 64 lines · Peng Fan
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxxand etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_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 <[email protected]>--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +3
-17@@ -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);
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 285
Quoted 4 lines
drivers/gpio/mxc_gpio.c +1
-0@@ -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.
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 291
Quoted 6 lines
drivers/gpio/mxc_gpio.c +6
-0+{ +struct mxc_gpio_plat *plat = device->platdata; +struct gpio_regs *regs; + +if (plat) +return 0 ;
Please add a comment as to why.
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 295
Quoted 4 lines
drivers/gpio/mxc_gpio.c +4
-0+ + regs =dev_get_addr (device); +if (!regs) +return -ENXIO ;
-ENODEV I think?
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 299
Quoted 4 lines
drivers/gpio/mxc_gpio.c +4
-0+ + 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.
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 302
Quoted 3 lines
drivers/gpio/mxc_gpio.c +3
-0+ + plat->regs = regs; + plat->bank_index = device->req_seq;
Why store this? You can access it anytime using the device pointer.
Quoted 7 lines
drivers/gpio/mxc_gpio.c +7
-0+ device->platdata = plat; + +return 0 ; +} + +static const struct udevice_id mxc_gpio_ids[] = { + { .compatible ="fsl,imx35-gpio" },
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 319
Quoted 10 lines
drivers/gpio/mxc_gpio.c +4
-0+ { } +}; +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.
Quoted 9 lines
drivers/gpio/mxc_gpio.c +9
-0+ .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) || \
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 338
Quoted 10 lines
drivers/gpio/mxc_gpio.c +10
-0+ 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?
Quoted 9 lines
drivers/gpio/mxc_gpio.c +1
-1};U_BOOT_DEVICES (mxc_gpios) = {@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {#endif };#endif +#endif --
Quoted 3 lines
1.8.4
Regards,
Simon
PATCH v3 4/4 dm:gpio:mxc add DT support
Hi Simon,
Quoted 70 lines · Simon Glass
Hi Peng,
Quoted 67 lines · Peng Fan
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxxand etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_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 <[email protected]>--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +3
-17@@ -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 ; }
Quoted 2 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 285
Quoted 1 line
drivers/gpio/mxc_gpio.c +1
-0+static int mxc_gpio_bind (struct udevice *device)s/device/dev/ as that is the convention.
Will fix this.
Quoted 8 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 291
Quoted 7 lines
drivers/gpio/mxc_gpio.c +6
-0+{ +struct mxc_gpio_plat *plat = device->platdata; +struct gpio_regs *regs; + +if (plat) +return 0 ;Please add a comment as to why.
Ok.
Quoted 6 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 295
Quoted 5 lines
drivers/gpio/mxc_gpio.c +4
-0+ + regs =dev_get_addr (device); +if (!regs) +return -ENXIO ;
-ENODEVI think?
Yeah. Right.
Quoted 6 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 299
Quoted 4 lines
drivers/gpio/mxc_gpio.c +4
-0+ + 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?
Quoted 5 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 302
Quoted 4 lines
drivers/gpio/mxc_gpio.c +3
-0+ + 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.
Quoted 20 lines
Quoted 8 lines
drivers/gpio/mxc_gpio.c +7
-0+ device->platdata = plat; + +return 0 ; +} + +static const struct udevice_id mxc_gpio_ids[] = { + { .compatible ="fsl,imx35-gpio" },drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 319
Quoted 10 lines
drivers/gpio/mxc_gpio.c +4
-0+ { } +}; +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.:
Quoted 4 lines
.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.
Quoted 13 lines
Quoted 13 lines
drivers/gpio/mxc_gpio.c +12
-0+ .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
Quoted 8 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 338
Quoted 7 lines
drivers/gpio/mxc_gpio.c +7
-0+#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 }, +#endifCan 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.
Quoted 15 lines
Quoted 10 lines
drivers/gpio/mxc_gpio.c +1
-1};U_BOOT_DEVICES (mxc_gpios) = {@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {#endif };#endif +#endif --Quoted 3 lines
1.8.4
Regards,
Simon
Regards,
Peng.
PATCH v3 4/4 dm:gpio:mxc add DT support
Hi Peng,
Quoted 113 lines · Peng Fan
Hi Simon,
Quoted 76 lines · Simon Glass
Hi Peng,
Quoted 72 lines · Peng Fan
This patch add DT support for mxc gpio driver.
There are one place using
CONFIG_OF_CONTROLmacro.
1. TheU_BOOT_DEVICESandmxc_platarray are complied out. To DT, platdata is alloced using calloc, so there is no need to usemxc_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 notgpio_requestwhen usinggpio_direction_xxx
and
etc, need to request gpio.
+ move the gpio settings fromboard_early_init_ftoboard_init
+ defineCONFIG_DM,CONFIG_DM_GPIO andCONFIG_OF_CONTROL
+ Add device tree file and do related configuration inmake ARCH=arm menuconfig
These will be done in future patches by step.Signed-off-by Peng Fan <[email protected]>--- drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 17 deletions(-)drivers/gpio/mxc_gpio.c +3
-17@@ -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);drivers/gpio/mxc_gpio.c +0
-0@@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)return 0 ; }Quoted 3 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 285
Quoted 1 line
drivers/gpio/mxc_gpio.c +1
-0+static int mxc_gpio_bind (struct udevice *device)s/device/dev/ as that is the convention.
Will fix this.
Quoted 10 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 291
Quoted 8 lines
drivers/gpio/mxc_gpio.c +6
-0+{ +struct mxc_gpio_plat *plat = device->platdata; +struct gpio_regs *regs; + +if (plat) +return 0 ;Please add a comment as to why.
Ok.
Quoted 8 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 295
Quoted 6 lines
drivers/gpio/mxc_gpio.c +4
-0+ + regs =dev_get_addr (device); +if (!regs) +return -ENXIO ;
-ENODEVI think?Yeah. Right.
Quoted 6 lines
Quoted 5 lines
drivers/gpio/mxc_gpio.c +4
-0+ + plat =calloc (1 ,sizeof (*plat)); +if (!plat) +return -ENOMEM ;
Quoted 7 lines
Quoted 2 lines
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 inU_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.
Quoted 8 lines
Quoted 7 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 302
Quoted 5 lines
drivers/gpio/mxc_gpio.c +3
-0+ + plat->regs = regs; + plat->bank_index = device->req_seq;Why store this? You can access it anytime using the device pointer.
Quoted 7 lines
To no DT,
bank_indexis statically intialized inmxc_platarray. I do not want to introudce#ifdef CONFIG_OF_CONTROLin probe function and introudceif (dev->of_offset == -1), so
store it tobank_index.
To no DT,if(plat) return 0;will return. Soplat->bank_index=device->req_seqwill only effects for DT.
Just want to support DT and no DT.
OK I think I understand.
Quoted 22 lines
Quoted 22 lines
Quoted 9 lines
drivers/gpio/mxc_gpio.c +7
-0+ device->platdata = plat; + +return 0 ; +} + +static const struct udevice_id mxc_gpio_ids[] = { + { .compatible ="fsl,imx35-gpio" },drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 319
Quoted 10 lines
drivers/gpio/mxc_gpio.c +4
-0+ { } +}; +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.:
Quoted 8 lines
Quoted 4 lines
.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_ptrwill make compiler complainmxc_gpio_idsdefined but not usedfor no DT, since#ifdef xxis not recommended to compiled outmxc_gpio_idsfor no DT.
Yes it will. What you have is OK.
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
drivers/gpio/mxc_gpio.c +13
-0+ .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)Quoted 8 lines
drivers/gpio/mxc_gpio.c static int mxc_gpio_probe(struct udevice *dev) · line 338
Quoted 6 lines
drivers/gpio/mxc_gpio.c +6
-0+ { 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 }, +#endifCan we remove the casts by changing the type to ulong?
Quoted 7 lines
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_platstruct.
If change the type to ulong, functions in this driver that uses platdata will casts it tostruct gpio_regs *. So i'd like not to remove the casts, since remove them inmxc_platwill 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