Re: [U-Boot] [PATCH] syscon: update syscon_node_to_regmap to use the DM functions

Hi Simon,

From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: mercredi 31 octobre 2018 01:11

HI Patrick,

On 30 October 2018 at 07:44, Patrick Delaunay patrick.delaunay@st.com wrote:

...

• Update the function syscon_node_to_regmap() to force bound on syscon uclass and directly use the list of device from DM.
• Remove the static list syscon_list.

This patch avoid issue (crash) when syscon_node_to_regmap() is called before and after reallocation (list content is invalid).

Signed-off-by: Patrick Delaunay patrick.delaunay@st.com

drivers/core/syscon-uclass.c | 55 ++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 38 deletions(-)

diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 303e166..aeb3583 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -123,52 +123,31 @@ U_BOOT_DRIVER(generic_syscon) = {

• The syscon node can be bound to another driver, but still works
• as a syscon provider.

*/ -static LIST_HEAD(syscon_list);

-struct syscon {

•   ofnode node;
  

•   struct regmap *regmap;
  

•   struct list_head list;
  

-};

-static struct syscon *of_syscon_register(ofnode node) +struct regmap *syscon_node_to_regmap(ofnode node) {

•   struct syscon *syscon;
  

•   struct udevice *dev, *parent;
  

•   ofnode ofnode = node;
  

What is the purpose of this variable if it is always set to 'node'?

No need, I remove it.

...

    int ret;

•   if (!uclass_get_device_by_ofnode(UCLASS_SYSCON, node, &dev))
  

•           return syscon_get_regmap(dev);
  

•   if (!ofnode_device_is_compatible(node, "syscon"))
            return ERR_PTR(-EINVAL);
  
  

•   syscon = malloc(sizeof(*syscon));
  

•   if (!syscon)
  

•           return ERR_PTR(-ENOMEM);
  

•   if (device_find_global_by_ofnode(ofnode, &parent))
  

•           parent = dm_root();
  

So if you fail to find the node, you use the root? Is this so that you have somewhere to 'hang' the device? I think this code could all use a few comments.

Yes exactly. it is to avoid binding error when the node with "syscon" compatible is not associated to a other U-Boot driver. But It is more a protection, normally if the driver is not bound to other U-boot driver, it should be bound to "syscon" at least.

I will add comment.

...

•   ret = regmap_init_mem(node, &syscon->regmap);
  

•   if (ret) {
  

•           free(syscon);
  

•   /* force bound to syscon class */
  

•   ret = device_bind_driver_to_node(parent, "syscon",
  

•                                    ofnode_get_name(node),
  

•                                    node, &dev);
  

•   if (ret)
            return ERR_PTR(ret);
  

•   }
  

•   list_add_tail(&syscon->list, &syscon_list);
  

•   return syscon;
  

-}

-struct regmap *syscon_node_to_regmap(ofnode node) -{

•   struct syscon *entry, *syscon = NULL;
  

•   list_for_each_entry(entry, &syscon_list, list)
  

•           if (ofnode_equal(entry->node, node)) {
  

•                   syscon = entry;
  

•                   break;
  

•           }
  

•   if (!syscon)
  

•           syscon = of_syscon_register(node);
  

•   if (IS_ERR(syscon))
  

•           return ERR_CAST(syscon);
  

•   ret = device_probe(dev);
  

•   if (ret)
  

•           return ERR_PTR(ret);
  
  

•   return syscon->regmap;
  

•   return syscon_get_regmap(dev);
  

}

2.7.4

Regards, Simon

Regards

Patrick