| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2477455.I6AxjaGPuJ@avalon>
Date: Fri, 14 Jun 2013 02:27:01 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: linux-arm-kernel@...ts.infradead.org
Cc: Heiko Stübner <heiko@...ech.de>,
Linus Walleij <linus.walleij@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Patrice Chotard <patrice.chotard.st@...il.com>
Subject: Re: [PATCH 1/2] pinctrl: add function to parse generic pinconfig properties from a dt node
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@...ech.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists