Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751728AbdLLLyc (ORCPT ); Tue, 12 Dec 2017 06:54:32 -0500 Received: from heliosphere.sirena.org.uk ([172.104.155.198]:36774 "EHLO heliosphere.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751400AbdLLLya (ORCPT ); Tue, 12 Dec 2017 06:54:30 -0500 Date: Tue, 12 Dec 2017 11:54:27 +0000 From: Mark Brown To: Maciej Purski Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Liam Girdwood , Rob Herring , Mark Rutland , Marek Szyprowski , Bartlomiej Zolnierkiewicz Subject: Re: [PATCH v3 4/4] regulator: core: Balance coupled regulators voltages Message-ID: <20171212115427.GG16323@sirena.org.uk> References: <1512639975-22241-1-git-send-email-m.purski@samsung.com> <1512639975-22241-5-git-send-email-m.purski@samsung.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="f5QefDQHtn8hx44O" Content-Disposition: inline In-Reply-To: <1512639975-22241-5-git-send-email-m.purski@samsung.com> X-Cookie: This screen intentionally left blank. User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2387 Lines: 66 --f5QefDQHtn8hx44O Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 07, 2017 at 10:46:15AM +0100, Maciej Purski wrote: > @@ -2447,10 +2482,9 @@ static int _regulator_is_enabled(struct regulator_= dev *rdev) > return rdev->desc->ops->is_enabled(rdev); > } > =20 > -static int _regulator_list_voltage(struct regulator *regulator, > - unsigned selector, int lock) > +static int _regulator_list_voltage(struct regulator_dev *rdev, > + unsigned selector, int lock) > { Please split this refactoring of _list_voltage() into a separate patch for ease of review. It can go in separately which will make this change smaller and easier to review. > @@ -2928,6 +2961,35 @@ static int regulator_set_voltage_unlocked(struct r= egulator *regulator, > if (ret < 0) > goto out2; > =20 > + /* > + * If the regulator is not coupled just set voltage normally, else > + * return after changing consumer demands without changing voltage. > + * This will be handled outside the function > + * by regulator_balance_coupled() > + */ > + if (!rdev->coupling_desc) { > + ret =3D regulator_set_voltage_rdev(regulator->rdev, > + min_uV, max_uV); > + if (ret < 0) > + goto out2; > + } As I think I said on the previous version I'm not enthusiastic about having two separate code paths for setting the voltage, it makes it much more likely that things will break especially given how rare coupled regulators are. It would be cleaner to make uncoupled regulators just be a special case of coupled regulators, that way more of the code is shared. To that end I'd adjust the code so that we always have a coupling descriptor and then handle the case where there's only one regulator described in there. --f5QefDQHtn8hx44O Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAlovw3IACgkQJNaLcl1U h9C9lwf/RQhhuU0Mh0v0rdWO1U9/Z262z3tkPLJ2N56TbCbLG/2AmpvvsFxEtNR3 GnlpedBKASMaVa8laIXqLzs1p93nRMkrhAUXx9DKroLU5Rdzrns6VmA+mD0DkV/8 0qXFD23JNyvnQej8wipBUmHO+kqbpdt3orjmdwlpp13qtsWGPXhG3ey2y4u+3E8i HQOhn7XWbCAiyOGwYrX2MbowWIDl5/d7aO1JMyR7PT58Fumi+x3/VTSOeH8FdgO+ 20GCtM9nrYXxzMYTQIgvkyuyL/+vEY1Aat+rZfB68+q9rUqsc2LTBXjvmrAWjjo2 PCfw/BbLp0E+U2R+lkRKz+KinU+zxQ== =c3bH -----END PGP SIGNATURE----- --f5QefDQHtn8hx44O--