Re: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Linus Walleij <linus.walleij@linaro.org>]

On Tue, Oct 18, 2022 at 9:42 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:

> #define PINCTRL_PIN(a, b) { .number = a, .name = b }
> #define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)
>
> The imx8qm_pinctrl_pads array gives each PIN a string type of name.  The array I defined in
> this patch adds another dimension information of the PIN, which is the GPIO index. These
> two are not duplicated information.
(...)
> The purpose for the array here is to record the relationship of a GPIO line with a PIN.
(...)
> From the PINCTRL driver, you may get the information regarding a PIN's Mux status
> and something relating to the signal settings like pullup/pulldown/drive strength. So
> you can know if a PIN is a GPIO function PIN or not, but you can't know which GPIO port and
> which GPIO line the PIN belongs to.

Cross-referencing GPIO line numbers to pins/pads is literally the
job of gpio-ranges (the DT property) and we have excellent support in
the GPIO library to do exactly this.

It can even be made using static data in the driver if gpio-ranges for
some reason cannot be encoded in the device tree.

(...)
> In order to build up the cross-reference between GPIO lines and the PINs, you need
> have a pre-prepared gpio-ranges mapping in advance. Because the relationship between
> the PIN and the GPIO line is not a linear, you end up to build up the gpio-ranges in the same
> way like the array in this patch. This turns to a chicken-egg problem.

I don't get it. gpio-ranges can contain any number of "holes" and whatever,
the name "rangeS" (plural) implies it can be an array of ranges. If you want
you can have a list of single-pin ranges, no problem.

> As I explained above, this is not the duplicated information. It adds another
> dimension of a information regarding a PIN. As these information is standalone
> and self-contained,  no matter how you change the pin tables, it won't impact
> the function here.

Since the information/setting pertains to an electronic or similar property
of a pin it falls under the pin config umbrella.

> This is a kind of GPIO wakeup function, and it happened to be given a name called
> pad wakeup. From the user point of view, it works the same way as the native
> GPIO wakeup. Except the name itself, it has nothing to do with the PINCTRL.

Hardly.

The MMC/SD card bus has ways of waking up devices by pulling a line
low, as does things such as UARTs. And then the pin isn't even used as
GPIO.

I bet a million to one that the exact same setting is used inside the
SCU for waking up on such events on the pins.

I don't believe that just because a pin is muxed to something else than GPIO
it cannot be programmed to wake the system up.

If you look into my presentation "building GPIO and pins from the ground up":
http://www.df.lth.se/~triad/papers/pincontrol.pdf
see the picture on page 13 and page 18. The asynchronous edge detector is
what takes the system online in sleep. That is a property of the pin
cell, it has
nothing to do with the GPIO block, which is further in.

It belongs in pin control because it is a property of the pin hardware.

> As I explained in the above, this is not the problem of two clients accessing the same
> resource, so there is no conflict between the two drivers. It works in the same way
> like SCU power domain driver and SCU clock driver. The communication integrity is
> guaranteed by the call of imx_scu_call_rpc itself.

I understand that it makes your life simpler to just implement this as a hack
in the MXC GPIO driver like this, but it is not a proper solution, and I ask you
to do the more complicated and work consuming task instead.

This is because it will have partitioned the problem in a clear way that is
recognizable by the maintainers and will scale to other SoCs in the future.

I also wonder where all the other i.MX maintainers are. I need their input
in this discussion.

Yours,
Linus Walleij