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

[andy . shevchenko]

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)...

> 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).

-- 
With Best Regards,
Andy Shevchenko