From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH V3] PM / Domains: Remove intermediate states from the power off sequence Date: Wed, 24 Jun 2015 10:57:01 -0700 Message-ID: <7hy4j9q9aq.fsf@deeprootsystems.com> References: <1434020737-13354-1-git-send-email-ulf.hansson@linaro.org> <7ha8w01qir.fsf@deeprootsystems.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from mail-pd0-f172.google.com ([209.85.192.172]:36854 "EHLO mail-pd0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752950AbbFXR5H (ORCPT ); Wed, 24 Jun 2015 13:57:07 -0400 Received: by pdcu2 with SMTP id u2so35284737pdc.3 for ; Wed, 24 Jun 2015 10:57:05 -0700 (PDT) In-Reply-To: (Ulf Hansson's message of "Thu, 18 Jun 2015 12:14:28 +0200") Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Ulf Hansson Cc: "Rafael J. Wysocki" , "linux-pm@vger.kernel.org" , Len Brown , Pavel Machek , Geert Uytterhoeven , Lina Iyer , Dmitry Torokhov , Axel Haslam , Krzysztof Kozlowski , Russell King , "linux-arm-kernel@lists.infradead.org" Ulf Hansson writes: [...] >> Minor nit: I think you should leave these 3 helpers and just simplify >> them. It will make the changes below easier to read as well. > > I would rather like to remove them. The reason is to create consistency. > > For the locking part, there are currently mixtures of > mutex_lock|unlock() and genpd_acquire|release_lock(). Following your > suggestion will leave around 6-7 places where mutex_lock() will remain > used (additionally for mutex_unlock()). So removing the helper > functions creates a consistent behaviour. OK, that makes more sense. > For the genpd->status, it's currently being assigned at various places > without using a helper function. Again I wanted to create a consistent > behaviour and make the code more readable. > >> >> Also, for the locking, Lina will be adding these locking functions back >> anyways, so let's just avoid the extra churn. > > Actually I think it becomes more evident what Lina's patchset does if > she re-introduces an API to deal with the locking. Moreover we can > "force" that patchset to not break consistently around the locking. > >> >> Otherwise, I think this version is looking really good. >> >> With the above tweaks, feel free to add >> >> Reviewed-by: Kevin Hilman >> >> Kevin > > Thanks a lot for reviewing! > > If you have a strong opinion about your suggestions, I will happily > adapt to them, please let me know. No strong option, you convinced me your way will actually make things more consistent, and Lina prefers them gone as well, so I'm fine with them gone (as you've done in V4). Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 From: khilman@kernel.org (Kevin Hilman) Date: Wed, 24 Jun 2015 10:57:01 -0700 Subject: [PATCH V3] PM / Domains: Remove intermediate states from the power off sequence In-Reply-To: (Ulf Hansson's message of "Thu, 18 Jun 2015 12:14:28 +0200") References: <1434020737-13354-1-git-send-email-ulf.hansson@linaro.org> <7ha8w01qir.fsf@deeprootsystems.com> Message-ID: <7hy4j9q9aq.fsf@deeprootsystems.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Ulf Hansson writes: [...] >> Minor nit: I think you should leave these 3 helpers and just simplify >> them. It will make the changes below easier to read as well. > > I would rather like to remove them. The reason is to create consistency. > > For the locking part, there are currently mixtures of > mutex_lock|unlock() and genpd_acquire|release_lock(). Following your > suggestion will leave around 6-7 places where mutex_lock() will remain > used (additionally for mutex_unlock()). So removing the helper > functions creates a consistent behaviour. OK, that makes more sense. > For the genpd->status, it's currently being assigned at various places > without using a helper function. Again I wanted to create a consistent > behaviour and make the code more readable. > >> >> Also, for the locking, Lina will be adding these locking functions back >> anyways, so let's just avoid the extra churn. > > Actually I think it becomes more evident what Lina's patchset does if > she re-introduces an API to deal with the locking. Moreover we can > "force" that patchset to not break consistently around the locking. > >> >> Otherwise, I think this version is looking really good. >> >> With the above tweaks, feel free to add >> >> Reviewed-by: Kevin Hilman >> >> Kevin > > Thanks a lot for reviewing! > > If you have a strong opinion about your suggestions, I will happily > adapt to them, please let me know. No strong option, you convinced me your way will actually make things more consistent, and Lina prefers them gone as well, so I'm fine with them gone (as you've done in V4). Kevin