From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752684AbdBCVvZ (ORCPT ); Fri, 3 Feb 2017 16:51:25 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:36006 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752469AbdBCVvX (ORCPT ); Fri, 3 Feb 2017 16:51:23 -0500 Date: Fri, 3 Feb 2017 13:51:20 -0800 From: Dmitry Torokhov To: Mark Brown Cc: Thierry Reding , Jelle van der Waa , Linus Walleij , linux-kernel@vger.kernel.org Subject: Re: regulator_get_optional() no longer returning NULL? Message-ID: <20170203215120.GA22933@dtor-ws> References: <20170201222331.GE40720@dtor-ws> <20170203112001.m5nwfrw7ickum3r7@sirena.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170203112001.m5nwfrw7ickum3r7@sirena.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 03, 2017 at 12:20:01PM +0100, Mark Brown wrote: > On Wed, Feb 01, 2017 at 02:23:31PM -0800, Dmitry Torokhov wrote: > > > It appears that [devm_]regulator_get_optional() and > > [devm_]gpiod_get_optional() behave irritatingly differently. While the > > latter returns NULL for non-existing GPIOs, the former started returning > > -ENODEV instead of NULL, starting with commit below. > > > Why did we do that? It is much more convenient to write: > > As the commit description for that commit says it's fixing a bug with us > not returning the error code we get from regulator_dev_lookup(). > > > I.e. it is nice to treat *all* codes returned by > > devm_regulator_get_optional() as fatal and NULL as special instead of > > vetting by hand (and having chance that list of vetted codes will bit > > rot). > > There's no real risk of bitrot I can see here, the only thing something > using optional regulator is looking for is that the regulator does not > exist and we have the specific error code -ENODEV for that. > > > Can we please revert this patch? > > That won't do what you want so I don't know why you're asking for it - > the default value of regulator is ERR_PTR(-EPROBE_DEFER) so if we skip > assigning the actual return code you'll get that and not NULL. I don't > recall that behaviour ever working, it's not what's documented so it > doesn't look like something broke here. If you wanted to implement it > you'd need to add new code to do it (eg, squash down -ENODEV in > _get_optional()). Yes, you are right. The _regulator_get() is a bit confusing, I have a patch series that hopefully makes it a bit more straightforward. Unfortunately it seems some of the drivers are not ready for regulator_get_optional() to return NULL, I'll have to prepare them first. > > However I am a bit dubious about doing that as I'm not thrilled with > adding in more things for people to check. Even if you check for NULL > with an optional regulator you'd still need to also go and check that > you didn't get an error code like -EPROBE_DEFER, you'd only get a NULL > in cases where the regulator does not exist. Given that I'm not sure > that it's actually simplifying things. Like I said, I want people to simply check for error/!error and have all errors be fatal, and the rest of the checks (when trying to use said regulator) should be in form of: if (blah->supply) regulator_enable(supply). Or even teach regulator_enable() to like NULLs. Allowing regulator_get_optional() return NULL will make it work similarly to gpiod_get_optional() which I think is a good thing. By the way, I see quite a few drivers using regulator_get_optional() for cases when regulator is mandatory, but it might be handled by the firmware. I.e. it seems they can be converted to regulator_get() with the expectation that it will give us dummy supply. Would such conversions be OK/accepted? Thanks. -- Dmitry