From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753045AbbCIHk3 (ORCPT ); Mon, 9 Mar 2015 03:40:29 -0400 Received: from bhuna.collabora.co.uk ([93.93.135.160]:41893 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752477AbbCIHk0 (ORCPT ); Mon, 9 Mar 2015 03:40:26 -0400 Message-ID: <54FD4E64.2070909@collabora.co.uk> Date: Mon, 09 Mar 2015 08:40:20 +0100 From: Javier Martinez Canillas User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 MIME-Version: 1.0 To: Mark Brown CC: Doug Anderson , Liam Girdwood , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume References: <1425328839-28330-1-git-send-email-javier.martinez@collabora.co.uk> <54F60602.3030505@collabora.co.uk> <54F70C5C.4080201@collabora.co.uk> <20150308193812.GW28806@sirena.org.uk> In-Reply-To: <20150308193812.GW28806@sirena.org.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/08/2015 08:38 PM, Mark Brown wrote: > On Wed, Mar 04, 2015 at 02:45:00PM +0100, Javier Martinez Canillas wrote: > >> The thing is that _regulator_is_enabled() used to return -EINVAL if >> the rdev didn't have an .is_enabled callback but that changed in >> commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if >> they don't report anything") and now returns 1 in that case. But >> _regulator_enable() was not changed and is still checking for -EINVAL >> which seems to me like a left over after the mentioned commit. > > You mean _do_enable(), not _enable() here. It's not really a leftover No, I meant _enable() here. What I said is that _enable() is checking if -EINVAL was returned by _is_enabled(): static int _regulator_enable(struct regulator_dev *rdev) { ... ret = _regulator_is_enabled(rdev); if (ret == -EINVAL || ret == 0) { if (!_regulator_can_change_status(rdev)) return -EPERM; ret = _regulator_do_enable(rdev); if (ret < 0) return ret; } else if (ret < 0) { rdev_err(rdev, "is_enabled() failed: %d\n", ret); return ret; } ... } and my point was that it is checking because _is_enabled() used to return -EINVAL if the regulator driver didn't provide a .is_enabled callback: static int _regulator_is_enabled(struct regulator_dev *rdev) { ... if (!rdev->desc->ops->is_enabled) return -EINVAL; return rdev->desc->ops->is_enabled(rdev); ... } so, if a driver didn't provide a way to query if the regulator was enabled, it was assumed that it was disabled. But after the mentioned commit, the assumption was changed and now not having .is_enabled means that it's enabled: static int _regulator_is_enabled(struct regulator_dev *rdev) { ... if (!rdev->desc->ops->is_enabled) return 1; return rdev->desc->ops->is_enabled(rdev); ... } So my question was if _is_enabled() returning -EINVAL should still mean that the regulator has to be enabled or the error has to be propagated since now -EINVAL will be returned by the driver .is_enabled callback. > as the two operations are doing somewhat different things and the > changes are a bit separate, _is_enabled() is reporting the current state > while _do_enable() is making a change so it should fail if it can't do > that. > Yes, I understand that. > A better way of writing it in the _do_enable() case is that it possibly > ought to be checking if the regulator is enabled before it does > anything, though for uncached regulator operations that then means an > extra I/O which isn't great. Given that I think rather than ignoring > the missing op it should instead fall back to checking _is_enabled() - > that way if we can read the state but not change it the right thing will > happen. I'll do a patch, probably tomorrow. > Best regards, Javier