Re: [PATCH 1/2] pinctrl: add function to parse generic pinconfig properties from a dt node

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Heiko,

Thank you for the patch. I've tested it on an sh73a0 KZM9G board with the sh-
pfc driver and it seems to work fine. Please see the code below for comments.

On Monday 10 June 2013 21:40:29 Heiko Stübner wrote:
> pinconf_generic_parse_dt_config() takes a node as input and generates an
> array of generic pinconfig values from the properties of this node.
> 
> As I couldn't find a mechanism to count the number of properties of a node
> the function uses internally an array to accept one of parameter and copies
> the real present options to a smaller variable at its end.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          |   38 +++++++++
>  drivers/pinctrl/pinconf-generic.c                  |   81 +++++++++++++++++
>  drivers/pinctrl/pinconf.h                          |    6 ++
>  3 files changed, 125 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index
> c95ea82..ef7cd57 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -126,3 +126,41 @@ device; they may be grandchildren, for example. Whether
> this is legal, and whether there is any interaction between the child and
> intermediate parent nodes, is again defined entirely by the binding for the
> individual pin controller device.
> +
> +== Using generic pinconfig options ==
> +
> +Generic pinconfig parameters can be used by defining a separate node
> containing +the applicable parameters (and optional values), like:
> +
> +pcfg_pull_up: pcfg_pull_up {
> +	bias-pull-up;
> +	drive-strength = <20>;
> +};
> +
> +This node should then be referenced in the appropriate pinctrl node as a
> phandle +and parsed in the driver using the pinconf_generic_parse_dt_config
> function. +
> +Supported configuration parameters are:
> +
> +bias-disable		- disable any pin bias
> +bias-high-impedance	- high impedance mode ("third-state", "floating")
> +bias-bus-hold		- latch weakly
> +bias-pull-up		- pull up the pin
> +bias-pull-down		- pull down the pin
> +bias-pull-pin-default	- use pin-default pull state
> +drive-push-pull		- drive actively high and low
> +drive-open-drain	- drive with open drain
> +drive-open-source	- drive with open source
> +drive-strength		- sink or source at most X mA
> +input-schmitt-enable	- enable schmitt-trigger mode
> +input-schmitt-disable	- disable schmitt-trigger mode
> +input-schmitt		- run in schmitt-trigger mode with hysteresis X
> +input-debounce		- debounce mode with debound time X
> +power-source		- select power source X
> +slew-rate		- use slew-rate X
> +low-power-mode		- low power mode
> +output-low		- set the pin to output mode with low level
> +output-high		- set the pin to output mode with high level
> +
> +More in-depth documentation on these parameters can be found in
> +<include/linux/pinctrl/pinconfig-generic.h>
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c index 9a6812b..3610e7b 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -21,6 +21,7 @@
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/of.h>
>  #include "core.h"
>  #include "pinconf.h"
> 
> @@ -139,3 +140,83 @@ void pinconf_generic_dump_config(struct pinctrl_dev
> *pctldev, }
>  EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
>  #endif
> +
> +#ifdef CONFIG_OF
> +struct pinconf_generic_dt_params {
> +	const char * const property;
> +	enum pin_config_param param;
> +	u32 default_value;
> +};
> +
> +static struct pinconf_generic_dt_params dt_params[] = {
> +	{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
> +	{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
> +	{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
> +	{ "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 0 },
> +	{ "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 0 },
> +	{ "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 0 },
> +	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
> +	{ "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
> +	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
> +	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
> +	{ "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
> +	{ "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 },
> +	{ "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 },
> +	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
> +	{ "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
> +	{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
> +	{ "low-power-mode", PIN_CONFIG_LOW_POWER_MODE, 0 },
> +	{ "output-low", PIN_CONFIG_OUTPUT, 0, },
> +	{ "output-high", PIN_CONFIG_OUTPUT, 1, },
> +};
> +
> +/**
> + * pinconf_generic_parse_dt_config()
> + * parse the config properties into generic pinconfig values.
> + * @np: node containing the pinconfig properties
> + * @configs: array with nconfigs entries containing the generic pinconf
> values + * @nconfigs: umber of configurations
> + */
> +int pinconf_generic_parse_dt_config(struct device_node *np,
> +				    unsigned long **configs,
> +				    unsigned int *nconfigs)
> +{
> +	unsigned long cfg[ARRAY_SIZE(dt_params)];

I'm a bit uneasy about allocating large arrays on the stack. Would it be 
better to dynamically allocate cfg ? I've used kzrealloc in my implementation 
to grow the config array every time a config was found, but that might not be 
the most efficient implementation, although I wonder how many configuration 
options we will see in practice in a single node.

> +	unsigned int ncfg = 0;
> +	int ret;
> +	int i;
> +	u32 val;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(dt_params); i++) {
> +		struct pinconf_generic_dt_params *par = &dt_params[i];
> +		ret = of_property_read_u32(np, par->property, &val);
> +
> +		/* property not found */
> +		if (ret == -EINVAL)
> +			continue;
> +
> +		/* use default value, when no value is specified */
> +		if (ret)
> +			val = par->default_value;
> +
> +		pr_debug("found %s with value %u\n", par->property, val);
> +		cfg[ncfg] = pinconf_to_config_packed(par->param, val);
> +		ncfg++;
> +	}

You could add

	if (ncfg == 0) {
		*configs = NULL;
		*nconfigs = 0;
		return 0;
	}

here.

Most of the issues I wanted to raise have already been addressed by comments 
sent to the list. Do you plan to send a v2 in the near future ?

> +
> +	/*
> +	 * Now limit the number of configs to the real number of
> +	 * found properties.
> +	 */
> +	*configs = kzalloc(ncfg * sizeof(unsigned long), GFP_KERNEL);
> +	if (!*configs)
> +		return -ENOMEM;
> +
> +	memcpy(*configs, &cfg, ncfg * sizeof(unsigned long));
> +	*nconfigs = ncfg;
> +	return 0;
> +}
> +#endif
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> index 92c7267..a4a5417 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -123,3 +123,9 @@ static inline void pinconf_generic_dump_config(struct
> pinctrl_dev *pctldev, return;
>  }
>  #endif
> +
> +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF)
> +int pinconf_generic_parse_dt_config(struct device_node *np,
> +				    unsigned long **configs,
> +				    unsigned int *nconfigs);
> +#endif
-- 
Regards,

Laurent Pinchart