[RFC] pinctrl: enhance reporting of errors when loading from DT

[RFC] pinctrl: enhance reporting of errors when loading from DT

[John Crispin]

There are a few places in the api where the code simply returns -EINVAL when
it finds an error. An example is pinmux_map_to_setting() which now reports an
error if we try to match a group with a function that it does not support.

The reporting of errors in pinconf_check_ops and pinmux_check_ops now has the
same style and is located inside the according functions and not the calling
code.

When the map is loaded from DT but the default state can not be found we get
an error to know that the code at least tried.

The patch also removes a stray word from one comment and a "->" from another
for the sake of consistency.

Signed-off-by: John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

There are a few places tagged with //FIXME. These are all the error paths i
checked and decided that we dont need to make them verbose. I tried to extract
as much .name info from pointers surrounding the code as possible to make the
message explicit. Let me know what you think.


 drivers/pinctrl/core.c       |   21 ++++++++++++---------
 drivers/pinctrl/devicetree.c |    9 ++++++---
 drivers/pinctrl/pinconf.c    |   10 ++++++++--
 drivers/pinctrl/pinmux.c     |   27 +++++++++++++++++++--------
 4 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 59027ab..0c4486a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1319,21 +1319,18 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	/* If we're implementing pinmuxing, check the ops for sanity */
 	if (pctldesc->pmxops) {
 		ret = pinmux_check_ops(pctldev);
-		if (ret) {
-			pr_err("%s pinmux ops lacks necessary functions\n",
-			       pctldesc->name);
+		if (ret)
+			//FIXME: moved to pinmux_check_ops so we se a difference
+			//between missing function name and missing callback
 			goto out_err;
-		}
 	}
 
 	/* If we're implementing pinconfig, check the ops for sanity */
 	if (pctldesc->confops) {
 		ret = pinconf_check_ops(pctldev);
-		if (ret) {
-			pr_err("%s pin config ops lacks necessary functions\n",
-			       pctldesc->name);
+		if (ret)
+			//FIXME:moved to pinconf_check_ops()
 			goto out_err;
-		}
 	}
 
 	/* Register all the pins */
@@ -1356,8 +1353,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 		struct pinctrl_state *s =
 			pinctrl_lookup_state_locked(pctldev->p,
 						    PINCTRL_STATE_DEFAULT);
+
+		/* if it is not 0 we should not get here, clear it anyhow */
+		ret = 0;
 		if (!IS_ERR(s))
-			pinctrl_select_state_locked(pctldev->p, s);
+			ret = pinctrl_select_state_locked(pctldev->p, s);
+		if (IS_ERR(s) || ret)
+			pr_err("%s: failed to lookup/select default state\n",
+				pctldesc->name);
 	}
 
 	mutex_unlock(&pinctrl_mutex);
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..06a9834 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -144,9 +144,11 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 		return -ENODEV;
 	}
 	ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(p->dev, "failed to load mapping from DT for %s\n",
+			dev_name(pctldev->dev));
 		return ret;
-
+	}
 	/* Stash the mapping table chunk away for later use */
 	return dt_remember_or_free_map(p, statename, pctldev, map, num_maps);
 }
@@ -230,6 +232,7 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 			ret = dt_to_map_one_config(p, statename, np_config);
 			of_node_put(np_config);
 			if (ret < 0)
+				//FIXME: no need for a error msg as dt_to_map_one_config is verbose
 				goto err;
 		}
 
@@ -237,10 +240,10 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 		if (!size) {
 			ret = dt_remember_dummy_state(p, statename);
 			if (ret < 0)
+				//FIXME: no need for a error msg as dt_remember_dummy_state is verbose
 				goto err;
 		}
 	}
-
 	return 0;
 
 err:
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 384dcc1..2c56f60 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -28,11 +28,17 @@ int pinconf_check_ops(struct pinctrl_dev *pctldev)
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
 	/* We must be able to read out pin status */
-	if (!ops->pin_config_get && !ops->pin_config_group_get)
+	if (!ops->pin_config_get && !ops->pin_config_group_get) {
+		dev_err(pctldev->dev,
+			"pinconf must be able to read out pin status\n");
 		return -EINVAL;
+	}
 	/* We have to be able to config the pins in SOME way */
-	if (!ops->pin_config_set && !ops->pin_config_group_set)
+	if (!ops->pin_config_set && !ops->pin_config_group_set) {
+		dev_err(pctldev->dev,
+			"pinconf has to be able to set a pins config\n");
 		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 1056e68..588156b 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -42,9 +42,10 @@ int pinmux_check_ops(struct pinctrl_dev *pctldev)
 	    !ops->get_function_name ||
 	    !ops->get_function_groups ||
 	    !ops->enable ||
-	    !ops->disable)
+	    !ops->disable) {
+		dev_err(pctldev->dev, "pinmux ops lacks necessary functions\n");
 		return -EINVAL;
-
+	}
 	/* Check that all functions registered have names */
 	nfuncs = ops->get_functions_count(pctldev);
 	while (selector < nfuncs) {
@@ -142,7 +143,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 		status = 0;
 
 	if (status) {
-		dev_err(pctldev->dev, "->request on device %s failed for pin %d\n",
+		dev_err(pctldev->dev, "request on device %s failed for pin %d\n",
 		       pctldev->desc->name, pin);
 		module_put(pctldev->owner);
 	}
@@ -304,7 +305,7 @@ static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
 		selector++;
 	}
 
-	pr_err("%s does not support function %s\n",
+	pr_err("%s: function \"%s\" not supported\n",
 	       pinctrl_dev_get_name(pctldev), function);
 	return -EINVAL;
 }
@@ -330,16 +331,21 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 
 	ret = pinmux_func_name_to_selector(pctldev, map->data.mux.function);
 	if (ret < 0)
+		//FIXME: pinmux_func_name_to_selector is verbose
 		return ret;
 	setting->data.mux.func = ret;
 
 	ret = pmxops->get_function_groups(pctldev, setting->data.mux.func,
 					  &groups, &num_groups);
 	if (ret < 0)
+		//FIXME: safe to assume that get_function_groups() is verbose ?
 		return ret;
-	if (!num_groups)
+	if (!num_groups) {
+		//FIXME: we still need to check if returned data is valid
+		dev_err(pctldev->dev, "could not get mux groups \"%s\"",
+		                                map->data.mux.function);
 		return -EINVAL;
-
+	}
 	if (map->data.mux.group) {
 		bool found = false;
 		group = map->data.mux.group;
@@ -349,14 +355,19 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 				break;
 			}
 		}
-		if (!found)
+		if (!found) {
+			dev_err(pctldev->dev,
+				"invalid group \"%s\" for function \"%s\"\n",
+				group, map->data.mux.function);
 			return -EINVAL;
+		}
 	} else {
 		group = groups[0];
 	}
 
 	ret = pinctrl_get_group_selector(pctldev, group);
 	if (ret < 0)
+		//FIXME: pinctrl_get_group_selector is verbose
 		return ret;
 	setting->data.mux.group = ret;
 
@@ -374,7 +385,7 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 		ret = pin_request(pctldev, pins[i], map->dev_name, NULL);
 		if (ret) {
 			dev_err(pctldev->dev,
-				"could not get request pin %d on device %s\n",
+				"could not request pin %d on device %s\n",
 				pins[i], pinctrl_dev_get_name(pctldev));
 			/* On error release all taken pins */
 			i--; /* this pin just failed */
-- 
1.7.9.1

Re: [RFC] pinctrl: enhance reporting of errors when loading from DT

[Stephen Warren]

On 04/24/2012 10:53 AM, John Crispin wrote:
> There are a few places in the api where the code simply returns -EINVAL when
> it finds an error. An example is pinmux_map_to_setting() which now reports an
> error if we try to match a group with a function that it does not support.
> 
> The reporting of errors in pinconf_check_ops and pinmux_check_ops now has the
> same style and is located inside the according functions and not the calling
> code.
> 
> When the map is loaded from DT but the default state can not be found we get
> an error to know that the code at least tried.
> 
> The patch also removes a stray word from one comment and a "->" from another
> for the sake of consistency.

> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c

>  	/* If we're implementing pinmuxing, check the ops for sanity */
>  	if (pctldesc->pmxops) {
>  		ret = pinmux_check_ops(pctldev);
> -		if (ret) {
> -			pr_err("%s pinmux ops lacks necessary functions\n",
> -			       pctldesc->name);
> +		if (ret)
> +			//FIXME: moved to pinmux_check_ops so we se a difference
> +			//between missing function name and missing callback

The changes at this and the next FIXME look OK to me. In order to
actually apply the patch, I think you'd need to repost without the FIXME
comments though.

> @@ -1356,8 +1353,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>  		struct pinctrl_state *s =
>  			pinctrl_lookup_state_locked(pctldev->p,
>  						    PINCTRL_STATE_DEFAULT);
> +
> +		/* if it is not 0 we should not get here, clear it anyhow */
> +		ret = 0;
>  		if (!IS_ERR(s))
> -			pinctrl_select_state_locked(pctldev->p, s);
> +			ret = pinctrl_select_state_locked(pctldev->p, s);
> +		if (IS_ERR(s) || ret)
> +			pr_err("%s: failed to lookup/select default state\n",
> +				pctldesc->name);

This seems a little muddled. Why not:

s = pinctrl_lookup_state_locked(...);
if (IS_ERR(s))
    dev_dbg(...);
} else {
    ret = pinctrl_select_state_locked(...);
    if (ret) {
        dev_err(...);
    }
}

Note that pinctrl_lookup_state_locked() failing isn't necessarily an
error; it's quite legitimate for a pin controller to not have any states
defined for itself, if all pinctrl states are represented in other drivers.

That said, perhaps we should require all pin controllers to have dummy
states defined?

pinctrl_select_state_locked() failing means that a state was defined,
but could not be selected - that's definitely an error.

Also, dev_err() should be used in preference to pr_err().

> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c

> @@ -144,9 +144,11 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
>  		return -ENODEV;
>  	}
>  	ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err(p->dev, "failed to load mapping from DT for %s\n",
> +			dev_name(pctldev->dev));

Perhaps we should rely on the individual pin controller's dt_node_to_map
function reporting the specific error - it can provide more information.

> @@ -304,7 +305,7 @@ static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
>  		selector++;
>  	}
>  
> -	pr_err("%s does not support function %s\n",
> +	pr_err("%s: function \"%s\" not supported\n",
>  	       pinctrl_dev_get_name(pctldev), function);

That change seems to be a no-op really.

> @@ -330,16 +331,21 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
>  
>  	ret = pinmux_func_name_to_selector(pctldev, map->data.mux.function);
>  	if (ret < 0)
> +		//FIXME: pinmux_func_name_to_selector is verbose
>  		return ret;
>  	setting->data.mux.func = ret;
>  
>  	ret = pmxops->get_function_groups(pctldev, setting->data.mux.func,
>  					  &groups, &num_groups);
>  	if (ret < 0)
> +		//FIXME: safe to assume that get_function_groups() is verbose ?
>  		return ret;
> -	if (!num_groups)
> +	if (!num_groups) {
> +		//FIXME: we still need to check if returned data is valid
> +		dev_err(pctldev->dev, "could not get mux groups \"%s\"",
> +		                                map->data.mux.function);

That's not accurate. The retrieval succeeded, but the number of groups
the function can be selected on was zero.

I think I added a bunch more error prints locally when I was debugging a
new board. I guess I should remove the cruft from the patch and rebase
it on top of the final version of this once you post it.

Re: [RFC] pinctrl: enhance reporting of errors when loading from DT

[Linus Walleij]

On Tue, Apr 24, 2012 at 6:53 PM, John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> wrote:

> There are a few places tagged with //FIXME.

No C99 comments. See Documentation/CodingStyle.

So if you're gonna keep them,
/* use plain C comments */

And follow up on Stephens comments...

Yours,
Linus Walleij

Re: [RFC] pinctrl: enhance reporting of errors when loading from DT

[John Crispin]

Hi Linus

>> There are a few places tagged with //FIXME.
> No C99 comments. See Documentation/CodingStyle.

the //FIXME bits are as described below the tear line only there to show
which places I checked. they obviously will be removed in the final patch
> So if you're gonna keep them,
> /* use plain C comments */
>
> And follow up on Stephens comments...
i will follow up on stephens comments asap ...

John

Re: [RFC] pinctrl: enhance reporting of errors when loading from DT

[John Crispin]

Hi Stephen,

> This seems a little muddled. Why not:
>
> s = pinctrl_lookup_state_locked(...);
> if (IS_ERR(s))
>     dev_dbg(...);
> } else {
>     ret = pinctrl_select_state_locked(...);
>     if (ret) {
>         dev_err(...);
>     }
> }
>
ok

> Note that pinctrl_lookup_state_locked() failing isn't necessarily an
> error; it's quite legitimate for a pin controller to not have any states
> defined for itself, if all pinctrl states are represented in other drivers.
>
> That said, perhaps we should require all pin controllers to have dummy
> states defined?
>
> pinctrl_select_state_locked() failing means that a state was defined,
> but could not be selected - that's definitely an error.
>
> Also, dev_err() should be used in preference to pr_err().

The rest of the function used pr_*() style api. However
pinctrl_register() is passed a device pointer. I will update the patch
to use dev_err() throughout the function.


>> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
>> @@ -144,9 +144,11 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
>>  		return -ENODEV;
>>  	}
>>  	ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		dev_err(p->dev, "failed to load mapping from DT for %s\n",
>> +			dev_name(pctldev->dev));
> Perhaps we should rely on the individual pin controller's dt_node_to_map
> function reporting the specific error - it can provide more information.

Ok, i will drop this bit


>> @@ -304,7 +305,7 @@ static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
>>  		selector++;
>>  	}
>>  
>> -	pr_err("%s does not support function %s\n",
>> +	pr_err("%s: function \"%s\" not supported\n",
>>  	       pinctrl_dev_get_name(pctldev), function);
> That change seems to be a no-op really.
>
I will drop this bit


>> @@ -330,16 +331,21 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
>>  
>>  	ret = pinmux_func_name_to_selector(pctldev, map->data.mux.function);
>>  	if (ret < 0)
>> +		//FIXME: pinmux_func_name_to_selector is verbose
>>  		return ret;
>>  	setting->data.mux.func = ret;
>>  
>>  	ret = pmxops->get_function_groups(pctldev, setting->data.mux.func,
>>  					  &groups, &num_groups);
>>  	if (ret < 0)
>> +		//FIXME: safe to assume that get_function_groups() is verbose ?
>>  		return ret;
>> -	if (!num_groups)
>> +	if (!num_groups) {
>> +		//FIXME: we still need to check if returned data is valid
>> +		dev_err(pctldev->dev, "could not get mux groups \"%s\"",
>> +		                                map->data.mux.function);
> That's not accurate. The retrieval succeeded, but the number of groups
> the function can be selected on was zero.

Let me change the text a bit

> I think I added a bunch more error prints locally when I was debugging a
> new board. I guess I should remove the cruft from the patch and rebase
> it on top of the final version of this once you post it

I will send the final version of my patch shortly. Feel free to use it
as a basis to merge with your debug prints.

Thanks for the review,
John

[RFC] pinctrl: enhance reporting of errors when loading from DT

[John Crispin]

There are a few places in the api where the code simply returns -EINVAL when
it finds an error. An example is pinmux_map_to_setting() which now reports an
error if we try to match a group with a function that it does not support.

The reporting of errors in pinconf_check_ops and pinmux_check_ops now has the
same style and is located inside the according functions and not the calling
code.

When the map is loaded from DT but the default state can not be found we get
an error to know that the code at least tried.

The patch also removes a stray word from one comment and a "->" from another
for the sake of consistency.

Signed-off-by: John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

There are a few places tagged with //FIXME. These are all the error paths i
checked and decided that we dont need to make them verbose. I tried to extract
as much .name info from pointers surrounding the code as possible to make the
message explicit. Let me know what you think.


 drivers/pinctrl/core.c       |   21 ++++++++++++---------
 drivers/pinctrl/devicetree.c |    9 ++++++---
 drivers/pinctrl/pinconf.c    |   10 ++++++++--
 drivers/pinctrl/pinmux.c     |   27 +++++++++++++++++++--------
 4 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 59027ab..0c4486a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1319,21 +1319,18 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	/* If we're implementing pinmuxing, check the ops for sanity */
 	if (pctldesc->pmxops) {
 		ret = pinmux_check_ops(pctldev);
-		if (ret) {
-			pr_err("%s pinmux ops lacks necessary functions\n",
-			       pctldesc->name);
+		if (ret)
+			//FIXME: moved to pinmux_check_ops so we se a difference
+			//between missing function name and missing callback
 			goto out_err;
-		}
 	}
 
 	/* If we're implementing pinconfig, check the ops for sanity */
 	if (pctldesc->confops) {
 		ret = pinconf_check_ops(pctldev);
-		if (ret) {
-			pr_err("%s pin config ops lacks necessary functions\n",
-			       pctldesc->name);
+		if (ret)
+			//FIXME:moved to pinconf_check_ops()
 			goto out_err;
-		}
 	}
 
 	/* Register all the pins */
@@ -1356,8 +1353,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 		struct pinctrl_state *s =
 			pinctrl_lookup_state_locked(pctldev->p,
 						    PINCTRL_STATE_DEFAULT);
+
+		/* if it is not 0 we should not get here, clear it anyhow */
+		ret = 0;
 		if (!IS_ERR(s))
-			pinctrl_select_state_locked(pctldev->p, s);
+			ret = pinctrl_select_state_locked(pctldev->p, s);
+		if (IS_ERR(s) || ret)
+			pr_err("%s: failed to lookup/select default state\n",
+				pctldesc->name);
 	}
 
 	mutex_unlock(&pinctrl_mutex);
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fcb1de4..06a9834 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -144,9 +144,11 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 		return -ENODEV;
 	}
 	ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(p->dev, "failed to load mapping from DT for %s\n",
+			dev_name(pctldev->dev));
 		return ret;
-
+	}
 	/* Stash the mapping table chunk away for later use */
 	return dt_remember_or_free_map(p, statename, pctldev, map, num_maps);
 }
@@ -230,6 +232,7 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 			ret = dt_to_map_one_config(p, statename, np_config);
 			of_node_put(np_config);
 			if (ret < 0)
+				//FIXME: no need for a error msg as dt_to_map_one_config is verbose
 				goto err;
 		}
 
@@ -237,10 +240,10 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 		if (!size) {
 			ret = dt_remember_dummy_state(p, statename);
 			if (ret < 0)
+				//FIXME: no need for a error msg as dt_remember_dummy_state is verbose
 				goto err;
 		}
 	}
-
 	return 0;
 
 err:
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 384dcc1..2c56f60 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -28,11 +28,17 @@ int pinconf_check_ops(struct pinctrl_dev *pctldev)
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
 	/* We must be able to read out pin status */
-	if (!ops->pin_config_get && !ops->pin_config_group_get)
+	if (!ops->pin_config_get && !ops->pin_config_group_get) {
+		dev_err(pctldev->dev,
+			"pinconf must be able to read out pin status\n");
 		return -EINVAL;
+	}
 	/* We have to be able to config the pins in SOME way */
-	if (!ops->pin_config_set && !ops->pin_config_group_set)
+	if (!ops->pin_config_set && !ops->pin_config_group_set) {
+		dev_err(pctldev->dev,
+			"pinconf has to be able to set a pins config\n");
 		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 1056e68..588156b 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -42,9 +42,10 @@ int pinmux_check_ops(struct pinctrl_dev *pctldev)
 	    !ops->get_function_name ||
 	    !ops->get_function_groups ||
 	    !ops->enable ||
-	    !ops->disable)
+	    !ops->disable) {
+		dev_err(pctldev->dev, "pinmux ops lacks necessary functions\n");
 		return -EINVAL;
-
+	}
 	/* Check that all functions registered have names */
 	nfuncs = ops->get_functions_count(pctldev);
 	while (selector < nfuncs) {
@@ -142,7 +143,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 		status = 0;
 
 	if (status) {
-		dev_err(pctldev->dev, "->request on device %s failed for pin %d\n",
+		dev_err(pctldev->dev, "request on device %s failed for pin %d\n",
 		       pctldev->desc->name, pin);
 		module_put(pctldev->owner);
 	}
@@ -304,7 +305,7 @@ static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
 		selector++;
 	}
 
-	pr_err("%s does not support function %s\n",
+	pr_err("%s: function \"%s\" not supported\n",
 	       pinctrl_dev_get_name(pctldev), function);
 	return -EINVAL;
 }
@@ -330,16 +331,21 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 
 	ret = pinmux_func_name_to_selector(pctldev, map->data.mux.function);
 	if (ret < 0)
+		//FIXME: pinmux_func_name_to_selector is verbose
 		return ret;
 	setting->data.mux.func = ret;
 
 	ret = pmxops->get_function_groups(pctldev, setting->data.mux.func,
 					  &groups, &num_groups);
 	if (ret < 0)
+		//FIXME: safe to assume that get_function_groups() is verbose ?
 		return ret;
-	if (!num_groups)
+	if (!num_groups) {
+		//FIXME: we still need to check if returned data is valid
+		dev_err(pctldev->dev, "could not get mux groups \"%s\"",
+		                                map->data.mux.function);
 		return -EINVAL;
-
+	}
 	if (map->data.mux.group) {
 		bool found = false;
 		group = map->data.mux.group;
@@ -349,14 +355,19 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 				break;
 			}
 		}
-		if (!found)
+		if (!found) {
+			dev_err(pctldev->dev,
+				"invalid group \"%s\" for function \"%s\"\n",
+				group, map->data.mux.function);
 			return -EINVAL;
+		}
 	} else {
 		group = groups[0];
 	}
 
 	ret = pinctrl_get_group_selector(pctldev, group);
 	if (ret < 0)
+		//FIXME: pinctrl_get_group_selector is verbose
 		return ret;
 	setting->data.mux.group = ret;
 
@@ -374,7 +385,7 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 		ret = pin_request(pctldev, pins[i], map->dev_name, NULL);
 		if (ret) {
 			dev_err(pctldev->dev,
-				"could not get request pin %d on device %s\n",
+				"could not request pin %d on device %s\n",
 				pins[i], pinctrl_dev_get_name(pctldev));
 			/* On error release all taken pins */
 			i--; /* this pin just failed */
-- 
1.7.9.1