[PATCH v1 0/2] pinctrl: generic: improve apply_setting error verbosity

[PATCH v1 0/2] pinctrl: generic: improve apply_setting error verbosity

[Matheus Castello]

This series is for improve verbosity in debug for error cases using pinconf 
generic API. Legacy pinctrl-vendor drivers prints error messages with more 
verbosity when parse invalid or not supported sub-nodes properties. In this 
series we try to show messages with more information about the pinctrl-vendor 
driver and sub-nodes that returned error when applied.

Let me know your opinion about this.

Matheus Castello (2):
  pinctrl: generic: add API to solve generic sub-node property name
  pinctrl: generic: improve apply_setting error verbosity

 drivers/pinctrl/pinconf-generic.c       | 61 +++++++++++++++++----------------
 drivers/pinctrl/pinconf.c               | 15 +++++++-
 include/linux/pinctrl/pinconf-generic.h | 32 +++++++++++++++++
 3 files changed, 77 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] pinctrl: generic: add API to solve generic sub-node property name

[Matheus Castello]

Set dt_params with the definitions of the generic sub-node properties
global in pinconf-generic.h and add a function to pinconf-generic API
to decode a packed param returning the string name from sub-node
property.

This can be useful in debug from pinctrl-vendor driver or even for
debug in pinconf driver.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/pinconf-generic.c       | 61 +++++++++++++++++----------------
 include/linux/pinctrl/pinconf-generic.h | 32 +++++++++++++++++
 2 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index b4f7f8a..78585bc 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -156,34 +156,6 @@ EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
 #endif
 
 #ifdef CONFIG_OF
-static const struct pinconf_generic_params dt_params[] = {
-	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
-	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
-	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
-	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
-	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
-	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
-	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
-	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
-	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
-	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
-	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
-	{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
-	{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
-	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
-	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
-	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
-	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
-	{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
-	{ "output-disable", PIN_CONFIG_OUTPUT_ENABLE, 0 },
-	{ "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
-	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
-	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
-	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
-	{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
-	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
-	{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
-};
 
 /**
  * parse_dt_cfg() - Parse DT pinconf parameters
@@ -247,14 +219,14 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		return -EINVAL;
 
 	/* allocate a temporary array big enough to hold one of each option */
-	max_cfg = ARRAY_SIZE(dt_params);
+	max_cfg = ARRAY_SIZE(pinconf_dt_params);
 	if (pctldev)
 		max_cfg += pctldev->desc->num_custom_params;
 	cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
 	if (!cfg)
 		return -ENOMEM;
 
-	parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
+	parse_dt_cfg(np, pinconf_dt_params, ARRAY_SIZE(pinconf_dt_params), cfg, &ncfg);
 	if (pctldev && pctldev->desc->num_custom_params &&
 		pctldev->desc->custom_params)
 		parse_dt_cfg(np, pctldev->desc->custom_params,
@@ -409,4 +381,33 @@ void pinconf_generic_dt_free_map(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_dt_free_map);
 
+const char *pinconf_generic_get_param_property_name(struct pinctrl_dev *pctldev,
+	unsigned int num_configs, unsigned long *param)
+{
+	struct pinctrl_desc *desc = pctldev->desc;
+	unsigned int num_params = ARRAY_SIZE(pinconf_dt_params),
+		num_custom_params = desc->num_custom_params,
+		i, j;
+
+	for (i = 0; i < num_configs; i++) {
+		enum pin_config_param pin_param =
+		pinconf_to_config_param(param[i]);
+
+		/* first search on default properties */
+		for (j = 0; j < num_params; j++) {
+			if (pin_param == pinconf_dt_params[j].param)
+				return pinconf_dt_params[j].property;
+		}
+
+		/* search on custom properties */
+		for (j = 0; j < num_custom_params; j++) {
+			if (pin_param == desc->custom_params[j].param)
+				return desc->custom_params[j].property;
+		}
+	}
+
+	return "property undefined";
+}
+EXPORT_SYMBOL_GPL(pinconf_generic_get_param_property_name);
+
 #endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c06806..c51f4b3 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -184,6 +184,35 @@ struct pinconf_generic_params {
 	u32 default_value;
 };
 
+static const struct pinconf_generic_params pinconf_dt_params[] = {
+	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
+	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
+	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
+	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
+	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
+	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
+	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
+	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
+	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
+	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
+	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
+	{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
+	{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
+	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
+	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
+	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
+	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
+	{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
+	{ "output-disable", PIN_CONFIG_OUTPUT_ENABLE, 0 },
+	{ "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
+	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
+	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
+	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
+	{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
+	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
+	{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
+};
+
 int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np, struct pinctrl_map **map,
 		unsigned *reserved_maps, unsigned *num_maps,
@@ -194,6 +223,9 @@ int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
 void pinconf_generic_dt_free_map(struct pinctrl_dev *pctldev,
 		struct pinctrl_map *map, unsigned num_maps);
 
+const char *pinconf_generic_get_param_property_name(struct pinctrl_dev *pctldev,
+		unsigned int num_configs, unsigned long *param);
+
 static inline int pinconf_generic_dt_node_to_map_group(
 		struct pinctrl_dev *pctldev, struct device_node *np_config,
 		struct pinctrl_map **map, unsigned *num_maps)
-- 
2.7.4

[PATCH v1 2/2] pinctrl: generic: improve apply_setting error verbosity

[Matheus Castello]

For generic pinconf: print the dev_error with the pinctrl vendor
driver name, error code, the sub-node property name used and the
pin that was tried to set.

Improves the undestading of the error if use a generic sub-node
property that generic-pinconf can do parse but the vendor pinctrl
driver does not support.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/pinconf.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index d3fe143..fd5962a 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -21,6 +21,7 @@
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include "core.h"
 #include "pinconf.h"
 
@@ -169,9 +170,21 @@ int pinconf_apply_setting(const struct pinctrl_setting *setting)
 				setting->data.configs.configs,
 				setting->data.configs.num_configs);
 		if (ret < 0) {
+#ifdef CONFIG_GENERIC_PINCONF
 			dev_err(pctldev->dev,
-				"pin_config_set op failed for pin %d\n",
+				"%s error %d seting %s for pin %d\n",
+				pctldev->desc->name, ret,
+				pinconf_generic_get_param_property_name(
+					pctldev, setting->data.configs.num_configs,
+					setting->data.configs.configs),
 				setting->data.configs.group_or_pin);
+#endif
+
+			dev_err(pctldev->dev,
+				"pin_config_set op failed for %s pin %d\n",
+				pctldev->desc->name,
+				setting->data.configs.group_or_pin);
+
 			return ret;
 		}
 		break;
-- 
2.7.4

Re: [PATCH v1 2/2] pinctrl: generic: improve apply_setting error verbosity

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

Hi Matheus,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.17-rc3 next-20180501]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matheus-Castello/pinctrl-generic-improve-apply_setting-error-verbosity/20180501-160704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-randconfig-x002-201817 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/pinctrl/pinconf.c: In function 'pinconf_apply_setting':
>> drivers/pinctrl/pinconf.c:177:5: error: implicit declaration of function 'pinconf_generic_get_param_property_name'; did you mean 'pinconf_generic_dump_config'? [-Werror=implicit-function-declaration]
        pinconf_generic_get_param_property_name(
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        pinconf_generic_dump_config
>> drivers/pinctrl/pinconf.c:175:26: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'int' [-Wformat=]
        "%s error %d seting %s for pin %d\n",
                            ~^
                            %d
   drivers/pinctrl/pinconf.c:177:5:
        pinconf_generic_get_param_property_name(
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         pctldev, setting->data.configs.num_configs,
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         setting->data.configs.configs),
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +177 drivers/pinctrl/pinconf.c

   150	
   151	int pinconf_apply_setting(const struct pinctrl_setting *setting)
   152	{
   153		struct pinctrl_dev *pctldev = setting->pctldev;
   154		const struct pinconf_ops *ops = pctldev->desc->confops;
   155		int ret;
   156	
   157		if (!ops) {
   158			dev_err(pctldev->dev, "missing confops\n");
   159			return -EINVAL;
   160		}
   161	
   162		switch (setting->type) {
   163		case PIN_MAP_TYPE_CONFIGS_PIN:
   164			if (!ops->pin_config_set) {
   165				dev_err(pctldev->dev, "missing pin_config_set op\n");
   166				return -EINVAL;
   167			}
   168			ret = ops->pin_config_set(pctldev,
   169					setting->data.configs.group_or_pin,
   170					setting->data.configs.configs,
   171					setting->data.configs.num_configs);
   172			if (ret < 0) {
   173	#ifdef CONFIG_GENERIC_PINCONF
   174				dev_err(pctldev->dev,
 > 175					"%s error %d seting %s for pin %d\n",
   176					pctldev->desc->name, ret,
 > 177					pinconf_generic_get_param_property_name(
   178						pctldev, setting->data.configs.num_configs,
   179						setting->data.configs.configs),
   180					setting->data.configs.group_or_pin);
   181	#endif
   182	
   183				dev_err(pctldev->dev,
   184					"pin_config_set op failed for %s pin %d\n",
   185					pctldev->desc->name,
   186					setting->data.configs.group_or_pin);
   187	
   188				return ret;
   189			}
   190			break;
   191		case PIN_MAP_TYPE_CONFIGS_GROUP:
   192			if (!ops->pin_config_group_set) {
   193				dev_err(pctldev->dev,
   194					"missing pin_config_group_set op\n");
   195				return -EINVAL;
   196			}
   197			ret = ops->pin_config_group_set(pctldev,
   198					setting->data.configs.group_or_pin,
   199					setting->data.configs.configs,
   200					setting->data.configs.num_configs);
   201			if (ret < 0) {
   202				dev_err(pctldev->dev,
   203					"pin_config_group_set op failed for group %d\n",
   204					setting->data.configs.group_or_pin);
   205				return ret;
   206			}
   207			break;
   208		default:
   209			return -EINVAL;
   210		}
   211	
   212		return 0;
   213	}
   214	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30311 bytes --]

Re: [PATCH v1 2/2] pinctrl: generic: improve apply_setting error verbosity

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

Hi Matheus,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.17-rc3 next-20180501]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matheus-Castello/pinctrl-generic-improve-apply_setting-error-verbosity/20180501-160704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-randconfig-s2-05011559 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//pinctrl/pinconf.c: In function 'pinconf_apply_setting':
>> drivers//pinctrl/pinconf.c:177:5: error: implicit declaration of function 'pinconf_generic_get_param_property_name' [-Werror=implicit-function-declaration]
        pinconf_generic_get_param_property_name(
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers//pinctrl/pinconf.c:175:26: warning: format '%s' expects argument of type 'char *', but argument 5 has type 'int' [-Wformat=]
        "%s error %d seting %s for pin %d\n",
                             ^
   cc1: some warnings being treated as errors

vim +/pinconf_generic_get_param_property_name +177 drivers//pinctrl/pinconf.c

   150	
   151	int pinconf_apply_setting(const struct pinctrl_setting *setting)
   152	{
   153		struct pinctrl_dev *pctldev = setting->pctldev;
   154		const struct pinconf_ops *ops = pctldev->desc->confops;
   155		int ret;
   156	
   157		if (!ops) {
   158			dev_err(pctldev->dev, "missing confops\n");
   159			return -EINVAL;
   160		}
   161	
   162		switch (setting->type) {
   163		case PIN_MAP_TYPE_CONFIGS_PIN:
   164			if (!ops->pin_config_set) {
   165				dev_err(pctldev->dev, "missing pin_config_set op\n");
   166				return -EINVAL;
   167			}
   168			ret = ops->pin_config_set(pctldev,
   169					setting->data.configs.group_or_pin,
   170					setting->data.configs.configs,
   171					setting->data.configs.num_configs);
   172			if (ret < 0) {
   173	#ifdef CONFIG_GENERIC_PINCONF
   174				dev_err(pctldev->dev,
   175					"%s error %d seting %s for pin %d\n",
   176					pctldev->desc->name, ret,
 > 177					pinconf_generic_get_param_property_name(
   178						pctldev, setting->data.configs.num_configs,
   179						setting->data.configs.configs),
   180					setting->data.configs.group_or_pin);
   181	#endif
   182	
   183				dev_err(pctldev->dev,
   184					"pin_config_set op failed for %s pin %d\n",
   185					pctldev->desc->name,
   186					setting->data.configs.group_or_pin);
   187	
   188				return ret;
   189			}
   190			break;
   191		case PIN_MAP_TYPE_CONFIGS_GROUP:
   192			if (!ops->pin_config_group_set) {
   193				dev_err(pctldev->dev,
   194					"missing pin_config_group_set op\n");
   195				return -EINVAL;
   196			}
   197			ret = ops->pin_config_group_set(pctldev,
   198					setting->data.configs.group_or_pin,
   199					setting->data.configs.configs,
   200					setting->data.configs.num_configs);
   201			if (ret < 0) {
   202				dev_err(pctldev->dev,
   203					"pin_config_group_set op failed for group %d\n",
   204					setting->data.configs.group_or_pin);
   205				return ret;
   206			}
   207			break;
   208		default:
   209			return -EINVAL;
   210		}
   211	
   212		return 0;
   213	}
   214	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32961 bytes --]

Re: [PATCH v2 0/2] pinctrl: generic: improve apply_setting error verbosity

[Matheus Castello]

This series is for improve verbosity in debug for error cases using pinconf 
generic API. Legacy pinctrl-vendor drivers prints error messages with more 
verbosity when parse invalid or not supported sub-nodes properties. In this 
series we try to show messages with more information about the pinctrl-vendor 
driver and sub-nodes that returned error when applied.

Let me know your opinion about this.

Tested Kbuild for arch/x86_64 arch/arm

Changes since v1:
(Suggested by Linus Walleij)
- Fix errors in Kbuild
- Use CONFIG_OF instead CONFIG_GENERIC_PINCONF for use it only in flatned 
device tree enable configurations

Matheus Castello (2):
  pinctrl: generic: add API to solve generic sub-node property name
  pinctrl: generic: improve apply_setting error verbosity

 drivers/pinctrl/pinconf-generic.c       | 61 +++++++++++++++++----------------
 drivers/pinctrl/pinconf.c               | 15 +++++++-
 include/linux/pinctrl/pinconf-generic.h | 32 +++++++++++++++++
 3 files changed, 77 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] pinctrl: generic: add API to solve generic sub-node property name

[Matheus Castello]

Set dt_params with the definitions of the generic sub-node properties
global in pinconf-generic.h and add a function to pinconf-generic API
to decode a packed param returning the string name from sub-node
property.

This can be useful in debug from pinctrl-vendor driver or even for
debug in pinconf driver.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/pinconf-generic.c       | 61 +++++++++++++++++----------------
 include/linux/pinctrl/pinconf-generic.h | 32 +++++++++++++++++
 2 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index b4f7f8a..78585bc 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -156,34 +156,6 @@ EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
 #endif
 
 #ifdef CONFIG_OF
-static const struct pinconf_generic_params dt_params[] = {
-	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
-	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
-	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
-	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
-	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
-	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
-	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
-	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
-	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
-	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
-	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
-	{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
-	{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
-	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
-	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
-	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
-	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
-	{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
-	{ "output-disable", PIN_CONFIG_OUTPUT_ENABLE, 0 },
-	{ "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
-	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
-	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
-	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
-	{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
-	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
-	{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
-};
 
 /**
  * parse_dt_cfg() - Parse DT pinconf parameters
@@ -247,14 +219,14 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		return -EINVAL;
 
 	/* allocate a temporary array big enough to hold one of each option */
-	max_cfg = ARRAY_SIZE(dt_params);
+	max_cfg = ARRAY_SIZE(pinconf_dt_params);
 	if (pctldev)
 		max_cfg += pctldev->desc->num_custom_params;
 	cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
 	if (!cfg)
 		return -ENOMEM;
 
-	parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
+	parse_dt_cfg(np, pinconf_dt_params, ARRAY_SIZE(pinconf_dt_params), cfg, &ncfg);
 	if (pctldev && pctldev->desc->num_custom_params &&
 		pctldev->desc->custom_params)
 		parse_dt_cfg(np, pctldev->desc->custom_params,
@@ -409,4 +381,33 @@ void pinconf_generic_dt_free_map(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_dt_free_map);
 
+const char *pinconf_generic_get_param_property_name(struct pinctrl_dev *pctldev,
+	unsigned int num_configs, unsigned long *param)
+{
+	struct pinctrl_desc *desc = pctldev->desc;
+	unsigned int num_params = ARRAY_SIZE(pinconf_dt_params),
+		num_custom_params = desc->num_custom_params,
+		i, j;
+
+	for (i = 0; i < num_configs; i++) {
+		enum pin_config_param pin_param =
+		pinconf_to_config_param(param[i]);
+
+		/* first search on default properties */
+		for (j = 0; j < num_params; j++) {
+			if (pin_param == pinconf_dt_params[j].param)
+				return pinconf_dt_params[j].property;
+		}
+
+		/* search on custom properties */
+		for (j = 0; j < num_custom_params; j++) {
+			if (pin_param == desc->custom_params[j].param)
+				return desc->custom_params[j].property;
+		}
+	}
+
+	return "property undefined";
+}
+EXPORT_SYMBOL_GPL(pinconf_generic_get_param_property_name);
+
 #endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c06806..c51f4b3 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -184,6 +184,35 @@ struct pinconf_generic_params {
 	u32 default_value;
 };
 
+static const struct pinconf_generic_params pinconf_dt_params[] = {
+	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
+	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
+	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
+	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
+	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
+	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
+	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
+	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
+	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
+	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
+	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
+	{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
+	{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
+	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
+	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
+	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
+	{ "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
+	{ "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
+	{ "output-disable", PIN_CONFIG_OUTPUT_ENABLE, 0 },
+	{ "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
+	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
+	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
+	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
+	{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
+	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
+	{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
+};
+
 int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np, struct pinctrl_map **map,
 		unsigned *reserved_maps, unsigned *num_maps,
@@ -194,6 +223,9 @@ int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
 void pinconf_generic_dt_free_map(struct pinctrl_dev *pctldev,
 		struct pinctrl_map *map, unsigned num_maps);
 
+const char *pinconf_generic_get_param_property_name(struct pinctrl_dev *pctldev,
+		unsigned int num_configs, unsigned long *param);
+
 static inline int pinconf_generic_dt_node_to_map_group(
 		struct pinctrl_dev *pctldev, struct device_node *np_config,
 		struct pinctrl_map **map, unsigned *num_maps)
-- 
2.7.4

[PATCH v2 2/2] pinctrl: generic: improve apply_setting error verbosity

[Matheus Castello]

For generic pinconf: print the dev_error with the pinctrl vendor
driver name, error code, the sub-node property name used and the
pin that was tried to set.

Improves the undestading of the error if use a generic sub-node
property that generic-pinconf can do parse but the vendor pinctrl
driver does not support.

Signed-off-by: Matheus Castello <matheus@castello.eng.br>
---
 drivers/pinctrl/pinconf.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index d3fe143..ced2b67 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -21,6 +21,7 @@
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include "core.h"
 #include "pinconf.h"
 
@@ -169,9 +170,21 @@ int pinconf_apply_setting(const struct pinctrl_setting *setting)
 				setting->data.configs.configs,
 				setting->data.configs.num_configs);
 		if (ret < 0) {
+#ifdef CONFIG_OF
 			dev_err(pctldev->dev,
-				"pin_config_set op failed for pin %d\n",
+				"%s error %d setting %s for pin %d\n",
+				pctldev->desc->name, ret,
+				pinconf_generic_get_param_property_name(
+					pctldev, setting->data.configs.num_configs,
+					setting->data.configs.configs),
 				setting->data.configs.group_or_pin);
+#endif
+
+			dev_err(pctldev->dev,
+				"pin_config_set op failed for %s pin %d\n",
+				pctldev->desc->name,
+				setting->data.configs.group_or_pin);
+
 			return ret;
 		}
 		break;
-- 
2.7.4

Re: [PATCH v2 2/2] pinctrl: generic: improve apply_setting error verbosity

[Linus Walleij]

On Tue, May 1, 2018 at 9:10 PM, Matheus Castello
<matheus@castello.eng.br> wrote:

> For generic pinconf: print the dev_error with the pinctrl vendor
> driver name, error code, the sub-node property name used and the
> pin that was tried to set.
>
> Improves the undestading of the error if use a generic sub-node
> property that generic-pinconf can do parse but the vendor pinctrl
> driver does not support.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

> +#include <linux/pinctrl/pinconf-generic.h>
>  #include "core.h"
>  #include "pinconf.h"
>
> @@ -169,9 +170,21 @@ int pinconf_apply_setting(const struct pinctrl_setting *setting)
>                                 setting->data.configs.configs,
>                                 setting->data.configs.num_configs);
>                 if (ret < 0) {
> +#ifdef CONFIG_OF

This doesn't seem right.

If this is restricted for OF only the root cause to why it is
like that needs to be found and the code refactored to fit anyone,
there is also ACPI support in the works I think, surely they should
be able to get verbose messages.

>                         dev_err(pctldev->dev,
> -                               "pin_config_set op failed for pin %d\n",
> +                               "%s error %d setting %s for pin %d\n",
> +                               pctldev->desc->name, ret,
> +                               pinconf_generic_get_param_property_name(
> +                                       pctldev, setting->data.configs.num_configs,
> +                                       setting->data.configs.configs),
>                                 setting->data.configs.group_or_pin);

This doesn't seem right. First argument is a %d, yet this
is pctldev->desc->name?

Something is fishy with the argument list.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] pinctrl: generic: add API to solve generic sub-node property name

[Linus Walleij]

On Tue, May 1, 2018 at 9:10 PM, Matheus Castello
<matheus@castello.eng.br> wrote:

> Set dt_params with the definitions of the generic sub-node properties
> global in pinconf-generic.h and add a function to pinconf-generic API
> to decode a packed param returning the string name from sub-node
> property.
>
> This can be useful in debug from pinctrl-vendor driver or even for
> debug in pinconf driver.
>
> Signed-off-by: Matheus Castello <matheus@castello.eng.br>

(...)

> +const char *pinconf_generic_get_param_property_name(struct pinctrl_dev *pctldev,
> +       unsigned int num_configs, unsigned long *param)
> +{
> +       struct pinctrl_desc *desc = pctldev->desc;
> +       unsigned int num_params = ARRAY_SIZE(pinconf_dt_params),
> +               num_custom_params = desc->num_custom_params,
> +               i, j;
> +
> +       for (i = 0; i < num_configs; i++) {
> +               enum pin_config_param pin_param =
> +               pinconf_to_config_param(param[i]);
> +
> +               /* first search on default properties */
> +               for (j = 0; j < num_params; j++) {
> +                       if (pin_param == pinconf_dt_params[j].param)
> +                               return pinconf_dt_params[j].property;
> +               }

If this is only for OF, we should rename the function to something including
*of* as well.

I would like it if you manage to break the dependency to OF though.

> +++ b/include/linux/pinctrl/pinconf-generic.h
>
> +static const struct pinconf_generic_params pinconf_dt_params[] = {
> +       { "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
> +       { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
> +       { "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
(...)

Is it really advisible to put big static constant arrays into header
files like this?

This will end up with a copy in each file where it is included and it is not a
small thing that gets replicated all over the place either.

What about just add it as extern and exporting the symbol if you
need it elsewhere?

Yours,
Linus Walleij

Re: [PATCH v2 2/2] pinctrl: generic: improve apply_setting error verbosity

[Matheus Castello]

Hi Linus,

thanks for the tips, I will study this to break dependency from OF.

Best Regards,
Matheus Castello

On 05/02/2018 08:51 AM, Linus Walleij wrote:
> On Tue, May 1, 2018 at 9:10 PM, Matheus Castello
> <matheus@castello.eng.br> wrote:
> 
>> For generic pinconf: print the dev_error with the pinctrl vendor
>> driver name, error code, the sub-node property name used and the
>> pin that was tried to set.
>>
>> Improves the undestading of the error if use a generic sub-node
>> property that generic-pinconf can do parse but the vendor pinctrl
>> driver does not support.
>>
>> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
> 
>> +#include <linux/pinctrl/pinconf-generic.h>
>>   #include "core.h"
>>   #include "pinconf.h"
>>
>> @@ -169,9 +170,21 @@ int pinconf_apply_setting(const struct pinctrl_setting *setting)
>>                                  setting->data.configs.configs,
>>                                  setting->data.configs.num_configs);
>>                  if (ret < 0) {
>> +#ifdef CONFIG_OF
> 
> This doesn't seem right.
> 
> If this is restricted for OF only the root cause to why it is
> like that needs to be found and the code refactored to fit anyone,
> there is also ACPI support in the works I think, surely they should
> be able to get verbose messages.
> 
>>                          dev_err(pctldev->dev,
>> -                               "pin_config_set op failed for pin %d\n",
>> +                               "%s error %d setting %s for pin %d\n",
>> +                               pctldev->desc->name, ret,
>> +                               pinconf_generic_get_param_property_name(
>> +                                       pctldev, setting->data.configs.num_configs,
>> +                                       setting->data.configs.configs),
>>                                  setting->data.configs.group_or_pin);
> 
> This doesn't seem right. First argument is a %d, yet this
> is pctldev->desc->name?
> 
> Something is fishy with the argument list.
> 
> Yours,
> Linus Walleij
>