Re: [PATCH 08/20] pinctrl: Assume map table entries can't have a NULLname field

[Dong Aisheng]

On Mon, Feb 20, 2012 at 2:45 PM, Stephen Warren <swarren@xxxxxxxxxx> 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.

> first mentioned state in the mapping table is error-prone, and a nasty
> special case to implement, given that a given the mapping table may define
> multiple states for a device.
>
User should know this constraint.

> 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))


>        if (dev)
> @@ -530,23 +530,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
>                pr_debug("in map, found pctldev %s to handle function %s",
>                         dev_name(pctldev->dev), map->function);
>
> -               /*
> -                * 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;
> -               }
> +               /* State name must be the one we're looking for */
> +               if (strcmp(map->name, name))
> +                       continue;
>
>                /*
>                 * This is for the case where no device name is given, we
>                 * already know that the function name matches from above
>                 * code.
>                 */
> -               if (!map->dev_name && (name != NULL))
> +               if (!map->dev_name)
>                        found_map = true;
Do we still need this?
Is there still a case the dev_name can be NULL?


>
>                /* If the mapping has a device set up it must match */
> @@ -570,16 +563,14 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
>        /* We should have atleast one map, right */
>        if (!num_maps) {
>                pr_err("could not find any mux maps for device %s, ID %s\n",
> -                      devname ? devname : "(anonymous)",
> -                      name ? name : "(undefined)");
> +                      devname ? devname : "(anonymous)", name);
>                kfree(p);
>                return ERR_PTR(-EINVAL);
>        }
>
>        pr_debug("found %u mux maps for device %s, UD %s\n",
>                 num_maps,
> -                devname ? devname : "(anonymous)",
> -                name ? name : "(undefined)");
> +                devname ? devname : "(anonymous)", name);
>
>        /* Add the pinmux to the global list */
>        mutex_lock(&pinctrl_list_mutex);
> --
> 1.7.5.4
>

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