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

[jmondi]

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@xxxxxxxxxx> wrote:
> > On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@xxxxxxxxxx> 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