Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20231110102054.1393570-1-joychakr@google.com> <CACRpkdZ9RHcHh4o5g62ywK0eQHpLZuGUF0Ud6jogk9Sfqe4krA@mail.gmail.com>
 <ZVQkLqDB3KtOlIpK@surfacebook.localdomain>
In-Reply-To: <ZVQkLqDB3KtOlIpK@surfacebook.localdomain>
From:   Joy Chakraborty <joychakr@google.com>
Date:   Thu, 16 Nov 2023 21:04:15 +0530
Message-ID: <CAOSNQF3QeFd857RCJE8wfJ=__-K7Bi4vfMeTVP-+O+LJ7y9SmQ@mail.gmail.com>
Subject: Re: [RFC PATCH] PM: runtime: Apply pinctrl settings if defined
To:     andy.shevchenko@gmail.com
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org, manugautam@google.com,
        aniketmaurya@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 15, 2023 at 7:21=E2=80=AFAM <andy.shevchenko@gmail.com> wrote:
>
> Tue, Nov 14, 2023 at 02:01:48PM +0100, Linus Walleij kirjoitti:
> > On Fri, Nov 10, 2023 at 11:21=E2=80=AFAM Joy Chakraborty <joychakr@goog=
le.com> wrote:
> >
> > > Apply pinctrl state from  runtime framework device state transtion.
> > >
> > > Pinctrl states if defined in DT are bookmarked in device structures
> > > but they need to be explicitly applied from device driver callbacks
> > > which is boiler plate code and also not present in many drivers.
> > >
> > > If there is a specific order of setting pinctrl state with other driv=
er
> > > actions then the device driver can choose to do it from its pm callba=
cks,
> > > in such a case this call will be a no-op from the pinctrl core framew=
ork
> > > since the desired pinctrl state would already be set.
> > >
> > > We could also add a Kconfig knob to enable/disable this, but I do not
> > > see a need to.
>
> Besides questionable code style (inline functions in the C file)...

Sure, I can change that.

>
> > It has a certain beauty to it does it not!
> >
> > The reason it wasn't done like this from the start was, if I recall cor=
rectly,
> > that in some cases a device needs to do the pin control state switching
> > in a special sequence with other operations, that can not be reordered,
> > i.e.:
> >
> > 1. The pin control state change is not context-free.
> >
> > 2. The order of events, i.e. context, does not necessarily match the
> >      order that Linux subsystems happen to do things.
> >
> > When looking through the kernel tree I don't see that people use
> > the sleep state and idle state much, so we could very well go
> > with this, and then expect people that need special-casing to name
> > their states differently.
> >
> > What do people thing about that?
>
> ...I think the patch is incomplete(?) due to misterious ways of PM runtim=
e
> calls. For example, in some cases we force runtime PM during system suspe=
nd
> which may have an undesired effect of the switching pin control states
> (hence glitches or some real issues with the hardware, up to hanging the
> system). Some pins may be critical to work with and shuffling their state=
s
> in an unappropriate time can lead to a disaster.
>
> So, I would consider this change okay if and only if it will have a detai=
led
> research for all existing users to prove there will be no changes in the =
whole
> set of possible scenarious (of system sleep / resume, runtime, runtime wi=
th a
> custom ->prepare callback and so on).
>

I tried to place the calls to set the pinctrl states after driver/user
callback  based on my understanding of runtime code so that existing
users do get a chance to set the state with any special sequence that
needs to be performed post which doing another call to set the state
would be ignored in the pinctrl framework.

But this only would be possible with the assumption that even in any
special sequences executed by users they set nothing but "default"
state in runtime_resume, "idle" state in runtime_idle and "'sleep"
state in their runtime suspend callbacks.
And like Andy mentions about "->prepare callback", if there are
drivers that are setting pinctrl state "default", "sleep" or "idle"
from any callback but
...
int (*runtime_suspend)(struct device *dev);
int (*runtime_resume)(struct device *dev);
int (*runtime_idle)(struct device *dev);
...
it could indeed be a problem.
I'll dig into users of pinctrl_select_sleep/default/idle and see if
there are such cases or if it could be done in some other way.

Thanks
Joy

> --
> With Best Regards,
> Andy Shevchenko
>
>