Message-ID: <61d19678-e90c-3b3a-099a-81200873edc8@gmail.com>
Date:   Tue, 26 Sep 2017 10:51:14 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        ext Tony Lindgren <tony@...mide.com>,
        Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Stephen Warren <swarren@...dia.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Al Cooper <alcooperx@...il.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a
 state

On 09/26/2017 07:16 AM, Charles Keepax wrote:
> On Fri, Sep 22, 2017 at 09:57:22AM -0700, Florian Fainelli wrote:
>> On 09/22/2017 06:20 AM, Charles Keepax wrote:
>>> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
>>>> On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>> I guess in our case we didn't really consider the restore aspect
>>> because we essentially get that for free from regmap. Regmap
>>> cache's all our register state and provides a mechanism to sync
>>> that back to the hardware, so we simply invoke that on resume and
>>> all our GPIO/pinctrl state is restored.
>>
>> As you may see, the problem in my case is that the hardware has only onw
>> pinctrl state: "default", and it loses its hardware register contents,
>> and because of early check in pinctrl_select_state(), we do nothing (the
>> state has not changed per-se), so we are left with SW thinking we
>> applied the "default" state again, while in fact we did not.
>>
> 
> That is exactly the situation we have on the CODECs when they go
> into runtime suspend, power is removed, and everything is back at
> defaults when we resume. Just in our case we re-apply the state
> as part of the CODEC resume using a regmap sync.

Do you just re-apply the previous state, or do you force a "fake" state
by moving to a state different than the current during suspend, just to
force a transition during resume?

> 
>> The approach taken here was to move this to the core pinctrl code
>> because this is not something a pinctrl consumer should be aware of,
>> when it calls pinctrl_select_state(), it should do what it asked for.
>>
> 
> Apologies if I have missed something here, but does the consumer
> not still to some extent need to be aware with this solution
> since it needs to re-request the pin state in resume?

Consumers may indeed have to call pinctrl_select_state() but because of
the current check that does:

if (p->state == state)

this is not happening, but you are absolutely right, consumers that wish
to see their pin state be (re)configured during driver resume absolutely
need to tell the core about it, I am not thinking about any of this
happening "under the hood", this absolutely would not be right.

> 
> I think that is really my only reservation here, is it feels
> like this should be something that is purely implemented on the
> provider, and be invisible to the consumer, and I am not clear
> this is.
> 
>> I also decided to make this a per-provider property as opposed to a
>> per-group property because chances are that the state retention is on a
>> per-controller basis, and not per-bank/group, although I may be wrong.
>>
> 
> It seems quite likely that this property would mostly be
> per-provider to me as well.
> 
> Thanks,
> Charles
> 


-- 
Florian

Powered by blists - more mailing lists