From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH v3 2/3] regulator: add support for SY8106A regulator Date: Tue, 24 Apr 2018 18:07:33 +0100 Message-ID: <20180424170733.GD22073@sirena.org.uk> References: <20180423144657.63264-1-icenowy@aosc.io> <20180423144657.63264-3-icenowy@aosc.io> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="BRE3mIcgqKzpedwo" Return-path: Content-Disposition: inline In-Reply-To: <20180423144657.63264-3-icenowy@aosc.io> Sender: linux-kernel-owner@vger.kernel.org To: Icenowy Zheng Cc: Liam Girdwood , Rob Herring , Maxime Ripard , Chen-Yu Tsai , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, Ondrej Jirman List-Id: devicetree@vger.kernel.org --BRE3mIcgqKzpedwo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote: > --- /dev/null > +++ b/drivers/regulator/sy8106a-regulator.c > @@ -0,0 +1,176 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * sy8106a-regulator.c - Regulator device driver for SY8106A Just make the entire thing a C++ comment so it looks consistent and joined up. > +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel) > +{ > + /* We use our set_voltage_sel in order to avoid unnecessary I2C > + * chatter, because the regulator_get_voltage_sel_regmap using > + * apply_bit would perform 4 unnecessary transfers instead of one, > + * increasing the chance of error. > + */ > + return regmap_write(rdev->regmap, rdev->desc->vsel_reg, > + sel | SY8106A_GO_BIT); Why would it do these extra transfers? Is this just the fact that you didn't set up a register cache (though the r/m/w cycle should only add the read)? We could put some logic in the core regmap code to detect that an _update_bits() call is going to write to the whole register, though it'd be even easier to just let this register be cached. Generally if we can usefully optimize things we should do it at the framework level. > + if (reg & SY8106A_GO_BIT) > + return reg & rdev->desc->vsel_mask; > + else > + return (chip->fixed_voltage - rdev->desc->min_uV) / > + rdev->desc->uV_step; You could use the standard get_voltage_sel() if you provide a mapping operation that set everything with _GO_BIT set to return the fixed voltage. Though looking at this it seems that the fixed voltage will always be one that could be set via the register anyway so I'm wondering if the easiest thing here isn't to just have the driver turn off _GO_BIT during probe() and not worry about the special case at runtime. --BRE3mIcgqKzpedwo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAlrfZFQACgkQJNaLcl1U h9ABoAf+MAY7wh6RxX+JxRaaZqaKm1sBzEsUizDdO0bmeMGUmy2XVE3nmGRUC5g9 5iEh9QeWock1uMhygZwIWNAudOUbzU2vj519R7fIDMSL/gSMs1pMGBOrQo/EYmpe 2yXh6dfVu1o3GJixqnwLlIEawT91IOqU7CzHq7LYLIn15FECsF9HqEI2+rEZokI4 zU5kqZTGwN6SAXCOTIgc/Rp7ZbwrL4vqyO9+lqNnG3sJKGmiwu/UzD1Ip6vU5Cuf nr9KJUX80xuINjo32OpZoMfw9CmL5Py6MU+GptiozCDnNDdRo+rBv8phvaUX+dly sFkRGlum9ixStWgoHz47moUxcOerxA== =Q4GJ -----END PGP SIGNATURE----- --BRE3mIcgqKzpedwo--