Re: [PATCH] ARM: dts: document pinctrl-single,pins when #pinctrl-cells = 2

[Trent Piepho <tpiepho@xxxxxxxxx>]

On Tue, Sep 29, 2020 at 10:15 PM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> * Trent Piepho <tpiepho@xxxxxxxxx> [200929 20:16]:
> > On Thu, Sep 24, 2020 at 12:04 AM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> > > Certainly different compatible strings can be used as needed.
> > > But pinctrl-single is not going to be am335x specific though :)
> > > We have quite a few SoCs using it:
> >
> > So what doesn't make sense to me, is to put something am335x specific
> > like two cells for conf and mux, into a generic driver like pinctrl
> > single.
>
> Treating conf and mux separately is not am335x specific. Linux treats
> them separately because the conf options typically can be described
> in a generic way while the mux is just signal routing.

There's already a generic pinconf feature for pinctrl-single.  It
seems like this could be used with am335x now without any changes,
e.g. by adding "pinctrl-single,drive-strength" to define the bits that
control drive strength.  It doesn't require added cells to use this.
Pin mux and configuration fields all have masks defined.

Example, add this:
#define PULL_MASK (PULL_UP|PULL_DISABLE)
pinctrl-single,bias-pullup = <PULL_DISABLE PULL_UP PULL_DISABLE PULL_MASK>;
pinctrl-single,bias-pulldown = <PULL_DISABLE 0 PULL_DISABLE PULL_MASK>;

If I read the driver right (the bindings doc is far from clear), then
I think that configures the pin with pad pull up/down disabled and
allows the generic pinconf system to enable the pullup or pulldown.
Combining the definition of the fields with the value to initialize it
in the same property doesn't make that much sense to me.

> With interrupts the IRQ_TYPE flags are generic and separate from the
> hardware specific cells. If we wanted to, we could have something
> similar for pinctrl framework.

pinctrl-cells is really pretty different from gpio-cells and
interrupt-cells.  The latter two are used in handles that allow an
external node to reference something in the node that defines the gpio
or interrupt cells.  The generic flags are used by an *unrelated
driver* to tell an platform specific interrupt controller driver it
should configure an edge triggered interrupt.  It makes it easier to
use the binding in the unrelated driver that needs an interrupt since
the flags are always the same.  But mainly it works because the gpio
and interrupt framework in the kernel already support these concepts,
so the flags can get passed as is to existing kernel functions.

But pinctrl-single,pins is only used inside pinctrl-single itself.
There's no handle anywhere like:
yoyodyne,reset = <&am335x_pinmux AM335X_PIN_FOO MUX_MODE7
GENERIC_PULLUP_ENABLED_FLAG>;
I don't see how one could get made either, since there's already a
pinctrl system and it just doesn't work that way.

The closest thing would be the generic pin config type bindings, which
go in the pinctrl driver's nodes, and look like this:
&am335x_pinmux {
    pinctrl_yoyo_reset: yoyogrp {
        pins = "foo";
        function = "gpio";
        bias-pull-up;
    };
};

That would work on some other boards.  To use pinctrl-single, you'd
need to have a binding that mapped pin and function names to numbers
(why couldn't the pincfg binding use numbers!) and the bits/mask for
pull up.  Which could be done.  But at that point pinctrl-single,pins
is replaced, it wouldn't even get used, so adding more cells to it
hasn't done anything for you.

> > Consider also that any future changes to the pinctrl-single bindings
> > would need to be backward compatible with a device tree binary where
> > two cells get combined.  So if the bindings being added here aren't
> > done, then adding them now creates an unnecessary additional version
> > to deal with for backward compatibility.
>
> I don't see issues with backward compabilty. If we specify that the
> controller uses #pinctrl-cells = <2>, and some additional property
> for specifying generic conf flags, we'd have a similar generic binding
> to the interrupt binding.

Is "some additional property for specifying generic conf flags"
different from the existing pinctrl-single,bias-pullup, etc.
properties?  Because splitting the data cell into two parts doesn't
make any difference to those.