Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662015 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Dong Aisheng <dongas86@gmail.com> - 2017-06-09 09:30 +0200
    Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-06-09 10:00 +0200
      Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-06-11 23:50 +0200
        Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-06-12 11:50 +0200
          Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Dong Aisheng <dongas86@gmail.com> - 2017-06-13 08:30 +0200
            Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-06-15 13:20 +0200
              Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Dong Aisheng <dongas86@gmail.com> - 2017-06-19 17:50 +0200

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

Hi Linus & j,

On Mon, May 29, 2017 at 6:42 PM, jmondi <jacopo@jmondi.org> 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@jmondi.org> 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@linaro.org>
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@linaro.org>

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

[toc] | [next] | [standalone]

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

Hi Dong,

On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
> Hi Linus & j,
>
> >>
> >> 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 admit a was buying a bit of time and post-poned the gentle ping for
any final word on this. But since you're asking I'll second your
question :)

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

Still waiting to have a feedback on which properties to add, that's
why I have not sent anything yet.

Thanks
   j

> 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@linaro.org>
> 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@linaro.org>
>
> 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

[toc] | [prev] | [next] | [standalone]

#1663157

On Fri, Jun 9, 2017 at 9:50 AM, jmondi <jacopo@jmondi.org> wrote:
> On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:

>> > 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 admit a was buying a bit of time and post-poned the gentle ping for
> any final word on this. But since you're asking I'll second your
> question :)

I suspect it is time to quote
Documentation/process/management-style.rst
(Torvalds):

1) Decisions

Everybody thinks managers make decisions, and that decision-making is
important.  The bigger and more painful the decision, the bigger the
manager must be to make it.  That's very deep and obvious, but it's not
actually true.

The name of the game is to **avoid** having to make a decision.  In
particular, if somebody tells you "choose (a) or (b), we really need you
to decide on this", you're in trouble as a manager.  The people you
manage had better know the details better than you, so if they come to
you for a technical decision, you're screwed.  You're clearly not
competent to make that decision for them.

(It goes on, it's the best part of the entire Documentation/* dir in my
opinion, please take the time to read it in full.)

So: what do you guys, using this feature, and Andy, who raised serious
concerns, think is the right binding? That is what *I* need to know.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

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

Hi Linus,

On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote:
> On Fri, Jun 9, 2017 at 9:50 AM, jmondi <jacopo@jmondi.org> wrote:
> > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
>
> >> > 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 admit a was buying a bit of time and post-poned the gentle ping for
> > any final word on this. But since you're asking I'll second your
> > question :)
>
> I suspect it is time to quote
> Documentation/process/management-style.rst
> (Torvalds):
>
> 1) Decisions
>
> Everybody thinks managers make decisions, and that decision-making is
> important.  The bigger and more painful the decision, the bigger the
> manager must be to make it.  That's very deep and obvious, but it's not
> actually true.
>
> The name of the game is to **avoid** having to make a decision.  In
> particular, if somebody tells you "choose (a) or (b), we really need you
> to decide on this", you're in trouble as a manager.  The people you
> manage had better know the details better than you, so if they come to
> you for a technical decision, you're screwed.  You're clearly not
> competent to make that decision for them.
>
> (It goes on, it's the best part of the entire Documentation/* dir in my
> opinion, please take the time to read it in full.)
>
> So: what do you guys, using this feature, and Andy, who raised serious
> concerns, think is the right binding? That is what *I* need to know.

Fair enough :)

I'll try to keep this short: I don't like "output-enable", and at the
same time I don't think "output-high" and "output-low" fit well for
this purpose, as they electrically means something different from what
our (and IMX) use case is: enabling/disabling input/output
buffers internal to pin controller/gpio block HW and not driving a value
there.

This seems clear to me from the "GPIO mode pitfalls" section of
pinctrl.txt documentation examples and from the fact that generic bindings
did not expose an "output" flag because if you drive an output line, you
reasonably either drive it high or low.

Unfortunately I cannot convince myself that the same reasons apply
to the input use case.  Enabling input on a pin implies the pinctrl/gpio driver
has to enable any input buffer required to use that pin as a properly
working input line, and enabling an input buffer implies being able to sense
the line value from there, so I don't see that much use for "input-buffer-enable"
alone.

So, even if bindings could look a bit weird as there won't be a direct
matching between properties names used to enable input/output buffers,
my vote is to add "output-buffer-enable" only, and keep using the
already there "input-enable" properties for the input use case.

Thanks
   j

>
> Yours,
> Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1664489

On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
> Hi Linus,
>
> On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote:
>> On Fri, Jun 9, 2017 at 9:50 AM, jmondi <jacopo@jmondi.org> wrote:
>> > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote:
>>
>> >> > 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 admit a was buying a bit of time and post-poned the gentle ping for
>> > any final word on this. But since you're asking I'll second your
>> > question :)
>>
>> I suspect it is time to quote
>> Documentation/process/management-style.rst
>> (Torvalds):
>>
>> 1) Decisions
>>
>> Everybody thinks managers make decisions, and that decision-making is
>> important.  The bigger and more painful the decision, the bigger the
>> manager must be to make it.  That's very deep and obvious, but it's not
>> actually true.
>>
>> The name of the game is to **avoid** having to make a decision.  In
>> particular, if somebody tells you "choose (a) or (b), we really need you
>> to decide on this", you're in trouble as a manager.  The people you
>> manage had better know the details better than you, so if they come to
>> you for a technical decision, you're screwed.  You're clearly not
>> competent to make that decision for them.
>>
>> (It goes on, it's the best part of the entire Documentation/* dir in my
>> opinion, please take the time to read it in full.)
>>
>> So: what do you guys, using this feature, and Andy, who raised serious
>> concerns, think is the right binding? That is what *I* need to know.
>
> Fair enough :)
>
> I'll try to keep this short: I don't like "output-enable", and at the
> same time I don't think "output-high" and "output-low" fit well for
> this purpose, as they electrically means something different from what
> our (and IMX) use case is: enabling/disabling input/output
> buffers internal to pin controller/gpio block HW and not driving a value
> there.
>
> This seems clear to me from the "GPIO mode pitfalls" section of
> pinctrl.txt documentation examples and from the fact that generic bindings
> did not expose an "output" flag because if you drive an output line, you
> reasonably either drive it high or low.
>
> Unfortunately I cannot convince myself that the same reasons apply
> to the input use case.  Enabling input on a pin implies the pinctrl/gpio driver
> has to enable any input buffer required to use that pin as a properly
> working input line, and enabling an input buffer implies being able to sense
> the line value from there, so I don't see that much use for "input-buffer-enable"
> alone.
>
> So, even if bindings could look a bit weird as there won't be a direct
> matching between properties names used to enable input/output buffers,
> my vote is to add "output-buffer-enable" only, and keep using the
> already there "input-enable" properties for the input use case.
>

Yes, it may be a bit weird.
I'm not pad internal details expert and can't tell much difference between
output-enable and output-buffer-enable.
I just feel a bit confuse if only using output-buffer-enable.

If enable both input and output, it becomes:
pinctrl_xxx: gpios_xxx_grp {
        pins = <
                ULP1_PAD_PTD0__PTD0
        >;
        input-enable;
        output-buffer-enable;
        bias-pull-up;
};

How about still use output-enable in pairs to input-enable but explain more
in comments?
Aslo update 'input-enable' comment to 'enable input buffer'.
e.g.
diff --git a/drivers/pinctrl/pinconf-generic.c
b/drivers/pinctrl/pinconf-generic.c
index 720a19f..96c83a4 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -172,6 +172,7 @@ static const struct pinconf_generic_params dt_params[] = {
        { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
        { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
        { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
+       { "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
        { "output-high", PIN_CONFIG_OUTPUT, 1, },
        { "output-low", PIN_CONFIG_OUTPUT, 0, },
        { "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
diff --git a/include/linux/pinctrl/pinconf-generic.h
b/include/linux/pinctrl/pinconf-generic.h
index 7620eb1..d30f4fe 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -59,9 +59,9 @@
  *     which means it will wait for signals to settle when reading inputs. The
  *     argument gives the debounce time in usecs. Setting the
  *     argument to zero turns debouncing off.
- * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does not
- *     affect the pin's ability to drive output.  1 enables input, 0 disables
- *     input.
+ * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
that this does
+ *     not affect the pin's ability to drive output.
+ *     1 enables input, 0 disables input.
  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
  *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
  *     the threshold value is given on a custom format as argument when
@@ -73,6 +73,9 @@
  *     operation, if several modes of operation are supported these can be
  *     passed in the argument on a custom form, else just use argument 1
  *     to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
+ *     a value.
+ *     1 enables output buffer, 0 disables output buffer.
  * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
  *     1 to indicate high level, argument 0 to indicate low level. (Please
  *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a

Or
invent both input-buffer-enable and output-buffer-enable and
deprecated input-enable?

Andy,
how about your comments?

Regards
Dong Aisheng

> Thanks
>    j
>
>>
>> Yours,
>> Linus Walleij

[toc] | [prev] | [next] | [standalone]

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

Hi Dong,

On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:
> On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
> > Fair enough :)
> >
> > I'll try to keep this short: I don't like "output-enable", and at the
> > same time I don't think "output-high" and "output-low" fit well for
> > this purpose, as they electrically means something different from what
> > our (and IMX) use case is: enabling/disabling input/output
> > buffers internal to pin controller/gpio block HW and not driving a value
> > there.
> >
> > This seems clear to me from the "GPIO mode pitfalls" section of
> > pinctrl.txt documentation examples and from the fact that generic bindings
> > did not expose an "output" flag because if you drive an output line, you
> > reasonably either drive it high or low.
> >
> > Unfortunately I cannot convince myself that the same reasons apply
> > to the input use case.  Enabling input on a pin implies the pinctrl/gpio driver
> > has to enable any input buffer required to use that pin as a properly
> > working input line, and enabling an input buffer implies being able to sense
> > the line value from there, so I don't see that much use for "input-buffer-enable"
> > alone.
> >
> > So, even if bindings could look a bit weird as there won't be a direct
> > matching between properties names used to enable input/output buffers,
> > my vote is to add "output-buffer-enable" only, and keep using the
> > already there "input-enable" properties for the input use case.
> >
>
> Yes, it may be a bit weird.
> I'm not pad internal details expert and can't tell much difference between
> output-enable and output-buffer-enable.
> I just feel a bit confuse if only using output-buffer-enable.

Yes it is, and I actually like your proposal, I was just trying to
make sure I was not confusing the property semantic with its
real-world effect.

If no one as different opinions on this, I can send a patch later to
add output-enable only, or since you have almost done it down here you
can do the same resusing what you have proposed below.
>
> If enable both input and output, it becomes:
> pinctrl_xxx: gpios_xxx_grp {
>         pins = <
>                 ULP1_PAD_PTD0__PTD0
>         >;
>         input-enable;
>         output-buffer-enable;
>         bias-pull-up;
> };
>
> How about still use output-enable in pairs to input-enable but explain more
> in comments?
> Aslo update 'input-enable' comment to 'enable input buffer'.
> e.g.
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c
> index 720a19f..96c83a4 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -172,6 +172,7 @@ static const struct pinconf_generic_params dt_params[] = {
>         { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
>         { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
>         { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
> +       { "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
>         { "output-high", PIN_CONFIG_OUTPUT, 1, },
>         { "output-low", PIN_CONFIG_OUTPUT, 0, },
>         { "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
> diff --git a/include/linux/pinctrl/pinconf-generic.h
> b/include/linux/pinctrl/pinconf-generic.h
> index 7620eb1..d30f4fe 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -59,9 +59,9 @@
>   *     which means it will wait for signals to settle when reading inputs. The
>   *     argument gives the debounce time in usecs. Setting the
>   *     argument to zero turns debouncing off.
> - * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does not
> - *     affect the pin's ability to drive output.  1 enables input, 0 disables
> - *     input.
> + * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
> that this does
> + *     not affect the pin's ability to drive output.
> + *     1 enables input, 0 disables input.

I would not mention the "input buffer" here, as enabling input implies enabling
the buffer if you want to read values from there. Actually I guess
there may be platforms where buffer enabling may be implicit, so I
would leave this out and let drivers handle it internally.

>   * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>   *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>   *     the threshold value is given on a custom format as argument when
> @@ -73,6 +73,9 @@
>   *     operation, if several modes of operation are supported these can be
>   *     passed in the argument on a custom form, else just use argument 1
>   *     to indicate low power mode, argument 0 turns low power mode off.
> + * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
> + *     a value.
> + *     1 enables output buffer, 0 disables output buffer.
>   * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
>   *     1 to indicate high level, argument 0 to indicate low level. (Please
>   *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
>
> Or
> invent both input-buffer-enable and output-buffer-enable and
> deprecated input-enable?
>
> Andy,
> how about your comments?
>
> Regards
> Dong Aisheng
>
> > Thanks
> >    j
> >
> >>
> >> Yours,
> >> Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1669379

Hi Jmondi,

On Thu, Jun 15, 2017 at 7:11 PM, jmondi <jacopo@jmondi.org> wrote:
> Hi Dong,
>
> On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:
>> On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
>> > Fair enough :)
>> >
>> > I'll try to keep this short: I don't like "output-enable", and at the
>> > same time I don't think "output-high" and "output-low" fit well for
>> > this purpose, as they electrically means something different from what
>> > our (and IMX) use case is: enabling/disabling input/output
>> > buffers internal to pin controller/gpio block HW and not driving a value
>> > there.
>> >
>> > This seems clear to me from the "GPIO mode pitfalls" section of
>> > pinctrl.txt documentation examples and from the fact that generic bindings
>> > did not expose an "output" flag because if you drive an output line, you
>> > reasonably either drive it high or low.
>> >
>> > Unfortunately I cannot convince myself that the same reasons apply
>> > to the input use case.  Enabling input on a pin implies the pinctrl/gpio driver
>> > has to enable any input buffer required to use that pin as a properly
>> > working input line, and enabling an input buffer implies being able to sense
>> > the line value from there, so I don't see that much use for "input-buffer-enable"
>> > alone.
>> >
>> > So, even if bindings could look a bit weird as there won't be a direct
>> > matching between properties names used to enable input/output buffers,
>> > my vote is to add "output-buffer-enable" only, and keep using the
>> > already there "input-enable" properties for the input use case.
>> >
>>
>> Yes, it may be a bit weird.
>> I'm not pad internal details expert and can't tell much difference between
>> output-enable and output-buffer-enable.
>> I just feel a bit confuse if only using output-buffer-enable.
>
> Yes it is, and I actually like your proposal, I was just trying to
> make sure I was not confusing the property semantic with its
> real-world effect.
>
> If no one as different opinions on this, I can send a patch later to
> add output-enable only, or since you have almost done it down here you
> can do the same resusing what you have proposed below.

Please feel free to do it.
Just one thing, may be we could also add PIN_CONFIG_OUTPUT_ENABLE
as well.

>>
>> If enable both input and output, it becomes:
>> pinctrl_xxx: gpios_xxx_grp {
>>         pins = <
>>                 ULP1_PAD_PTD0__PTD0
>>         >;
>>         input-enable;
>>         output-buffer-enable;
>>         bias-pull-up;
>> };
>>
>> How about still use output-enable in pairs to input-enable but explain more
>> in comments?
>> Aslo update 'input-enable' comment to 'enable input buffer'.
>> e.g.
>> diff --git a/drivers/pinctrl/pinconf-generic.c
>> b/drivers/pinctrl/pinconf-generic.c
>> index 720a19f..96c83a4 100644
>> --- a/drivers/pinctrl/pinconf-generic.c
>> +++ b/drivers/pinctrl/pinconf-generic.c
>> @@ -172,6 +172,7 @@ static const struct pinconf_generic_params dt_params[] = {
>>         { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 },
>>         { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 },
>>         { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 },
>> +       { "output-enable", PIN_CONFIG_OUTPUT_ENABLE, 1 },
>>         { "output-high", PIN_CONFIG_OUTPUT, 1, },
>>         { "output-low", PIN_CONFIG_OUTPUT, 0, },
>>         { "power-source", PIN_CONFIG_POWER_SOURCE, 0 },
>> diff --git a/include/linux/pinctrl/pinconf-generic.h
>> b/include/linux/pinctrl/pinconf-generic.h
>> index 7620eb1..d30f4fe 100644
>> --- a/include/linux/pinctrl/pinconf-generic.h
>> +++ b/include/linux/pinctrl/pinconf-generic.h
>> @@ -59,9 +59,9 @@
>>   *     which means it will wait for signals to settle when reading inputs. The
>>   *     argument gives the debounce time in usecs. Setting the
>>   *     argument to zero turns debouncing off.
>> - * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does not
>> - *     affect the pin's ability to drive output.  1 enables input, 0 disables
>> - *     input.
>> + * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
>> that this does
>> + *     not affect the pin's ability to drive output.
>> + *     1 enables input, 0 disables input.
>
> I would not mention the "input buffer" here, as enabling input implies enabling
> the buffer if you want to read values from there. Actually I guess
> there may be platforms where buffer enabling may be implicit, so I
> would leave this out and let drivers handle it internally.
>

I'm fine with it.

Regards
Dong Aisheng

>>   * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>>   *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>>   *     the threshold value is given on a custom format as argument when
>> @@ -73,6 +73,9 @@
>>   *     operation, if several modes of operation are supported these can be
>>   *     passed in the argument on a custom form, else just use argument 1
>>   *     to indicate low power mode, argument 0 turns low power mode off.
>> + * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
>> + *     a value.
>> + *     1 enables output buffer, 0 disables output buffer.
>>   * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
>>   *     1 to indicate high level, argument 0 to indicate low level. (Please
>>   *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
>>
>> Or
>> invent both input-buffer-enable and output-buffer-enable and
>> deprecated input-enable?
>>
>> Andy,
>> how about your comments?
>>
>> Regards
>> Dong Aisheng
>>
>> > Thanks
>> >    j
>> >
>> >>
>> >> Yours,
>> >> Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web