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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Mon, Oct 24, 2022 at 5:05 PM Shenwei Wang <shenwei.wang@xxxxxxx> wrote:

> > 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.
> >
>
> The current implementation is using the static data inside the gpio driver, although it
> is not used the gpio-range data structure.

So get to it. Use gpio-ranges and put this functionality in the pin control
portions.

> > 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.
> >
>
>  No matter you prepare the mapping by using the gpio-ranges or the static array in the
> driver,  they serve the same purpose. The only user to use the mapping is the gpio driver,
> and the mapping is constant for a SoC family. That's why I chose to define the array inside
> the driver.

This is not an excuse to not use gpio-ranges and duplicate code and
data. Switch to using gpio-ranges.

> > 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.
> >
>
> That's another kind of wakup method. Currently we are talking about the pad
> wakeup. The examples you gave like SD/UART, they are not pad wakeup feature,
> we can call them as IP native wakeup.

Hm intersting!

> For those IP blocks' native wakeup, they are not managed by SCU. It is managed by
> the block's driver directly. For example, the UART's native wakeup function is
> enabled or disabled via the driver itself, and no communication required for SCU.

OK I was wrong about that.

> > > 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 not a hack because the imx_scu_call_rpc function is designed to serve multiple threads
> use case. As the pad wakeup function is an independent feature accompany
> with the gpio function, I prefer to put the implementation inside the gpio driver.

Our job as maintainers is to make sure the code is easy to maintain.

If duplicate data is stored in the GPIO driver and the pin control driver
just because gpio-ranges are not used to be able to put the wakeup code
together with the rest of the pad/pin related data, that means more work for
us.

For this reason:
1. use gpio-ranges to reference the pin control subsystem
2. Act on the config inside the SCU pin control driver which already knows
  about all pads/pins.

It just makes more sense for us.

Yours,
Linus Walleij