Re: API for pull-up/pull-down configuration

[Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>]

Hello Linus,

On Sun, 16 Dec 2018 01:05:17 +0100, Linus Walleij wrote:

> weird that you write this just after my note to Marek (who just made a
> tremendous
> job modernizing this driver):
> https://marc.info/?l=linux-gpio&m=154479904816811&w=2

I rebased on top of Marek changes, and they are indeed really good, and
make the PCA953x driver a lot better. Thanks Marek!


> 1. The really complex way and refactor it into a GPIO+pin controller
> combo as we did with
> drivers/pinctrl/pinctrl-sx150x.c
> drivers/pinctrl/pinctrl-mcp23s08.c
> If the chip logic is "complicated enough" this is definately the way to
> go. If the chip support muxing in other functions than GPIO it is the
> only reasonable way to go.

For something like PCA953x, it seems really overkill, no?

> 2. The simple way. We already support a whole bunch of standard
> pin config in the .set_config() callback. Debounce (which this silicon
> seem to support) can be implemented right off to begin with.
> We also support this callback for open drain/source configuration.
> But in theory any of the stuff from include/linux/pinctrl/pinconf-generic.h
> can be passed to this function (that is why we unified it).

I went for this solution, and just posted an initial proposal for
review, see:

  https://marc.info/?l=linux-gpio&m=154653367807930&w=2

It implements some simple GPIO_PULL_UP and GPIO_PULL_DOWN flags that
can be used from the Device Tree.

Let me know what you think of this proposal.

> For this (2) approach, it would be possible to add some more pin
> config options, which I think is all right for anything that is
> GPIO only (no muxing). There is no stopping it.
> 
> Paradoxically you can probably support any config with board files,
> and even userspace access for this approach, but you will run into
> a problem when you get to device tree:
> -
> foo-gpios = <&gpio0 12 GPIO_FOO>;
> 
> This last flag needs to encode all options and we have only 32
> bits in it I suppose.
> 
> Of course it is simple to just support GPIO_PULL_UP and
> GPIO_PULL_DOWN, but I have seen so much silicon supporting
> setting the number of Ohm for this. This is not the only
> pin config that may need an argument: drive strength is the
> same (I think this hardware supports that too...)
> 
> So a compromise would be to allow GPIO_PULL_UP and
> GPIO_PULL_DOWN, GPIO_DRIVE_EXTRA_JUICE flags
> if and only if they are just off/on
> switches (boolean) and any more complicated hardware will
> be required to implement proper pin control.
> 
> This would have to be discussed by the DT maintainers though.
> It might be necessary to introduce fourcell GPIO specifiers
> to handle the arguments.
> 
> foo-gpios = <&gpio0 12 GPIO_PULL_UP 8>; // 8 Ohm
> 
> What do you think?

I think at some point the idea of passing all flags/parameters through
GPIO cells in the DT is not going to scale. If we really want to
define a lot of additional parameters on a per-GPIO basis in the DT,
then I would suggest that we use sub-nodes in the GPIO controllers.
Something like this:

	gpio-controller@e000a000 {
		compatible = ...
		reg = <0xe000a000 0x1000>;
		gpio-controller;

		gpio11: gpio@11 {
			pull-up = <8>;
		};

		gpio12: gpio@12 {
			drive = "open-drain";
		};
	};

	foo {
		gpios = <&gpio11>;
	};

	bar {
		gpios = <&gpio12>;
	};

Of course, this is a much larger rework, and the current representation
with one DT cell for the GPIO flags will anyway be preserved.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com