On 12/13/2012 06:37 AM, Linus Walleij wrote:
> This implements pin multiplexing and pin configuration for the
> Nomadik pin controller using the device tree.
> diff --git a/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt
> b/Documentation/devicetree/bindings/pinctrl/ste,nomadik.txt
> +Required properties:
> +- compatible: "stericsson,nmk_pinctrl"
Minor nit: Is it more common to use - rather than _ in compatible values?
> +ST Ericsson's pin configuration nodes act as a container for an abitrary
> number of
Typo in "arbitrary" above.
> +subnodes. Each of these subnodes represents some desired configuration for a
> +pin, a group, or a list of pins or groups. This configuration can include the
> +mux function to select on those pin(s)/group(s), and various pin
> configuration
> +parameters, such as inputn output, pull up, pull down...
Typo in "input" above.
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content.
> +
> +Required subnode-properties:
> +- ste,pins : An array of strings. Each string contains the name of a pin or
> + group.
The vendor prefix here is ste, but it's stericsson in the compatible
value above. They should be consistent. ste is nice since it's shorter,
but I see stericsson in
Documentation/devicetree/bindings/vendor-prefixes.txt... I wonder if you
could register ste there too?
> +Optional subnode-properties:
> +- ste,function: A string containing the name of the function to mux to the
> + pin or group.
> +
> +- ste,input: no parameter, set pin in input with no pull
> mode.
> +- ste,input_pull_up: no parameter, set pin in input with pull up
> mode.
> +- ste,input_pull_down: no parameter, set pin in input with
> pull down mode.
DT property names should definitely use - not _ as a delimiter.
> +- ste,sleep_input: no parameter, set pin in sleep input with no
> pull mode.
> +- ste,sleep_input_pull_up: no parameter, set pin in sleep input with pull
> up mode.
> +- ste,sleep_input_pull_down: no parameter, set pin in sleep input with pull
> down mode.
Would ste,sleep-input = <0/1/2> be better? Not really a big deal either way.
> +- ste,sleep_pdis_mode: integer, 0: pdis disabled, 1: pdis
> enable.
Should the binding document mention what "pdis" is. It's probably not
necessary so long as "pdis" is something you can search for in the HW
documentation.
> +Valid values for pin and group name are in
> Drivers/pinctrl/pinctrl-nomadik-db8500.c
Hmm. That's rather tying the DT binding documentation to Linux code, and
DT binding docs should be OS-agnostic. Are the names identical to the HW
documentation? If so, perhaps you could reference that instead of the
Linux driver.
> diff --git a/drivers/pinctrl/pinctrl-nomadik.c
> b/drivers/pinctrl/pinctrl-nomadik.c
> +static int nmk_dt_reserve_map(struct pinctrl_map **map, unsigned
> *reserved_maps,
> +static int nmk_dt_add_map_mux(struct pinctrl_map **map, unsigned
> *reserved_maps,
> +static int nmk_dt_add_map_configs(struct pinctrl_map **map,
Those (and hence perhaps code that calls it?) is cut/paste from
pinctrl-tegra.c. Is it worth lifting the functions out into some common
code to share it?
> +static const char * nmk_find_pin_name(struct pinctrl_dev *pctldev, const
> char *pin_name)
> + if (sscanf((char *) pin_name, "GPIO%d",&pin_number) == 1)
> + for(i = 0; i < npct->soc->npins; i++)
There are some white-space issues there:
* Shouldn't be a space after the cast in sscanf() call.
* Should be a space after comma in sscanf() call.
* Should be a space after "for".
> +int nmk_pinctrl_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> + for (i = 0; i < ARRAY_SIZE(nmk_cfg_params); i++) {
> + unsigned long cfg = 0;
> + int val;
> +
> + ret = of_property_read_u32(np, nmk_cfg_params[i].property,
> &val);
A lot of those are Booleans not U32s...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
