lkml.org 
[lkml]   [2014]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 2/3] dt-bindings: Add pinctrl bindings for mt65xx/mt81xx.
    From
    On Thu, Nov 27, 2014 at 11:18 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
    > On Thu, Nov 27, 2014 at 09:44:42AM +0100, Linus Walleij wrote:
    >> On Tue, Nov 11, 2014 at 1:38 PM, Hongzhou Yang
    >> <hongzhou.yang@mediatek.com> wrote:

    >> > +- mediatek,pins: 2 integers array, represents gpio pinmux number and config
    >> > + setting. The format as following
    >> > +
    >> > + node {
    >> > + mediatek,pins = <PIN_NUMBER_PINMUX>;
    >> > + GENERIC_PINCONFIG;
    >> > + };
    >>
    >> As suggested by Sacha, use just "pins" and define the binding as a patch
    >> to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
    >> that is generic for multiplexing, so we get some order here.
    >>
    >> I want you however to put pin multiplexing and pin configuration into
    >> different nodes if possible. I don't like combines muxing and config
    >> nodes. If necessary tag the node with something.
    >
    > Why? I think the properties can live happily together, even when the
    > parsing functions go to the pinctrl core.

    I'm worried about the semantic ambiguity between the pins = <...>;
    property on different pin controllers, whether they are based on
    function+group or per-pin. It's not even up to me to decide,
    this is for the DT binding people.

    In this case:

    pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
    <MT8135_PIN_101_SCL0__FUNC_SCL0>;

    Each element is a 32bit unsigned where the lower and higher
    16 bits have different meanings.

    In some other pin controller (using generic bindings and
    already merged! arch/arm/boot/dts/ste-href-ab8500.dtsi):

    gpio39 {
    gpio39_default_mode: gpio39_default {
    default_mux {
    function = "gpio";
    groups = "gpio39_a_1";
    };
    default_cfg {
    pins = "GPIO39_E16";
    input-enable;
    bias-pull-down;
    };
    };
    };

    Can we get away with using the same core parser with this
    as well, here the nodes are split and using strings to identify
    pins, not 32bit numbers.

    I am worried about semantic coexistance...

    >> > + i2c0_pins_a: i2c0@0 {
    >> > + pins1 {
    >> > + mediatek,pins = <MT8135_PIN_100_SDA0__FUNC_SDA0>,
    >> > + <MT8135_PIN_101_SCL0__FUNC_SCL0>;
    >> > + bias-disable;
    >> > + };
    >> > + };
    >>
    >> I would split it up.
    >>
    >> i2c0_pins_a: i2c0@0 {
    >> pins1 {
    >> pins = <MT8135_PIN_100_SDA0>;
    >> function = <MT8135_PIN_100_FUNC_SDA0>;
    >> };
    >
    > The reason to put this in a single define was to make writing the device
    > trees less error prone. When you have two arrays you must correlate the
    > entries.

    I see the upside. I'm just worried about ambiguity when comparing
    different device trees to each other, because they should be about
    readability and understanding, not confusing...

    >> One node for the multiplexing, one node for the config. This is the
    >> pattern used by most drivers, so I want to have this structure.
    >>
    >> It is also easy to tell one node from the other: if it contains "function"
    >> we know it's a multiplexing node, if it doesn't, it's a config node.
    >
    > So when merging these together a node is always multiplexing and
    > configuration. But do we really win anything if both are separated? When
    > both are separated you still need an array of pins in the nodes to
    > tell which pins this node is for. If this array also contains the
    > mux information then this won't hurt. You can still ignore it.

    Well we definately have to support the case with split config and
    muxing nodes at least. I am very worried that we would get into
    ambguities where that is not possible.

    Yours,
    Linus Walleij


    \
     
     \ /
      Last update: 2014-11-28 17:41    [W:3.459 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site