Pulls and drive strengths in the pinctrl world

Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Linus,

I'm currently working towards adapting exynos5250-snow (the ARM
Chromebook) to work well in the new world of pinctrl.  We've got a
backport of exynos5250 pinctrl in our kernel-3.8 tree and are now
fixing all of the bugs that have popped up.  Patches will be sent
upstream (where applicable) shortly.


...but I'm running into an issue when trying to specify pullups /
pulldowns and drive strengths on lines that are just GPIOs or just
interrupts.  This is important not just for power usage but also for
proper functioning (the default internal pulldown was overpowering the
weak external pullup in one case).  In the old GPIO specifier you
could do specify pulls, but the new simpler one doesn't allow it.

I've managed to make things work and you can see my progress at
<https://gerrit.chromium.org/gerrit/#/c/51232/>, but it feels a bit
awkward.  Is there a better way?  If so, then I'm all ears!  :)


If not, then I guess that what I have will have to do for now.  ...but
I really wish that:

* The GPIO specifier could specify initial drive strength and pull
values for pins.
* The interrupt specifier could specify pull values for pins (drive
strength shouldn't be needed since these are inputs).

Some examples from the gerrit CL referenced above...

Here's how I need to do things when I'm using "just an interrupt":

  pinctrl@11400000 {
    cyapa_irq: cyapa-irq {
      samsung,pins = "gpx1-2";
      samsung,pin-function = <0xf>;
      samsung,pin-pud = <0>;
      samsung,pin-drv = <0>;
    };
  };

  trackpad {
    reg = <0x67>;
    compatible = "cypress,cyapa";
    interrupts = <2 0>;
    interrupt-parent = <&gpx1>;
    pinctrl-names = "default";
    pinctrl-0 = <&cyapa_irq>;
    wakeup-source;
  };


I really wish I could add a 3rd number to the interrupt specifier for
pud and skip the pinctrl bit:

  trackpad {
    reg = <0x67>;
    compatible = "cypress,cyapa";
    interrupts = <2 0 0>;
    interrupt-parent = <&gpx1>;
    wakeup-source;
  };


An example with the GPIO specifier instead of the interrupt one:

  pinctrl@11400000 {
    ptn3460_gpios: ptn3460-gpios {
      samsung,pins = "gpy2-5", "gpx1-5";
      samsung,pin-function = <1>;
      samsung,pin-pud = <0>;
      samsung,pin-drv = <0>;
    };
  };

  ptn3460-bridge@20 {
    compatible = "nxp,ptn3460";
    reg = <0x20>;
    powerdown-gpio = <&gpy2 5 0>;
    reset-gpio = <&gpx1 5 0>;
    edid-emulation = <5>;
    pinctrl-names = "default";
    pinctrl-0 = <&ptn3460_gpios>;
  };


I don't want to specify function/direction (code can handle that), but do wish
I could specify the pulls and strength.  Perhaps:

  ptn3460-bridge@20 {
    compatible = "nxp,ptn3460";
    reg = <0x20>;
    powerdown-gpio = <&gpy2 5 0 0 0>;
    reset-gpio = <&gpx1 5 0 0 0>;
    edid-emulation = <5>;
  };


-Doug

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

Hi Doug,

There is no better way at the moment, but...

On Wednesday 15 of May 2013 09:44:22 Doug Anderson wrote:
> Linus,
> 
> I'm currently working towards adapting exynos5250-snow (the ARM
> Chromebook) to work well in the new world of pinctrl.  We've got a
> backport of exynos5250 pinctrl in our kernel-3.8 tree and are now
> fixing all of the bugs that have popped up.  Patches will be sent
> upstream (where applicable) shortly.
> 
> 
> ...but I'm running into an issue when trying to specify pullups /
> pulldowns and drive strengths on lines that are just GPIOs or just
> interrupts.  This is important not just for power usage but also for
> proper functioning (the default internal pulldown was overpowering the
> weak external pullup in one case).  In the old GPIO specifier you
> could do specify pulls, but the new simpler one doesn't allow it.
> 
> I've managed to make things work and you can see my progress at
> <https://gerrit.chromium.org/gerrit/#/c/51232/>, but it feels a bit
> awkward.  Is there a better way?  If so, then I'm all ears!  :)
> 
> 
> If not, then I guess that what I have will have to do for now.  ...but
> I really wish that:
> 
> * The GPIO specifier could specify initial drive strength and pull
> values for pins.
> * The interrupt specifier could specify pull values for pins (drive
> strength shouldn't be needed since these are inputs).
> 
> Some examples from the gerrit CL referenced above...
> 
> Here's how I need to do things when I'm using "just an interrupt":
> 
>   pinctrl@11400000 {
>     cyapa_irq: cyapa-irq {
>       samsung,pins = "gpx1-2";
>       samsung,pin-function = <0xf>;

You can omit samsung,pin-function here.

>       samsung,pin-pud = <0>;
>       samsung,pin-drv = <0>;

For inputs I guess you can omit samsung,pin-drv as well.

>     };
>   };
> 
>   trackpad {
>     reg = <0x67>;
>     compatible = "cypress,cyapa";
>     interrupts = <2 0>;
>     interrupt-parent = <&gpx1>;
>     pinctrl-names = "default";
>     pinctrl-0 = <&cyapa_irq>;
>     wakeup-source;
>   };
> 
> 
> I really wish I could add a 3rd number to the interrupt specifier for
> pud and skip the pinctrl bit:
> 
>   trackpad {
>     reg = <0x67>;
>     compatible = "cypress,cyapa";
>     interrupts = <2 0 0>;

Hmm, looks pretty good to me.

>     interrupt-parent = <&gpx1>;
>     wakeup-source;
>   };
> 
> 
> An example with the GPIO specifier instead of the interrupt one:
> 
>   pinctrl@11400000 {
>     ptn3460_gpios: ptn3460-gpios {
>       samsung,pins = "gpy2-5", "gpx1-5";
>       samsung,pin-function = <1>;
>       samsung,pin-pud = <0>;
>       samsung,pin-drv = <0>;
>     };
>   };
> 
>   ptn3460-bridge@20 {
>     compatible = "nxp,ptn3460";
>     reg = <0x20>;
>     powerdown-gpio = <&gpy2 5 0>;
>     reset-gpio = <&gpx1 5 0>;
>     edid-emulation = <5>;
>     pinctrl-names = "default";
>     pinctrl-0 = <&ptn3460_gpios>;
>   };
> 
> 
> I don't want to specify function/direction (code can handle that), but
> do wish I could specify the pulls and strength.  Perhaps:
> 
>   ptn3460-bridge@20 {
>     compatible = "nxp,ptn3460";
>     reg = <0x20>;
>     powerdown-gpio = <&gpy2 5 0 0 0>;
>     reset-gpio = <&gpx1 5 0 0 0>;

This looks fine to me as well.

Implementation of both shouldn't be too complicated, so it might be worth 
giving a try. Keep in mind that old bindings must be supported as well 
(based on #interrupt-cells and #gpio-cells values, I guess).

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Olof Johansson]

On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:

>> I don't want to specify function/direction (code can handle that), but
>> do wish I could specify the pulls and strength.  Perhaps:
>>
>>   ptn3460-bridge@20 {
>>     compatible = "nxp,ptn3460";
>>     reg = <0x20>;
>>     powerdown-gpio = <&gpy2 5 0 0 0>;
>>     reset-gpio = <&gpx1 5 0 0 0>;
>
> This looks fine to me as well.
>
> Implementation of both shouldn't be too complicated, so it might be worth
> giving a try. Keep in mind that old bindings must be supported as well
> (based on #interrupt-cells and #gpio-cells values, I guess).

Given the late discovery of this pretty major drawback, I don't think
we should worry too much about backwards compatibility in this case
and instead just move everyone over asap to whatever the new binding
is (when we agree on something).

Also, it looks like the gpio bindings were never updated with the
pinctrl changes. Tsk, tsk, tsk. Bad.

We have the option of either adding new fields for pulls and strength,
but we can also split the existing flags field similar to how we did
with the old binding, and take 8 bits each for pulls and strength, or
somesuch. That'll be less of a change w.r.t. existing device trees and
bindings.



-Olof

Re: Pulls and drive strengths in the pinctrl world

[Linus Walleij]

On Wed, May 15, 2013 at 6:44 PM, Doug Anderson <dianders@google.com> wrote:

Pls include Stephen Warren on mailings regarding DT mappings,
he's very good at this stuff.

> I'm running into an issue when trying to specify pullups /
> pulldowns and drive strengths on lines that are just GPIOs or just
> interrupts.  This is important not just for power usage but also for
> proper functioning (the default internal pulldown was overpowering the
> weak external pullup in one case).  In the old GPIO specifier you
> could do specify pulls, but the new simpler one doesn't allow it.

Background:

The idea with the subsystems is that the GPIO subsystem will
handle any aspect of "GPIOs" which are not necessarily
synonymous to pins.

So the two subsystems are orthogonal and the decisions in
each subsystem may result on combined electrical effects.

If there are cross-dependencies, GPIO ranges are used to
cross-map the GPIOs to pins.

> I've managed to make things work and you can see my progress at
> <https://gerrit.chromium.org/gerrit/#/c/51232/>, but it feels a bit
> awkward.  Is there a better way?  If so, then I'm all ears!  :

This seems good. default states are used to set up pins.

But please use the preprocessor to provide symbolic names for
the configurations. See for example these two patches from
J-C:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg32164.html

> I really wish that:
>
> * The GPIO specifier could specify initial drive strength and pull
> values for pins.
> * The interrupt specifier could specify pull values for pins (drive
> strength shouldn't be needed since these are inputs).

The subsystem does not differentiate between different configs,
what you say is that you want GPIO and interrupt specifiers
to pass arbitrary configs.

> Here's how I need to do things when I'm using "just an interrupt":
>
>   pinctrl@11400000 {
>     cyapa_irq: cyapa-irq {
>       samsung,pins = "gpx1-2";
>       samsung,pin-function = <0xf>;
>       samsung,pin-pud = <0>;
>       samsung,pin-drv = <0>;
>     };
>   };
>
>   trackpad {
>     reg = <0x67>;
>     compatible = "cypress,cyapa";
>     interrupts = <2 0>;
>     interrupt-parent = <&gpx1>;
>     pinctrl-names = "default";
>     pinctrl-0 = <&cyapa_irq>;
>     wakeup-source;
>   };
>
>
> I really wish I could add a 3rd number to the interrupt specifier for
> pud and skip the pinctrl bit:
>
>   trackpad {
>     reg = <0x67>;
>     compatible = "cypress,cyapa";
>     interrupts = <2 0 0>;
>     interrupt-parent = <&gpx1>;
>     wakeup-source;
>   };

I don't think the idea with device tree is to write as compact trees
as possible, but as expressive and exact yet abstract trees as
possible for OS independence.

Instead of referring to a node containing the config relevant for
another piece of hardware and associating this with the ampersand
notation (as is done with regulators, clocks, dma channels ...)
you here want to break that pattern totally and just hardcode
a numeric argument into the specifier. (Well could be a #define
using includes, but...)

That is not the pattern used so far I've seen to indicate dependent
resources. Dependent resources are passed using the ampersand.

While a number may suffice to describe all config for your hardware,
other pinctrl hardware needs more than one single numeric
argument. And device trees should be similar to each other.

If you're not doing that using the ampersand, the same could
potentially be done for a regulator powering a GPIO pin
and you get a fourth argument, and a fifth argument supplying
the color of the LED at the other end and ... I guess this is
why ampersands are being used.

Other than that I think you should ask a DT expert and I'm
no such.

> An example with the GPIO specifier instead of the interrupt one:
>
>   pinctrl@11400000 {
>     ptn3460_gpios: ptn3460-gpios {
>       samsung,pins = "gpy2-5", "gpx1-5";
>       samsung,pin-function = <1>;
>       samsung,pin-pud = <0>;
>       samsung,pin-drv = <0>;
>     };
>   };
>
>   ptn3460-bridge@20 {
>     compatible = "nxp,ptn3460";
>     reg = <0x20>;
>     powerdown-gpio = <&gpy2 5 0>;
>     reset-gpio = <&gpx1 5 0>;
>     edid-emulation = <5>;
>     pinctrl-names = "default";
>     pinctrl-0 = <&ptn3460_gpios>;
>   };
>
>
> I don't want to specify function/direction (code can handle that), but do wish
> I could specify the pulls and strength.  Perhaps:
>
>   ptn3460-bridge@20 {
>     compatible = "nxp,ptn3460";
>     reg = <0x20>;
>     powerdown-gpio = <&gpy2 5 0 0 0>;
>     reset-gpio = <&gpx1 5 0 0 0>;
>     edid-emulation = <5>;
>   };

Dito.

Yours,
Linus Walleij

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

Thanks for your comments.  I'm glad I'm not totally off-track.  I'll
respond to most things in reply to Linus' email, but a few here:

On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>   pinctrl@11400000 {
>>     cyapa_irq: cyapa-irq {
>>       samsung,pins = "gpx1-2";
>>       samsung,pin-function = <0xf>;
>
> You can omit samsung,pin-function here.

One potential reason for leaving them is the hopes that it might cause
a little less line glitching, especially in the case of outputs.
There is some delay between the pinmux being configured at the start
of device probe and the device actually claiming the GPIO.  Things
might be worse in the case of deferred probe (?).  Can you think of
any reason to remove (other than yet more lines of device tree to deal
with)?

>
>>       samsung,pin-pud = <0>;
>>       samsung,pin-drv = <0>;
>
> For inputs I guess you can omit samsung,pin-drv as well.

I will probably leave them even for inputs.  They shouldn't matter but
I like the idea of initting things to a known state...

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Linus,

Thank you for your comments.  See below...

Stephen: sorry for missing you earlier!  :(

On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> But please use the preprocessor to provide symbolic names for
> the configurations. See for example these two patches from
> J-C:
> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg32164.html

Ah, that does look nice!  This probably needs to be addressed in a
separate patch to cleanup all of the samsung pinctrl devicetrees.

> I don't think the idea with device tree is to write as compact trees
> as possible, but as expressive and exact yet abstract trees as
> possible for OS independence.

The compactness was one benefit, but also it was about trying to avoid
excessive duplication of information.  I found it awkward that I
needed to list that my interrupt was "gpx1-2" in two different ways.

I would find it just as good if I could express things like this (for
interrupts):

  pinctrl@11400000 {
    cyapa_irq: cyapa-irq {
      samsung,pins = "gpx1-2";
      samsung,pin-function = <0xf>;
      samsung,pin-pud = <0>;
      samsung,pin-drv = <0>;

      interrupt-controller;
      #interrupt-cells = <1>;
    };
  };

  trackpad {
    reg = <0x67>;
    compatible = "cypress,cyapa";
    interrupt-parent = <&cyapa_irq>;
    interrupts = <0>;
    wakeup-source;
  };

In this case I'm not saying that my interrupt parent is "gpx1-2" in
two separate places that could diverge.

I could come up with a similar example for GPIOs.


-Doug

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 14:19:18 Doug Anderson wrote:
> Tomasz,
> 
> Thanks for your comments.  I'm glad I'm not totally off-track.  I'll
> respond to most things in reply to Linus' email, but a few here:
> 
> On Wed, May 15, 2013 at 10:26 AM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> >>   pinctrl@11400000 {
> >>   
> >>     cyapa_irq: cyapa-irq {
> >>     
> >>       samsung,pins = "gpx1-2";
> >>       samsung,pin-function = <0xf>;
> > 
> > You can omit samsung,pin-function here.
> 
> One potential reason for leaving them is the hopes that it might cause
> a little less line glitching, especially in the case of outputs.
> There is some delay between the pinmux being configured at the start
> of device probe and the device actually claiming the GPIO.  Things
> might be worse in the case of deferred probe (?).  Can you think of
> any reason to remove (other than yet more lines of device tree to deal
> with)?

Well, actually in case of interrupts the function should not be configured 
manually, because it is likely to cause a false interrupt to be caught, 
before appropriate interrupt trigger type is configured. The correct way 
is to leave setting pin function to EINT to the pin control driver once 
the trigger gets configured (the pin control driver configures pin 
function from set_irq_type callback).

> >>       samsung,pin-pud = <0>;
> >>       samsung,pin-drv = <0>;
> > 
> > For inputs I guess you can omit samsung,pin-drv as well.
> 
> I will probably leave them even for inputs.  They shouldn't matter but
> I like the idea of initting things to a known state...

Well, the binding you proposed for interrupts doesn't initialize it. This 
is why I pointed that it can be omitted using current way as well.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 14:31:20 Doug Anderson wrote:
> Linus,
> 
> Thank you for your comments.  See below...
> 
> Stephen: sorry for missing you earlier!  :(
> 
> On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
> 
> <linus.walleij@linaro.org> wrote:
> > But please use the preprocessor to provide symbolic names for
> > the configurations. See for example these two patches from
> > J-C:
> > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg321
> > 64.html
> Ah, that does look nice!  This probably needs to be addressed in a
> separate patch to cleanup all of the samsung pinctrl devicetrees.
> 
> > I don't think the idea with device tree is to write as compact trees
> > as possible, but as expressive and exact yet abstract trees as
> > possible for OS independence.
> 
> The compactness was one benefit, but also it was about trying to avoid
> excessive duplication of information.  I found it awkward that I
> needed to list that my interrupt was "gpx1-2" in two different ways.
> 
> I would find it just as good if I could express things like this (for
> interrupts):
> 
>   pinctrl@11400000 {
>     cyapa_irq: cyapa-irq {
>       samsung,pins = "gpx1-2";
>       samsung,pin-function = <0xf>;
>       samsung,pin-pud = <0>;
>       samsung,pin-drv = <0>;
> 
>       interrupt-controller;
>       #interrupt-cells = <1>;
>     };
>   };
> 
>   trackpad {
>     reg = <0x67>;
>     compatible = "cypress,cyapa";
>     interrupt-parent = <&cyapa_irq>;
>     interrupts = <0>;
>     wakeup-source;
>   };
> 
> In this case I'm not saying that my interrupt parent is "gpx1-2" in
> two separate places that could diverge.

This will be hard, since the phandle in interrupt-parent is represented by 
an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit awkward 
to me.

Since we are already going to modify the binding, let's think a bit more 
about this problem and try to figure out a solution that doesn't add any 
disadvantages (at least any significant) to avoid such situation in future 
again.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 23:41:54 Tomasz Figa wrote:
> On Wednesday 15 of May 2013 14:31:20 Doug Anderson wrote:
> > Linus,
> > 
> > Thank you for your comments.  See below...
> > 
> > Stephen: sorry for missing you earlier!  :(
> > 
> > On Wed, May 15, 2013 at 11:29 AM, Linus Walleij
> > 
> > <linus.walleij@linaro.org> wrote:
> > > But please use the preprocessor to provide symbolic names for
> > > the configurations. See for example these two patches from
> > > J-C:
> > > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg3
> > > 21
> > > 64.html
> > 
> > Ah, that does look nice!  This probably needs to be addressed in a
> > separate patch to cleanup all of the samsung pinctrl devicetrees.
> > 
> > > I don't think the idea with device tree is to write as compact trees
> > > as possible, but as expressive and exact yet abstract trees as
> > > possible for OS independence.
> > 
> > The compactness was one benefit, but also it was about trying to avoid
> > excessive duplication of information.  I found it awkward that I
> > needed to list that my interrupt was "gpx1-2" in two different ways.
> > 
> > I would find it just as good if I could express things like this (for
> > 
> > interrupts):
> >   pinctrl@11400000 {
> >   
> >     cyapa_irq: cyapa-irq {
> >     
> >       samsung,pins = "gpx1-2";
> >       samsung,pin-function = <0xf>;
> >       samsung,pin-pud = <0>;
> >       samsung,pin-drv = <0>;
> >       
> >       interrupt-controller;
> >       #interrupt-cells = <1>;
> >     
> >     };
> >   
> >   };
> >   
> >   trackpad {
> >   
> >     reg = <0x67>;
> >     compatible = "cypress,cyapa";
> >     interrupt-parent = <&cyapa_irq>;
> >     interrupts = <0>;
> >     wakeup-source;
> >   
> >   };
> > 
> > In this case I'm not saying that my interrupt parent is "gpx1-2" in
> > two separate places that could diverge.
> 
> This will be hard, since the phandle in interrupt-parent is represented
> by an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit
> awkward to me.

Ahh, not even saying that a single interrupt signal is not really an 
interrupt controller, which would make device tree diverge from real 
hardware description.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

On Wed, May 15, 2013 at 2:36 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> One potential reason for leaving them is the hopes that it might cause
>> a little less line glitching, especially in the case of outputs.
>> There is some delay between the pinmux being configured at the start
>> of device probe and the device actually claiming the GPIO.  Things
>> might be worse in the case of deferred probe (?).  Can you think of
>> any reason to remove (other than yet more lines of device tree to deal
>> with)?
>
> Well, actually in case of interrupts the function should not be configured
> manually, because it is likely to cause a false interrupt to be caught,
> before appropriate interrupt trigger type is configured. The correct way
> is to leave setting pin function to EINT to the pin control driver once
> the trigger gets configured (the pin control driver configures pin
> function from set_irq_type callback).

Ah, OK.  I'll set to input for these.


>> I will probably leave them even for inputs.  They shouldn't matter but
>> I like the idea of initting things to a known state...
>
> Well, the binding you proposed for interrupts doesn't initialize it. This
> is why I pointed that it can be omitted using current way as well.

Agreed.  ...though you could say that the actual code in that case
would just be setting the drive strength to 0 (for consistency).  ;)

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

On Wed, May 15, 2013 at 2:41 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> This will be hard, since the phandle in interrupt-parent is represented by
> an IRQ domain in kernel code. One-interrupt IRQ domains seem a bit awkward
> to me.
>
> Since we are already going to modify the binding, let's think a bit more
> about this problem and try to figure out a solution that doesn't add any
> disadvantages (at least any significant) to avoid such situation in future
> again.

I'm definitely not super familiar with the implementation at that
level of detail, so don't take my proposed syntax as something I've
thought all the way through.  ...but hopefully you understand what I'm
getting at in terms of eliminating duplication?

Thanks!  :)

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 15:01:23 Doug Anderson wrote:
> Tomasz,
> 
> On Wed, May 15, 2013 at 2:41 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > This will be hard, since the phandle in interrupt-parent is
> > represented by an IRQ domain in kernel code. One-interrupt IRQ
> > domains seem a bit awkward to me.
> > 
> > Since we are already going to modify the binding, let's think a bit
> > more about this problem and try to figure out a solution that doesn't
> > add any disadvantages (at least any significant) to avoid such
> > situation in future again.
> 
> I'm definitely not super familiar with the implementation at that
> level of detail, so don't take my proposed syntax as something I've
> thought all the way through.  ...but hopefully you understand what I'm
> getting at in terms of eliminating duplication?

Yes. I don't like the current way too much either, duplication being one 
of the reasons.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Stephen Warren]

On 05/15/2013 12:29 PM, Linus Walleij wrote:
> On Wed, May 15, 2013 at 6:44 PM, Doug Anderson <dianders@google.com> wrote:
...
>> Here's how I need to do things when I'm using "just an interrupt":
>>
>>   pinctrl@11400000 {
>>     cyapa_irq: cyapa-irq {
>>       samsung,pins = "gpx1-2";
>>       samsung,pin-function = <0xf>;
>>       samsung,pin-pud = <0>;
>>       samsung,pin-drv = <0>;
>>     };
>>   };
>>
>>   trackpad {
>>     reg = <0x67>;
>>     compatible = "cypress,cyapa";
>>     interrupts = <2 0>;
>>     interrupt-parent = <&gpx1>;
>>     pinctrl-names = "default";
>>     pinctrl-0 = <&cyapa_irq>;
>>     wakeup-source;
>>   };

I don't really see much disadvantage here; the interrupt bindings
specify things related to interrupts and the pinctrl bindings specify
thing related to pin configuration.

If you want to condense the DT, I'd suggest using a the pinctrl hogging
feature, i.e. don't put pinctrl-* properties in the trackpad node, but
rather define a system-wide "default" pinctrl state in the pin
controller node itself. That configuration will be applied as soon as
the pin controller driver is registered. That'd be the same as the
above, with the following added:

pinctrl@11400000 {
    pinctrl-names = "default";
    pinctrl-0 = <&cyapa_irq>;
};

except that the pinctrl-0 property would probably end up configuring a
whole bunch of basic pinctrl state rather than just that one interrupt pin.

I prefer to put all the static pinctrl configuration in the pinctrl hog,
and only the dynamic stuff in the individual device nodes.

I know LinusW won't like this suggestion much though:-)

>> I really wish I could add a 3rd number to the interrupt specifier for
>> pud and skip the pinctrl bit:
>>
>>   trackpad {
>>     reg = <0x67>;
>>     compatible = "cypress,cyapa";
>>     interrupts = <2 0 0>;
>>     interrupt-parent = <&gpx1>;
>>     wakeup-source;
>>   };

I don't like that myself, since it makes the interrupt binding (and I
assume you'd want to go back to the similar misuse in the GPIO binding)
also configure pinctrl-related stuff.

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz / Linus,

On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Yes. I don't like the current way too much either, duplication being one
> of the reasons.

Do you have any other ideas?  It sounds like Linus didn't like my
suggestion and makes some good points...

I don't have any other great ideas other than having an argument about
whether the concept of pulls should be added to the GPIO subsystem
(and backed by pinmux).  Then we could make an argument that default
pull state belonged in the GPIO specifier.  OK, maybe we should just
pretend that I didn't bring that up.  ;)

...but I'm definitely interested in other ideas to eliminate the
duplication.  Until then I'm planning on submitting what I have
locally.  I'll probably send some version of it upstream fairly soon.
I will probably submit without trying to get all the preprocessor
symbols names done and will understand if Linus NAKs because of that.
I don't have time to take that on at the moment but can always come
back and rework that patch later.  ;)

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Stephen,

On Wed, May 15, 2013 at 4:51 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> I don't really see much disadvantage here; the interrupt bindings
> specify things related to interrupts and the pinctrl bindings specify
> thing related to pin configuration.

OK.  If this is the best way then I can accept that and maybe we
should just drop this thread.  What do people think?  It means less
work for me in the short term since I've already got it implemented
that way and then I don't need to submit any patches to try to change
things!  ;)


> If you want to condense the DT, I'd suggest using a the pinctrl hogging
> feature, i.e. don't put pinctrl-* properties in the trackpad node, but
> rather define a system-wide "default" pinctrl state in the pin
> controller node itself. That configuration will be applied as soon as
> the pin controller driver is registered. That'd be the same as the
> above, with the following added:
>
> pinctrl@11400000 {
>     pinctrl-names = "default";
>     pinctrl-0 = <&cyapa_irq>;
> };
>
> except that the pinctrl-0 property would probably end up configuring a
> whole bunch of basic pinctrl state rather than just that one interrupt pin.
>
> I prefer to put all the static pinctrl configuration in the pinctrl hog,
> and only the dynamic stuff in the individual device nodes.
>
> I know LinusW won't like this suggestion much though:-)

Ah right!  I forgot about hogs in this case.  That's also reasonable
as a solution and is similar to what we've got in the tree for
powerdown configuration of pins (I'll try to post this patch soon too,
WIP at <https://gerrit.chromium.org/gerrit/#/c/51292/> and
<https://gerrit.chromium.org/gerrit/#/c/51372/>.

It sounds like there's a bit of disagreement about the "best way" so
I'll probably keep the way I have.  ...but I'll keep hogs in my back
pocket.


> I don't like that myself, since it makes the interrupt binding (and I
> assume you'd want to go back to the similar misuse in the GPIO binding)
> also configure pinctrl-related stuff.

Fair enough.  :)

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> Tomasz / Linus,
> 
> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > Yes. I don't like the current way too much either, duplication being
> > one of the reasons.
> 
> Do you have any other ideas?  It sounds like Linus didn't like my
> suggestion and makes some good points...

I don't have anything interesting at the moment. It's a bit late now here 
(2 AM), so I'm going to get some sleep first.

Also after reading Stephen's reply, I'm wondering if hogging wouldn't 
solve the problem indeed. (It might have to be fixed on pinctrl-samsung 
first, as last time I tried to use it, it caused some errors from pinctrl 
core, but haven't time to track them down, as it wasn't anything important 
at that time).

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Wednesday 15 of May 2013 17:03:44 Doug Anderson wrote:
> Stephen,
> 
> On Wed, May 15, 2013 at 4:51 PM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> > I don't really see much disadvantage here; the interrupt bindings
> > specify things related to interrupts and the pinctrl bindings specify
> > thing related to pin configuration.
> 
> OK.  If this is the best way then I can accept that and maybe we
> should just drop this thread.  What do people think?  It means less
> work for me in the short term since I've already got it implemented
> that way and then I don't need to submit any patches to try to change
> things!  ;)
> 
> > If you want to condense the DT, I'd suggest using a the pinctrl
> > hogging
> > feature, i.e. don't put pinctrl-* properties in the trackpad node, but
> > rather define a system-wide "default" pinctrl state in the pin
> > controller node itself. That configuration will be applied as soon as
> > the pin controller driver is registered. That'd be the same as the
> > above, with the following added:
> > 
> > pinctrl@11400000 {
> > 
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&cyapa_irq>;
> > 
> > };
> > 
> > except that the pinctrl-0 property would probably end up configuring a
> > whole bunch of basic pinctrl state rather than just that one interrupt
> > pin.
> > 
> > I prefer to put all the static pinctrl configuration in the pinctrl
> > hog, and only the dynamic stuff in the individual device nodes.
> > 
> > I know LinusW won't like this suggestion much though:-)
> 
> Ah right!  I forgot about hogs in this case.  That's also reasonable
> as a solution and is similar to what we've got in the tree for
> powerdown configuration of pins (I'll try to post this patch soon too,
> WIP at <https://gerrit.chromium.org/gerrit/#/c/51292/> and
> <https://gerrit.chromium.org/gerrit/#/c/51372/>.

Hmm, last thing before I fell asleep: We already have support for power 
down configuration in pinctrl-samsung. See samsung,pin-conpdn and 
samsung,pin-pudpdn.

Also I already have patches for suspend/resume support of pinctrl-samsung 
and pinctrl-exynos, as well as configuration of wake-up sources. I'm going 
to post them soon.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Stephen Warren]

On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
>> Tomasz / Linus,
>>
>> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa <tomasz.figa@gmail.com> 
> wrote:
>>> Yes. I don't like the current way too much either, duplication being
>>> one of the reasons.
>>
>> Do you have any other ideas?  It sounds like Linus didn't like my
>> suggestion and makes some good points...
> 
> I don't have anything interesting at the moment. It's a bit late now here 
> (2 AM), so I'm going to get some sleep first.
> 
> Also after reading Stephen's reply, I'm wondering if hogging wouldn't 
> solve the problem indeed. (It might have to be fixed on pinctrl-samsung 
> first, as last time I tried to use it, it caused some errors from pinctrl 
> core, but haven't time to track them down, as it wasn't anything important 
> at that time).

One issue I noticed with the DT fragments earlier in this thread. It
looks like hogs in the Samsung pinctrl bingings end up looking like:

pinctrl {
    pina {
        samsung,pins = <PIN_A PIN_B PIN_C>;
        samsung,pin-function = <0xf>;
        samsung,pin-pud = <0>;
        ...
    };
    pinp {
        samsung,pins = <PIN_P PIN_Q>;
        samsung,pin-function = <0xe>;
        samsung,pin-pud = <1>;
        ...
    };
    pinx {
        samsung,pins = <PIN_X PIN_Y PIN_Z>;
        samsung,pin-function = <0xd>;
        samsung,pin-pud = <2>;
        ...
    };

    pinctrl-names = "default";
    pinctrl-0 = <&pina &pinp &pinx>;
};

That pinctrl-0 property could get rather large (hard to write/maintain,
unwieldy) if it needs to set up lots of different configurations. That's
why I made the equivalent Tegra bindings be:

pinctrl {
    pins_default {
        pina {
            samsung,pins = <PIN_A PIN_B PIN_C>;
            samsung,pin-function = <0xf>;
            samsung,pin-pud = <0>;
            ...
        };
        pinp {
            samsung,pins = <PIN_P PIN_Q>;
            samsung,pin-function = <0xe>;
            samsung,pin-pud = <1>;
            ...
        };
        pinx {
            samsung,pins = <PIN_X PIN_Y PIN_Z>;
            samsung,pin-function = <0xd>;
            samsung,pin-pud = <2>;
            ...
        };
    };

    pinctrl-names = "default";
    pinctrl-0 = <&pins_default>;
};

The extra level within the "pinctrl configuration node" ("pins_default"
here) makes the pinctrl-0 property a lot easier to write, and the
advantage happens at every use-site that needs to configure different
subsets of the relevant pins in different ways.

If you're changing all the bindings anyway, introducing this extra level
might be something to think about.

I did try to explain my philosophy here when we all got together to
design the pinctrl bindings, but I obviously didn't explain it well
enough, or people didn't like it anyway.

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

On Wed, May 15, 2013 at 5:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> I don't have anything interesting at the moment. It's a bit late now here
> (2 AM), so I'm going to get some sleep first.

Sorry for keeping you up.  Sleep is good!


> Also after reading Stephen's reply, I'm wondering if hogging wouldn't
> solve the problem indeed. (It might have to be fixed on pinctrl-samsung
> first, as last time I tried to use it, it caused some errors from pinctrl
> core, but haven't time to track them down, as it wasn't anything important
> at that time).

I will give it a shot tomorrow morning and see how it looks.  I'll
also evaluate Stephen's suggestions then once I've see how it looks
with the current bindings...

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

On Wed, May 15, 2013 at 5:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hmm, last thing before I fell asleep: We already have support for power
> down configuration in pinctrl-samsung. See samsung,pin-conpdn and
> samsung,pin-pudpdn.

Dang it! OK, we'll work on using those.


> Also I already have patches for suspend/resume support of pinctrl-samsung
> and pinctrl-exynos, as well as configuration of wake-up sources. I'm going
> to post them soon.

Huh, I wonder if they look just like the one we've been working on:
* https://gerrit.chromium.org/gerrit/#/c/51336/
* https://gerrit.chromium.org/gerrit/#/c/51342/

Those are about ready for upstream, too.  I was going to send them
this morning when I found out that we were missing a bunch of pinctrl
patches and had to rework then.  :(

Anyway, we can compare our solutions and figure out which is better.
;)  I'm happy with anything that works!

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz / Stephen,

On Wed, May 15, 2013 at 5:55 PM, Doug Anderson <dianders@google.com> wrote:
>> Also after reading Stephen's reply, I'm wondering if hogging wouldn't
>> solve the problem indeed. (It might have to be fixed on pinctrl-samsung
>> first, as last time I tried to use it, it caused some errors from pinctrl
>> core, but haven't time to track them down, as it wasn't anything important
>> at that time).
>
> I will give it a shot tomorrow morning and see how it looks.  I'll
> also evaluate Stephen's suggestions then once I've see how it looks
> with the current bindings...

I wrote this out and it had some nice properties (it was a little more
concise), but had the downside that there's no reference from the GPIO
usage back to the pinmux.  I'd rather have the reference there in the
hopes that it will help others get things right when they make changes
to the dts file (they'll notice the reference and know that they need
to change that too).

...so I think the summary is: I'm OK with keeping what we have.  It
may be a little awkward in some ways but it's definitely worth it to
get all of the benefits of the pinmux / GPIO separation.  :)

For the curious of what my prototype looked like (feel free to
ignore--I'm not planning on keeping this and I didn't actually try
testing it), I've included it below.  This is just the bit from
"cros5250-common" (the common file shared among several similar
boards), so I'd need something similar in "exynos5250-snow").

pinctrl@11400000 {
  /* Default states for hogs follow */
  nopull_inputs_cros5250_a: nopull-inputs-cros5250-a {
    samsung,pins =
      "gpx1-2", /* trackpad */
      "gpx1-3", /* gpio-keys - power */
      "gpx3-2"; /* max77686 */
    samsung,pin-function = <0>;
    samsung,pin-pud = <0>;
    samsung,pin-drv = <0>;
  };

  pulldown_inputs_cros5250-a: pulldown-inputs-cros5250_a {
    samsung,pins = "gpx3-7"; /* hdmi */
    samsung,pin-function = <0>;
    samsung,pin-pud = <1>;
    samsung,pin-drv = <0>;
  };

  simple_outputs_cros5250-a: simple-outputs-cros5250_a {
    samsung,pins =
      "gpx0-1", /* wifi-en */
      "gpx0-2", /* wifi-rst */
      "gpx1-7"; /* max98095-en */
    samsung,pin-function = <1>;
    samsung,pin-pud = <0>;
    samsung,pin-drv = <0>;
  }

  pinctrl-names = "default";
  pinctrl-0 = <&nopull_inputs_cros5250_a
               &pulldown_inputs_cros5250_a
               &simple_outputs_cros5250_a>;
};

pinctrl@13400000 {
  simple_outputs_cros5250-b: simple-outputs-cros5250_b {
    samsung,pins = "gpe1-0" /* hsic reset */;
    samsung,pin-function = <1>;
    samsung,pin-pud = <0>;
    samsung,pin-drv = <0>;

  };
  pinctrl-names = "default";
  pinctrl-0 = <&simple_outputs_cros5250_b>;
};

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Linus Walleij]

On Thu, May 16, 2013 at 2:03 AM, Doug Anderson <dianders@google.com> wrote:

>> I prefer to put all the static pinctrl configuration in the pinctrl hog,
>> and only the dynamic stuff in the individual device nodes.
>>
>> I know LinusW won't like this suggestion much though:-)

(It's not that bad actually...)

> Ah right!  I forgot about hogs in this case.  That's also reasonable
> as a solution and is similar to what we've got in the tree for
> powerdown configuration of pins (I'll try to post this patch soon too,
> WIP at <https://gerrit.chromium.org/gerrit/#/c/51292/> and
> <https://gerrit.chromium.org/gerrit/#/c/51372/>.

I don't like these Gerrit patches really, it's better to move
this to the pinctrl core using hogs.

If you look in drivers/pinctr/core.c you can find this:

pinctrl_register()
{
(...)
        if (!IS_ERR(pctldev->p)) {
                pctldev->hog_default =
                        pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
                if (IS_ERR(pctldev->hog_default)) {
                        dev_dbg(dev, "failed to lookup the default state\n");
                } else {
                        if (pinctrl_select_state(pctldev->p,
                                                pctldev->hog_default))
                                dev_err(dev,
                                        "failed to select default state\n");
                }

                pctldev->hog_sleep =
                        pinctrl_lookup_state(pctldev->p,
                                                    PINCTRL_STATE_SLEEP);
                if (IS_ERR(pctldev->hog_sleep))
                        dev_dbg(dev, "failed to lookup the sleep state\n");
        }

Just add another state, pctldev->hog_shutdown to this, and
add an operation pinctrl_force_poweroff() in the same spirit as
pinctrl_force_sleep() that we already have.

Add a new state to include/linux/pinctrl/pinctrl-state.h:
#define PINCTRL_STATE_POWEROFF "poweroff"

And define you pin table to hog these pins with the mentioned
default and poweroff states.

Result: pinctrl core keeps track of your offstate too.

Yours,
Linus Walleij

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

Hi Linus,

On Friday 17 of May 2013 10:38:53 Linus Walleij wrote:
> On Thu, May 16, 2013 at 2:03 AM, Doug Anderson <dianders@google.com> 
wrote:
> >> I prefer to put all the static pinctrl configuration in the pinctrl
> >> hog, and only the dynamic stuff in the individual device nodes.
> >> 
> >> I know LinusW won't like this suggestion much though:-)
> 
> (It's not that bad actually...)
> 
> > Ah right!  I forgot about hogs in this case.  That's also reasonable
> > as a solution and is similar to what we've got in the tree for
> > powerdown configuration of pins (I'll try to post this patch soon too,
> > WIP at <https://gerrit.chromium.org/gerrit/#/c/51292/> and
> > <https://gerrit.chromium.org/gerrit/#/c/51372/>.
> 
> I don't like these Gerrit patches really, it's better to move
> this to the pinctrl core using hogs.
> 
> If you look in drivers/pinctr/core.c you can find this:
> 
> pinctrl_register()
> {
> (...)
>         if (!IS_ERR(pctldev->p)) {
>                 pctldev->hog_default =
>                         pinctrl_lookup_state(pctldev->p,
> PINCTRL_STATE_DEFAULT); if (IS_ERR(pctldev->hog_default)) {
>                         dev_dbg(dev, "failed to lookup the default
> state\n"); } else {
>                         if (pinctrl_select_state(pctldev->p,
>                                                 pctldev->hog_default))
>                                 dev_err(dev,
>                                         "failed to select default
> state\n"); }
> 
>                 pctldev->hog_sleep =
>                         pinctrl_lookup_state(pctldev->p,
>                                                    
> PINCTRL_STATE_SLEEP); if (IS_ERR(pctldev->hog_sleep))
>                         dev_dbg(dev, "failed to lookup the sleep
> state\n"); }
> 
> Just add another state, pctldev->hog_shutdown to this, and
> add an operation pinctrl_force_poweroff() in the same spirit as
> pinctrl_force_sleep() that we already have.
> 
> Add a new state to include/linux/pinctrl/pinctrl-state.h:
> #define PINCTRL_STATE_POWEROFF "poweroff"
> 
> And define you pin table to hog these pins with the mentioned
> default and poweroff states.
> 
> Result: pinctrl core keeps track of your offstate too.

Power down mode settings on our pin controller are completely unrelated to 
normal mode settings. You can set them once in appropriate registers and 
pins are switched to them automatically when the SoC enters sleep mode.

So IMHO in our case power mode settings are just additional pin 
configuration options, next to pull-up/-down and driver strength.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Linus Walleij]

On Fri, May 17, 2013 at 11:09 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:

>> Just add another state, pctldev->hog_shutdown to this, and
>> add an operation pinctrl_force_poweroff() in the same spirit as
>> pinctrl_force_sleep() that we already have.
>>
>> Add a new state to include/linux/pinctrl/pinctrl-state.h:
>> #define PINCTRL_STATE_POWEROFF "poweroff"
>>
>> And define you pin table to hog these pins with the mentioned
>> default and poweroff states.
>>
>> Result: pinctrl core keeps track of your offstate too.
>
> Power down mode settings on our pin controller are completely unrelated to
> normal mode settings. You can set them once in appropriate registers and
> pins are switched to them automatically when the SoC enters sleep mode.

Aha so it's actually automatic sleep modes, not power down
(as in, disconnect the power and then push the "on" button to
get it back up).

Please remember to document it per above in the code and the
device tree, so everybody understands what it is.

> So IMHO in our case power mode settings are just additional pin
> configuration options, next to pull-up/-down and driver strength.

I see. Yes that is different.

You might want to have a debugfs file in your driver for inspecting
them though, that sounds like it could be helpful. I'd recommend
augmenting your .pin_config_dbg_show() callback in the
struct pinconf_ops to display this for each pin, in addition to the
current configuration.

Yours,
Linus Walleij

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:22 Wed 15 May     , Stephen Warren wrote:
> On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> >> Tomasz / Linus,
> >>
> >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
> > wrote:
> >>> Yes. I don't like the current way too much either, duplication being
> >>> one of the reasons.
> >>
> >> Do you have any other ideas?  It sounds like Linus didn't like my
> >> suggestion and makes some good points...
> > 
> > I don't have anything interesting at the moment. It's a bit late now here 
> > (2 AM), so I'm going to get some sleep first.
> > 
> > Also after reading Stephen's reply, I'm wondering if hogging wouldn't 
> > solve the problem indeed. (It might have to be fixed on pinctrl-samsung 
> > first, as last time I tried to use it, it caused some errors from pinctrl 
> > core, but haven't time to track them down, as it wasn't anything important 
> > at that time).
> 
> One issue I noticed with the DT fragments earlier in this thread. It
> looks like hogs in the Samsung pinctrl bingings end up looking like:
> 
> pinctrl {
>     pina {
>         samsung,pins = <PIN_A PIN_B PIN_C>;
>         samsung,pin-function = <0xf>;
>         samsung,pin-pud = <0>;
>         ...

I have a huge issue here that we had on at91 too

we are going to have a huge numbet of node

and on at91 we handle the pin the same way as samsung
and ST have also a similiar IP

so I'll prefer to reuse the AT91 DT bindings

as said by Linus I just push a cleanup of the magic by using Macro
which make it really readable now

some extract of the sama5 pinctrl

	mmc0 {
		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
			atmel,pins =
				<AT91_PIOD 9 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK */
				 AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A MCI0_CDA with pullup */
				 AT91_PIOD 1 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */
		};
		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
			atmel,pins =
				<AT91_PIOD 2 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD2 periph A MCI0_DA1 with pullup */
				 AT91_PIOD 3 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */
				 AT91_PIOD 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3 with pullup */
		};
		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
			atmel,pins =
				<AT91_PIOD 5 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD5 periph A MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */
				 AT91_PIOD 6 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 with pullup, conflicts with TIOB0, PWML2 */
				 AT91_PIOD 7 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, conlicts with TCLK0, PWMH3 */
				 AT91_PIOD 8 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup, conflicts with PWML3 */
		};
	};

of sam9g45

	i2c_gpio2 {
		pinctrl_i2c_gpio2: i2c_gpio2-0 {
			atmel,pins =
				<AT91_PIOB 4 AT91_PERIPH_GPIO AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio multidrive I2C2 data */
				 AT91_PIOB 5 AT91_PERIPH_GPIO AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock */
		};
	};

so we could share the c code too

Best Regards,
J,
>     };
>     pinp {
>         samsung,pins = <PIN_P PIN_Q>;
>         samsung,pin-function = <0xe>;
>         samsung,pin-pud = <1>;
>         ...
>     };
>     pinx {
>         samsung,pins = <PIN_X PIN_Y PIN_Z>;
>         samsung,pin-function = <0xd>;
>         samsung,pin-pud = <2>;
>         ...
>     };
> 
>     pinctrl-names = "default";
>     pinctrl-0 = <&pina &pinp &pinx>;
> };
> 
> That pinctrl-0 property could get rather large (hard to write/maintain,
> unwieldy) if it needs to set up lots of different configurations. That's
> why I made the equivalent Tegra bindings be:
> 
> pinctrl {
>     pins_default {
>         pina {
>             samsung,pins = <PIN_A PIN_B PIN_C>;
>             samsung,pin-function = <0xf>;
>             samsung,pin-pud = <0>;
>             ...
>         };
>         pinp {
>             samsung,pins = <PIN_P PIN_Q>;
>             samsung,pin-function = <0xe>;
>             samsung,pin-pud = <1>;
>             ...
>         };
>         pinx {
>             samsung,pins = <PIN_X PIN_Y PIN_Z>;
>             samsung,pin-function = <0xd>;
>             samsung,pin-pud = <2>;
>             ...
>         };
>     };
> 
>     pinctrl-names = "default";
>     pinctrl-0 = <&pins_default>;
> };
> 
> The extra level within the "pinctrl configuration node" ("pins_default"
> here) makes the pinctrl-0 property a lot easier to write, and the
> advantage happens at every use-site that needs to configure different
> subsets of the relevant pins in different ways.
> 
> If you're changing all the bindings anyway, introducing this extra level
> might be something to think about.
> 
> I did try to explain my philosophy here when we all got together to
> design the pinctrl bindings, but I obviously didn't explain it well
> enough, or people didn't like it anyway.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

Hi Linus,

On Friday 17 of May 2013 13:59:54 Linus Walleij wrote:
> On Fri, May 17, 2013 at 11:09 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >> Just add another state, pctldev->hog_shutdown to this, and
> >> add an operation pinctrl_force_poweroff() in the same spirit as
> >> pinctrl_force_sleep() that we already have.
> >> 
> >> Add a new state to include/linux/pinctrl/pinctrl-state.h:
> >> #define PINCTRL_STATE_POWEROFF "poweroff"
> >> 
> >> And define you pin table to hog these pins with the mentioned
> >> default and poweroff states.
> >> 
> >> Result: pinctrl core keeps track of your offstate too.
> > 
> > Power down mode settings on our pin controller are completely unrelated to
> > normal mode settings. You can set them once in appropriate registers and
> > pins are switched to them automatically when the SoC enters sleep mode.
> 
> Aha so it's actually automatic sleep modes, not power down
> (as in, disconnect the power and then push the "on" button to
> get it back up).
> 
> Please remember to document it per above in the code and the
> device tree, so everybody understands what it is.

Sure.

> > So IMHO in our case power mode settings are just additional pin
> > configuration options, next to pull-up/-down and driver strength.
> 
> I see. Yes that is different.
> 
> You might want to have a debugfs file in your driver for inspecting
> them though, that sounds like it could be helpful. I'd recommend
> augmenting your .pin_config_dbg_show() callback in the
> struct pinconf_ops to display this for each pin, in addition to the
> current configuration.

Seems reasonable.

Best regards,
-- 
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics

Re: Pulls and drive strengths in the pinctrl world

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 5:38 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> You might want to have a debugfs file in your driver for inspecting
>> them though, that sounds like it could be helpful. I'd recommend
>> augmenting your .pin_config_dbg_show() callback in the
>> struct pinconf_ops to display this for each pin, in addition to the
>> current configuration.
>
> Seems reasonable.

If you do this I'd love to be CCed.  I have a super-ugly userspace
tool that shows this info and have been contemplating moving it into
debugfs and cleaning it up there.

See <https://gerrit.chromium.org/gerrit/#/c/51380/>

A small snippet of the output looks like:

GPIO x3[4]@0x11400c60: con=0x0 dat=0 pulldwn(1) drv=1(0)
GPIO x3[5]@0x11400c60: con=0xf dat=1  nopull(0) drv=1(0)
GPIO x3[6]@0x11400c60: con=0x0 dat=1 pulldwn(1) drv=1(0)
GPIO x3[7]@0x11400c60: con=0x0 dat=0 pulldwn(1) drv=1(0)
GPIO e0[0]@0x13400000: con=0x0 dat=0 pulldwn(1) drv=1(0) pdn= low(0),
pdn= nopull(0)
GPIO e0[1]@0x13400000: con=0x0 dat=0 pulldwn(1) drv=1(0) pdn=  in(2),
pdn=pulldwn(1)
GPIO e0[2]@0x13400000: con=0x0 dat=0 pulldwn(1) drv=1(0) pdn=  in(2),
pdn=pulldwn(1)

-Doug

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

Hi Jean-Christophe,

On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:22 Wed 15 May     , Stephen Warren wrote:
> > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > >> Tomasz / Linus,
> > >> 
> > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > >> <tomasz.figa@gmail.com>
> > > 
> > > wrote:
> > >>> Yes. I don't like the current way too much either, duplication
> > >>> being
> > >>> one of the reasons.
> > >> 
> > >> Do you have any other ideas?  It sounds like Linus didn't like my
> > >> suggestion and makes some good points...
> > > 
> > > I don't have anything interesting at the moment. It's a bit late now
> > > here (2 AM), so I'm going to get some sleep first.
> > > 
> > > Also after reading Stephen's reply, I'm wondering if hogging
> > > wouldn't
> > > solve the problem indeed. (It might have to be fixed on
> > > pinctrl-samsung
> > > first, as last time I tried to use it, it caused some errors from
> > > pinctrl core, but haven't time to track them down, as it wasn't
> > > anything important at that time).
> > 
> > One issue I noticed with the DT fragments earlier in this thread. It
> > looks like hogs in the Samsung pinctrl bingings end up looking like:
> > 
> > pinctrl {
> > 
> >     pina {
> >     
> >         samsung,pins = <PIN_A PIN_B PIN_C>;
> >         samsung,pin-function = <0xf>;
> >         samsung,pin-pud = <0>;
> >         ...
> 
> I have a huge issue here that we had on at91 too
> 
> we are going to have a huge numbet of node
> 
> and on at91 we handle the pin the same way as samsung
> and ST have also a similiar IP
> 
> so I'll prefer to reuse the AT91 DT bindings
> 
> as said by Linus I just push a cleanup of the magic by using Macro
> which make it really readable now
> 
> some extract of the sama5 pinctrl
> 
> 	mmc0 {
> 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> 			atmel,pins =
> 				<AT91_PIOD 9 AT91_PERIPH_A 
AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> 			atmel,pins =
> 				<AT91_PIOD 2 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP	/* PD2 periph A
> MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */ AT91_PIOD
> 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3 
with
> pullup */ };
> 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> 			atmel,pins =
> 				<AT91_PIOD 5 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP	/* PD5 periph A
> MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 with
> pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, conlicts
> with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup, 
conflicts
> with PWML3 */ };
> 	};
> 
> of sam9g45
> 
> 	i2c_gpio2 {
> 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> 			atmel,pins =
> 				<AT91_PIOB 4 AT91_PERIPH_GPIO 
AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock */ };
> 	};
> 
> so we could share the c code too

I'd have a question with regard to AT91 bindings.

Using Samsung bindings we don't need to specify all configuration options 
for a pin, only those that are relevant for the platform. Do your bindings 
allow this?

Apparently AT91 has less configurable things and those available are 
usually always configured together so it's not a problem. But on our SoCs 
we have a bit more of them:
- function (input, output, special functions)
- pull-down/-up
- driver strength
- power down mode function (input, output low, output high, retention)
- power down mode pull-down/-up
- one could argue that default output value could be set this way as well, 
by adding samsung,pin-value property.

Best regards,
Tomasz

> Best Regards,
> J,
> 
> >     };
> >     pinp {
> >     
> >         samsung,pins = <PIN_P PIN_Q>;
> >         samsung,pin-function = <0xe>;
> >         samsung,pin-pud = <1>;
> >         ...
> >     
> >     };
> >     pinx {
> >     
> >         samsung,pins = <PIN_X PIN_Y PIN_Z>;
> >         samsung,pin-function = <0xd>;
> >         samsung,pin-pud = <2>;
> >         ...
> >     
> >     };
> >     
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&pina &pinp &pinx>;
> > 
> > };
> > 
> > That pinctrl-0 property could get rather large (hard to
> > write/maintain,
> > unwieldy) if it needs to set up lots of different configurations.
> > That's why I made the equivalent Tegra bindings be:
> > 
> > pinctrl {
> > 
> >     pins_default {
> >     
> >         pina {
> >         
> >             samsung,pins = <PIN_A PIN_B PIN_C>;
> >             samsung,pin-function = <0xf>;
> >             samsung,pin-pud = <0>;
> >             ...
> >         
> >         };
> >         pinp {
> >         
> >             samsung,pins = <PIN_P PIN_Q>;
> >             samsung,pin-function = <0xe>;
> >             samsung,pin-pud = <1>;
> >             ...
> >         
> >         };
> >         pinx {
> >         
> >             samsung,pins = <PIN_X PIN_Y PIN_Z>;
> >             samsung,pin-function = <0xd>;
> >             samsung,pin-pud = <2>;
> >             ...
> >         
> >         };
> >     
> >     };
> >     
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&pins_default>;
> > 
> > };
> > 
> > The extra level within the "pinctrl configuration node"
> > ("pins_default"
> > here) makes the pinctrl-0 property a lot easier to write, and the
> > advantage happens at every use-site that needs to configure different
> > subsets of the relevant pins in different ways.
> > 
> > If you're changing all the bindings anyway, introducing this extra
> > level might be something to think about.
> > 
> > I did try to explain my philosophy here when we all got together to
> > design the pinctrl bindings, but I obviously didn't explain it well
> > enough, or people didn't like it anyway.
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:17 Fri 17 May     , Tomasz Figa wrote:
> Hi Jean-Christophe,
> 
> On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 18:22 Wed 15 May     , Stephen Warren wrote:
> > > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > > >> Tomasz / Linus,
> > > >> 
> > > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > > >> <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > wrote:
> > > >>> Yes. I don't like the current way too much either, duplication
> > > >>> being
> > > >>> one of the reasons.
> > > >> 
> > > >> Do you have any other ideas?  It sounds like Linus didn't like my
> > > >> suggestion and makes some good points...
> > > > 
> > > > I don't have anything interesting at the moment. It's a bit late now
> > > > here (2 AM), so I'm going to get some sleep first.
> > > > 
> > > > Also after reading Stephen's reply, I'm wondering if hogging
> > > > wouldn't
> > > > solve the problem indeed. (It might have to be fixed on
> > > > pinctrl-samsung
> > > > first, as last time I tried to use it, it caused some errors from
> > > > pinctrl core, but haven't time to track them down, as it wasn't
> > > > anything important at that time).
> > > 
> > > One issue I noticed with the DT fragments earlier in this thread. It
> > > looks like hogs in the Samsung pinctrl bingings end up looking like:
> > > 
> > > pinctrl {
> > > 
> > >     pina {
> > >     
> > >         samsung,pins = <PIN_A PIN_B PIN_C>;
> > >         samsung,pin-function = <0xf>;
> > >         samsung,pin-pud = <0>;
> > >         ...
> > 
> > I have a huge issue here that we had on at91 too
> > 
> > we are going to have a huge numbet of node
> > 
> > and on at91 we handle the pin the same way as samsung
> > and ST have also a similiar IP
> > 
> > so I'll prefer to reuse the AT91 DT bindings
> > 
> > as said by Linus I just push a cleanup of the magic by using Macro
> > which make it really readable now
> > 
> > some extract of the sama5 pinctrl
> > 
> > 	mmc0 {
> > 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> > 			atmel,pins =
> > 				<AT91_PIOD 9 AT91_PERIPH_A 
> AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> > */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> > MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> > 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> > 			atmel,pins =
> > 				<AT91_PIOD 2 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP	/* PD2 periph A
> > MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */ AT91_PIOD
> > 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3 
> with
> > pullup */ };
> > 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> > 			atmel,pins =
> > 				<AT91_PIOD 5 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP	/* PD5 periph A
> > MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> > AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 with
> > pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, conlicts
> > with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup, 
> conflicts
> > with PWML3 */ };
> > 	};
> > 
> > of sam9g45
> > 
> > 	i2c_gpio2 {
> > 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> > 			atmel,pins =
> > 				<AT91_PIOB 4 AT91_PERIPH_GPIO 
> AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> > multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> > AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock */ };
> > 	};
> > 
> > so we could share the c code too
> 
> I'd have a question with regard to AT91 bindings.
> 
> Using Samsung bindings we don't need to specify all configuration options 
> for a pin, only those that are relevant for the platform. Do your bindings 
> allow this?
on at91 we have this too we just put NONE, and I'm planning to allow to drop
the last option too (not yet implement)
> 
> Apparently AT91 has less configurable things and those available are 
> usually always configured together so it's not a problem. But on our SoCs 
> we have a bit more of them:
> - function (input, output, special functions)
> - pull-down/-up
> - driver strength
> - power down mode function (input, output low, output high, retention)
> - power down mode pull-down/-up
> - one could argue that default output value could be set this way as well, 
> by adding samsung,pin-value property.

on Atmel you have

first a pin need to be muxed as a gpio or a function name periph
 depending on the SoC we can have up to 4 function mode + gpio

then each pin have feature that are independent of the mux function

Bits used for CONFIG: (4th parameter)
PULL_UP         (1 << 0): indicate this pin need a pull up.
MULTIDRIVE      (1 << 1): indicate this pin need to be configured as multidrive.
DEGLITCH        (1 << 2): indicate this pin need deglitch.
PULL_DOWN       (1 << 3): indicate this pin need a pull down.
DIS_SCHMIT      (1 << 4): indicate this pin need to disable schmit trigger.
DEBOUNCE        (1 << 16): indicate this pin need debounce.
DEBOUNCE_VAL    (0x3fff << 17): debounce val.

today I was planning to update the binding to allow to this

instead of writing this

dbgu {
	pinctrl_dbgu: dbgu-0 {
		atmel,pins =
			<AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE
			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
	};
};

we will write this

dbgu {
	pinctrl_dbgu: dbgu-0 {
		atmel,pins =
			<AT91_PIOB 30 AT91_PERIPH_A>,
			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
	};
};

so a pin can have 3 or more parameter

so as a generic binding only the 3 first will be namdatory (bank pinnp muxid)
the rest will driver specific

for power down I plan to define an other node
dbgu {
	pinctrl_dbgu_sleep: dbgu_sleep-0 {
		atmel,pins =
			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_DOWN>;
	};
};

Best Regards,
J.
> 
> Best regards,
> Tomasz
> 
> > Best Regards,
> > J,
> > 
> > >     };
> > >     pinp {
> > >     
> > >         samsung,pins = <PIN_P PIN_Q>;
> > >         samsung,pin-function = <0xe>;
> > >         samsung,pin-pud = <1>;
> > >         ...
> > >     
> > >     };
> > >     pinx {
> > >     
> > >         samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > >         samsung,pin-function = <0xd>;
> > >         samsung,pin-pud = <2>;
> > >         ...
> > >     
> > >     };
> > >     
> > >     pinctrl-names = "default";
> > >     pinctrl-0 = <&pina &pinp &pinx>;
> > > 
> > > };
> > > 
> > > That pinctrl-0 property could get rather large (hard to
> > > write/maintain,
> > > unwieldy) if it needs to set up lots of different configurations.
> > > That's why I made the equivalent Tegra bindings be:
> > > 
> > > pinctrl {
> > > 
> > >     pins_default {
> > >     
> > >         pina {
> > >         
> > >             samsung,pins = <PIN_A PIN_B PIN_C>;
> > >             samsung,pin-function = <0xf>;
> > >             samsung,pin-pud = <0>;
> > >             ...
> > >         
> > >         };
> > >         pinp {
> > >         
> > >             samsung,pins = <PIN_P PIN_Q>;
> > >             samsung,pin-function = <0xe>;
> > >             samsung,pin-pud = <1>;
> > >             ...
> > >         
> > >         };
> > >         pinx {
> > >         
> > >             samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > >             samsung,pin-function = <0xd>;
> > >             samsung,pin-pud = <2>;
> > >             ...
> > >         
> > >         };
> > >     
> > >     };
> > >     
> > >     pinctrl-names = "default";
> > >     pinctrl-0 = <&pins_default>;
> > > 
> > > };
> > > 
> > > The extra level within the "pinctrl configuration node"
> > > ("pins_default"
> > > here) makes the pinctrl-0 property a lot easier to write, and the
> > > advantage happens at every use-site that needs to configure different
> > > subsets of the relevant pins in different ways.
> > > 
> > > If you're changing all the bindings anyway, introducing this extra
> > > level might be something to think about.
> > > 
> > > I did try to explain my philosophy here when we all got together to
> > > design the pinctrl bindings, but I obviously didn't explain it well
> > > enough, or people didn't like it anyway.
> > > _______________________________________________
> > > devicetree-discuss mailing list
> > > devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > > https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Saturday 18 of May 2013 10:18:47 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> On 14:17 Fri 17 May     , Tomasz Figa wrote:
> > Hi Jean-Christophe,
> > 
> > On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> > > On 18:22 Wed 15 May     , Stephen Warren wrote:
> > > > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > > > >> Tomasz / Linus,
> > > > >> 
> > > > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > > > >> <tomasz.figa@gmail.com>
> > > > > 
> > > > > wrote:
> > > > >>> Yes. I don't like the current way too much either, duplication
> > > > >>> being
> > > > >>> one of the reasons.
> > > > >> 
> > > > >> Do you have any other ideas?  It sounds like Linus didn't like
> > > > >> my
> > > > >> suggestion and makes some good points...
> > > > > 
> > > > > I don't have anything interesting at the moment. It's a bit late
> > > > > now
> > > > > here (2 AM), so I'm going to get some sleep first.
> > > > > 
> > > > > Also after reading Stephen's reply, I'm wondering if hogging
> > > > > wouldn't
> > > > > solve the problem indeed. (It might have to be fixed on
> > > > > pinctrl-samsung
> > > > > first, as last time I tried to use it, it caused some errors
> > > > > from
> > > > > pinctrl core, but haven't time to track them down, as it wasn't
> > > > > anything important at that time).
> > > > 
> > > > One issue I noticed with the DT fragments earlier in this thread.
> > > > It
> > > > looks like hogs in the Samsung pinctrl bingings end up looking
> > > > like:
> > > > 
> > > > pinctrl {
> > > > 
> > > >     pina {
> > > >     
> > > >         samsung,pins = <PIN_A PIN_B PIN_C>;
> > > >         samsung,pin-function = <0xf>;
> > > >         samsung,pin-pud = <0>;
> > > >         ...
> > > 
> > > I have a huge issue here that we had on at91 too
> > > 
> > > we are going to have a huge numbet of node
> > > 
> > > and on at91 we handle the pin the same way as samsung
> > > and ST have also a similiar IP
> > > 
> > > so I'll prefer to reuse the AT91 DT bindings
> > > 
> > > as said by Linus I just push a cleanup of the magic by using Macro
> > > which make it really readable now
> > > 
> > > some extract of the sama5 pinctrl
> > > 
> > > 	mmc0 {
> > > 	
> > > 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> > > 		
> > > 			atmel,pins =
> > > 			
> > > 				<AT91_PIOD 9 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> > 
> > > */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> > > MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> > > AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> > > 
> > > 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> > > 		
> > > 			atmel,pins =
> > > 			
> > > 				<AT91_PIOD 2 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_PULL_UP	/* PD2 periph A
> > 
> > > MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> > > AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */
> > > AT91_PIOD
> > > 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3
> > 
> > with
> > 
> > > pullup */ };
> > > 
> > > 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> > > 		
> > > 			atmel,pins =
> > > 			
> > > 				<AT91_PIOD 5 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_PULL_UP	/* PD5 periph A
> > 
> > > MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> > > AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 
with
> > > pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> > > AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, 
conlicts
> > > with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> > > AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup,
> > 
> > conflicts
> > 
> > > with PWML3 */ };
> > > 
> > > 	};
> > > 
> > > of sam9g45
> > > 
> > > 	i2c_gpio2 {
> > > 	
> > > 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> > > 		
> > > 			atmel,pins =
> > > 			
> > > 				<AT91_PIOB 4 AT91_PERIPH_GPIO
> > 
> > AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> > 
> > > multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> > > AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock 
*/ };
> > > 
> > > 	};
> > > 
> > > so we could share the c code too
> > 
> > I'd have a question with regard to AT91 bindings.
> > 
> > Using Samsung bindings we don't need to specify all configuration
> > options for a pin, only those that are relevant for the platform. Do
> > your bindings allow this?
> 
> on at91 we have this too we just put NONE, and I'm planning to allow to
> drop the last option too (not yet implement)
> 
> > Apparently AT91 has less configurable things and those available are
> > usually always configured together so it's not a problem. But on our
> > SoCs we have a bit more of them:
> > - function (input, output, special functions)
> > - pull-down/-up
> > - driver strength
> > - power down mode function (input, output low, output high, retention)
> > - power down mode pull-down/-up
> > - one could argue that default output value could be set this way as
> > well, by adding samsung,pin-value property.
> 
> on Atmel you have
> 
> first a pin need to be muxed as a gpio or a function name periph
>  depending on the SoC we can have up to 4 function mode + gpio
> 
> then each pin have feature that are independent of the mux function
> 
> Bits used for CONFIG: (4th parameter)
> PULL_UP         (1 << 0): indicate this pin need a pull up.
> MULTIDRIVE      (1 << 1): indicate this pin need to be configured as
> multidrive. DEGLITCH        (1 << 2): indicate this pin need deglitch.
> PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> DIS_SCHMIT      (1 << 4): indicate this pin need to disable schmit
> trigger. DEBOUNCE        (1 << 16): indicate this pin need debounce.
> DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> 
> today I was planning to update the binding to allow to this
> 
> instead of writing this
> 
> dbgu {
> 	pinctrl_dbgu: dbgu-0 {
> 		atmel,pins =
> 			<AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE
> 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> 	};
> };
> 
> we will write this
> 
> dbgu {
> 	pinctrl_dbgu: dbgu-0 {
> 		atmel,pins =
> 			<AT91_PIOB 30 AT91_PERIPH_A>,
> 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> 	};
> };
> 
> so a pin can have 3 or more parameter
> 
> so as a generic binding only the 3 first will be namdatory (bank pinnp
> muxid) the rest will driver specific
> 
> for power down I plan to define an other node
> dbgu {
> 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> 		atmel,pins =
> 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> 			 AT91_PIOB 31 AT91_PERIPH_A 
AT91_PINCTRL_PULL_DOWN>;
> 	};
> };

I'm afraid this won't work for Samsung SoCs. In our case normal and power 
down settings are completely unrelated, i.e. stored in separate registers 
and one doesn't affect another (a system controller automatically switches 
between normal and power down settings when entering or leaving low power 
modes, like SoC-level suspend).

Personally I'd prefer a solution with separate property for each 
parameter, because it's much more flexible and allows shorter lines, 
making device tree sources more readable.

Best regards,
Tomasz

> 
> Best Regards,
> J.
> 
> > Best regards,
> > Tomasz
> > 
> > > Best Regards,
> > > J,
> > > 
> > > >     };
> > > >     pinp {
> > > >     
> > > >         samsung,pins = <PIN_P PIN_Q>;
> > > >         samsung,pin-function = <0xe>;
> > > >         samsung,pin-pud = <1>;
> > > >         ...
> > > >     
> > > >     };
> > > >     pinx {
> > > >     
> > > >         samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > > >         samsung,pin-function = <0xd>;
> > > >         samsung,pin-pud = <2>;
> > > >         ...
> > > >     
> > > >     };
> > > >     
> > > >     pinctrl-names = "default";
> > > >     pinctrl-0 = <&pina &pinp &pinx>;
> > > > 
> > > > };
> > > > 
> > > > That pinctrl-0 property could get rather large (hard to
> > > > write/maintain,
> > > > unwieldy) if it needs to set up lots of different configurations.
> > > > That's why I made the equivalent Tegra bindings be:
> > > > 
> > > > pinctrl {
> > > > 
> > > >     pins_default {
> > > >     
> > > >         pina {
> > > >         
> > > >             samsung,pins = <PIN_A PIN_B PIN_C>;
> > > >             samsung,pin-function = <0xf>;
> > > >             samsung,pin-pud = <0>;
> > > >             ...
> > > >         
> > > >         };
> > > >         pinp {
> > > >         
> > > >             samsung,pins = <PIN_P PIN_Q>;
> > > >             samsung,pin-function = <0xe>;
> > > >             samsung,pin-pud = <1>;
> > > >             ...
> > > >         
> > > >         };
> > > >         pinx {
> > > >         
> > > >             samsung,pins = <PIN_X PIN_Y PIN_Z>;
> > > >             samsung,pin-function = <0xd>;
> > > >             samsung,pin-pud = <2>;
> > > >             ...
> > > >         
> > > >         };
> > > >     
> > > >     };
> > > >     
> > > >     pinctrl-names = "default";
> > > >     pinctrl-0 = <&pins_default>;
> > > > 
> > > > };
> > > > 
> > > > The extra level within the "pinctrl configuration node"
> > > > ("pins_default"
> > > > here) makes the pinctrl-0 property a lot easier to write, and the
> > > > advantage happens at every use-site that needs to configure
> > > > different
> > > > subsets of the relevant pins in different ways.
> > > > 
> > > > If you're changing all the bindings anyway, introducing this extra
> > > > level might be something to think about.
> > > > 
> > > > I did try to explain my philosophy here when we all got together
> > > > to
> > > > design the pinctrl bindings, but I obviously didn't explain it
> > > > well
> > > > enough, or people didn't like it anyway.
> > > > _______________________________________________
> > > > devicetree-discuss mailing list
> > > > devicetree-discuss@lists.ozlabs.org
> > > > https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:57 Sat 18 May     , Tomasz Figa wrote:
> On Saturday 18 of May 2013 10:18:47 Jean-Christophe PLAGNIOL-VILLARD 
> wrote:
> > On 14:17 Fri 17 May     , Tomasz Figa wrote:
> > > Hi Jean-Christophe,
> > > 
> > > On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD 
> wrote:
> > > > On 18:22 Wed 15 May     , Stephen Warren wrote:
> > > > > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > > > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > > > > >> Tomasz / Linus,
> > > > > >> 
> > > > > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > > > > >> <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > > 
> > > > > > wrote:
> > > > > >>> Yes. I don't like the current way too much either, duplication
> > > > > >>> being
> > > > > >>> one of the reasons.
> > > > > >> 
> > > > > >> Do you have any other ideas?  It sounds like Linus didn't like
> > > > > >> my
> > > > > >> suggestion and makes some good points...
> > > > > > 
> > > > > > I don't have anything interesting at the moment. It's a bit late
> > > > > > now
> > > > > > here (2 AM), so I'm going to get some sleep first.
> > > > > > 
> > > > > > Also after reading Stephen's reply, I'm wondering if hogging
> > > > > > wouldn't
> > > > > > solve the problem indeed. (It might have to be fixed on
> > > > > > pinctrl-samsung
> > > > > > first, as last time I tried to use it, it caused some errors
> > > > > > from
> > > > > > pinctrl core, but haven't time to track them down, as it wasn't
> > > > > > anything important at that time).
> > > > > 
> > > > > One issue I noticed with the DT fragments earlier in this thread.
> > > > > It
> > > > > looks like hogs in the Samsung pinctrl bingings end up looking
> > > > > like:
> > > > > 
> > > > > pinctrl {
> > > > > 
> > > > >     pina {
> > > > >     
> > > > >         samsung,pins = <PIN_A PIN_B PIN_C>;
> > > > >         samsung,pin-function = <0xf>;
> > > > >         samsung,pin-pud = <0>;
> > > > >         ...
> > > > 
> > > > I have a huge issue here that we had on at91 too
> > > > 
> > > > we are going to have a huge numbet of node
> > > > 
> > > > and on at91 we handle the pin the same way as samsung
> > > > and ST have also a similiar IP
> > > > 
> > > > so I'll prefer to reuse the AT91 DT bindings
> > > > 
> > > > as said by Linus I just push a cleanup of the magic by using Macro
> > > > which make it really readable now
> > > > 
> > > > some extract of the sama5 pinctrl
> > > > 
> > > > 	mmc0 {
> > > > 	
> > > > 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> > > > 		
> > > > 			atmel,pins =
> > > > 			
> > > > 				<AT91_PIOD 9 AT91_PERIPH_A
> > > 
> > > AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> > > 
> > > > */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> > > > MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> > > > AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> > > > 
> > > > 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> > > > 		
> > > > 			atmel,pins =
> > > > 			
> > > > 				<AT91_PIOD 2 AT91_PERIPH_A
> > > 
> > > AT91_PINCTRL_PULL_UP	/* PD2 periph A
> > > 
> > > > MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> > > > AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */
> > > > AT91_PIOD
> > > > 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3
> > > 
> > > with
> > > 
> > > > pullup */ };
> > > > 
> > > > 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> > > > 		
> > > > 			atmel,pins =
> > > > 			
> > > > 				<AT91_PIOD 5 AT91_PERIPH_A
> > > 
> > > AT91_PINCTRL_PULL_UP	/* PD5 periph A
> > > 
> > > > MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> > > > AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 
> with
> > > > pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> > > > AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, 
> conlicts
> > > > with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> > > > AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup,
> > > 
> > > conflicts
> > > 
> > > > with PWML3 */ };
> > > > 
> > > > 	};
> > > > 
> > > > of sam9g45
> > > > 
> > > > 	i2c_gpio2 {
> > > > 	
> > > > 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> > > > 		
> > > > 			atmel,pins =
> > > > 			
> > > > 				<AT91_PIOB 4 AT91_PERIPH_GPIO
> > > 
> > > AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> > > 
> > > > multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> > > > AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock 
> */ };
> > > > 
> > > > 	};
> > > > 
> > > > so we could share the c code too
> > > 
> > > I'd have a question with regard to AT91 bindings.
> > > 
> > > Using Samsung bindings we don't need to specify all configuration
> > > options for a pin, only those that are relevant for the platform. Do
> > > your bindings allow this?
> > 
> > on at91 we have this too we just put NONE, and I'm planning to allow to
> > drop the last option too (not yet implement)
> > 
> > > Apparently AT91 has less configurable things and those available are
> > > usually always configured together so it's not a problem. But on our
> > > SoCs we have a bit more of them:
> > > - function (input, output, special functions)
> > > - pull-down/-up
> > > - driver strength
> > > - power down mode function (input, output low, output high, retention)
> > > - power down mode pull-down/-up
> > > - one could argue that default output value could be set this way as
> > > well, by adding samsung,pin-value property.
> > 
> > on Atmel you have
> > 
> > first a pin need to be muxed as a gpio or a function name periph
> >  depending on the SoC we can have up to 4 function mode + gpio
> > 
> > then each pin have feature that are independent of the mux function
> > 
> > Bits used for CONFIG: (4th parameter)
> > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > MULTIDRIVE      (1 << 1): indicate this pin need to be configured as
> > multidrive. DEGLITCH        (1 << 2): indicate this pin need deglitch.
> > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > DIS_SCHMIT      (1 << 4): indicate this pin need to disable schmit
> > trigger. DEBOUNCE        (1 << 16): indicate this pin need debounce.
> > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > 
> > today I was planning to update the binding to allow to this
> > 
> > instead of writing this
> > 
> > dbgu {
> > 	pinctrl_dbgu: dbgu-0 {
> > 		atmel,pins =
> > 			<AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE
> > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > 	};
> > };
> > 
> > we will write this
> > 
> > dbgu {
> > 	pinctrl_dbgu: dbgu-0 {
> > 		atmel,pins =
> > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > 	};
> > };
> > 
> > so a pin can have 3 or more parameter
> > 
> > so as a generic binding only the 3 first will be namdatory (bank pinnp
> > muxid) the rest will driver specific
> > 
> > for power down I plan to define an other node
> > dbgu {
> > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > 		atmel,pins =
> > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > 			 AT91_PIOB 31 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_DOWN>;
> > 	};
> > };
> 
> I'm afraid this won't work for Samsung SoCs. In our case normal and power 
> down settings are completely unrelated, i.e. stored in separate registers 
> and one doesn't affect another (a system controller automatically switches 
> between normal and power down settings when entering or leaving low power 
> modes, like SoC-level suspend).
and?

on at91 I've x banks of registers to handle each gpio bank

on ST with have same situation but the controller work the same way at the end

so we need to factorise code
> 
> Personally I'd prefer a solution with separate property for each 
> parameter, because it's much more flexible and allows shorter lines, 
> making device tree sources more readable.
we already discuss this on the ML the one property perline will endup with
100s of node if not 1000s so we all do agree we do not want this in the DT

Specially here as you will often describe only one pin per node
we must avoid this

and your controller work the same way as AT91 or ST (STB) work

Best Regards,
J.

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Saturday 18 of May 2013 18:30:01 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> On 16:57 Sat 18 May     , Tomasz Figa wrote:
> > On Saturday 18 of May 2013 10:18:47 Jean-Christophe PLAGNIOL-VILLARD
> > 
> > wrote:
> > > On 14:17 Fri 17 May     , Tomasz Figa wrote:
> > > > Hi Jean-Christophe,
> > > > 
> > > > On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD
> > 
> > wrote:
> > > > > On 18:22 Wed 15 May     , Stephen Warren wrote:
> > > > > > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > > > > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > > > > > >> Tomasz / Linus,
> > > > > > >> 
> > > > > > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > > > > > >> <tomasz.figa@gmail.com>
> > > > > > > 
> > > > > > > wrote:
> > > > > > >>> Yes. I don't like the current way too much either,
> > > > > > >>> duplication
> > > > > > >>> being
> > > > > > >>> one of the reasons.
> > > > > > >> 
> > > > > > >> Do you have any other ideas?  It sounds like Linus didn't
> > > > > > >> like
> > > > > > >> my
> > > > > > >> suggestion and makes some good points...
> > > > > > > 
> > > > > > > I don't have anything interesting at the moment. It's a bit
> > > > > > > late
> > > > > > > now
> > > > > > > here (2 AM), so I'm going to get some sleep first.
> > > > > > > 
> > > > > > > Also after reading Stephen's reply, I'm wondering if hogging
> > > > > > > wouldn't
> > > > > > > solve the problem indeed. (It might have to be fixed on
> > > > > > > pinctrl-samsung
> > > > > > > first, as last time I tried to use it, it caused some errors
> > > > > > > from
> > > > > > > pinctrl core, but haven't time to track them down, as it
> > > > > > > wasn't
> > > > > > > anything important at that time).
> > > > > > 
> > > > > > One issue I noticed with the DT fragments earlier in this
> > > > > > thread.
> > > > > > It
> > > > > > looks like hogs in the Samsung pinctrl bingings end up looking
> > > > > > like:
> > > > > > 
> > > > > > pinctrl {
> > > > > > 
> > > > > >     pina {
> > > > > >     
> > > > > >         samsung,pins = <PIN_A PIN_B PIN_C>;
> > > > > >         samsung,pin-function = <0xf>;
> > > > > >         samsung,pin-pud = <0>;
> > > > > >         ...
> > > > > 
> > > > > I have a huge issue here that we had on at91 too
> > > > > 
> > > > > we are going to have a huge numbet of node
> > > > > 
> > > > > and on at91 we handle the pin the same way as samsung
> > > > > and ST have also a similiar IP
> > > > > 
> > > > > so I'll prefer to reuse the AT91 DT bindings
> > > > > 
> > > > > as said by Linus I just push a cleanup of the magic by using
> > > > > Macro
> > > > > which make it really readable now
> > > > > 
> > > > > some extract of the sama5 pinctrl
> > > > > 
> > > > > 	mmc0 {
> > > > > 	
> > > > > 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> > > > > 		
> > > > > 			atmel,pins =
> > > > > 			
> > > > > 				<AT91_PIOD 9 AT91_PERIPH_A
> > > > 
> > > > AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> > > > 
> > > > > */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 
periph
> > > > > A
> > > > > MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> > > > > AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with 
pullup */
> > > > > };
> > > > > 
> > > > > 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> > > > > 		
> > > > > 			atmel,pins =
> > > > > 			
> > > > > 				<AT91_PIOD 2 AT91_PERIPH_A
> > > > 
> > > > AT91_PINCTRL_PULL_UP	/* PD2 periph A
> > > > 
> > > > > MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> > > > > AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */
> > > > > AT91_PIOD
> > > > > 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A 
MCI0_DA3
> > > > 
> > > > with
> > > > 
> > > > > pullup */ };
> > > > > 
> > > > > 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> > > > > 		
> > > > > 			atmel,pins =
> > > > > 			
> > > > > 				<AT91_PIOD 5 AT91_PERIPH_A
> > > > 
> > > > AT91_PINCTRL_PULL_UP	/* PD5 periph A
> > > > 
> > > > > MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> > > > > AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5
> > 
> > with
> > 
> > > > > pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> > > > > AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup,
> > 
> > conlicts
> > 
> > > > > with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> > > > > AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with 
pullup,
> > > > 
> > > > conflicts
> > > > 
> > > > > with PWML3 */ };
> > > > > 
> > > > > 	};
> > > > > 
> > > > > of sam9g45
> > > > > 
> > > > > 	i2c_gpio2 {
> > > > > 	
> > > > > 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> > > > > 		
> > > > > 			atmel,pins =
> > > > > 			
> > > > > 				<AT91_PIOB 4 AT91_PERIPH_GPIO
> > > > 
> > > > AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> > > > 
> > > > > multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> > > > > AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock
> > 
> > */ };
> > 
> > > > > 	};
> > > > > 
> > > > > so we could share the c code too
> > > > 
> > > > I'd have a question with regard to AT91 bindings.
> > > > 
> > > > Using Samsung bindings we don't need to specify all configuration
> > > > options for a pin, only those that are relevant for the platform.
> > > > Do
> > > > your bindings allow this?
> > > 
> > > on at91 we have this too we just put NONE, and I'm planning to allow
> > > to
> > > drop the last option too (not yet implement)
> > > 
> > > > Apparently AT91 has less configurable things and those available
> > > > are
> > > > usually always configured together so it's not a problem. But on
> > > > our
> > > > SoCs we have a bit more of them:
> > > > - function (input, output, special functions)
> > > > - pull-down/-up
> > > > - driver strength
> > > > - power down mode function (input, output low, output high,
> > > > retention)
> > > > - power down mode pull-down/-up
> > > > - one could argue that default output value could be set this way
> > > > as
> > > > well, by adding samsung,pin-value property.
> > > 
> > > on Atmel you have
> > > 
> > > first a pin need to be muxed as a gpio or a function name periph
> > > 
> > >  depending on the SoC we can have up to 4 function mode + gpio
> > > 
> > > then each pin have feature that are independent of the mux function
> > > 
> > > Bits used for CONFIG: (4th parameter)
> > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > MULTIDRIVE      (1 << 1): indicate this pin need to be configured as
> > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > deglitch.
> > > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable schmit
> > > trigger. DEBOUNCE        (1 << 16): indicate this pin need debounce.
> > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > 
> > > today I was planning to update the binding to allow to this
> > > 
> > > instead of writing this
> > > 
> > > dbgu {
> > > 
> > > 	pinctrl_dbgu: dbgu-0 {
> > > 	
> > > 		atmel,pins =
> > > 		
> > > 			<AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE
> > > 			
> > > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > > 	
> > > 	};
> > > 
> > > };
> > > 
> > > we will write this
> > > 
> > > dbgu {
> > > 
> > > 	pinctrl_dbgu: dbgu-0 {
> > > 	
> > > 		atmel,pins =
> > > 		
> > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > 			
> > > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > > 	
> > > 	};
> > > 
> > > };
> > > 
> > > so a pin can have 3 or more parameter
> > > 
> > > so as a generic binding only the 3 first will be namdatory (bank
> > > pinnp
> > > muxid) the rest will driver specific
> > > 
> > > for power down I plan to define an other node
> > > dbgu {
> > > 
> > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > 	
> > > 		atmel,pins =
> > > 		
> > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > 			
> > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_PULL_DOWN>;
> > 
> > > 	};
> > > 
> > > };
> > 
> > I'm afraid this won't work for Samsung SoCs. In our case normal and
> > power down settings are completely unrelated, i.e. stored in separate
> > registers and one doesn't affect another (a system controller
> > automatically switches between normal and power down settings when
> > entering or leaving low power modes, like SoC-level suspend).
> 
> and?

Pin configuration node on Exynos SoCs will need 7 values for each pin in 
samsung,pins property, just like in following example:

mmc0 {
	mmc0_bus1: mmc0-bus1 {
		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN PDN_PULL_UP>;
	};
	/* ... */
};

Now if I just want to enable pull-up for a single pin, I will have to add 
following node:

foo {
	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
};

while with current bindings I can simply omit properties that I don't care 
about (or are going to be set up correctly by other means - e.g. 
gpio_direction_input() or request_irq(), ending with following node:

foo {
	samsung,pins = "gpk1-2";
	samsung,pin-pud = <3>;
};

This is all I need to configure for simple things like open-drain 
interrupt lines.

Another thing is that we're using one driver for many SoCs, which have 
different variants of the controller. So for example some of them don't 
have driver strength configuration (S3C24xx, S3C64xx), other don't have 
power down mode configuration (S3C24xx) and even some of the banks on some 
SoCs don't support particular type of configuration (alive banks of SoCs 
>= S3C64xx don't have power down mode configuration, because they are 
always on).

> on at91 I've x banks of registers to handle each gpio bank
> 
> on ST with have same situation but the controller work the same way at
> the end
> 
> so we need to factorise code
> 
> > Personally I'd prefer a solution with separate property for each
> > parameter, because it's much more flexible and allows shorter lines,
> > making device tree sources more readable.
> 
> we already discuss this on the ML the one property perline will endup
> with 100s of node if not 1000s so we all do agree we do not want this
> in the DT

Could you point me to this discussion, please? I'd really like to take a 
look.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

> > > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > > MULTIDRIVE      (1 << 1): indicate this pin need to be configured as
> > > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > > deglitch.
> > > > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable schmit
> > > > trigger. DEBOUNCE        (1 << 16): indicate this pin need debounce.
> > > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > > 
> > > > today I was planning to update the binding to allow to this
> > > > 
> > > > instead of writing this
> > > > 
> > > > dbgu {
> > > > 
> > > > 	pinctrl_dbgu: dbgu-0 {
> > > > 	
> > > > 		atmel,pins =
> > > > 		
> > > > 			<AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE
> > > > 			
> > > > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > > > 	
> > > > 	};
> > > > 
> > > > };
> > > > 
> > > > we will write this
> > > > 
> > > > dbgu {
> > > > 
> > > > 	pinctrl_dbgu: dbgu-0 {
> > > > 	
> > > > 		atmel,pins =
> > > > 		
> > > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > > 			
> > > > 			 AT91_PIOB 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > > > 	
> > > > 	};
> > > > 
> > > > };
> > > > 
> > > > so a pin can have 3 or more parameter
> > > > 
> > > > so as a generic binding only the 3 first will be namdatory (bank
> > > > pinnp
> > > > muxid) the rest will driver specific
> > > > 
> > > > for power down I plan to define an other node
> > > > dbgu {
> > > > 
> > > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > > 	
> > > > 		atmel,pins =
> > > > 		
> > > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > > 			
> > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > > 
> > > AT91_PINCTRL_PULL_DOWN>;
> > > 
> > > > 	};
> > > > 
> > > > };
> > > 
> > > I'm afraid this won't work for Samsung SoCs. In our case normal and
> > > power down settings are completely unrelated, i.e. stored in separate
> > > registers and one doesn't affect another (a system controller
> > > automatically switches between normal and power down settings when
> > > entering or leaving low power modes, like SoC-level suspend).
> > 
> > and?
> 
> Pin configuration node on Exynos SoCs will need 7 values for each pin in 
> samsung,pins property, just like in following example:
> 
> mmc0 {
> 	mmc0_bus1: mmc0-bus1 {
> 		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN PDN_PULL_UP>;
> 	};
> 	/* ... */
> };
> 
> Now if I just want to enable pull-up for a single pin, I will have to add 
> following node:
> 
> foo {
> 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
> };
> 

no you will not

foo {
	pins = <GPK1 2 NONE PULL_UP>;
};

how a pin can not have mux?

> while with current bindings I can simply omit properties that I don't care 
> about (or are going to be set up correctly by other means - e.g. 
> gpio_direction_input() or request_irq(), ending with following node:
> 
> foo {
> 	samsung,pins = "gpk1-2";
> 	samsung,pin-pud = <3>;
> };

except here you will 100s of NODE which we do NOT want in the dtb
> 
> This is all I need to configure for simple things like open-drain 
> interrupt lines.
> 
> Another thing is that we're using one driver for many SoCs, which have 
> different variants of the controller. So for example some of them don't 
> have driver strength configuration (S3C24xx, S3C64xx), other don't have 
> power down mode configuration (S3C24xx) and even some of the banks on some 
> SoCs don't support particular type of configuration (alive banks of SoCs 

same on at91 some IP have less feature
and some SoC have the IP/die but not the same pins package

the key point is to share the pin DT handling between at91, ST and Samsung
ofcourse all the IP will have more or less parameter per pin but the basic is
the same for DT and C code

> >= S3C64xx don't have power down mode configuration, because they are 
> always on).
> 
> > on at91 I've x banks of registers to handle each gpio bank
> > 
> > on ST with have same situation but the controller work the same way at
> > the end
> > 
> > so we need to factorise code
> > 
> > > Personally I'd prefer a solution with separate property for each
> > > parameter, because it's much more flexible and allows shorter lines,
> > > making device tree sources more readable.
> > 
> > we already discuss this on the ML the one property perline will endup
> > with 100s of node if not 1000s so we all do agree we do not want this
> > in the DT
> 
> Could you point me to this discussion, please? I'd really like to take a 
> look.

you have to search this on the dt ML, it was about the clk bindings IIRC and
agree on this at Prague durring kernel Summit

Best Regards,
J.

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Sunday 19 of May 2013 11:17:36 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > > > MULTIDRIVE      (1 << 1): indicate this pin need to be
> > > > > configured as
> > > > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > > > deglitch.
> > > > > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > > > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable
> > > > > schmit
> > > > > trigger. DEBOUNCE        (1 << 16): indicate this pin need
> > > > > debounce.
> > > > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > > > 
> > > > > today I was planning to update the binding to allow to this
> > > > > 
> > > > > instead of writing this
> > > > > 
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_A 
AT91_PINCTRL_NONE
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP>;
> > > > > 	
> > > > > 	};
> > > > > 
> > > > > };
> > > > > 
> > > > > we will write this
> > > > > 
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP>;
> > > > > 	
> > > > > 	};
> > > > > 
> > > > > };
> > > > > 
> > > > > so a pin can have 3 or more parameter
> > > > > 
> > > > > so as a generic binding only the 3 first will be namdatory (bank
> > > > > pinnp
> > > > > muxid) the rest will driver specific
> > > > > 
> > > > > for power down I plan to define an other node
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > > > 
> > > > AT91_PINCTRL_PULL_DOWN>;
> > > > 
> > > > > 	};
> > > > > 
> > > > > };
> > > > 
> > > > I'm afraid this won't work for Samsung SoCs. In our case normal
> > > > and
> > > > power down settings are completely unrelated, i.e. stored in
> > > > separate
> > > > registers and one doesn't affect another (a system controller
> > > > automatically switches between normal and power down settings when
> > > > entering or leaving low power modes, like SoC-level suspend).
> > > 
> > > and?
> > 
> > Pin configuration node on Exynos SoCs will need 7 values for each pin
> > in samsung,pins property, just like in following example:
> > 
> > mmc0 {
> > 
> > 	mmc0_bus1: mmc0-bus1 {
> > 	
> > 		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN PDN_PULL_UP>;
> > 	
> > 	};
> > 	/* ... */
> > 
> > };
> > 
> > Now if I just want to enable pull-up for a single pin, I will have to
> > add following node:
> > 
> > foo {
> > 
> > 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
> > 
> > };
> 
> no you will not
> 
> foo {
> 	pins = <GPK1 2 NONE PULL_UP>;
> };

OK, this will work in case of one pin, but if you need two it starts to 
become problematic. Let's look at an example:

We have two pins for which we don't need to specify power down mode 
settings (e.g. they are in alive banks):

foo {
 	pins = <GPK1 2 NONE PULL_UP>,
		<GPK1 3 NONE PULL_UP>;
};

After compilation you will get just a series of u32 values, like

foo {
	pins = <1 2 255 3 1 3 255 3>;
};

How are you going to distinguish such setup with:

foo {
 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>,
		<GPK1 3 NONE PULL_UP NONE NONE NONE>;
};

which translates to

foo {
	pins = <1 2 255 3 255 255 255 1 3 255 3 255 255 255>;
};

I mean, you don't know where one entry ends and another starts, if you 
allow to left out some values.

> how a pin can not have mux?

I don't always want to change the mux. Sometimes I just want to change one 
of the other parameters. For example, I don't want to switch the pin to 
interrupt mode on driver probe (it is a separate pin mux value), but 
rather after the trigger type gets configured, which is done by 
request_irq() based on trigger flags.

> > while with current bindings I can simply omit properties that I don't
> > care about (or are going to be set up correctly by other means - e.g.
> > gpio_direction_input() or request_irq(), ending with following node:
> > 
> > foo {
> > 
> > 	samsung,pins = "gpk1-2";
> > 	samsung,pin-pud = <3>;
> > 
> > };
> 
> except here you will 100s of NODE which we do NOT want in the dtb

Is this really an issue?

We are already using this method to describe really complex boards (not 
necessarily in mainline) and we don't have any problems.

> > This is all I need to configure for simple things like open-drain
> > interrupt lines.
> > 
> > Another thing is that we're using one driver for many SoCs, which have
> > different variants of the controller. So for example some of them
> > don't
> > have driver strength configuration (S3C24xx, S3C64xx), other don't
> > have
> > power down mode configuration (S3C24xx) and even some of the banks on
> > some SoCs don't support particular type of configuration (alive banks
> > of SoCs
> same on at91 some IP have less feature
> and some SoC have the IP/die but not the same pins package
> 
> the key point is to share the pin DT handling between at91, ST and
> Samsung ofcourse all the IP will have more or less parameter per pin
> but the basic is the same for DT and C code
> 
> > >= S3C64xx don't have power down mode configuration, because they are
> > 
> > always on).
> > 
> > > on at91 I've x banks of registers to handle each gpio bank
> > > 
> > > on ST with have same situation but the controller work the same way
> > > at
> > > the end
> > > 
> > > so we need to factorise code
> > > 
> > > > Personally I'd prefer a solution with separate property for each
> > > > parameter, because it's much more flexible and allows shorter
> > > > lines,
> > > > making device tree sources more readable.
> > > 
> > > we already discuss this on the ML the one property perline will
> > > endup
> > > with 100s of node if not 1000s so we all do agree we do not want
> > > this
> > > in the DT
> > 
> > Could you point me to this discussion, please? I'd really like to take
> > a look.
> 
> you have to search this on the dt ML, it was about the clk bindings IIRC
> and agree on this at Prague durring kernel Summit

OK. Will look for it.

Best regards,
Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:46 Sun 19 May     , Tomasz Figa wrote:
> On Sunday 19 of May 2013 11:17:36 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > > > > MULTIDRIVE      (1 << 1): indicate this pin need to be
> > > > > > configured as
> > > > > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > > > > deglitch.
> > > > > > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > > > > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable
> > > > > > schmit
> > > > > > trigger. DEBOUNCE        (1 << 16): indicate this pin need
> > > > > > debounce.
> > > > > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > > > > 
> > > > > > today I was planning to update the binding to allow to this
> > > > > > 
> > > > > > instead of writing this
> > > > > > 
> > > > > > dbgu {
> > > > > > 
> > > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > > 	
> > > > > > 		atmel,pins =
> > > > > > 		
> > > > > > 			<AT91_PIOB 30 AT91_PERIPH_A 
> AT91_PINCTRL_NONE
> > > > > > 			
> > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP>;
> > > > > > 	
> > > > > > 	};
> > > > > > 
> > > > > > };
> > > > > > 
> > > > > > we will write this
> > > > > > 
> > > > > > dbgu {
> > > > > > 
> > > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > > 	
> > > > > > 		atmel,pins =
> > > > > > 		
> > > > > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > > > > 			
> > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP>;
> > > > > > 	
> > > > > > 	};
> > > > > > 
> > > > > > };
> > > > > > 
> > > > > > so a pin can have 3 or more parameter
> > > > > > 
> > > > > > so as a generic binding only the 3 first will be namdatory (bank
> > > > > > pinnp
> > > > > > muxid) the rest will driver specific
> > > > > > 
> > > > > > for power down I plan to define an other node
> > > > > > dbgu {
> > > > > > 
> > > > > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > > > > 	
> > > > > > 		atmel,pins =
> > > > > > 		
> > > > > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > > > > 			
> > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > > > > 
> > > > > AT91_PINCTRL_PULL_DOWN>;
> > > > > 
> > > > > > 	};
> > > > > > 
> > > > > > };
> > > > > 
> > > > > I'm afraid this won't work for Samsung SoCs. In our case normal
> > > > > and
> > > > > power down settings are completely unrelated, i.e. stored in
> > > > > separate
> > > > > registers and one doesn't affect another (a system controller
> > > > > automatically switches between normal and power down settings when
> > > > > entering or leaving low power modes, like SoC-level suspend).
> > > > 
> > > > and?
> > > 
> > > Pin configuration node on Exynos SoCs will need 7 values for each pin
> > > in samsung,pins property, just like in following example:
> > > 
> > > mmc0 {
> > > 
> > > 	mmc0_bus1: mmc0-bus1 {
> > > 	
> > > 		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN PDN_PULL_UP>;
> > > 	
> > > 	};
> > > 	/* ... */
> > > 
> > > };
> > > 
> > > Now if I just want to enable pull-up for a single pin, I will have to
> > > add following node:
> > > 
> > > foo {
> > > 
> > > 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
> > > 
> > > };
> > 
> > no you will not
> > 
> > foo {
> > 	pins = <GPK1 2 NONE PULL_UP>;
> > };
> 
> OK, this will work in case of one pin, but if you need two it starts to 
> become problematic. Let's look at an example:
> 
> We have two pins for which we don't need to specify power down mode 
> settings (e.g. they are in alive banks):
> 
> foo {
>  	pins = <GPK1 2 NONE PULL_UP>,
> 		<GPK1 3 NONE PULL_UP>;
> };

as done for cs-gpios
> 
> After compilation you will get just a series of u32 values, like
> 
> foo {
> 	pins = <1 2 255 3 1 3 255 3>;
> };



> 
> How are you going to distinguish such setup with:
> 
> foo {
>  	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>,
> 		<GPK1 3 NONE PULL_UP NONE NONE NONE>;
> };
> 
>> which translates to
> 
> foo {
> 	pins = <1 2 255 3 255 255 255 1 3 255 3 255 255 255>;
> };
> 
> I mean, you don't know where one entry ends and another starts, if you 
> allow to left out some values.

on gpios we can do so, I want to have the same feature here
> 
> > how a pin can not have mux?
> 
> I don't always want to change the mux. Sometimes I just want to change one 
> of the other parameters. For example, I don't want to switch the pin to 
> interrupt mode on driver probe (it is a separate pin mux value), but 
> rather after the trigger type gets configured, which is done by 
> request_irq() based on trigger flags.
so request the same

> 
> > > while with current bindings I can simply omit properties that I don't
> > > care about (or are going to be set up correctly by other means - e.g.
> > > gpio_direction_input() or request_irq(), ending with following node:
> > > 
> > > foo {
> > > 
> > > 	samsung,pins = "gpk1-2";
> > > 	samsung,pin-pud = <3>;
> > > 
> > > };
> > 
> > except here you will 100s of NODE which we do NOT want in the dtb
> 
> Is this really an issue?
just one example

slow down the boot
> 
> We are already using this method to describe really complex boards (not 
> necessarily in mainline) and we don't have any problems.
> 
> > > This is all I need to configure for simple things like open-drain
> > > interrupt lines.
> > > 
> > > Another thing is that we're using one driver for many SoCs, which have
> > > different variants of the controller. So for example some of them
> > > don't
> > > have driver strength configuration (S3C24xx, S3C64xx), other don't
> > > have
> > > power down mode configuration (S3C24xx) and even some of the banks on
> > > some SoCs don't support particular type of configuration (alive banks
> > > of SoCs
> > same on at91 some IP have less feature
> > and some SoC have the IP/die but not the same pins package
> > 
> > the key point is to share the pin DT handling between at91, ST and
> > Samsung ofcourse all the IP will have more or less parameter per pin
> > but the basic is the same for DT and C code
> > 
> > > >= S3C64xx don't have power down mode configuration, because they are
> > > 
> > > always on).
> > > 
> > > > on at91 I've x banks of registers to handle each gpio bank
> > > > 
> > > > on ST with have same situation but the controller work the same way
> > > > at
> > > > the end
> > > > 
> > > > so we need to factorise code
> > > > 
> > > > > Personally I'd prefer a solution with separate property for each
> > > > > parameter, because it's much more flexible and allows shorter
> > > > > lines,
> > > > > making device tree sources more readable.
> > > > 
> > > > we already discuss this on the ML the one property perline will
> > > > endup
> > > > with 100s of node if not 1000s so we all do agree we do not want
> > > > this
> > > > in the DT
> > > 
> > > Could you point me to this discussion, please? I'd really like to take
> > > a look.
> > 
> > you have to search this on the dt ML, it was about the clk bindings IIRC
> > and agree on this at Prague durring kernel Summit
> 
> OK. Will look for it.
> 
> Best regards,
> Tomasz
> 

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Sunday 19 of May 2013 12:39:26 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:46 Sun 19 May     , Tomasz Figa wrote:
> > On Sunday 19 of May 2013 11:17:36 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> > > > > > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > > > > > MULTIDRIVE      (1 << 1): indicate this pin need to be
> > > > > > > configured as
> > > > > > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > > > > > deglitch.
> > > > > > > PULL_DOWN       (1 << 3): indicate this pin need a pull
> > > > > > > down.
> > > > > > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable
> > > > > > > schmit
> > > > > > > trigger. DEBOUNCE        (1 << 16): indicate this pin need
> > > > > > > debounce.
> > > > > > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > > > > > 
> > > > > > > today I was planning to update the binding to allow to this
> > > > > > > 
> > > > > > > instead of writing this
> > > > > > > 
> > > > > > > dbgu {
> > > > > > > 
> > > > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > > > 	
> > > > > > > 		atmel,pins =
> > > > > > > 		
> > > > > > > 			<AT91_PIOB 30 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_NONE
> > 
> > > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_PULL_UP>;
> > 
> > > > > > > 	};
> > > > > > > 
> > > > > > > };
> > > > > > > 
> > > > > > > we will write this
> > > > > > > 
> > > > > > > dbgu {
> > > > > > > 
> > > > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > > > 	
> > > > > > > 		atmel,pins =
> > > > > > > 		
> > > > > > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > > > > > 			
> > > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > 
> > AT91_PINCTRL_PULL_UP>;
> > 
> > > > > > > 	};
> > > > > > > 
> > > > > > > };
> > > > > > > 
> > > > > > > so a pin can have 3 or more parameter
> > > > > > > 
> > > > > > > so as a generic binding only the 3 first will be namdatory
> > > > > > > (bank
> > > > > > > pinnp
> > > > > > > muxid) the rest will driver specific
> > > > > > > 
> > > > > > > for power down I plan to define an other node
> > > > > > > dbgu {
> > > > > > > 
> > > > > > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > > > > > 	
> > > > > > > 		atmel,pins =
> > > > > > > 		
> > > > > > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > > > > > 			
> > > > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > > > > > 
> > > > > > AT91_PINCTRL_PULL_DOWN>;
> > > > > > 
> > > > > > > 	};
> > > > > > > 
> > > > > > > };
> > > > > > 
> > > > > > I'm afraid this won't work for Samsung SoCs. In our case
> > > > > > normal
> > > > > > and
> > > > > > power down settings are completely unrelated, i.e. stored in
> > > > > > separate
> > > > > > registers and one doesn't affect another (a system controller
> > > > > > automatically switches between normal and power down settings
> > > > > > when
> > > > > > entering or leaving low power modes, like SoC-level suspend).
> > > > > 
> > > > > and?
> > > > 
> > > > Pin configuration node on Exynos SoCs will need 7 values for each
> > > > pin
> > > > in samsung,pins property, just like in following example:
> > > > 
> > > > mmc0 {
> > > > 
> > > > 	mmc0_bus1: mmc0-bus1 {
> > > > 	
> > > > 		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN 
PDN_PULL_UP>;
> > > > 	
> > > > 	};
> > > > 	/* ... */
> > > > 
> > > > };
> > > > 
> > > > Now if I just want to enable pull-up for a single pin, I will have
> > > > to
> > > > add following node:
> > > > 
> > > > foo {
> > > > 
> > > > 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
> > > > 
> > > > };
> > > 
> > > no you will not
> > > 
> > > foo {
> > > 
> > > 	pins = <GPK1 2 NONE PULL_UP>;
> > > 
> > > };
> > 
> > OK, this will work in case of one pin, but if you need two it starts
> > to
> > become problematic. Let's look at an example:
> > 
> > We have two pins for which we don't need to specify power down mode
> > settings (e.g. they are in alive banks):
> > 
> > foo {
> > 
> >  	pins = <GPK1 2 NONE PULL_UP>,
> >  	
> > 		<GPK1 3 NONE PULL_UP>;
> > 
> > };
> 
> as done for cs-gpios

Hmm? Could you explain?

> > After compilation you will get just a series of u32 values, like
> > 
> > foo {
> > 
> > 	pins = <1 2 255 3 1 3 255 3>;
> > 
> > };
> > 
> > 
> > 
> > 
> > How are you going to distinguish such setup with:
> > 
> > foo {
> > 
> >  	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>,
> >  	
> > 		<GPK1 3 NONE PULL_UP NONE NONE NONE>;
> > 
> > };
> > 
> >> which translates to
> > 
> > foo {
> > 
> > 	pins = <1 2 255 3 255 255 255 1 3 255 3 255 255 255>;
> > 
> > };
> > 
> > I mean, you don't know where one entry ends and another starts, if you
> > allow to left out some values.
> 
> on gpios we can do so, I want to have the same feature here
>

AFAIK, in case of GPIOs it depends on #gpio-cells property of GPIO 
provider node and GPIO specifiers of the same provider always need the 
same amount of cells. Correct me if I'm missing something.

> > > how a pin can not have mux?
> > 
> > I don't always want to change the mux. Sometimes I just want to change
> > one of the other parameters. For example, I don't want to switch the
> > pin to interrupt mode on driver probe (it is a separate pin mux
> > value), but rather after the trigger type gets configured, which is
> > done by request_irq() based on trigger flags.
> 
> so request the same

Sure, this will work, but is respecifying the same value and reconfiguring 
the pin with the same value multiple times really what we want?

As opposed to the amount of device tree nodes, this has the potential of 
slowing down the boot.

> > > > while with current bindings I can simply omit properties that I
> > > > don't
> > > > care about (or are going to be set up correctly by other means -
> > > > e.g.
> > > > gpio_direction_input() or request_irq(), ending with following
> > > > node:
> > > > 
> > > > foo {
> > > > 
> > > > 	samsung,pins = "gpk1-2";
> > > > 	samsung,pin-pud = <3>;
> > > > 
> > > > };
> > > 
> > > except here you will 100s of NODE which we do NOT want in the dtb
> > 
> > Is this really an issue?
> 
> just one example
> 
> slow down the boot

I haven't noticed any slow down. (533 MHz ARM1176jzf-s in S3C6410)

Best regards,
Tomasz

> > We are already using this method to describe really complex boards
> > (not
> > necessarily in mainline) and we don't have any problems.
> > 
> > > > This is all I need to configure for simple things like open-drain
> > > > interrupt lines.
> > > > 
> > > > Another thing is that we're using one driver for many SoCs, which
> > > > have
> > > > different variants of the controller. So for example some of them
> > > > don't
> > > > have driver strength configuration (S3C24xx, S3C64xx), other don't
> > > > have
> > > > power down mode configuration (S3C24xx) and even some of the banks
> > > > on
> > > > some SoCs don't support particular type of configuration (alive
> > > > banks
> > > > of SoCs
> > > 
> > > same on at91 some IP have less feature
> > > and some SoC have the IP/die but not the same pins package
> > > 
> > > the key point is to share the pin DT handling between at91, ST and
> > > Samsung ofcourse all the IP will have more or less parameter per pin
> > > but the basic is the same for DT and C code
> > > 
> > > > >= S3C64xx don't have power down mode configuration, because they
> > > > >are
> > > > 
> > > > always on).
> > > > 
> > > > > on at91 I've x banks of registers to handle each gpio bank
> > > > > 
> > > > > on ST with have same situation but the controller work the same
> > > > > way
> > > > > at
> > > > > the end
> > > > > 
> > > > > so we need to factorise code
> > > > > 
> > > > > > Personally I'd prefer a solution with separate property for
> > > > > > each
> > > > > > parameter, because it's much more flexible and allows shorter
> > > > > > lines,
> > > > > > making device tree sources more readable.
> > > > > 
> > > > > we already discuss this on the ML the one property perline will
> > > > > endup
> > > > > with 100s of node if not 1000s so we all do agree we do not want
> > > > > this
> > > > > in the DT
> > > > 
> > > > Could you point me to this discussion, please? I'd really like to
> > > > take
> > > > a look.
> > > 
> > > you have to search this on the dt ML, it was about the clk bindings
> > > IIRC and agree on this at Prague durring kernel Summit
> > 
> > OK. Will look for it.
> > 
> > Best regards,
> > Tomasz

Re: Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:22 Wed 15 May     , Stephen Warren wrote:
> > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > >> Tomasz / Linus,
> > >> 
> > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > >> <tomasz.figa@gmail.com>
> > > 
> > > wrote:
> > >>> Yes. I don't like the current way too much either, duplication
> > >>> being
> > >>> one of the reasons.
> > >> 
> > >> Do you have any other ideas?  It sounds like Linus didn't like my
> > >> suggestion and makes some good points...
> > > 
> > > I don't have anything interesting at the moment. It's a bit late now
> > > here (2 AM), so I'm going to get some sleep first.
> > > 
> > > Also after reading Stephen's reply, I'm wondering if hogging
> > > wouldn't
> > > solve the problem indeed. (It might have to be fixed on
> > > pinctrl-samsung
> > > first, as last time I tried to use it, it caused some errors from
> > > pinctrl core, but haven't time to track them down, as it wasn't
> > > anything important at that time).
> > 
> > One issue I noticed with the DT fragments earlier in this thread. It
> > looks like hogs in the Samsung pinctrl bingings end up looking like:
> > 
> > pinctrl {
> > 
> >     pina {
> >     
> >         samsung,pins = <PIN_A PIN_B PIN_C>;
> >         samsung,pin-function = <0xf>;
> >         samsung,pin-pud = <0>;
> >         ...
> 
> I have a huge issue here that we had on at91 too
> 
> we are going to have a huge numbet of node
> 
> and on at91 we handle the pin the same way as samsung
> and ST have also a similiar IP
> 
> so I'll prefer to reuse the AT91 DT bindings
> 
> as said by Linus I just push a cleanup of the magic by using Macro
> which make it really readable now
> 
> some extract of the sama5 pinctrl
> 
> 	mmc0 {
> 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> 			atmel,pins =
> 				<AT91_PIOD 9 AT91_PERIPH_A 
AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> 			atmel,pins =
> 				<AT91_PIOD 2 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP	/* PD2 periph A
> MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */ AT91_PIOD
> 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3 
with
> pullup */ };
> 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> 			atmel,pins =
> 				<AT91_PIOD 5 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP	/* PD5 periph A
> MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 with
> pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, conlicts
> with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup, 
conflicts
> with PWML3 */ };
> 	};
> 
> of sam9g45
> 
> 	i2c_gpio2 {
> 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> 			atmel,pins =
> 				<AT91_PIOB 4 AT91_PERIPH_GPIO 
AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock */ };
> 	};
> 
> so we could share the c code too

OK. After thinking about it a bit more recently, I think your solution 
might be fine.

However there is one thing I'm worried about. As far as I remember, when 
setting a function (mux selector), pinctrl core calls pin_request() to 
request all pins of the group for the device which requested the 
configuration.

Now if we use hogs to set up default configuration of pins, all of them 
would get requested for the pin controller and then further pin control 
configuration done by device drivers would fail. This is why I wanted to 
allow setting pinmux and pinconf separately, without one requiring 
another.

Best regards,
Tomasz

> Best Regards,
> J,
> 
> >     };
> >     pinp {
> >     
> >         samsung,pins = <PIN_P PIN_Q>;
> >         samsung,pin-function = <0xe>;
> >         samsung,pin-pud = <1>;
> >         ...
> >     
> >     };
> >     pinx {
> >     
> >         samsung,pins = <PIN_X PIN_Y PIN_Z>;
> >         samsung,pin-function = <0xd>;
> >         samsung,pin-pud = <2>;
> >         ...
> >     
> >     };
> >     
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&pina &pinp &pinx>;
> > 
> > };
> > 
> > That pinctrl-0 property could get rather large (hard to
> > write/maintain,
> > unwieldy) if it needs to set up lots of different configurations.
> > That's why I made the equivalent Tegra bindings be:
> > 
> > pinctrl {
> > 
> >     pins_default {
> >     
> >         pina {
> >         
> >             samsung,pins = <PIN_A PIN_B PIN_C>;
> >             samsung,pin-function = <0xf>;
> >             samsung,pin-pud = <0>;
> >             ...
> >         
> >         };
> >         pinp {
> >         
> >             samsung,pins = <PIN_P PIN_Q>;
> >             samsung,pin-function = <0xe>;
> >             samsung,pin-pud = <1>;
> >             ...
> >         
> >         };
> >         pinx {
> >         
> >             samsung,pins = <PIN_X PIN_Y PIN_Z>;
> >             samsung,pin-function = <0xd>;
> >             samsung,pin-pud = <2>;
> >             ...
> >         
> >         };
> >     
> >     };
> >     
> >     pinctrl-names = "default";
> >     pinctrl-0 = <&pins_default>;
> > 
> > };
> > 
> > The extra level within the "pinctrl configuration node"
> > ("pins_default"
> > here) makes the pinctrl-0 property a lot easier to write, and the
> > advantage happens at every use-site that needs to configure different
> > subsets of the relevant pins in different ways.
> > 
> > If you're changing all the bindings anyway, introducing this extra
> > level might be something to think about.
> > 
> > I did try to explain my philosophy here when we all got together to
> > design the pinctrl bindings, but I obviously didn't explain it well
> > enough, or people didn't like it anyway.
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:28 Tue 21 May     , Tomasz Figa wrote:
> On Friday 17 of May 2013 14:26:25 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 18:22 Wed 15 May     , Stephen Warren wrote:
> > > On 05/15/2013 06:13 PM, Tomasz Figa wrote:
> > > > On Wednesday 15 of May 2013 16:55:37 Doug Anderson wrote:
> > > >> Tomasz / Linus,
> > > >> 
> > > >> On Wed, May 15, 2013 at 3:06 PM, Tomasz Figa
> > > >> <tomasz.figa@gmail.com>
> > > > 
> > > > wrote:
> > > >>> Yes. I don't like the current way too much either, duplication
> > > >>> being
> > > >>> one of the reasons.
> > > >> 
> > > >> Do you have any other ideas?  It sounds like Linus didn't like my
> > > >> suggestion and makes some good points...
> > > > 
> > > > I don't have anything interesting at the moment. It's a bit late now
> > > > here (2 AM), so I'm going to get some sleep first.
> > > > 
> > > > Also after reading Stephen's reply, I'm wondering if hogging
> > > > wouldn't
> > > > solve the problem indeed. (It might have to be fixed on
> > > > pinctrl-samsung
> > > > first, as last time I tried to use it, it caused some errors from
> > > > pinctrl core, but haven't time to track them down, as it wasn't
> > > > anything important at that time).
> > > 
> > > One issue I noticed with the DT fragments earlier in this thread. It
> > > looks like hogs in the Samsung pinctrl bingings end up looking like:
> > > 
> > > pinctrl {
> > > 
> > >     pina {
> > >     
> > >         samsung,pins = <PIN_A PIN_B PIN_C>;
> > >         samsung,pin-function = <0xf>;
> > >         samsung,pin-pud = <0>;
> > >         ...
> > 
> > I have a huge issue here that we had on at91 too
> > 
> > we are going to have a huge numbet of node
> > 
> > and on at91 we handle the pin the same way as samsung
> > and ST have also a similiar IP
> > 
> > so I'll prefer to reuse the AT91 DT bindings
> > 
> > as said by Linus I just push a cleanup of the magic by using Macro
> > which make it really readable now
> > 
> > some extract of the sama5 pinctrl
> > 
> > 	mmc0 {
> > 		pinctrl_mmc0_clk_cmd_dat0: mmc0_clk_cmd_dat0 {
> > 			atmel,pins =
> > 				<AT91_PIOD 9 AT91_PERIPH_A 
> AT91_PINCTRL_NONE	/* PD9 periph A MCI0_CK
> > */ AT91_PIOD 0 AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD0 periph A
> > MCI0_CDA with pullup */ AT91_PIOD 1 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP>;	/* PD1 periph A MCI0_DA0 with pullup */ };
> > 		pinctrl_mmc0_dat1_3: mmc0_dat1_3 {
> > 			atmel,pins =
> > 				<AT91_PIOD 2 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP	/* PD2 periph A
> > MCI0_DA1 with pullup */ AT91_PIOD 3 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP	/* PD3 periph A MCI0_DA2 with pullup */ AT91_PIOD
> > 4 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;	/* PD4 periph A MCI0_DA3 
> with
> > pullup */ };
> > 		pinctrl_mmc0_dat4_7: mmc0_dat4_7 {
> > 			atmel,pins =
> > 				<AT91_PIOD 5 AT91_PERIPH_A 
> AT91_PINCTRL_PULL_UP	/* PD5 periph A
> > MCI0_DA4 with pullup, conflicts with TIOA0, PWMH2 */ AT91_PIOD 6
> > AT91_PERIPH_A AT91_PINCTRL_PULL_UP	/* PD6 periph A MCI0_DA5 with
> > pullup, conflicts with TIOB0, PWML2 */ AT91_PIOD 7 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP	/* PD7 periph A MCI0_DA6 with pullup, conlicts
> > with TCLK0, PWMH3 */ AT91_PIOD 8 AT91_PERIPH_A
> > AT91_PINCTRL_PULL_UP>;	/* PD8 periph A MCI0_DA7 with pullup, 
> conflicts
> > with PWML3 */ };
> > 	};
> > 
> > of sam9g45
> > 
> > 	i2c_gpio2 {
> > 		pinctrl_i2c_gpio2: i2c_gpio2-0 {
> > 			atmel,pins =
> > 				<AT91_PIOB 4 AT91_PERIPH_GPIO 
> AT91_PINCTRL_MULTI_DRIVE	/* PB4 gpio
> > multidrive I2C2 data */ AT91_PIOB 5 AT91_PERIPH_GPIO
> > AT91_PINCTRL_MULTI_DRIVE>;	/* PB5 gpio multidrive I2C2 clock */ };
> > 	};
> > 
> > so we could share the c code too
> 
> OK. After thinking about it a bit more recently, I think your solution 
> might be fine.
> 
> However there is one thing I'm worried about. As far as I remember, when 
> setting a function (mux selector), pinctrl core calls pin_request() to 
> request all pins of the group for the device which requested the 
> configuration.
> 
> Now if we use hogs to set up default configuration of pins, all of them 
> would get requested for the pin controller and then further pin control 
> configuration done by device drivers would fail. This is why I wanted to 
> allow setting pinmux and pinconf separately, without one requiring 
> another.
> 

I undertatnd you use default config for power management optimisation, is it
not?

so you need to set different state in the device instead of using hogs.
default, sleep, etc..

I get an other issue too I have the same pin requested more than one time by
multiple driver.

As thoses pins are used for external memory interface, but they have the same
config. The current pinctrl implementation does not allow share pin.

Best Regards,
J.

Re: Pulls and drive strengths in the pinctrl world

[Stephen Warren]

On 05/18/2013 10:30 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:57 Sat 18 May     , Tomasz Figa wrote:
...
>> Personally I'd prefer a solution with separate property for each 
>> parameter, because it's much more flexible and allows shorter lines, 
>> making device tree sources more readable.
>
> we already discuss this on the ML the one property perline will endup with
> 100s of node if not 1000s so we all do agree we do not want this in the DT

If you introduce s second level of nodes into the DT like the Tegra
bindings do, that's really not an issue.

For Tegra, each pinctrl state is a node.

Within this, there are a bunch of child nodes, each of which applies to
n pins, and sets up an arbitrary set of parameters; some nodes can set
up mux functions, some can set up e.g. pull-up, etc. The same pin can be
affected by multiple of these nodes.

This all allows you to group together common settings and avoid
duplication and having too many nodes.

Then, client drivers' pincrl-0 properties just reference a single
top-level "state" node, not each of the 10/100/... child nodes.

Take a look at something like nodes state_i2xmux_* in
arch/arm/boot/dts/tegra20-seaboard.dts.

Re: Pulls and drive strengths in the pinctrl world

[Stephen Warren]

On 05/19/2013 03:17 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
...
> how a pin can not have mux?

Well, if that's the way HW is designed, that's just the way it is.

There are certainly pins on Tegra which don't have a mux in HW, but have
some configuration options such as drive strength that can be configured.

Re: Pulls and drive strengths in the pinctrl world

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:42 Thu 23 May     , Stephen Warren wrote:
> On 05/19/2013 03:17 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> ...
> > how a pin can not have mux?
> 
> Well, if that's the way HW is designed, that's just the way it is.
> 
> There are certainly pins on Tegra which don't have a mux in HW, but have
> some configuration options such as drive strength that can be configured.

on Samsung it's not the case I mean

on at91 we have fixed mux and configurable mux

On Tegra IIRC it's the same

Best Regards,
J.
>