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

Add bi-directional and output-enable pin configuration properties.

bi-directional allows to specify when a pin shall operate in input and
output mode at the same time. This is particularly useful in platforms
where input and output buffers have to be manually enabled.

output-enable is just syntactic sugar to specify that a pin shall
operate in output mode, ignoring the provided argument.
This pairs with input-enable pin configuration option.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++
 drivers/pinctrl/pinconf-generic.c                              | 3 +++
 include/linux/pinctrl/pinconf-generic.h                        | 3 +++
 3 files changed, 8 insertions(+)

On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add bi-directional and output-enable pin configuration properties.
>
> bi-directional allows to specify when a pin shall operate in input and
> output mode at the same time. This is particularly useful in platforms
> where input and output buffers have to be manually enabled.
>
> output-enable is just syntactic sugar to specify that a pin shall
> operate in output mode, ignoring the provided argument.
> This pairs with input-enable pin configuration option.

For me it looks like you are trying to alias open-drain + bias or
alike. Don't actually see the benefit of it.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++
>  drivers/pinctrl/pinconf-generic.c                              | 3 +++
>  include/linux/pinctrl/pinconf-generic.h                        | 3 +++
>  3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index bf3f7b0..f2ed458 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -222,6 +222,7 @@ bias-bus-hold               - latch weakly
>  bias-pull-up           - pull up the pin
>  bias-pull-down         - pull down the pin
>  bias-pull-pin-default  - use pin-default pull state
> +bi-directional         - pin supports simultaneous input/output operations
>  drive-push-pull                - drive actively high and low
>  drive-open-drain       - drive with open drain
>  drive-open-source      - drive with open source
> @@ -234,6 +235,7 @@ input-debounce              - debounce mode with debound time X
>  power-source           - select between different power supplies
>  low-power-enable       - enable low power mode
>  low-power-disable      - disable low power mode
> +output-enable          - enable output on pin regardless of output value
>  output-low             - set the pin to output mode with low level
>  output-high            - set the pin to output mode with high level
>  slew-rate              - set the slew rate
> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
> index ce3335a..03e6808 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -35,6 +35,7 @@ static const struct pin_config_item conf_items[] = {
>         PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
>                                 "input bias pull to pin specific state", NULL, false),
>         PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false),
> +       PCONFDUMP(PIN_CONFIG_BIDIRECTIONAL, "bi-directional pin operations", NULL, false),
>         PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false),
>         PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
>         PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
> @@ -160,6 +161,7 @@ static const struct pinconf_generic_params dt_params[] = {
>         { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 },
>         { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 },
>         { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 },
> +       { "bi-directional", PIN_CONFIG_BIDIRECTIONAL, 1 },
>         { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 },
>         { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
>         { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
> @@ -172,6 +174,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, 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..279e3c5 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -42,6 +42,8 @@
>   * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
>   *     impedance to VDD). If the argument is != 0 pull-up is enabled,
>   *     if it is 0, pull-up is total, i.e. the pin is connected to VDD.
> + * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow simultaneous
> + *     input and output operations.
>   * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
>   *     collector) which means it is usually wired with other output ports
>   *     which are then pulled up with an external resistor. Setting this
> @@ -96,6 +98,7 @@ enum pin_config_param {
>         PIN_CONFIG_BIAS_PULL_DOWN,
>         PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
>         PIN_CONFIG_BIAS_PULL_UP,
> +       PIN_CONFIG_BIDIRECTIONAL,
>         PIN_CONFIG_DRIVE_OPEN_DRAIN,
>         PIN_CONFIG_DRIVE_OPEN_SOURCE,
>         PIN_CONFIG_DRIVE_PUSH_PULL,
> --
> 2.7.4
>

On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
>> Add bi-directional and output-enable pin configuration properties.
>>
>> bi-directional allows to specify when a pin shall operate in input and
>> output mode at the same time. This is particularly useful in platforms
>> where input and output buffers have to be manually enabled.
>>
>> output-enable is just syntactic sugar to specify that a pin shall
>> operate in output mode, ignoring the provided argument.
>> This pairs with input-enable pin configuration option.
>
> For me it looks like you are trying to alias open-drain + bias or
> alike. Don't actually see the benefit of it.

Andy is bringing up a valid point. And I remember asking about
this before.

What does "bi-directional" really mean, electrically speaking?

Does is just mean open drain and/or open source actually?
(See Documentation/gpio/driver.txt for an explanation of
how open drain/source works.)

When you set an output without setting a value, what happens
electrically?

Isn't this bias-high-impedance / High-Z?

Hopefully you can find the answer from Renesas hardware dept.

You can certainly call it whatever the datasheet calls it
in your driver #defines but for the DT bindings we would
ideally have the physical world things.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Linus,

On Fri, Apr 28, 2017 at 10:32 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
>> <jacopo+renesas@jmondi.org> wrote:
>>> Add bi-directional and output-enable pin configuration properties.
>>>
>>> bi-directional allows to specify when a pin shall operate in input and
>>> output mode at the same time. This is particularly useful in platforms
>>> where input and output buffers have to be manually enabled.
>>>
>>> output-enable is just syntactic sugar to specify that a pin shall
>>> operate in output mode, ignoring the provided argument.
>>> This pairs with input-enable pin configuration option.
>>
>> For me it looks like you are trying to alias open-drain + bias or
>> alike. Don't actually see the benefit of it.
>
> Andy is bringing up a valid point. And I remember asking about
> this before.
>
> What does "bi-directional" really mean, electrically speaking?
>
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of
> how open drain/source works.)
>
> When you set an output without setting a value, what happens
> electrically?
>
> Isn't this bias-high-impedance / High-Z?
>
> Hopefully you can find the answer from Renesas hardware dept.
>
> You can certainly call it whatever the datasheet calls it
> in your driver #defines but for the DT bindings we would
> ideally have the physical world things.

FWIW, you have already applied v4.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday, April 28, 2017, Linus Walleij wrote:
> > For me it looks like you are trying to alias open-drain + bias or

> > alike. Don't actually see the benefit of it.

> 

> Andy is bringing up a valid point. And I remember asking about this before.

> 

> What does "bi-directional" really mean, electrically speaking?

> 

> Does is just mean open drain and/or open source actually?

> (See Documentation/gpio/driver.txt for an explanation of how open

> drain/source works.)

> 

> When you set an output without setting a value, what happens electrically?

> 

> Isn't this bias-high-impedance / High-Z?

> 

> Hopefully you can find the answer from Renesas hardware dept.

> 

> You can certainly call it whatever the datasheet calls it in your driver

> #defines but for the DT bindings we would ideally have the physical world

> things.


The main reason is this pin controller is too dumb to do what it's supposed to with 1 register setting.


Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain). The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.
In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

# side note, the way the registers are arranged is also ridiculous in my opinion. I'm not a fan of this particular IP.

The good news is the RZ/A1 is the only chip series I've seen with this PFC IP (I can't even figure out where it came from internally). And as far as I know, it will not appear in any other RZ series chips.



Chris

On Fri, Apr 28, 2017 at 3:07 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Linus Walleij wrote:
>> > For me it looks like you are trying to alias open-drain + bias or
>> > alike. Don't actually see the benefit of it.
>>
>> Andy is bringing up a valid point. And I remember asking about this before.
>>
>> What does "bi-directional" really mean, electrically speaking?

> Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain).

Can you point to schematics and electrical characteristics of such buffer?
(Yes, I can imagine one case where it's possible to have an
"automatic" switch based on which current is bigger output of your
side or remote's. But! I would like to see actual specifications to
prove this or otherwise.)

> The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.

> In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

Please, post a link to it or copy essential parts.
I'm quite skeptical  that cheap hardware can implement something more
costable than simplest open-source / open-drain + bias.

T24gRnJpZGF5LCBBcHJpbCAyOCwgMjAxNywgQW5keSBTaGV2Y2hlbmtvIHdyb3RlOg0KPiA+IElu
IHRoZSBSWi9BMSBIVyBtYW51YWwgeW91IGNhbiBraW5kIG9mIHNlZSB0aGF0IGluIDU0LjE4IFBv
cnQgQ29udHJvbA0KPiBMb2dpY2FsIERpYWdyYW0gKGJ1dCB0aGF0IHdhc24ndCBvYnZpb3VzIHRv
IG1lIGF0IGZpcnN0KS4NCj4gDQo+IFBsZWFzZSwgcG9zdCBhIGxpbmsgdG8gaXQgb3IgY29weSBl
c3NlbnRpYWwgcGFydHMuDQoNCg0KVGhpcyBib2FyZCB0aGUgUlovQTEgR0VOTUFJIGJvYXJkLg0K
aHR0cHM6Ly93d3cucmVuZXNhcy5jb20vZW4tdXMvcHJvZHVjdHMvc29mdHdhcmUtdG9vbHMvYm9h
cmRzLWFuZC1raXRzL2V2YWx1YXRpb24tZGVtby1zb2x1dGlvbi1ib2FyZHMvZ2VubWFpLWNwdS1i
b2FyZC1ydGs3NzIxMDBiYzAwMDAwYnIuaHRtbA0KDQoNClRoZSBzY2hlbWF0aWMgaXMgaW5jbHVk
ZWQgaW4gdGhlICJVc2VyJ3MgbWFudWFsIg0KaHR0cHM6Ly93d3cucmVuZXNhcy5jb20vZW4tdXMv
ZG9jL3Byb2R1Y3RzL3Rvb2wvZG9jLzAwMy9yMjB1dDI1OTZlal9yN3M3MjEwMGV2dW0ucGRmDQoN
Cg0KVGhlIFJaL0ExSCBIYXJkd2FyZSBtYW51YWwgaXMgaGVyZToNCmh0dHBzOi8vd3d3LnJlbmVz
YXMuY29tL2VuLXVzL2RvY3VtZW50L2h3LW1hbnVhbD9od0xheWVyU2hvd0ZsZz10cnVlJnByZExh
eWVySWQ9MTg2Mzc0JmxheWVyTmFtZT1SWiUyNTJGQTFIJmNvcm9uclNlcnZpY2U9ZG9jdW1lbnQt
cHJkLXNlYXJjaCZod0RvY1VybD0lMkZlbi11cyUyRmRvYyUyRnByb2R1Y3RzJTJGbXB1bWN1JTJG
ZG9jJTJGcnolMkZyMDF1aDA0MDNlajAzMDBfcnpfYTFoLnBkZiZoYXNoS2V5PTU0ZjMzNTc1Mzc0
MmI1YWRkNTI0ZDQ3MjViNzI0MmU2DQoNCg0KQ2hhcHRlciA1NCBpcyB0aGUgcG9ydC9waW4gY29u
dHJvbGxlci4NCg0KIjU0LjE4IFBvcnQgQ29udHJvbCBMb2dpY2FsIERpYWdyYW0iIGlzIHRoZSBk
aWFncmFtIEkgd2FzIHRhbGtpbmcgYWJvdXQuIE5vdGUgdGhhdCBpcyBzYXlzICJOb3RlOiBUaGlz
IGZpZ3VyZSBzaG93cyB0aGUgbG9naWMgZm9yIHJlZmVyZW5jZSwgbm90IHRoZSBjaXJjdWl0LiIN
Cg0KIjU0LjMuMTMgUG9ydCBCaWRpcmVjdGlvbiBDb250cm9sIFJlZ2lzdGVyIChQQkRDbikiIGlz
IHRoZSBtYWdpYyByZWdpc3RlciBuZWVkZWQgdG8gZ2V0IHNvbWUgcGlucyB0byB3b3JrLg0KDQoN
Cj4gSSdtIHF1aXRlIHNrZXB0aWNhbCAgdGhhdCBjaGVhcCBoYXJkd2FyZSBjYW4gaW1wbGVtZW50
IHNvbWV0aGluZyBtb3JlDQo+IGNvc3RhYmxlIHRoYW4gc2ltcGxlc3Qgb3Blbi1zb3VyY2UgLyBv
cGVuLWRyYWluICsgYmlhcw0KDQpJIGRvbid0IHRoaW5rIHRoaXMgaXMgYW4gb3Blbi1zb3VyY2Ug
LyBvcGVuLWRyYWluICsgYmlhcyBpc3N1ZS4gSXQncyBhICJ0aGUgaW50ZXJuYWwgc2lnbmFsIHBh
dGhzIGFyZSBub3QgZ2V0dGluZyBob29rZWQgdXAgY29ycmVjdGx5IiBpc3N1ZS4NCg0KDQpSZWdh
cmRsZXNzLCBvbiB0aGlzIHBhcnQsIHdlIG5lZWRlZCBhIHdheSB0byBmbGFnIHRoYXQgc29tZSBw
aW5zIHdoZW4gcHV0IGluIHNvbWUgZnVuY3Rpb24gbW9kZXMgbmVlZGVkICdhbiBleHRyYSByZWdp
c3RlciBzZXR0aW5nJy4gQXQgZmlyc3Qgd2UgdHJpZWQgdG8gc25lYWsgdGhhdCBpbmZvIGluIHdp
dGggYSBzaW1wbGUgI2RlZmluZSBpbiB0aGUgcGluL3Bpbm11eCBEVCBub2RlIHByb3BlcnRpZXMu
IEJ1dCwgTGludXMgZGlkbid0IHdhbnQgaXQgdGhlcmUgc28gd2UgaGFkIHRvIG1ha2UgdXAgYSBu
ZXcgZ2VuZXJpYyBwcm9wZXJ0eSBjYWxsZWQgImJpLWRpcmVjdGlvbmFsIi4NCg0KV2hhdCBpcyB5
b3VyIGVuZCBnb2FsIGhlcmU/IEdldCAiYmktZGlyZWN0aW9uYWwiIGNoYW5nZWQgdG8gc29tZXRo
aW5nIGVsc2U/DQoNCg0KQ2hyaXMNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Apr 28, 2017 at 4:18 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
>> Logical Diagram (but that wasn't obvious to me at first).
>>
>> Please, post a link to it or copy essential parts.

> The schematic is included in the "User's manual"
> https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf

Not that one I would like to see...

> The RZ/A1H Hardware manual is here:
> https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true&prdLayerId=186374&layerName=RZ%252FA1H&coronrService=document-prd-search&hwDocUrl=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf&hashKey=54f335753742b5add524d4725b7242e6
>
> Chapter 54 is the port/pin controller.
>
> "54.18 Port Control Logical Diagram" is the diagram I was talking about. Note that is says "Note: This figure shows the logic for reference, not the circuit."
>
> "54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register needed to get some pins to work.

This is useful. Thanks.

>> I'm quite skeptical  that cheap hardware can implement something more
>> costable than simplest open-source / open-drain + bias
>
> I don't think this is an open-source / open-drain + bias issue. It's a "the internal signal paths are not getting hooked up correctly" issue.

Had you read the following, esp. Note there?

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

For me manual is clearly tells about this settings:
"This register enables or disables the input buffer while the output
buffer is enabled."

> Regardless, on this part, we needed a way to flag that some pins when put in some function modes needed 'an extra register setting'. At first we tried to sneak that info in with a simple #define in the pin/pinmux DT node properties. But, Linus didn't want it there so we had to make up a new generic property called "bi-directional".
>
> What is your end goal here? Get "bi-directional" changed to something else?

My goal is to reduce amount of (useless) entities. See Occam's razor
for details.

Linus, for me it looks like better to revert that change, until we
will have clear picture why existing configuration parameters can't
work.

T24gRnJpZGF5LCBBcHJpbCAyOCwgMjAxNywgQW5keSBTaGV2Y2hlbmtvIHdyb3RlOg0KPiBIYWQg
eW91IHJlYWQgdGhlIGZvbGxvd2luZywgZXNwLiBOb3RlIHRoZXJlPw0KPiANCj4gKiBAUElOX0NP
TkZJR19JTlBVVF9FTkFCTEU6IGVuYWJsZSB0aGUgcGluJ3MgaW5wdXQuICBOb3RlIHRoYXQgdGhp
cyBkb2VzDQo+IG5vdA0KPiAqICAgICAgYWZmZWN0IHRoZSBwaW4ncyBhYmlsaXR5IHRvIGRyaXZl
IG91dHB1dC4gIDEgZW5hYmxlcyBpbnB1dCwgMA0KPiBkaXNhYmxlcw0KPiAqICAgICAgaW5wdXQu
DQo+IA0KPiBGb3IgbWUgbWFudWFsIGlzIGNsZWFybHkgdGVsbHMgYWJvdXQgdGhpcyBzZXR0aW5n
czoNCj4gIlRoaXMgcmVnaXN0ZXIgZW5hYmxlcyBvciBkaXNhYmxlcyB0aGUgaW5wdXQgYnVmZmVy
IHdoaWxlIHRoZSBvdXRwdXQNCj4gYnVmZmVyIGlzIGVuYWJsZWQuIg0KDQoNCkJ1dCwgdGhlbiBp
ZiB3ZSB1c2UgImlucHV0LWVuYWJsZSIgdG8gZ2V0IGJpLWRpcmVjdGlvbmFsIGZ1bmN0aW9uYWxp
dHksIG5vdyB3ZSBuZWVkIHNvbWV0aGluZyB0byByZXBsYWNlIHdoYXQgd2Ugd2VyZSB1c2luZyAi
aW5wdXQtZW5hYmxlIiBmb3IuDQpXZSB3ZXJlIHVzaW5nICJpbnB1dC1lbmFibGUiIHRvIHNpZ25h
bCB3aGVuIHRoZSBwaW4gZnVuY3Rpb24gdGhhdCB3ZSBzZXQgYWxzbyBuZWVkcyB0byBiZSBmb3Jj
aWJsZSBzZXQgdG8gaW5wdXQgYnkgdGhlIHNvZnR3YXJlIChvbmNlIGFnYWluLCBiZWNhdXNlIHRo
ZSBIVyBpcyBub3Qgc21hcnQgZW5vdWdoIHRvIGRvIGl0IG9uIGl0cyBvd24pLCBidXQgaXMgZGlm
ZmVyZW50IHRoYW4gdGhlIGJpLWRpcmVjdGlvbmFsIGZ1bmN0aW9uYWxpdHkgKGllLCBhIGRpZmZl
cmVudCByZWdpc3RlciBzZXR0aW5nKS4NCg0KDQpTbywgaWYgd2UgcmVwbGFjZSAiYmktZGlyZWN0
aW9uYWwiIHdpdGggImlucHV0LWVuYWJsZSIgKHNpbmNlIGxvZ2ljYWxseSBpbnRlcm5hbGx5IHRo
YXQgaXMgd2hhdCBpcyBnb2luZyBvbiksIHdoYXQgZG8gd2UgdXNlIGZvciB0aGUgc3BlY2lhbCBw
aW5zIHRoYXQgdGhlIEhXIG1hbnVhbCBzYXlzICJoZXksIHlvdSBuZWVkIHRvIG1hbnVhbGx5IHNl
dCB0aGVzZSBwaW5zIHRvIGlucHV0IHdpdGggU1cgYmVjYXVzZSB0aGUgcGluIHNlbGVjdGlvbiBI
VyBjYW4ndCBkbyBpdCBjb3JyZWN0bHkpIi4gTm90ZSB0aGF0IHdlIGFkZGVkIGEgZW5hYmxlLW91
dHB1dCBmb3IgdGhlIHNhbWUgcmVhc29uLg0KU2VlIFJaL0ExSCBIVyBNYW51YWwgc2VjdGlvbiAi
VGFibGUgNTQuNyBBbHRlcm5hdGl2ZSBGdW5jdGlvbnMgdGhhdCBQSVBDbi5QSVBDbm0gQml0IFNo
b3VsZCBiZSBTZXQgdG8gMCINCg0KDQpDaHJpcw0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Apr 28, 2017 at 6:16 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> Had you read the following, esp. Note there?
>>
>> * @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.
>>
>> For me manual is clearly tells about this settings:
>> "This register enables or disables the input buffer while the output
>> buffer is enabled."
>
>
> But, then if we use "input-enable" to get bi-directional functionality,

It seems you are missing the point from electrical prospective.
Standard pin buffers (electrically) means input buffer and output
buffer that are driven independently in most cases.

Here is one example:
https://electronics.stackexchange.com/questions/96932/internal-circuitry-of-io-ports-in-mcu/96953#96953

(That's what I asked before as a schematic)

> now we need something to replace what we were using "input-enable" for.

No.

> We were using "input-enable" to signal when the pin function that we set also needs to be forcible set to input by the software (once again, because the HW is not smart enough to do it on its own), but is different than the bi-directional functionality (ie, a different register setting).

You are trying to introduce an abstraction, called BiDi, which is
*not* a separate thing from a set of pin properties.

> So, if we replace "bi-directional" with "input-enable" (since logically internally that is what is going on), what do we use for the special pins that the HW manual says "hey, you need to manually set these pins to input with SW because the pin selection HW can't do it correctly)".

Yes.
BiDi is an alias to output + input enable + other pin configuration
parameters (a set depends on real HW needs).

> Note that we added a enable-output for the same reason.
> See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that PIPCn.PIPCnm Bit Should be Set to 0"

Perhaps needs to be revisited as well.

P.S. It looks like more and more software engineers are going far from
real hardware when developing drivers...

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > We were using "input-enable" to signal when the pin function that we set

> also needs to be forcible set to input by the software (once again,

> because the HW is not smart enough to do it on its own), but is different

> than the bi-directional functionality (ie, a different register setting).

> 

> You are trying to introduce an abstraction, called BiDi, which is

> *not* a separate thing from a set of pin properties.


Note, I'm talking about 2 different issues we had:

1) Pins that need input and output buffers enabled during normal use. We created "bi-directional" for that.

2) For whatever reason, the HW manual points out that the PFC hardware can't really automatically set buffers enables correctly for some pin instances, and we have to manually assign the pin as input or output using another register. For that, we were using "input-enable" and "output-enable".


For #2:

> > Note that we added a enable-output for the same reason.

> > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that

> PIPCn.PIPCnm Bit Should be Set to 0"

> 

> Perhaps needs to be revisited as well.


Sorry, we didn't 'add' anything new. The property "output-enable", (ie, PIN_CONFIG_OUTPUT) already existed and describes what we are doing in the case for output.

But, we still have the issue that we have 2 cases that need the input enabled, but they are not the same situation, so we can't just use "input-enable" for both.


My only suggestion is (and I'm not sure this is possible in the driver):

"input-enable"  : case #2 where you need the pin to be forced as an input
"output-enable" : case #2 where you need the pin to be forced as an output
"input-enable" + "output-enable" : case #1 (replaces "bi-directional").

For example:

	i2c2_pins: i2c2 {
		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
		input-enable;
		output-enable;
	};

So in the SW driver, if we see both, that will signal to us what is going on and what to do about it (as in, set the bi-directional register and not the input direction register).


Thoughts?


Chris

On Fri, Apr 28, 2017 at 12:09 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

>> You can certainly call it whatever the datasheet calls it
>> in your driver #defines but for the DT bindings we would
>> ideally have the physical world things.
>
> FWIW, you have already applied v4.

Yeah I noticed when sending pull requests.. let's see if it stays in or
I will have to revert it for more discussion.

Sorry for being so flimsy at times, I guess I'm overloaded.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Linus, for me it looks like better to revert that change, until we
> will have clear picture why existing configuration parameters can't
> work.

Yeah I'll revert the binding for fixes.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Linus,

On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > Linus, for me it looks like better to revert that change, until we
> > will have clear picture why existing configuration parameters can't
> > work.
>
> Yeah I'll revert the binding for fixes.
>

As it seems we won't be able to proceed with the currently proposed solution,
would that be acceptable now that we use the "pinmux" property to add
flags as BIDIR and SWIO_[INPUT|OUTPUT] directly there?
This was my original proposal, rejected because we were using the "pins"
property at the time.

Quoting to the description of "pinmux":

"Each individual pin controller driver bindings documentation shall
specify how those values (pin IDs and pin multiplexing configuration)
are defined and assembled together"

Do you think the "flags" we have failed to describe as generic pin
configuration properties, fit the definition of "pin multiplexing
configuration" to be assembled with pin IDs?

As a reference this was the proposed bindings in v3:
https://www.spinics.net/lists/linux-renesas-soc/msg12792.html

Have a look at Pin multiplexing sub-nodes examples 2 and 3, with
"pinmux" in place of "renesas,pins" property.

Thanks
   j

> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>
>> > Linus, for me it looks like better to revert that change, until we
>> > will have clear picture why existing configuration parameters can't
>> > work.
>>
>> Yeah I'll revert the binding for fixes.

> As it seems we won't be able to proceed with the currently proposed solution,
> would that be acceptable now that we use the "pinmux" property to add
> flags as BIDIR

Can you explain what does this *electrically* mean?
Second question, what makes it differ to what already exists?

>  and SWIO_[INPUT|OUTPUT] directly there?

Ditto.

> This was my original proposal, rejected because we were using the "pins"
> property at the time.

T24gTW9uZGF5LCBNYXkgMDgsIDIwMTcsIEFuZHkgU2hldmNoZW5rbyB3cm90ZToNCj4gT24gTW9u
LCBNYXkgOCwgMjAxNyBhdCA3OjAxIFBNLCBqbW9uZGkgPGphY29wb0BqbW9uZGkub3JnPiB3cm90
ZToNCj4gPiBPbiBTdW4sIE1heSAwNywgMjAxNyBhdCAwOTo1Mjo0OUFNICswMjAwLCBMaW51cyBX
YWxsZWlqIHdyb3RlOg0KPiA+PiBPbiBGcmksIEFwciAyOCwgMjAxNyBhdCA0OjUzIFBNLCBBbmR5
IFNoZXZjaGVua28NCj4gPj4gPGFuZHkuc2hldmNoZW5rb0BnbWFpbC5jb20+IHdyb3RlOg0KPiA+
Pg0KPiA+PiA+IExpbnVzLCBmb3IgbWUgaXQgbG9va3MgbGlrZSBiZXR0ZXIgdG8gcmV2ZXJ0IHRo
YXQgY2hhbmdlLCB1bnRpbCB3ZQ0KPiA+PiA+IHdpbGwgaGF2ZSBjbGVhciBwaWN0dXJlIHdoeSBl
eGlzdGluZyBjb25maWd1cmF0aW9uIHBhcmFtZXRlcnMgY2FuJ3QNCj4gPj4gPiB3b3JrLg0KPiA+
Pg0KPiA+PiBZZWFoIEknbGwgcmV2ZXJ0IHRoZSBiaW5kaW5nIGZvciBmaXhlcy4NCj4gDQo+ID4g
QXMgaXQgc2VlbXMgd2Ugd29uJ3QgYmUgYWJsZSB0byBwcm9jZWVkIHdpdGggdGhlIGN1cnJlbnRs
eSBwcm9wb3NlZA0KPiBzb2x1dGlvbiwNCj4gPiB3b3VsZCB0aGF0IGJlIGFjY2VwdGFibGUgbm93
IHRoYXQgd2UgdXNlIHRoZSAicGlubXV4IiBwcm9wZXJ0eSB0byBhZGQNCj4gPiBmbGFncyBhcyBC
SURJUg0KPiANCj4gQ2FuIHlvdSBleHBsYWluIHdoYXQgZG9lcyB0aGlzICplbGVjdHJpY2FsbHkq
IG1lYW4/DQoNCkJpLURpcmVjdGlvbmFsOg0KDQpGb3IgYW55IHBpbiB0aGF0IG5lZWRzIHRvIGRy
aXZlIChzZW5kKSBvciBzZW5zZSAocmVjZWl2ZSkgc2lnbmFscywgdGhlIHBpbg0KbXV4IGNvbnRy
b2xsZXIgY2FuIG9ubHkgZW5hYmxlIDEgZGlyZWN0aW9uIG9mIGJ1ZmZlcnMgKGluIHRoaXMgY2Fz
ZSwgaXQNCm9ubHkgdGhlIG91dHB1dCBidWZmZXJzKS4gVGhlcmVmb3JlIHRoZSBhcHByb3ByaWF0
ZSBiaXQgaW4gdGhlDQonYmktZGlyZWN0aW9uYWwnIHJlZ2lzdGVyIGlzIHNldCBpbiBvcmRlciB0
byBlbmFibGUgdGhlIHNpZ25hbCBwYXRoIGluIGJvdGgNCmRpcmVjdGlvbnMgKGllLCBlbmFibGUg
dGhlIGlucHV0IGJ1ZmZlcnMpLg0KDQoNCj4gPiAgYW5kIFNXSU9fW0lOUFVUfE9VVFBVVF0gZGly
ZWN0bHkgdGhlcmU/DQoNCkluIHRoZSBoYXJkd2FyZSBtYW51YWwsIHRoZXJlIGlzIGEgbGlzdCBv
ZiBwaW4gZnVuY3Rpb25zIHRoYXQgaWYgeW91IHdhbnQNCnRvIHVzZSwgeW91IGNhbm5vdCB1c2Ug
dGhlIHN0YW5kIHBpbm11eCByZWdpc3RlciBtZXRob2QgdGhhdCB5b3UgdXNlIGZvcg0KYWxsIHRo
ZSBvdGhlciBwaW5zLiBJbnN0ZWFkLCB5b3UgYXJlIGluc3RydWN0ZWQgdG8gZG8gYSBkaWZmZXJl
bnQNCnByb2NlZHVyZS4gSWYgY291cnNlIGVsZWN0cmljYWxseSwgaW5wdXQvb3V0cHV0IGJ1ZmZl
cnMgYXJlIHN0aWxsIHR1cm5lZA0Kb24vb2ZmIG9yIHdoYXRldmVyLCBidXQgdGhlIHJvb3QgcmVh
c29uIG9mIHdoeSB5b3UgbmVlZCB0byBkbyB0aGlzDQpkaWZmZXJlbnRseSBmb3IgdGhlc2Ugc3Bl
Y2lmaWMgcGluL2Z1bmN0aW9uIGlzIG5vdCBrbm93bi4NCg0KVGhlICJTV0lPXyIgcG9ydGlvbiBv
ZiB0aGUgbmFtaW5nIGNvbWVzIGZyb20gdGhlIGhhcmR3YXJlIG1hbnVhbCB3aGljaA0KcmVmZXJz
IHRvIHRoaXMgYXMgIlNvZnR3YXJlIEkvTyBDb250cm9sIEFsdGVybmF0aXZlIE1vZGUiICh3aGlj
aCBpbiBteQ0Kb3BpbmlvbiBtZWFucyB0aGUgSFcgZ3V5cyBjb3VsZG4ndCBnZXQgdGhlIHBpbiBk
aXJlY3Rpb25zL2J1ZmZlcnMgdG8gYmUgc2V0DQphdXRvbWF0aWNhbGx5IGZvciBzb21lIHJlYXNv
biwgc28gdGhleSBkZWNpZGVkIGl0J3MgdGhlIFNXIGd1eXMgcHJvYmxlbSBub3cNCmZvciB0aG9z
ZSBwaW5zKQ0KDQoNCj4gU2Vjb25kIHF1ZXN0aW9uLCB3aGF0IG1ha2VzIGl0IGRpZmZlciB0byB3
aGF0IGFscmVhZHkgZXhpc3RzPw0KDQpXZSBoYXZlIDMgZGlmZmVyZW50IGZsYWdzIChwcm9wZXJ0
aWVzKSB0aGF0IG5lZWQgdG8gYmUgc3BlY2lmaWVkIGZvciBzb21lDQpwaW5zIGluIHNvbWUgY2ly
Y3Vtc3RhbmNlcy4NCkF0IGZpcnN0LCB3ZSBqdXN0IHRyaWVkIHRvIHBhc3MgdGhpcyBhZGRpdGlv
bmFsIGluZm9ybWF0aW9uIGluIHdoZW4NCmRlZmluaW5nIHdoYXQgZnVuY3Rpb24gdGhlIHBpbiBz
aG91bGQgYmUganVzdCBmb3IgdGhvc2UgcGlucyB3aG9zZQ0KZGlyZWN0aW9uIChpZSwgYnVmZmVy
cykgd291bGQgbm90IGJlIHNldCB1cCBhdXRvbWF0aWNhbGx5Lg0KSG93ZXZlciwgdGhpcyB3YXMg
cmVqZWN0ZWQgYW5kIHdlIHdlcmUgdG9sZCB0byBwaWNrIGZyb20gdGhlIGV4aXN0aW5nIHNldA0K
Z2VuZXJpYyBwcm9wZXJ0aWVzLg0KQnV0LCAzIGRpZmZlcmVudCBnZW5lcmljIHBpbmN0cmwgcHJv
cGVydGllcyB0aGF0IGZpdCB3aGF0IHdlIG5lZWRlZCBkaWRuJ3QNCmV4aXN0LiBTbywgd2UgdXNl
ZCB0aGUgZXhpc3RpbmcgImlucHV0LWVuYWJsZSIgYW5kICJvdXRwdXQtZW5hYmxlIiwgYnV0DQp0
aGVuIGNyZWF0ZWQgImJpLWRpcmVjdGlvbmFsIi4NCg0KDQpDaHJpcw0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy,

On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >>
> >> > Linus, for me it looks like better to revert that change, until we
> >> > will have clear picture why existing configuration parameters can't
> >> > work.
> >>
> >> Yeah I'll revert the binding for fixes.
>
> > As it seems we won't be able to proceed with the currently proposed solution,
> > would that be acceptable now that we use the "pinmux" property to add
> > flags as BIDIR
>
> Can you explain what does this *electrically* mean?

I really don't know what to add to what Chris said in his 2 previous
replies to the same question, and I don't know if I can even get this
information as the most detailed drawing I can provide is what you
have seen already at page 2696 Fig. 54.1 of the following document.

https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c

From my perspective these flags are configurations internal to the pin
controller hardware used to enable/disable input buffers when a pin needs to
perform in both direction.
The level of detail I can provide on this is the logical diagram we have pointed
you to already.

As I assume you are trying to get this answer from us in order to
avoid duplicating things in pin controller sub-system, and I
understand this, but my question here was "can we have those flags as part
of the pinmux property argument list, as that property description
seems to allow us to do that, instead of making them generic pin
configuration properties and upset other developers?"

Anyway, I still fail to see why those configuration flags, only
affecting the way the pin controller hardware enables/disables
its internal buffers and its internal operations have to be
described in term of their externally visible electrically characteristics.

> Second question, what makes it differ to what already exists?

To me, what already exists are pin configuration properties generic to
the whole pin controller subsystem, and I understand you don't want to
see duplication there.

At the same time, to me, those flags are settings the pin controller
wants to have specified by software to overcome its hw design flaws,
and are intended to configure its internal buffers in a way it cannot
do by itself for some very specific operation modes (they are listed
in the hw reference manual, it's not something you can chose to
configure or not, if you want a pin working in i2c mode, you HAVE to
pass those flags to pin controller).

Thanks
   j

Edit: I see Chris have now replied as well so I leave the SWIO part
out, as his answer is already better than what I can give you.

>
> >  and SWIO_[INPUT|OUTPUT] directly there?
>
> Ditto.
>
> > This was my original proposal, rejected because we were using the "pins"
> > property at the time.
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, May 8, 2017 at 8:25 PM, jmondi <jacopo@jmondi.org> wrote:
> Andy,
>
> On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
>> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
>> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> wrote:
>> >>
>> >> > Linus, for me it looks like better to revert that change, until we
>> >> > will have clear picture why existing configuration parameters can't
>> >> > work.
>> >>
>> >> Yeah I'll revert the binding for fixes.
>>
>> > As it seems we won't be able to proceed with the currently proposed solution,
>> > would that be acceptable now that we use the "pinmux" property to add
>> > flags as BIDIR
>>
>> Can you explain what does this *electrically* mean?
>
> I really don't know what to add to what Chris said in his 2 previous
> replies to the same question, and I don't know if I can even get this
> information as the most detailed drawing I can provide is what you
> have seen already at page 2696 Fig. 54.1 of the following document.
>
> https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c

I didn't see before this document. (I had downloaded what Chris
referred to, which has less than 1200 pages).

The figure you pointed to is really nice and explains it, thank you.

So, BiDi in this hardware is just helper to enable Input
simultaneously when you enable output.

This makes me wonder what prevents you to do this in two steps in software?
So, basically in terms of pin control framework you define this pin
configuration as

1. PIN_CONFIG_INPUT_ENABLE:
2. PIN_CONFIG_OUTPUT:

(or wise versa)

> From my perspective these flags are configurations internal to the pin
> controller hardware used to enable/disable input buffers when a pin needs to
> perform in both direction.

> The level of detail I can provide on this is the logical diagram we have pointed
> you to already.
>
> As I assume you are trying to get this answer from us in order to
> avoid duplicating things in pin controller sub-system, and I
> understand this, but my question here was "can we have those flags as part
> of the pinmux property argument list, as that property description
> seems to allow us to do that, instead of making them generic pin
> configuration properties and upset other developers?"

I guess Linus is better than me could answer to this.

> Anyway, I still fail to see why those configuration flags, only
> affecting the way the pin controller hardware enables/disables
> its internal buffers and its internal operations have to be
> described in term of their externally visible electrically characteristics.
>
>> Second question, what makes it differ to what already exists?
>
> To me, what already exists are pin configuration properties generic to
> the whole pin controller subsystem, and I understand you don't want to
> see duplication there.
>
> At the same time, to me, those flags are settings the pin controller
> wants to have specified by software to overcome its hw design flaws,
> and are intended to configure its internal buffers in a way it cannot
> do by itself for some very specific operation modes (they are listed
> in the hw reference manual, it's not something you can chose to
> configure or not, if you want a pin working in i2c mode, you HAVE to
> pass those flags to pin controller).

So, when you configuring pinmux to use group of pins to be i2c, what
does prevent you to apply those settings implicitly?

On Mon, May 8, 2017 at 8:02 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, May 08, 2017, Andy Shevchenko wrote:
>> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
>> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> wrote:
>> >>
>> >> > Linus, for me it looks like better to revert that change, until we
>> >> > will have clear picture why existing configuration parameters can't
>> >> > work.
>> >>
>> >> Yeah I'll revert the binding for fixes.
>>
>> > As it seems we won't be able to proceed with the currently proposed
>> solution,
>> > would that be acceptable now that we use the "pinmux" property to add
>> > flags as BIDIR
>>
>> Can you explain what does this *electrically* mean?
>
> Bi-Directional:
>
> For any pin that needs to drive (send) or sense (receive) signals, the pin
> mux controller can only enable 1 direction of buffers (in this case, it
> only the output buffers). Therefore the appropriate bit in the
> 'bi-directional' register is set in order to enable the signal path in both
> directions (ie, enable the input buffers).

So, I see this way how it can be enabled:
1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set
2. BiDi bit is set and BUFOE is set

Now the question is what the real use case for 2?

If we find one we need to rename and fix a description of the pin
control configuration property.

like:
@PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.
...
Note: As long as it's enabled the pin's input enabled as well and vice versa.

>> >  and SWIO_[INPUT|OUTPUT] directly there?
>
> In the hardware manual, there is a list of pin functions that if you want
> to use, you cannot use the stand pinmux register method that you use for
> all the other pins. Instead, you are instructed to do a different
> procedure. If course electrically, input/output buffers are still turned
> on/off or whatever, but the root reason of why you need to do this
> differently for these specific pin/function is not known.
>
> The "SWIO_" portion of the naming comes from the hardware manual which
> refers to this as "Software I/O Control Alternative Mode" (which in my
> opinion means the HW guys couldn't get the pin directions/buffers to be set
> automatically for some reason, so they decided it's the SW guys problem now
> for those pins)

Okay, these are related to pin muxing explicitly.
So, you have 10 functions overall?
What prevents you describe them accordingly and hide this
implementation detail in the driver?

>> Second question, what makes it differ to what already exists?
>
> We have 3 different flags (properties) that need to be specified for some
> pins in some circumstances.
> At first, we just tried to pass this additional information in when
> defining what function the pin should be just for those pins whose
> direction (ie, buffers) would not be set up automatically.
> However, this was rejected and we were told to pick from the existing set
> generic properties.
> But, 3 different generic pinctrl properties that fit what we needed didn't
> exist. So, we used the existing "input-enable" and "output-enable", but
> then created "bi-directional".

Yes, that figure helped me a lot to understand.

Hello Andy,

On Monday, May 08, 2017, Andy Shevchenko wrote:
> > Bi-Directional:

> >

> > For any pin that needs to drive (send) or sense (receive) signals, the

> pin

> > mux controller can only enable 1 direction of buffers (in this case, it

> > only the output buffers). Therefore the appropriate bit in the

> > 'bi-directional' register is set in order to enable the signal path in

> both

> > directions (ie, enable the input buffers).

> 

> So, I see this way how it can be enabled:

> 1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set

> 2. BiDi bit is set and BUFOE is set

> 

> Now the question is what the real use case for 2?


For #1, IP_ENI and IP_ENO are controlled by PFC/PFCE/PFCAE.
Those basically equate to the pin function register (as in,
what IP block gets wired up to each pin.) However, it
seems that PFC/PFCE/PFCAE cannot enable both IP_ENI and
IP_ENO signals at the same time (the diagram doesn't really
show you that piece of info), hence they had to make an
'override' register can called it PBDC (bir-dir register)
to manually turn the input buffers on when needed.

Seems like a HW hack if you ask me.


> If we find one we need to rename and fix a description of the pin

> control configuration property.

> 

> like:

> @PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.

> ...

> Note: As long as it's enabled the pin's input enabled as well and vice

> versa.


So your suggestion is to rename PIN_CONFIG_OUTPUT to
PIN_CONFIG_OUTPUT_INPUT_ENABLE?

I would say the description for @PIN_CONFIG_INPUT_ENABLE is probably
'technically' correct for our bi-dir needs, but I didn't like it
because it might confuse users making a DT file for their board (unless
of course they studied the hardware manual in detail to finally come
to the conclusion of the screwy PFC hardware).


> >> >  and SWIO_[INPUT|OUTPUT] directly there?

> >

> > In the hardware manual, there is a list of pin functions that if you

> want

> > to use, you cannot use the stand pinmux register method that you use for

> > all the other pins. Instead, you are instructed to do a different

> > procedure. If course electrically, input/output buffers are still turned

> > on/off or whatever, but the root reason of why you need to do this

> > differently for these specific pin/function is not known.

> >

> > The "SWIO_" portion of the naming comes from the hardware manual which

> > refers to this as "Software I/O Control Alternative Mode" (which in my

> > opinion means the HW guys couldn't get the pin directions/buffers to be

> set

> > automatically for some reason, so they decided it's the SW guys problem

> now

> > for those pins)

> 

> Okay, these are related to pin muxing explicitly.

> So, you have 10 functions overall?

> What prevents you describe them accordingly and hide this

> implementation detail in the driver?


The one issue I was trying to avoid by hiding things in the driver
with some type of look-up table was that this series of parts comes
in different subsets/packages and sometimes the functions comes out
on different port numbers. So now you need multiple look-up tables
and then also a way to signal what part/package you have so you use
the correct look-up table. It seemed easier (and safer) to just pass
the info in (if you needed it) in the Device Tree for the board.

Of these 'special' pins (Table 54.7):

For 16 of them, they truly can operate as input or output, so
the user needs to specify that direction they need in the DT.

For LVDS, Sound and WDT, those will be fixed, so they would be
the only ones you could do a table for, but as I mentioned, Sound
and WDT don't always come out in the same place so a lookup table
isn't so cut and dry.


> >> Second question, what makes it differ to what already exists?

> >

> > We have 3 different flags (properties) that need to be specified for

> some

> > pins in some circumstances.

> > At first, we just tried to pass this additional information in when

> > defining what function the pin should be just for those pins whose

> > direction (ie, buffers) would not be set up automatically.

> > However, this was rejected and we were told to pick from the existing

> set

> > generic properties.

> > But, 3 different generic pinctrl properties that fit what we needed

> didn't

> > exist. So, we used the existing "input-enable" and "output-enable", but

> > then created "bi-directional".

> 

> Yes, that figure helped me a lot to understand.


I see from your other email you sent to Jacopo, it looks like the link
I sent you only brought you to the shorter 'data sheet' version of the
hardware manual, not the full manual that includes 'Figure 54.1'.
Sorry about that.



Chris

On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@jmondi.org> wrote:

> From my perspective these flags are configurations internal to the pin
> controller hardware used to enable/disable input buffers when a pin needs to
> perform in both direction.
> The level of detail I can provide on this is the logical diagram we have pointed
> you to already.
>
> As I assume you are trying to get this answer from us in order to
> avoid duplicating things in pin controller sub-system, and I
> understand this, but my question here was "can we have those flags as part
> of the pinmux property argument list, as that property description
> seems to allow us to do that, instead of making them generic pin
> configuration properties and upset other developers?"

Pinmux with all it's magic flags baked into one is not any better
or any more readable. The solution is already very pretty except
for these two flags which I am sure we can agree on a way forward
for.

What we choose between is not this or another less transparent
pin configuration mechanism, the mechanism (whether magic bits
to pinmux or reasonable properties) does not matter.

There is a strong preference to use the generic bindings.

So the discussion is whether to use:

bi-directional;
output-enable;

Or some already defined config flags.

If these are too idiomatic to be used by others, they should anyways
look similar, like:

renesas,bi-directional;
renesas,output-enable;

Like the Qualcomm weirdness found in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

qcom,pull-up-strength = <..>;

Check how they use
#define PMIC_GPIO_CONF_PULL_UP                 (PIN_CONFIG_END + 1)

Etc.

> Anyway, I still fail to see why those configuration flags, only
> affecting the way the pin controller hardware enables/disables
> its internal buffers and its internal operations have to be
> described in term of their externally visible electrically characteristics.

To me internal vs external is not what matters. What matters is
if this is likely to pop up in more platforms, and then the property
should be generic.

The generic pin config definitions are likely to be picked up by other
standards and even be inspiration to hardware engineers so that
is why it matters so much.

> To me, what already exists are pin configuration properties generic to
> the whole pin controller subsystem, and I understand you don't want to
> see duplication there.
>
> At the same time, to me, those flags are settings the pin controller
> wants to have specified by software to overcome its hw design flaws,
> and are intended to configure its internal buffers in a way it cannot
> do by itself for some very specific operation modes (they are listed
> in the hw reference manual, it's not something you can chose to
> configure or not, if you want a pin working in i2c mode, you HAVE to
> pass those flags to pin controller).

Sounds like a case for

renesas,bi-directional;
renesas,output-enable;

following the Qualcomm pattern in that case.

But let's see if something else comes out of this discussion.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Andy,

On Mon, May 08, 2017 at 08:47:17PM +0300, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 8:25 PM, jmondi <jacopo@jmondi.org> wrote:
> > Andy,
> >
> > On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
> >> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> >> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> >> <andy.shevchenko@gmail.com> wrote:
> >> >>
> >> >> > Linus, for me it looks like better to revert that change, until we
> >> >> > will have clear picture why existing configuration parameters can't
> >> >> > work.
> >> >>
> >> >> Yeah I'll revert the binding for fixes.
> >>
> >> > As it seems we won't be able to proceed with the currently proposed solution,
> >> > would that be acceptable now that we use the "pinmux" property to add
> >> > flags as BIDIR
> >>
> >> Can you explain what does this *electrically* mean?
> >
> > I really don't know what to add to what Chris said in his 2 previous
> > replies to the same question, and I don't know if I can even get this
> > information as the most detailed drawing I can provide is what you
> > have seen already at page 2696 Fig. 54.1 of the following document.
> >
> > https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c
>
> I didn't see before this document. (I had downloaded what Chris
> referred to, which has less than 1200 pages).
>
> The figure you pointed to is really nice and explains it, thank you.

Oh sorry, I thought you had seen this already :)
>
> So, BiDi in this hardware is just helper to enable Input
> simultaneously when you enable output.
>
> This makes me wonder what prevents you to do this in two steps in software?
> So, basically in terms of pin control framework you define this pin
> configuration as
>
> 1. PIN_CONFIG_INPUT_ENABLE:
> 2. PIN_CONFIG_OUTPUT:
>
> (or wise versa)
>

That could be doable, as when we're collecting generic pin
configuration to apply to the pin I can simply check if both of them
are enabled.

That would feel un-natural in dts anyway, for someone that is not that
into the pin controller sub system details.
If I would have to do something like  this, not knowing all the
reasonable pre-conditions we've been discussing about

pins {
    pinmux = < .. >;
    input-enable;
    output-high; /* or output-low, we can ignore the argument here */
}

In place of

pins {
   pinmux = < .. >;
   renesas,bi-directional;
}

And the hardware manual speaks of "bi-directional" everywhere, I would
be wondering what those guys implementing this were thinking :)

> > From my perspective these flags are configurations internal to the pin
> > controller hardware used to enable/disable input buffers when a pin needs to
> > perform in both direction.
>
> > The level of detail I can provide on this is the logical diagram we have pointed
> > you to already.
> >
> > As I assume you are trying to get this answer from us in order to
> > avoid duplicating things in pin controller sub-system, and I
> > understand this, but my question here was "can we have those flags as part
> > of the pinmux property argument list, as that property description
> > seems to allow us to do that, instead of making them generic pin
> > configuration properties and upset other developers?"
>
> I guess Linus is better than me could answer to this.
>
> > Anyway, I still fail to see why those configuration flags, only
> > affecting the way the pin controller hardware enables/disables
> > its internal buffers and its internal operations have to be
> > described in term of their externally visible electrically characteristics.
> >
> >> Second question, what makes it differ to what already exists?
> >
> > To me, what already exists are pin configuration properties generic to
> > the whole pin controller subsystem, and I understand you don't want to
> > see duplication there.
> >
> > At the same time, to me, those flags are settings the pin controller
> > wants to have specified by software to overcome its hw design flaws,
> > and are intended to configure its internal buffers in a way it cannot
> > do by itself for some very specific operation modes (they are listed
> > in the hw reference manual, it's not something you can chose to
> > configure or not, if you want a pin working in i2c mode, you HAVE to
> > pass those flags to pin controller).
>
> So, when you configuring pinmux to use group of pins to be i2c, what
> does prevent you to apply those settings implicitly?
>

Chris already gave some valid reasons why it would be hard to do this
considering the different part numbers this driver may handle, but I
would also like to add that I have counted > 100 cases where
bi-directional flag has to be applied just in the first 5 IO ports (on a
total of 13).

As there are RZ systems out there running with just < 9MB of SRAM,
adding a static table (or several, considering the different part numbers)
with at least 300 entries, is a considerable waste :(

For SWIO it would be easier, there are just 16 cases, all of them
listed in the hardware reference manual as Chris said.

Thanks
   j

> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Linus et al,

On Mon, May 8, 2017 at 11:19 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@jmondi.org> wrote:
>> From my perspective these flags are configurations internal to the pin
>> controller hardware used to enable/disable input buffers when a pin needs to
>> perform in both direction.
>> The level of detail I can provide on this is the logical diagram we have pointed
>> you to already.
>>
>> As I assume you are trying to get this answer from us in order to
>> avoid duplicating things in pin controller sub-system, and I
>> understand this, but my question here was "can we have those flags as part
>> of the pinmux property argument list, as that property description
>> seems to allow us to do that, instead of making them generic pin
>> configuration properties and upset other developers?"
>
> Pinmux with all it's magic flags baked into one is not any better
> or any more readable. The solution is already very pretty except
> for these two flags which I am sure we can agree on a way forward
> for.
>
> What we choose between is not this or another less transparent
> pin configuration mechanism, the mechanism (whether magic bits
> to pinmux or reasonable properties) does not matter.
>
> There is a strong preference to use the generic bindings.
>
> So the discussion is whether to use:
>
> bi-directional;
> output-enable;
>
> Or some already defined config flags.
>
> If these are too idiomatic to be used by others, they should anyways
> look similar, like:
>
> renesas,bi-directional;
> renesas,output-enable;
>
> Like the Qualcomm weirdness found in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>
> qcom,pull-up-strength = <..>;
>
> Check how they use
> #define PMIC_GPIO_CONF_PULL_UP                 (PIN_CONFIG_END + 1)

The question I'm asking myself is: are these settings related to pin
configuration (i.e. depending on the use of the pin, and several settings
are valid, depending on the use case), or are they related to pinmux
(i.e. defined by the function)?

Configuring drive strength or pull-up are clearly related to pin
configuration.  Depending on what you connect, or on how you connect it,
you want a different drive strength, or choose a different output buffer
type (e.g. totem pole vs. open-collector). All of these are valid
configurations, depending on the use case.

But the settings RZ/A1H needs are different.  Some (e.g. "input-enable")
may sound like related to pin configuration. However, the big discerning
factor is that these settings are implied by the pinmux function. Their
presence is purely dictated by the chosen pinmux function. There's no use
case for doing otherwise (i.e. adding them when not needed, or removing
them when needed, according to the datasheet).

Note that e.g. the existing "input-enable" property is clearly a pin
configuration property. This is even reflected in DT, as the property is
not specified as part of the "pinmux" property, but in the surrounding pin
subnode of the pin-controller.

Hence I think we should not use generic pin properties, but consider these
settings to be part of pinmux configuration.
As having large tables in the driver is undesirable, I think storing the
settings in the "pinmux" property (by encoding them as flags passed to the
RZA1_PINMUX() macro) is our best option.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> The question I'm asking myself is: are these settings related to pin
> configuration (i.e. depending on the use of the pin, and several settings
> are valid, depending on the use case), or are they related to pinmux
> (i.e. defined by the function)?

If they are intrinsic to the function, i.e. whenever someone wants to use
that function they need to do this (nb the Documentation/pinctrl.txt
definition of "function", nothing else) then this should IMO not be in
the device tree at all, but hard-coded in the driver.

E.g. if someone needs to use a function such as "i2c0", on whatever
pins, then it should just be set up by the driver, no DT involved. It is a
hardware pecularity, drivers drive hardware so...

If however it depends on which pins it is used with, then it is pin
configuration and should use a DT property.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

Oops missed this:

> Hence I think we should not use generic pin properties, but consider these
> settings to be part of pinmux configuration.
> As having large tables in the driver is undesirable, I think storing the
> settings in the "pinmux" property (by encoding them as flags passed to the
> RZA1_PINMUX() macro) is our best option.

I think it is better to have large tables in the driver in this case.

It is the lesser evil.

Having unintelligible and hard to grasp stuff in the device tree that
no user will understand or dare to touch is not good, then it is better
to have it with the code, where it is being used, so the developers of
the driver can see it when they are dealing with this (quirky) hardware.

As you say this is actually fixing hardware bugs, we can expect these
quirky tables to be gone in the next hardware generation, right?

Then the right place for it is in the quirky driver for the quirky
first-generation
hardware.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Linus,

On Fri, May 12, 2017 at 11:04 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>
> Oops missed this:
>
>> Hence I think we should not use generic pin properties, but consider these
>> settings to be part of pinmux configuration.
>> As having large tables in the driver is undesirable, I think storing the
>> settings in the "pinmux" property (by encoding them as flags passed to the
>> RZA1_PINMUX() macro) is our best option.
>
> I think it is better to have large tables in the driver in this case.

Jacopo, Chris: Would two bits per pin/function (none, input, output, bidir)
be sufficient?
That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes.
Plus code to handle it. After all not that bad...

> It is the lesser evil.
>
> Having unintelligible and hard to grasp stuff in the device tree that
> no user will understand or dare to touch is not good, then it is better
> to have it with the code, where it is being used, so the developers of
> the driver can see it when they are dealing with this (quirky) hardware.
>
> As you say this is actually fixing hardware bugs, we can expect these
> quirky tables to be gone in the next hardware generation, right?

Let's hope so. Chris has a better crystal ball than I have ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Friday, May 12, 2017, linux-renesas-soc-owner@vger.kernel.org wrote:
> As you say this is actually fixing hardware bugs, we can expect these

> quirky tables to be gone in the next hardware generation, right?


I see this particular pin controller as a one shot deal. For the next
part in this series we are moving to a completely different controller
which you could probably get away with simply using pinctrl-single.


> Then the right place for it is in the quirky driver for the quirky

> first-generation

> hardware.


Maybe to be more clear, I would say the design is not a 'bug', but
rather an under sight of the original designers where are they stared
to need pins that would be both in and out, they started tacking on
more hardware.


Chris

Hi Geert and Linus,

On Friday, May 12, 2017, Geert Uytterhoeven wrote:
> Jacopo, Chris: Would two bits per pin/function (none, input, output,

> bidir)

> be sufficient?

> That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes.

> Plus code to handle it. After all not that bad...


OK...I give up!
If that's what it takes to get it, I'm fine.

NOTE, your math is a little off, the issue is that depending on the
function that you use, you might need to do extra settings, so you'd
have to have a lookup table for every pin & function.
Each pin can have 1 of 8 functions (which is good because a 'byte' has
8 bits).

So,
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if bi-dir is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if input is needed)
 12 ports x 16 pins => 384 bytes  (this table would just be for checking if input is needed)
                     ------------
                     1,152 bytes

But then...there are package variations so you need another entire
table for those parts.
   1,152 bytes x 2 = 2,304 bytes

However, if these tables are constants, they will reside in flash for the
XIP_KERNEL systems, so that's OK.

#What we should really do is just make a look-up table (tables) for the
'special' ones. But, we can have that discussion in a different thread.



One final note:

There is still a need for "input-enable" and "output-enable" for the timer
pins. Because, when you choose the pin to be connected to the MTU2 timer,
the pin can be used as either input-capture/output-compare/PWM and that's
the user's choice. So that's probably a valid usage of the generic pin
properties for configuration.


Sorry Jacopo, but we'll need another round of patches.
It sounds like for sure the bi-direction needs to get ripped back out.


Chris

Hi Chris,

On Fri, May 12, 2017 at 2:13 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, May 12, 2017, Geert Uytterhoeven wrote:
>> Jacopo, Chris: Would two bits per pin/function (none, input, output,
>> bidir)
>> be sufficient?
>> That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes.
>> Plus code to handle it. After all not that bad...
>
> OK...I give up!
> If that's what it takes to get it, I'm fine.
>
> NOTE, your math is a little off, the issue is that depending on the
> function that you use, you might need to do extra settings, so you'd
> have to have a lookup table for every pin & function.
> Each pin can have 1 of 8 functions (which is good because a 'byte' has
> 8 bits).
>
> So,
>  12 ports x 16 pins => 384 bytes  (this table would just be for checking if bi-dir is needed)
>  12 ports x 16 pins => 384 bytes  (this table would just be for checking if input is needed)
>  12 ports x 16 pins => 384 bytes  (this table would just be for checking if input is needed)
         ------------
>                      1,152 bytes

12 x 16 = 192, not 384.

Do you need all possible combinations of input, output, and bi-dir?
I assumed they're mutually exclusive. If not, you need 3 bits/pin/function.

> But then...there are package variations so you need another entire
> table for those parts.
>    1,152 bytes x 2 = 2,304 bytes

With packages, do you mean e.g. RZ/A1H vs. RZ/A1L? These indeed differ, but
should use different compatible values.
Or do you mean QFP/BGA256 vs. BGA324? Isn't the former a subset of the latter?

> #What we should really do is just make a look-up table (tables) for the
> 'special' ones. But, we can have that discussion in a different thread.

Yep, depending on what gives the smallest code/data size.

> There is still a need for "input-enable" and "output-enable" for the timer
> pins. Because, when you choose the pin to be connected to the MTU2 timer,
> the pin can be used as either input-capture/output-compare/PWM and that's
> the user's choice. So that's probably a valid usage of the generic pin
> properties for configuration.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SGkgR2VlcnQsDQoNCk9uIEZyaWRheSwgTWF5IDEyLCAyMDE3LCBHZWVydCBVeXR0ZXJob2V2ZW4g
d3JvdGU6DQo+IDEyIHggMTYgPSAxOTIsIG5vdCAzODQuDQoNCk9wcHMsIG15IG1hdGggd2FzIG9m
ZiENCg0KICAoSSB0aGluayBJIG5lZWQgYW5vdGhlciBjdXAgb2YgY29mZmVlIHRoaXMgbW9ybmlu
ZykNCg0KDQo+IERvIHlvdSBuZWVkIGFsbCBwb3NzaWJsZSBjb21iaW5hdGlvbnMgb2YgaW5wdXQs
IG91dHB1dCwgYW5kIGJpLWRpcj8NCj4gSSBhc3N1bWVkIHRoZXkncmUgbXV0dWFsbHkgZXhjbHVz
aXZlLiBJZiBub3QsIHlvdSBuZWVkIDMgYml0cy9waW4vZnVuY3Rpb24NCg0KWW91J3JlIHJpZ2h0
LCB0aGV5IGFyZSBtdXR1YWxseSBleGNsdXNpdmUsIHNvIDIgYml0cyBhcmUgZmluZS4NCg0KQWxz
bywgdGhlIG90aGVyIHBpbm91dCB2YXJpYXRpb25zIGNoaXBzIChSWi9BMUwsQTFMVSxBMUxDKSBv
bmx5IGhhdmUgMTAgcG9ydHM6DQoNCiAxMiBwb3J0cyB4IDE2IHBpbnMgeCAyYml0cy1wZXItYml0
ID0+IDM4NCBieXRlcw0KIDEwIHBvcnRzIHggMTYgcGlucyB4IDJiaXRzLXBlci1iaXQgPT4gMzIw
IGJ5dGVzDQoNCiAzODQgKyAzMjAgPSA3MDQgYnl0ZXMgKG9mIGNvbnN0IGRhdGEpDQoNCg0KPiBX
aXRoIHBhY2thZ2VzLCBkbyB5b3UgbWVhbiBlLmcuIFJaL0ExSCB2cy4gUlovQTFMPw0KDQpZZXMu
DQoNCg0KPiBUaGVzZSBpbmRlZWQgZGlmZmVyLA0KPiBidXQNCj4gc2hvdWxkIHVzZSBkaWZmZXJl
bnQgY29tcGF0aWJsZSB2YWx1ZXMuDQoNCk9LLiBTaW5jZSByN3M3MjEwMC5kdHNpIGhhcw0KICAg
IGNvbXBhdGlibGUgPSAicmVuZXNhcyxyN3M3MjEwMC1wb3J0cyI7DQoNCldlJ2xsIGp1c3Qgb3Zl
cnJpZGUgdGhhdCBpbiBzYXkgbXktcnphMWwtYm9hcmQuZHRzDQoNCiZwaW5jdHJsIHsNCgljb21w
YXRpYmxlID0gInJlbmVzYXMscjdzNzIxMDItcG9ydHMiOw0KfTsNCgkNCiAgYW5kIHRoZW4gbG9v
ayBmb3IgdGhhdCBpbiB0aGUgZHJpdmVyLg0KCQ0KDQo+IE9yIGRvIHlvdSBtZWFuIFFGUC9CR0Ey
NTYgdnMuIEJHQTMyND8gSXNuJ3QgdGhlIGZvcm1lciBhIHN1YnNldCBvZiB0aGUNCj4gbGF0dGVy
Pw0KDQpUaGUgInBhY2thZ2UiIGRvZXNuJ3QgbWF0dGVyLCBpdCdzIHRoZSBwaW4gYXNzaWdubWVu
dHMgb24gdGhlIGRpZToNCiAgUlovQTFIID0gUlovQTFNID0gcGluIGFzc2lnbm1lbnRzICMxDQog
IFJaL0ExTCA9IFJaL0ExTFUgPSBSWi9BMUxDID0gcGluIGFzc2lnbm1lbnRzICMyDQoNClRoZW4g
ZWFjaCBvZiB0aG9zZSBwYXJ0cyBhYm92ZSBoYXZlIG11bHRpcGxlICdwYWNrYWdlIG9wdGlvbnMn
LCBidXQgb2YNCmNvdXJzZSBkb2Vzbid0IGNoYW5nZSB0aGUgYWN0dWFsIHBpbi9mdW5jdGlvbiBh
c3NpZ25tZW50cyAodGhhdCdzIHBhcnQNCm9mIHRoZSBzaWxpY29uIGRpZSkNCg0KQ2hyaXMNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 9, 2017 at 5:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@jmondi.org> wrote:
>
>> From my perspective these flags are configurations internal to the pin
>> controller hardware used to enable/disable input buffers when a pin needs to
>> perform in both direction.
>> The level of detail I can provide on this is the logical diagram we have pointed
>> you to already.
>>
>> As I assume you are trying to get this answer from us in order to
>> avoid duplicating things in pin controller sub-system, and I
>> understand this, but my question here was "can we have those flags as part
>> of the pinmux property argument list, as that property description
>> seems to allow us to do that, instead of making them generic pin
>> configuration properties and upset other developers?"
>
> Pinmux with all it's magic flags baked into one is not any better
> or any more readable. The solution is already very pretty except
> for these two flags which I am sure we can agree on a way forward
> for.
>
> What we choose between is not this or another less transparent
> pin configuration mechanism, the mechanism (whether magic bits
> to pinmux or reasonable properties) does not matter.
>
> There is a strong preference to use the generic bindings.
>
> So the discussion is whether to use:
>
> bi-directional;
> output-enable;
>
> Or some already defined config flags.
>
> If these are too idiomatic to be used by others, they should anyways
> look similar, like:
>
> renesas,bi-directional;
> renesas,output-enable;
>
> Like the Qualcomm weirdness found in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
>
> qcom,pull-up-strength = <..>;
>
> Check how they use
> #define PMIC_GPIO_CONF_PULL_UP                 (PIN_CONFIG_END + 1)
>
> Etc.
>
>> Anyway, I still fail to see why those configuration flags, only
>> affecting the way the pin controller hardware enables/disables
>> its internal buffers and its internal operations have to be
>> described in term of their externally visible electrically characteristics.
>
> To me internal vs external is not what matters. What matters is
> if this is likely to pop up in more platforms, and then the property
> should be generic.
>
> The generic pin config definitions are likely to be picked up by other
> standards and even be inspiration to hardware engineers so that
> is why it matters so much.
>
>> To me, what already exists are pin configuration properties generic to
>> the whole pin controller subsystem, and I understand you don't want to
>> see duplication there.
>>
>> At the same time, to me, those flags are settings the pin controller
>> wants to have specified by software to overcome its hw design flaws,
>> and are intended to configure its internal buffers in a way it cannot
>> do by itself for some very specific operation modes (they are listed
>> in the hw reference manual, it's not something you can chose to
>> configure or not, if you want a pin working in i2c mode, you HAVE to
>> pass those flags to pin controller).
>
> Sounds like a case for
>
> renesas,bi-directional;
> renesas,output-enable;
>
> following the Qualcomm pattern in that case.
>
> But let's see if something else comes out of this discussion.
>

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.

Regards
Dong Aisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Linus,

On Tue, May 23, 2017 at 06:08:31PM +0800, Dong Aisheng wrote:
> On Tue, May 9, 2017 at 5:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@jmondi.org> wrote:
> >
> >> From my perspective these flags are configurations internal to the pin
> >> controller hardware used to enable/disable input buffers when a pin needs to
> >> perform in both direction.
> >> The level of detail I can provide on this is the logical diagram we have pointed
> >> you to already.
> >>
> >> As I assume you are trying to get this answer from us in order to
> >> avoid duplicating things in pin controller sub-system, and I
> >> understand this, but my question here was "can we have those flags as part
> >> of the pinmux property argument list, as that property description
> >> seems to allow us to do that, instead of making them generic pin
> >> configuration properties and upset other developers?"
> >
> > Pinmux with all it's magic flags baked into one is not any better
> > or any more readable. The solution is already very pretty except
> > for these two flags which I am sure we can agree on a way forward
> > for.
> >
> > What we choose between is not this or another less transparent
> > pin configuration mechanism, the mechanism (whether magic bits
> > to pinmux or reasonable properties) does not matter.
> >
> > There is a strong preference to use the generic bindings.
> >
> > So the discussion is whether to use:
> >
> > bi-directional;
> > output-enable;
> >
> > Or some already defined config flags.
> >
> > If these are too idiomatic to be used by others, they should anyways
> > look similar, like:
> >
> > renesas,bi-directional;
> > renesas,output-enable;
> >
> > Like the Qualcomm weirdness found in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
> >
> > qcom,pull-up-strength = <..>;
> >
> > Check how they use
> > #define PMIC_GPIO_CONF_PULL_UP                 (PIN_CONFIG_END + 1)
> >
> > Etc.
> >
> >> Anyway, I still fail to see why those configuration flags, only
> >> affecting the way the pin controller hardware enables/disables
> >> its internal buffers and its internal operations have to be
> >> described in term of their externally visible electrically characteristics.
> >
> > To me internal vs external is not what matters. What matters is
> > if this is likely to pop up in more platforms, and then the property
> > should be generic.
> >
> > The generic pin config definitions are likely to be picked up by other
> > standards and even be inspiration to hardware engineers so that
> > is why it matters so much.
> >
> >> To me, what already exists are pin configuration properties generic to
> >> the whole pin controller subsystem, and I understand you don't want to
> >> see duplication there.
> >>
> >> At the same time, to me, those flags are settings the pin controller
> >> wants to have specified by software to overcome its hw design flaws,
> >> and are intended to configure its internal buffers in a way it cannot
> >> do by itself for some very specific operation modes (they are listed
> >> in the hw reference manual, it's not something you can chose to
> >> configure or not, if you want a pin working in i2c mode, you HAVE to
> >> pass those flags to pin controller).
> >
> > Sounds like a case for
> >
> > renesas,bi-directional;
> > renesas,output-enable;
> >
> > following the Qualcomm pattern in that case.
> >
> > But let's see if something else comes out of this discussion.
> >
>
> 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.

I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked
in some previous email. I can send another version of that patch with
only 'output-enable' if you wish.

Once we reach consesus, I can then send v6 of our pin controller driver
based on that.

Thanks
   j

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0
>
> Regards
> Dong Aisheng
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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"?

> 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.

> 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 :/

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

>
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello Jacopo and Linus,

On Monday, May 29, 2017, jmondi wrote:
> > > 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.


Here is what I think:


In the case of this driver, after we remove the 'bi-directional'
properties and hide the other odd-ball pin configurations in an internal
table, we are left with the MTU2 timer pins that can be either input or
output depending on what you want to do with them.

 * If you want to use a MTU2 channel as a PWM, you set the pin as an
   output.
 * If you want to use a MTU2 channel as a input capture, you set the
   pin as an input.

They are simply "direction-input" and "direction-output" properties that
don't really need to talk about "buffers".


But, instead of making any new properties, for the Renesas driver, let's
just stick with what already exists today: 
 * If you want a MTU2 channel as a PWM: select "output-low"
 * If you want a MTU2 channel as a input capture: select "input-enable"


Side Note: You can also use output-high in addition to output-low
  because it doesn't matter (the driver can't set the pin level anyway
  because as soon as you assign the pin to MTU2, the MTU2 controls the
  pin, not the PFC). So the Renesas driver can check for both.



Chris

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html