Hi Jmondi,

On Thu, Jun 15, 2017 at 7:11 PM, jmondi [off-list ref] wrote:

Hi Dong,

On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:

quoted

On Mon, Jun 12, 2017 at 5:44 PM, jmondi [off-list ref] wrote:

quoted

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.

quoted

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

quoted

  * @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

quoted

Thanks
   j

quoted

Yours,
Linus Walleij