Re: [RFC PATCH] PM: runtime: Apply pinctrl settings if defined

[Joy Chakraborty]

On Wed, Nov 15, 2023 at 7:21 AM <andy.shevchenko@xxxxxxxxx> wrote:
>
> Tue, Nov 14, 2023 at 02:01:48PM +0100, Linus Walleij kirjoitti:
> > On Fri, Nov 10, 2023 at 11:21 AM Joy Chakraborty <joychakr@xxxxxxxxxx> 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 driver
> > > actions then the device driver can choose to do it from its pm callbacks,
> > > in such a case this call will be a no-op from the pinctrl core framework
> > > 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 correctly,
> > 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 runtime
> calls. For example, in some cases we force runtime PM during system suspend
> 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 states
> in an unappropriate time can lead to a disaster.
>
> So, I would consider this change okay if and only if it will have a detailed
> 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 with 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
>
>