From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752209AbdLJXiI (ORCPT ); Sun, 10 Dec 2017 18:38:08 -0500 Received: from mail-qt0-f195.google.com ([209.85.216.195]:40672 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751137AbdLJXiF (ORCPT ); Sun, 10 Dec 2017 18:38:05 -0500 X-Google-Smtp-Source: AGs4zMZ8s9NCP+/atBfWxQGx8PQaDdt06KjAmDRuS+D0BSFzzUfnTyqL7bIq704hAwLVPVep/IblQg== Subject: Re: [PATCH v2 2/2] pinctrl: Allow indicating loss of pin states during low-power To: Linus Walleij Cc: Tony Lindgren , linux-gpio@vger.kernel.org, Rob Herring , Mark Rutland , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , Charles Keepax , Charles Keepax , Stephen Warren , Andy Shevchenko , Al Cooper , bcm-kernel-feedback-list References: <20171102231551.16220-1-f.fainelli@gmail.com> <20171102231551.16220-3-f.fainelli@gmail.com> <20171129170247.GI28152@atomide.com> <96cf5d74-3acf-07b9-9ad8-1011cd99a860@gmail.com> From: Florian Fainelli Message-ID: <908c66f9-f9bd-a4df-e241-75595a3a3e27@gmail.com> Date: Sun, 10 Dec 2017 15:38:00 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/02/2017 04:48 AM, Linus Walleij wrote: > On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli wrote: >> On 11/29/2017 09:02 AM, Tony Lindgren wrote: > >>> 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