Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

[Dong Aisheng]

Hi Linus & j,

On Mon, May 29, 2017 at 6:42 PM, jmondi <jacopo@xxxxxxxxxx> wrote:
> Hi Linus,
>
> On Mon, May 29, 2017 at 10:45:44AM +0200, Linus Walleij wrote:
>> On Tue, May 23, 2017 at 8:37 PM, jmondi <jacopo@xxxxxxxxxx> wrote:
>>
>> >> I did not follow too much.
>> >> But it seems IMX7ULP/Vybrid to be also a fan of generic
>> >> output-enable/input-enable
>> >> property.
>> >>
>> >> See:
>> >> Figure 5-2. GPIO PAD in Page 241
>> >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf
>> >>
>> >> It has separate register bits to control input buffer enable and
>> >> output buffer enable
>> >> and we need set it property for GPIO function.
>> >
>> > As it seems we have another user for 'output-enable' here, what if we just
>> > add that one to the generic bindings properties list, and we keep
>> > 'bi-directional' (which seems to be the most debated property we have
>> > added) out of generic properties?
>> >
>> > We can handle 'bi-directional' pins with static tables in our pin
>> > controller driver and not have it anywhere in DT.
>>
>> This sounds like a viable approach.
>>
>> I just want to know if "output-enable" is the right name?
>> "output-buffer-enable"?
>
> Great! Thanks!
>
> On naming: if we need "output-buffer-enable" should we add
> "input-buffer-enable" as well?
>
> Currently we are using "input-enable" to pair with "output-enable",
> but as you said, just "output-enable" when "output-high" and
> "output-low" are there already seems a bit confusing.
> At the same time "input-buffer-enable" seems to actually be just
> electrically equivalent to "input-enable", so adding it is a bit of a
> waste as well.
>
> I see three options here:
>
> 1) Add "output-buffer-enable" and "input-buffer-enable"
> we end up with
> "output-high"
> "output-low"
> "input-enable"
> "output-buffer-enable"
> "input-buffer-enable"
>
> 2) Add "output-buffer-enable" only
> we end up with
> "output-high"
> "output-low"
> "input-enable"
> "output-buffer-enable"
>
> Binding may be confusing as in one case we use "output-buffer-enable"
> while in the other "input-enable"
>
> 3) Add "output-enable" only
> "output-high"
> "output-low"
> "input-enable"
> "output-enable"
>
> As you, I don't like "output-enable" that much but it pairs better with
> "input-enable".
>
> I'll let you and DT people decide on this, as it's really an ABI definition
> problem and you have better judgment there.
>

What's the final decision of this?

I saw the following revert patch in pinctrl-next but did not see a successive
patch to add output-enable back?

IMX7ULP pinctrl driver is pending on this because it needs use both
input-enable and output-enable if we want to make them generic property.

commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b
Author: Linus Walleij <linus.walleij@xxxxxxxxxx>
Date:   Mon May 8 10:48:21 2017 +0200

    Revert "pinctrl: generic: Add bi-directional and output-enable"

    This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0.

    It turns out that applying these generic properties was
    premature: the properties used in the driver using this
    are of unclear electrical nature and the subject need to
    be discussed.

    Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

Regards
Dong Aisheng

>>
>> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked
>> > in some previous email.
>>
>> I'm just overloaded. I sent that revert to Torvalds today.
>
> Thank you. Didn't want to put pressure ;)
>>
>> > I can send another version of that patch with
>> > only 'output-enable' if you wish.
>>
>> That's what we want.
>>
>> > Once we reach consesus, I can then send v6 of our pin controller driver
>> > based on that.
>>
>> OK sounds like a plan.
>>
>> Sorry for the mess, I'm just trying to get this right :/
>
> Not a mess, and thanks for your effort in maintaining  all of this
>
> Thanks
>    j
>>
>> Yours,
>> Linus Walleij