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

Groups > linux.kernel > #1652304 > unrolled thread

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

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-05-29 10:50 +0200
    Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-05-29 12:50 +0200
      RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-05-30 16:20 +0200

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

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

[toc] | [next] | [standalone]

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web