Hi Andy,

On Mon, 5 Aug 2024 22:20:56 +0200
Andy Shevchenko [off-list ref] wrote:

On Mon, Aug 5, 2024 at 12:19 PM Herve Codina [off-list ref] wrote:

quoted

From: Clément Léger <clement.leger@bootlin.com>

Syscon releasing is not supported.
Without release function, unbinding a driver that uses syscon whether
explicitly or due to a module removal left the used syscon in a in-use
state.

For instance a syscon_node_to_regmap() call from a consumer retrieve a  

retrieves?

Indeed, will be fixed.

quoted

syscon regmap instance. Internally, syscon_node_to_regmap() can create
syscon instance and add it to the existing syscon list. No API is
available to release this syscon instance, remove it from the list and
free it when it is not used anymore.

Introduce reference counting in syscon in order to keep track of syscon
usage using syscon_{get,put}() and add a device managed version of
syscon_regmap_lookup_by_phandle(), to automatically release the syscon
instance on the consumer removal.  

...

quoted

-       if (!syscon)
+       if (!syscon) {
                syscon = of_syscon_register(np, check_res);
+               if (IS_ERR(syscon))
+                       return ERR_CAST(syscon);
+       } else {
+               syscon_get(syscon);
+       }  

  if (syscon)
    return syscon_get();

?

quoted

+       return syscon;  

Yes and further more, I will remove also the unneeded IS_ERR() and ERR_CAST().
This will lead to just:

	if (syscon)
		return syscon_get(syscon);

	return of_syscon_register(np, check_res);

...

quoted

+static struct regmap *__devm_syscon_get(struct device *dev,
+                                       struct syscon *syscon)
+{
+       struct syscon **ptr;
+
+       if (IS_ERR(syscon))
+               return ERR_CAST(syscon);
+
+       ptr = devres_alloc(devm_syscon_release, sizeof(struct syscon *), GFP_KERNEL);
+       if (!ptr) {
+               syscon_put(syscon);
+               return ERR_PTR(-ENOMEM);
+       }
+
+       *ptr = syscon;
+       devres_add(dev, ptr);
+
+       return syscon->regmap;  

Can't the devm_add_action_or_reset() be used in this case? If so,
perhaps a comment to explain why?

There is no reason to avoid the use of devm_add_action_or_reset() here.
So, I will use it in the next iteration.

Thanks for your review.

Best regards,
Hervé