From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751887AbdBFMIn (ORCPT ); Mon, 6 Feb 2017 07:08:43 -0500 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:44188 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751414AbdBFMIm (ORCPT ); Mon, 6 Feb 2017 07:08:42 -0500 Date: Mon, 6 Feb 2017 12:08:28 +0000 From: Mark Brown To: Dmitry Torokhov Cc: Liam Girdwood , linux-kernel@vger.kernel.org Message-ID: <20170206120828.nqxcbkafjpgxur6b@sirena.org.uk> References: <20170203231619.8013-1-dmitry.torokhov@gmail.com> <20170203231619.8013-4-dmitry.torokhov@gmail.com> <20170204105614.ebbzeg7s3uhpgkrg@sirena.org.uk> <20170204181318.GB12980@dtor-ws> <20170205160737.zx3zxaniacjvlrf7@sirena.org.uk> <20170206043033.GA1900@dtor-ws> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fnluwp4qfpuczzzt" Content-Disposition: inline In-Reply-To: <20170206043033.GA1900@dtor-ws> X-Cookie: Please go away. User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:470:1f1d:6b5:7e7a:91ff:fede:4a45 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH 4/4] regulator: core: make bulk API support optional supplies X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: No (on mezzanine.sirena.org.uk); Unknown failure Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --fnluwp4qfpuczzzt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Feb 05, 2017 at 08:30:33PM -0800, Dmitry Torokhov wrote: > On Sun, Feb 05, 2017 at 05:07:37PM +0100, Mark Brown wrote: > > The tlv320aic32x4 driver isn't a particularly well written driver in > > this regard in the first place - I don't recall the details but I > > strongly suspect that the driver is an example of abuse of the optional > > API and that of the supplies possibly only ldoin is actually optional. > > I would expect that this should look *much* more like sgtl5000, or > OK, I think the optional support in regulator_bulk_get will help > sgtl5000 as well, as you would not have to go through contortions of > requesting the optional regulator first and then figuring out if it > should be included or excluded from the bulk supply list. It does make things better, though I'm not sure that simply open coding the optional regulators wouldn't be just as good. I'm not 100% sure that this is what the driver actually should be doing - there were lots of problems with that driver figuring out what on earth it was supposed to do as there were some older revisions of the chip with very serious errata which used to be handled in the driver but broken the production versions. There's a degree of "it works, don't touch it" going on. > > I'd be a lot happier if I were seeing examples where this helped a lot > > and the original code looked good, I've not really been seeing those > > though. > If you can point me at a few examples where original code looks good I > can see how it can be converter to bulk with optionals support. The main one is the MMC core (which is what this was written for). > > A lot of the examples of use of optional regulators that I see > > (including both those in drivers/input FWIW) don't look like they are > > for supplies that should be optional. > Fair enough. The regulators there are not optional, rather we are trying > to avoid delays (i.e. doing msleep(N)) if regulators are not descried by > the firmware (incomplete DT in OF system, or ACPI system). Will you > accept: > bool regulator_is_dummy(struct regulator *r) > { > return r == dummy_regulator_rdev; > } This is obviously a total failure of abstraction. > or maybe even better > bool regulator_is_always_on(struct regulator *r) > { > return r->always_on; > } > Then we can switch to straightforward (non-optional) regulator_get() in > input and still avoid waiting when we don't actually control regulators. This is also an abstraction failure, though it is better, and doesn't quite do what you want since if the regulator is shared you may still end up delaying when you don't need to. What the drivers should be doing is registering a notifier which they use to check when the regulator was last enabled and using that to tell them if they need to delay (with bonus points for adjusting the delay if we're part way through the needed time). It's probably worth having a helper which registers notifiers and provides query functions that wraps this up, updates a timestamp for the last enable and has a helper drivers can use to check if it was within a given period, this is probably going to be a useful enough pattern for that. Drivers can then just enable and ask if how long ago the last physical enable happened. Depending on how severe the delays are and if anything else needs doing (like resetting registers) a simpler thing can be to use a notifier to determine if the power has been removed and reinitialize if that's happened. --fnluwp4qfpuczzzt Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAliYZzgACgkQJNaLcl1U h9D+xQf/X4RlPYY+PgzkHXG9R6XxYZIUQEnLBWII9pO0V1tDwGzwCEx+EFaPwb5x cqidJ5g15rF0SmC3MQoGCR3sZdEZvvMkyMV+MA+j0e/ZWSo6S4Du66rki/Xa64ei Kys/mEa42mnRM2cDYCWqMdFHmjTzxc0oY3WU12fSiXdnLx75Gvi3j1dpM3rYb0qu Zs5FvhLPKLAKQ4VT+vCDPCCG3B/oWUXfPl92FGqDQCtO4FCEGnhBvYYBD76+vyUV WFs7NsVDal6PHBfFfRuuP9bMgIuyT0OKYiWDdIh7ZwHD2Vv3bZltPS2pVMTT8OTb Fo8WZbI6LXSWH2KwX8KNNO6yL3CQxA== =qvng -----END PGP SIGNATURE----- --fnluwp4qfpuczzzt--