diff mbox series

[v1,1/1] pinctrl: Move for_each_maps() to namespace and hide iterator inside

Message ID 20221109155724.42354-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/1] pinctrl: Move for_each_maps() to namespace and hide iterator inside |

Commit Message

Andy Shevchenko Nov. 9, 2022, 3:57 p.m. UTC 
  First of all, while for_each_maps() is private to pin control subsystem
it's still better to have it put into a namespace.

Besides that, users are not relying on iterator variable, so hide it
inside for-loop.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/core.c |  6 ++----
 drivers/pinctrl/core.h | 10 +++++-----
 2 files changed, 7 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Nov. 9, 2022, 4:08 p.m. UTC | #1 
On Wed, Nov 09, 2022 at 05:57:24PM +0200, Andy Shevchenko wrote:
> First of all, while for_each_maps() is private to pin control subsystem
> it's still better to have it put into a namespace.
> 
> Besides that, users are not relying on iterator variable, so hide it
> inside for-loop.

...

> +#define for_each_pin_map(_maps_node_, _map_)						\
> +	list_for_each_entry(_maps_node_, &pinctrl_maps, node)				\
> +		for (unsigned int __i = 0;						\

> +		     _map_ = &_maps_node_->maps[__i], __i < _maps_node_->num_maps;	\

Hmm... I think this is actually not okay, if we have maps be NULL and
num_maps = 0, KABOOM is guaranteed.

I will experiment and update this.

Meanwhile, Linus, do you think this change is useful?

> +		     __i++)
Linus Walleij Nov. 10, 2022, 9:55 a.m. UTC | #2 
On Wed, Nov 9, 2022 at 5:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Nov 09, 2022 at 05:57:24PM +0200, Andy Shevchenko wrote:
> > First of all, while for_each_maps() is private to pin control subsystem
> > it's still better to have it put into a namespace.
> >
> > Besides that, users are not relying on iterator variable, so hide it
> > inside for-loop.
>
> ...
>
> > +#define for_each_pin_map(_maps_node_, _map_)                                         \
> > +     list_for_each_entry(_maps_node_, &pinctrl_maps, node)                           \
> > +             for (unsigned int __i = 0;                                              \
>
> > +                  _map_ = &_maps_node_->maps[__i], __i < _maps_node_->num_maps;      \
>
> Hmm... I think this is actually not okay, if we have maps be NULL and
> num_maps = 0, KABOOM is guaranteed.
>
> I will experiment and update this.

OK

> Meanwhile, Linus, do you think this change is useful?

Even if just a name change, it makes things better by being more
readable so yes :)

Yours,
Linus Walleij
Andy Shevchenko Nov. 11, 2022, 12:01 p.m. UTC | #3 
On Thu, Nov 10, 2022 at 10:55:47AM +0100, Linus Walleij wrote:
> On Wed, Nov 9, 2022 at 5:08 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 09, 2022 at 05:57:24PM +0200, Andy Shevchenko wrote:

...

> > Meanwhile, Linus, do you think this change is useful?
> 
> Even if just a name change, it makes things better by being more
> readable so yes :)

v2 has been sent.
diff mbox series

Patch

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9e57f4c62e60..f2f99972a0d3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1028,7 +1028,6 @@  static struct pinctrl *create_pinctrl(struct device *dev,
 	struct pinctrl *p;
 	const char *devname;
 	struct pinctrl_maps *maps_node;
-	int i;
 	const struct pinctrl_map *map;
 	int ret;
 
@@ -1054,7 +1053,7 @@  static struct pinctrl *create_pinctrl(struct device *dev,
 
 	mutex_lock(&pinctrl_maps_mutex);
 	/* Iterate over the pin control maps to locate the right ones */
-	for_each_maps(maps_node, i, map) {
+	for_each_pin_map(maps_node, map) {
 		/* Map must be for this device */
 		if (strcmp(map->dev_name, devname))
 			continue;
@@ -1805,13 +1804,12 @@  static inline const char *map_type(enum pinctrl_map_type type)
 static int pinctrl_maps_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_maps *maps_node;
-	int i;
 	const struct pinctrl_map *map;
 
 	seq_puts(s, "Pinctrl maps:\n");
 
 	mutex_lock(&pinctrl_maps_mutex);
-	for_each_maps(maps_node, i, map) {
+	for_each_pin_map(maps_node, map) {
 		seq_printf(s, "device %s\nstate %s\ntype %s (%d)\n",
 			   map->dev_name, map->name, map_type(map->type),
 			   map->type);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 840103c40c14..8b90b4f0bc7e 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -242,8 +242,8 @@  extern int pinctrl_force_default(struct pinctrl_dev *pctldev);
 extern struct mutex pinctrl_maps_mutex;
 extern struct list_head pinctrl_maps;
 
-#define for_each_maps(_maps_node_, _i_, _map_) \
-	list_for_each_entry(_maps_node_, &pinctrl_maps, node) \
-		for (_i_ = 0, _map_ = &_maps_node_->maps[_i_]; \
-			_i_ < _maps_node_->num_maps; \
-			_i_++, _map_ = &_maps_node_->maps[_i_])
+#define for_each_pin_map(_maps_node_, _map_)						\
+	list_for_each_entry(_maps_node_, &pinctrl_maps, node)				\
+		for (unsigned int __i = 0;						\
+		     _map_ = &_maps_node_->maps[__i], __i < _maps_node_->num_maps;	\
+		     __i++)