Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: zx2UunMdyIEFUMMI4Ku5+haOGurduL+VYwGSWUaXkhzX+umGjOEjNJEyAe0jX7akTlxLrxt9WK
 R75V1cunU8f5TXAjcx0SX5R4rNKtXcnLGTyrNyQFD4yvJe6D8rw/5ZEdBxOmJvklRTX3ZhBhYu
 IRX/kGfhHHtMVCqNk9SR8fo8232xlDUPcXM+76bmnGXMIt7kpcL/wYKzbIJF91TgqlITU9CVAD
 +hu7BUUa2TjVksILXNSLpLZ4v6suzh+51w89EkJxAC9VRdXqiDkawHcIuYIxnjuw6qGX8izK+2
 Ep4=
References: <20201008130515.2385825-1-lars.povlsen@microchip.com> <20201008130515.2385825-2-lars.povlsen@microchip.com> <CACRpkdaFYoXFUuWow5s9TitrRDhMW=wiaxgfMcY6sQkYYgC-Lw@mail.gmail.com> <87d01ryb04.fsf@soft-dev15.microsemi.net> <CACRpkdZbxohoxg3bN-XpLXwBUgTDByoP9sNEocUt10R6t9FgXQ@mail.gmail.com>
From:   Lars Povlsen <lars.povlsen@microchip.com>
To:     Linus Walleij <linus.walleij@linaro.org>
CC:     Lars Povlsen <lars.povlsen@microchip.com>,
        Rob Herring <robh+dt@kernel.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v5 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver
In-Reply-To: <CACRpkdZbxohoxg3bN-XpLXwBUgTDByoP9sNEocUt10R6t9FgXQ@mail.gmail.com>
Date:   Tue, 13 Oct 2020 16:39:23 +0200
Message-ID: <877druxk9w.fsf@soft-dev15.microsemi.net>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


Linus Walleij writes:

> On Fri, Oct 9, 2020 at 12:00 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
>
>> > So here reg = 0 and the out port has reg 1. Isn't that what you also put
>> > in the second cell of the GPIO phandle? Then why? The driver
>> > can very well just parse its own reg property and fill that in.
>>
>> NO! The second cell is the second dimension - NOT the direction. As I
>> wrote previously, the direction is now inherent from the handle, ie. the
>> "reg" value of the handle.
>
> OK I get it ... I think :)

Great!

>
>> The hardware describe a "port" and a "bit index" addressing, where the
>> second cell in
>>
>>   gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;
>>
>> is the "bit index" - not the "reg" from the phandle.
>
> As long as the bindings specify exactly what is meant by bit index
> and the tupe (port, bit_index) is what uniquely addresses a certain
> GPIO line then it is fine I suppose.
>

Yes, that is confirmed.


>> In the example above, note
>>
>>   ngpios = <96>;
>>
>> As the "port" is [0; 31], this defines "bit index" to be [0; 2], so the
>> (input) GPIO cells will be:
>>
>> p0b0, p0b1, p0b2
>> ...
>> p31b0, p31b1, p31b2
>>
>> being identical to
>>
>> <&sgpio_inX 0 0 GPIO_OUT_LOW>
>> <&sgpio_inX 0 1 GPIO_OUT_LOW>
>> <&sgpio_inX 0 2 GPIO_OUT_LOW>
>> ...
>> <&sgpio_inX 31 0 GPIO_OUT_LOW>
>> <&sgpio_inX 31 1 GPIO_OUT_LOW>
>> <&sgpio_inX 31 2 GPIO_OUT_LOW>
>>
>> ('X' being the SGPIO controller instance).
>
> So 32 possible ports with 3 possible bit indexes on each?
> This constraint should go into the bindings as well so it becomes
> impossible to put in illegal port numbers or bit indices.
>
> (Use the YAML min/max constraints, I suppose?)
>

Yes, I will to see if constraints in the GPIO args is possible.

>> So no, there *really* is a need for a 3-cell GPIO specifier (or whatever
>> its called).
>
> If that is the natural way to address the hardware lines
> and what is used in the documentation then it's fine, it's just so
> unorthodox that I have to push back on it a bit you know.
>

Yes, this piece of hw is certainly not a stock GPIO controller, so that
was kinda expected. But I think we ended up with an abstraction that
fits as good as possible.

I will send a new (last?) revision that includes the suggestions from
Rob tomorrow.

Thank you for your time and comments (also Rob!)

---Lars

-- 
Lars Povlsen,
Microchip