On 12/02/2017 04:48 AM, Linus Walleij wrote:

On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli [off-list ref] wrote:

quoted

On 11/29/2017 09:02 AM, Tony Lindgren wrote:

quoted

quoted

Hmm well typically a device driver that loses it's context just does
save and restore of the registers in runtime PM suspend/resume
as needed. In this case it would mean duplicating the state for
potentially for hundreds of registers.. So using the existing
state in the pinctrl subsystem totally makes sense for the pins.

Florian do you have other reasons why this should be done in the
pinctrl framework instead of the driver? Might be worth describing
the reasoning in the patch descriptions :)

The pinctrl provider driver that I am using is pinctrl-single, which has
proper suspend/resume callbacks but those are not causing any HW
programming to happen because of the (p->state == state) check, hence
this patch series.

So we are talking about these callbacks, correct?

#ifdef CONFIG_PM
static int pinctrl_single_suspend(struct platform_device *pdev,
                                        pm_message_t state)
{
        struct pcs_device *pcs;

        pcs = platform_get_drvdata(pdev);
        if (!pcs)
                return -EINVAL;

        return pinctrl_force_sleep(pcs->pctl);
}

static int pinctrl_single_resume(struct platform_device *pdev)
{
        struct pcs_device *pcs;

        pcs = platform_get_drvdata(pdev);
        if (!pcs)
                return -EINVAL;

        return pinctrl_force_default(pcs->pctl);
}
#endif

Which falls through to this:

/**
 * pinctrl_force_sleep() - turn a given controller device into sleep state
 * @pctldev: pin controller device
 */
int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
{
        if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
                return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
        return 0;
}
EXPORT_SYMBOL_GPL(pinctrl_force_sleep);

/**
 * pinctrl_force_default() - turn a given controller device into default state
 * @pctldev: pin controller device
 */
int pinctrl_force_default(struct pinctrl_dev *pctldev)
{
        if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
                return pinctrl_select_state(pctldev->p, pctldev->hog_default);
        return 0;
}
EXPORT_SYMBOL_GPL(pinctrl_force_default);

So am I right in assuming it is actually the hogs that is your biggest
problem, and those are the states that get lost over suspend/resume
that are especially problematic?

I.e. you don't have any problem with any non-hogged pinctrl
handles, those are handled just fine in the suspend/resume
paths of the client drivers?

If this is the case, it changes the problem scope slightly.

It is fair that functions named *force* should actually enforce
programming a state.

So then I would suggest somethin else: break pinctrl_select_state()
into two:

pinctrl_select_state() that works just like before, checking if
(p->state == state) but which calls a static function
pinctrl_select_state_commit() that commits the change unconditonally.
Then alter pinctrl_force_sleep() and pinctrl_force_sleep() to call
that function.

This should solve your problem without having to alter the semantics
of pinctrl_select_state() for everyone.

This was exactly what I proposed initially here:

http://patchwork.ozlabs.org/patch/734326/

I really want to get this fixed, but I can't do that if we keep losing
the context of the discussion (pun intended) :).
-- 
Florian