Re: [PATCH V2 3/3] pinctrl: palmas: add pincontrol driver

From: Stephen Warren
Date: Mon Aug 05 2013 - 18:04:33 EST

On 08/05/2013 01:33 AM, Laxman Dewangan wrote:
> TI Palmas series Power Management IC have multiple pins which can be
> configured for different functionality. This pins can be configured
> for different function. Also their properties like pull up/down,
> open drain enable/disable are configurable.
> Add support for pincontrol driver Palmas series device like TPS65913,
> TPS80036. The driver supports to be register from DT only.

> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-palmas.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-palmas.txt

> +Required subnode-properties:
> +- pinctrl-pins: An array of strings. Each string contains the name of a pin.
> + Valid values for these names are listed below.

s/pinctrl-pins/pins/ I think.

> +Optional subnode-properties:
> +- pinctrl-function: string containing the name of the function to mux to the
> + pin. Valid values for function names are listed below. Please refer the
> + datasheet of the device to determine which are valid for each pin.

s/pinctrl-function/function/ I think.

> +The properties "pinctrl-pins" and "pinctrl-function" ara generic pincontrol
> +properties and described in pinctrl-bindings.txt.
> +
> +Following are other valid propeties for Palams pinctrol dt node which are
> +described in pinctrl-bindings.txt.
> +bias-disable, bias-pull-up, bias-pull-down, bias-pull-pin-default
> +drive-open-drain: Value will represent the open drain to be enable/disable
> + of pins.
> + 0: Disable, 1: Enable.

I'd be tempted to replace everything I quoted above with something a
little simpler to avoid any redundancy with pinctrl-bindings.txt:

This binding uses the following generic properties as defined in

Required: pins
Options: function, bias-disable, bias-pull-up, bias-pull-down,
bias-pin-default, drive-open-drain.

> +Note that many of these properties are only valid for certain specific pins.
> +See the Palmas device datasheet for complete details regarding which pins
> +support which functionality.
> +
> +Valid values for pin names are:
> +pins: gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7, gpio8, gpio9,

"pins:" is redundant here.

> + gpio10, gpio11, gpio12, gpio13, gpio14, gpio15, vac, powergood,
> + nreswarm, pwrdown, gpadc_start, reset_in, nsleep, enable1, enable2,
> + int.
> +
> +function: gpio, led, pwm, regen, sysen, clk32kgaudio, id, vbus_det, chrg_det,

I think you would want to structure this list the same as the pins list,
so prefix it with "Valid values for function names are:", and delete
"function:" at the start of the list.

> +There is 4 special function: opt0, opt1, opt2 and opt3. If this functions are

s/this/any of these/

> +selected then directly pins register will be written with 0, 1, 2 and 3


> +repectively if it is valid for that pins.


Hmm. I'm not sure I like mixing those functions with the more "semantic"
function names above. I'd prefer the driver support either the semantic
names or the raw names, but not both. It's not really a big deal though
I suppose.

I didn't review the code, just the binding.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at