Messages in this thread | | | From | Stephen Warren <> | Date | Tue, 21 Feb 2012 09:38:47 -0800 | Subject | RE: [PATCH 08/20] pinctrl: Assume map table entries can't have a NULL name field |
| |
Dong Aisheng wrote at Tuesday, February 21, 2012 6:08 AM: > On Mon, Feb 20, 2012 at 2:45 PM, Stephen Warren <swarren@nvidia.com> wrote: > > pinctrl_register_mappings() already requires that every mapping table > > entry have a non-NULL name field. > > > > Logically, this makes sense too; drivers should always request a specific > > named state so they know what they're getting. Relying on getting the > > Hmm? It makes me a little confused. > IIRC we allow the driver to request NULL map name in the linaro > connect discussion. > For those devices they do not want to support multi states, they > really don't want to care about the state name. > The original way is convenient for such a case.
With device tree, every state is always going to have /some/ name; the binding we discussed didn't include any way to specify "no name" or "default name" or "default state", so this will keeps things a little more consistent.
I possibly could be persuaded to allow NULL state names in the mapping table. However, if so, I think they should work a little differently to the code currently in pinctrl: Right now, a NULL parameter to pinmux_get() allows matching against /any/ state name in the table. If we changed this to only allow matching against mapping table with a NULL name, that might be OK, since it'd be very explicit what it matched. But, as I mentione above, migrating such a driver to device tree would require changing this so I'm not inclined to like that solution without strong requirements.
In other words, what we have right now is something like:
if name: match = !strcmp(name, map->name) else: match = true
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
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > > index b6e3c35..5e30d91 100644 > > --- a/drivers/pinctrl/core.c > > +++ b/drivers/pinctrl/core.c > > @@ -488,8 +488,8 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name) > > int i; > > struct pinctrl_map const *map; > > > > - /* We must have dev or ID or both */ > > - if (!dev && !name) > > + /* We must have a state name */ > > + if (WARN_ON(!name)) > > return ERR_PTR(-EINVAL); > > We're already using the pinctrl device name for hog map. > So should it be something like: > if (WARN_ON(!dev) || WARN_ON(!name))
That's only true starting with the next patch in this series, which does modify the code roughly as you described.
-- nvpublic
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |