On Thu, Dec 5, 2013 at 11:15 AM,  [off-list ref] wrote:

Laurent Pinchart wrote:

quoted

On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:

quoted

Controlling GPIO through pinconf seems to be very twisted.
Usually it is the other way around.

I refer you to pinctrl.txt. What I'm trying to avoid here is the "GPIO mode pitfall", where we can /only/ usefully manipulate the lines through the Linux gpio API.

I want to be able to drive a function line low in sleep mode, without having to write code that uses the gpio_ API. I want to be able to express that as PIN_CONFIG_OUTPUT 0.

Where we currently stand locally is that we have a load of code that use the legacy function GPIOs thus:

resume:
    gpio_request(GPIO_FN_XXX)  // Set function mux on GPIO36 pin
suspend:
    gpio_request(GPIO_PORT36)  // Set GPIO mux on pin
    gpio_direction_output(GPIO_PORT36, 0);

which is exactly the scenario described in "GPIO mode pitfalls". Some drivers do this manually, and some use a local pinctrl-ish framework; they're passed big platform data tables with lists of suspend+active GPIO numbers, directions, values and pulls, and then pass those tables back to a library mux-change routine in their suspend/resume handlers.

By my understanding of the general pinctrl concept, we should be converting that to the lovely:

resume:
    pinctrl_pm_select_default_state(dev);
suspend:
    pinctrl_pm_select_sleep_state(dev);

This observation is correct, and I agree with this reasoning. The PM
states are quite new and may
take some time before they "sink in" in the kernel though, but ideally
people should be using
them like that (atleast IMO).

quoted

quoted

This looks messy. The intention with the pin config subsystem is to
break things apart and make them understandable that way, and to
handle electronic stuff in pin config whereas driving a line high/low
happens in the GPIO subsystem.

Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, and the
whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It means we can't specify idle pinctrl
states that drive lines. We have to do the GPIO/function shuffle.

Well maybe. My point was that it is OK to drive GPIO lines as part of a state
which is only relevant for a device driver, and where using the GPIO API
in top would only bloat and make things hard to follow.

And you're right, it is a little messy in the pin driver implementation, which is
what I'm finding, and what leads me here to question the detail. Nevertheless,
I've found implementing PIN_CONFIG_OUTPUT on this hardware design is
possible, and produces elegant driver+board code.

I believe you.

In my initial prototype, PIN_CONFIG_OUTPUT is only available if the gpio-mode
function has been selected.

That is even more careful code than most drivers write, so sounds
very nice.

But maybe this could be made hidden and implicit,
as per the GPIO API. Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
implicitly selects the GPIO mux (for output/input), and
PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
restores the previously requested?

I'd recommend trying to keep as close to the semantics descrived
in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

quoted

quoted

This must be some misunderstanding. DT does not specify
application/ordering semantics. It does not define sequences at all.
The device driver parsing out whatever is in the device tree has to
take care of ordering stuff and driving the hardware.

Well, the pinctrl core does process its tables in order. So in the case where a
pinconf setting may interact with a pinmux setting, the ordering that the
pinconf+pinmux entries get processed could be significant. Eg if
PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

OK so we solved the similar situation where configs had to be
applied in some certain order for the pin config API by passing
the array of configs down to the driver and let is handle this.

So can we think of an API that gives the drivers the freedom to
express or behave differently here?

We *could* add something like void (* pin_state_commence) ()
to the pinctrl_ops so the driver could cache mux and config ops
and wait for the commence signal before writing anything to the
hardware, and that would be pretty complex.

If it is always the same for every state setting then maybe we
can move this to the core so that we'd add some semantic
flag like

enum state_application_order {
  PINCTRL_ORDER_UNDEFINED,
  PINCTRL_MUX_THEN_CONFIG,
  PINCTRL_CONFIG_THEN_MUX,
};

Then the core needs to alter behaviour on this flag. Any
suggestions welcome, but I won't apply any patches like that
unless there is a driver making use of it.

Yours,
Linus Walleij