Date:	Wed, 22 Feb 2012 19:39:42 -0800
From:	Stephen Warren <swarren@...dia.com>
To:	Dong Aisheng <aisheng.dong@...escale.com>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	"B29396@...escale.com" <B29396@...escale.com>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
	"dongas86@...il.com" <dongas86@...il.com>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"thomas.abraham@...aro.org" <thomas.abraham@...aro.org>,
	"tony@...mide.com" <tony@...mide.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 08/20] pinctrl: Assume map table entries can't have a
 NULL name field

Dong Aisheng wrote at Wednesday, February 22, 2012 8:35 PM:
> On Wed, Feb 22, 2012 at 10:05:20AM -0800, Stephen Warren wrote:
> > Dong Aisheng wrote at Tuesday, February 21, 2012 11:35 PM:
> > > On Tue, Feb 21, 2012 at 09:46:01AM -0800, Stephen Warren wrote:
> > > ....
> > > > > we might just
> > > > >
> > > > > #define PIN_MAP_NAME_DEFAULT "default"
> > > > > In <linux/pinctrl/consumer.h> and <linux/pinctrl/machine.h> alike,
> > > > > maybe in some <linux/pinctrl/mapnames.h> that both of these
> > > > > include?
> > > > >
> > > > > the have the driver ask for:
> > > > >
> > > > > sirfport->p = pinctrl_get(&pdev->dev, PIN_MAP_NAME_DEFAULT);
> > > > >
> > > > > (Similar changes can be done for U300, naming all its map
> > > > > "default".)
> > > >
> > > > I guess we could just modify pinmux_get() such that if NULL is passed as
> > > > the state name, it uses "default" instead internally. The disadvantage I
> > > > see here is that someone reading the client driver and writing the mapping
> > > > table then has to know that pinmux_get() does that internally, rather than
> > > > it being obvious right in the client driver code.
> > >
> > > It looks like a way out.
> >
> > > The left problem seems to be that we may force the mapping table writer to
> > > specify "default" name for map at least.
> > > Is there any way to avoid it?
> >
> > That specific restriction has been present since the very first patch
> > that created the pinctrl subsystem.
>
> Since i saw some code in pinctrl subsystem formerly like:
> /*
>  * If we're looking for a specific named map, this must match,
>  * else we loop and look for the next.
>  */
> if (name != NULL) {
> 	if (map->name == NULL)
> 		continue;
> 	if (strcmp(map->name, name))
> 		continue;
> }
> So i guessed the map->name could be NULL in the original design.
> Of course, if we used the convenience macro to define map, there's no
> chance to make map name NULL.

Admittedly, the code is a little inconsistent. I was thinking of:

+int __init pinmux_register_mappings(struct pinmux_map const *maps,
...
+       for (i = 0; i < num_maps; i++) {
+               /* Sanity check the mapping */
+               if (!maps[i].name) {
+                       pr_err("failed to register map %d: "
+                              "no map name given\n", i);
+                       return -EINVAL;
+               }

> We did not force users to must convenience macros, right?

No. In fact, my patches for Tegra's board file didn't always use them.

> > > I just think it may be better if we do not have such restriction.
> >
> > Given that all map entries have a name, I'd really prefer that all drivers
> > had to request a specific matching name, rather than saying "just give me
> > whatever is in the mapping table".
> >
> > I don't see this as a restriction, but more of a correctness issue;
> > every mapping table entry has a name and drivers (or device tree bindings)
> > define what that name must be, so why shouldn't drivers be required to
> > request that specific name, and experience an error if the mapping table
> > author didn't create that name?
>
> Yes, i agree.
> Based on this point, i feel maybe internally convert NULL to a "default" name for
> pinctrl_get does not make too much sense.
> 
> > regulator_get(dev, id) requires id != NULL.
> >
> > clk_get(dev, con_id) requires con_id != NULL in order to match a clock
> > that has a non-NULL con_id defined (although clocks with a NULL con_id
> > are also legal, in which case, the con_id parameter to clk_get is
> > ignored)
> >
> > So pinctrl_get(dev, state) requiring state != NULL seems consistent
> > with existing practice.
>
> Yes, it's true if given all map entries have a name.
> 
> Now i'm intend to the way you mentioned at:
> https://lkml.org/lkml/2012/2/21/213
> 
> And i agree with the way you said:
> If we continue to allow NULL names, I think we should change that to:
> if name:
>     match = map->name && !strcmp(name, map->name)
> else:
>     match = !map->name
> 
> And i think allowing map name to be NULL will make life easy for those
> devices who do not support multi states.

It does make it a little convenient for board files, but isn't really
practical to implement for device-tree unless we modify the binding to
have something that means "no name". I'd rather not allow more options
in the binding though.

What's wrong with writing e.g. PINCTRL_DEFAULT instead of NULL when calling
pinctrl_get() or pinctrl_lookup_state() though? Given pinctrl.h having:

#define PINCTRL_DEFAULT "default"

> Originally I'm wondering how about more than one entries with a NULL name.
> But now i think it's actually not an issue since we indeed support multi
> entries with the same map name.

Yes.

If the mapping table contains a number of entries with the same string name,
they're all part of the same single state with that name.

If entries with a name field set to NULL are allowed and exist, I'd similarly
consider them all part of the same state, which just doesn't have a name.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists