[3/3] pinctrl: Add documentation for pinctrl-msm8x74

This adds initial documentation for the pinctrl-msm8x74 driver.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 .../bindings/pinctrl/qcom,msm8x74-pinctrl.txt      | 86 ++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,msm8x74-pinctrl.txt

On 11/23/2013 04:38 PM, Bjorn Andersson wrote:
> This adds initial documentation for the pinctrl-msm8x74 driver.

> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8x74-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8x74-pinctrl.txt

> +Qualcomm MSM8x74 TLMM block
> +
> +Required properties:
> +- compatible: "qcom,msm8x74-pinctrl"
> +- reg: Should be the base address of the TLMM block.

base address *and length*?

> +- interrupts: Should be the irq of the TLMM summary interrupt.

s/irq/IRQ/

> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- #interrupt-cells: Should be two.
> +- gpio-controller: Marks the device node as a GPIO controller.
> +- #gpio-cells : Should be two.
> +                The first cell is the gpio pin number and the
> +                second cell is used for optional parameters.
> +
> +Please refer to ../gpio/gpio.txt for a general description of GPIO bindings.

It's probably worth linking to ../interrupt-controller/interrupts.txt too.

> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Each subnode describes properties for the given pins.

Which subnodes? It's probably worth describing what subnodes can exist.
Perhaps try cribbing e.g. the 3 paragraphs in the Tegra binding that
appear right after the "Please refer to pinctrl-bindings.txt..." that's
there.

> +Required subnode-properties:
> +- qcom,pins: An array of strings, each matching a pin or group to be
> +             configured. Possible values are listed below.

It isn't clear from this that these properties exist in a sub-sub-node,
not just a sub-node, of the main pinctrl node.

> +Optional subnode-properties:
> +- qcom,function: A name of the function to be muxed to the specified
> +                 pins. Possible values are listed below.

Are "pins" and "functions" generic enough now that they don't need a
vendor prefix? Both those property names are certainly mentioned in
pinctrl-bindings.txt.

> +- drive-strength: Configure the drive strength of the specified pins.

What units? I assume the definition matches that in
pinctrl-bindings.txt? Perhaps it'd be simpler to say "The following
generic properties as defined in pinctrl-bindings.txt: pins, function,
bias-disable, ...".

> +Valid values for qcom,function are:
> +  blsp_i2c2, blsp_i2c6, blsp_i2c11, blsp_spi1, blsp_uart2, blsp_uart8, slimbus
> +
> +  (Note that this is not yet the complete list of functions)

It'd be best if the complete list were defined from the start. That
said, I suppose adding new values into the list would be
backwards-compatible (if not forwards-compatible i.e. new DTs usable
with old kernels).

> +Example:
> +
> +	msmgpio: pinctrl@fd510000 {
...
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&uart2_default>;
> +
> +		uart2 {

What does that node represent? Other pinctrl bindings  only have 1 or 2
levels of subnodes, not 3.

> +			uart2_default: uart2_default {
> +				mux {
> +					qcom,pins = "gpio4", "gpio5";
> +					qcom,function = "blsp_uart2";
> +				};


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 11/25/13 4:14 PM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:

>On 11/23/2013 04:38 PM, Bjorn Andersson wrote:
>> This adds initial documentation for the pinctrl-msm8x74 driver.
>
>> diff --git 
>>a/Documentation/devicetree/bindings/pinctrl/qcom,msm8x74-pinctrl.txt
>>b/Documentation/devicetree/bindings/pinctrl/qcom,msm8x74-pinctrl.txt
>
>> +Qualcomm MSM8x74 TLMM block
>> +
>> +Required properties:
>> +- compatible: "qcom,msm8x74-pinctrl"
>> +- reg: Should be the base address of the TLMM block.
>
>base address *and length*?

Of course

>
>> +- interrupts: Should be the irq of the TLMM summary interrupt.
>
>s/irq/IRQ/

OK

>
>> +- interrupt-controller: Marks the device node as an interrupt
>>controller.
>> +- #interrupt-cells: Should be two.
>> +- gpio-controller: Marks the device node as a GPIO controller.
>> +- #gpio-cells : Should be two.
>> +                The first cell is the gpio pin number and the
>> +                second cell is used for optional parameters.
>> +
>> +Please refer to ../gpio/gpio.txt for a general description of GPIO
>>bindings.
>
>It's probably worth linking to ../interrupt-controller/interrupts.txt too.

Will do.

>
>> +Please refer to pinctrl-bindings.txt in this directory for details of
>>the
>> +common pinctrl bindings used by client devices, including the meaning
>>of the
>> +phrase "pin configuration node".
>> +
>> +Each subnode describes properties for the given pins.
>
>Which subnodes? It's probably worth describing what subnodes can exist.
>Perhaps try cribbing e.g. the 3 paragraphs in the Tegra binding that
>appear right after the "Please refer to pinctrl-bindings.txt..." that's
>there.

I see that most other documents have a variation of those paragraphs, so
I better follow that then.

>
>> +Required subnode-properties:
>> +- qcom,pins: An array of strings, each matching a pin or group to be
>> +             configured. Possible values are listed below.
>
>It isn't clear from this that these properties exist in a sub-sub-node,
>not just a sub-node, of the main pinctrl node.
>
>> +Optional subnode-properties:
>> +- qcom,function: A name of the function to be muxed to the specified
>> +                 pins. Possible values are listed below.
>
>Are "pins" and "functions" generic enough now that they don't need a
>vendor prefix? Both those property names are certainly mentioned in
>pinctrl-bindings.txt.

I've missed that these where in pinconf-generic. I based
msm_dt_subnode_to_map
on the tegra implementation and made changes to support pinconf-generic,
which
now seems identical with pinconf_generic_dt_subnode_to_map.
I'll use that directly instead.

>
>> +- drive-strength: Configure the drive strength of the specified pins.
>
>What units? I assume the definition matches that in
>pinctrl-bindings.txt? Perhaps it'd be simpler to say "The following
>generic properties as defined in pinctrl-bindings.txt: pins, function,
>bias-disable, ...".

Sounds much better than duplicating that information here.

>
>> +Valid values for qcom,function are:
>> +  blsp_i2c2, blsp_i2c6, blsp_i2c11, blsp_spi1, blsp_uart2, blsp_uart8,
>>slimbus
>> +
>> +  (Note that this is not yet the complete list of functions)
>
>It'd be best if the complete list were defined from the start. That
>said, I suppose adding new values into the list would be
>backwards-compatible (if not forwards-compatible i.e. new DTs usable
>with old kernels).

This list is enough for the currently supported (and published on
linux-arm-msm)
drivers for the 8x74 based DragonBoard. I wanted to get some feedback
before adding
more of the functions in the msm8x74 tables.

>
>> +Example:
>> +
>> +	msmgpio: pinctrl@fd510000 {
>...
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&uart2_default>;
>> +
>> +		uart2 {
>
>What does that node represent? Other pinctrl bindings  only have 1 or 2
>levels of subnodes, not 3.

I liked the concept of grouping related states/muxes together and then I
saw Linus'
ux500 series that did the same thing. But as it's just a matter of taste I
should
probably remove it from the example and just make sure it's clear above
that such
structure is okay.

>
>> +			uart2_default: uart2_default {
>> +				mux {
>> +					qcom,pins = "gpio4", "gpio5";
>> +					qcom,function = "blsp_uart2";
>> +				};

Thanks for your comments!

Regards,
Bjorn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 26, 2013 at 1:52 AM, "Andersson, Björn"
<Bjorn.Andersson@sonymobile.com> wrote:
> On 11/25/13 4:14 PM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:

>>> +Optional subnode-properties:
>>> +- qcom,function: A name of the function to be muxed to the specified
>>> +                 pins. Possible values are listed below.
>>
>>Are "pins" and "functions" generic enough now that they don't need a
>>vendor prefix? Both those property names are certainly mentioned in
>>pinctrl-bindings.txt.
>
> I've missed that these where in pinconf-generic. I based
> msm_dt_subnode_to_map
> on the tegra implementation and made changes to support pinconf-generic,
> which
> now seems identical with pinconf_generic_dt_subnode_to_map.
> I'll use that directly instead.

OK that's essentially one of my comments on patch 1/3 addressed,
so looking forward to see v2.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html