From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752030AbcF0PKZ (ORCPT ); Mon, 27 Jun 2016 11:10:25 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:47010 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751644AbcF0PKX (ORCPT ); Mon, 27 Jun 2016 11:10:23 -0400 Date: Mon, 27 Jun 2016 16:10:08 +0100 From: Mark Brown To: megous@megous.com Cc: dev@linux-sunxi.org, linux-arm-kernel@lists.infradead.org, Liam Girdwood , open list Message-ID: <20160627151008.GZ28202@sirena.org.uk> References: <20160625034511.7966-1-megous@megous.com> <20160625034511.7966-5-megous@megous.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="aeKQ3o3ktSNpH7mO" Content-Disposition: inline In-Reply-To: <20160625034511.7966-5-megous@megous.com> X-Cookie: Beware of Bigfoot! User-Agent: Mutt/1.6.0 (2016-04-01) X-SA-Exim-Connect-IP: 2a01:348:6:8808:fab::3 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH v2 04/14] regulator: SY8106A regulator driver X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --aeKQ3o3ktSNpH7mO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jun 25, 2016 at 05:45:01AM +0200, megous@megous.com wrote: > + config.init_data =3D of_get_regulator_init_data(dev, dev->of_node, &sy8= 106a_reg); > + if (!config.init_data) { > + return -EINVAL; > + } This is broken, constraints are entirely optional - the driver should just instantiate no matter what (if anything) the constraints are. This means people can read the current state even if there is no need for runtime configuration. > + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV = +=20 > + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK)); This is just noise, remove it - if we want to list the voltage for regulators at probe we should be doing it consistently for all regulators in the core rather than in individual drivers. > +/* > + * This is useless for OF-enabled devices, but it is needed by I2C subsy= stem > + */ > +static const struct i2c_device_id sy8106a_i2c_id[] =3D { > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); No, this is not the case - supporting DT does not mean that other enumeration mechanisms can't be supported and there's no reason not to list a sensible ID here. --aeKQ3o3ktSNpH7mO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXcUHPAAoJECTWi3JdVIfQW84H/1TWkl8DMuSCbRaqGfAa4toC 8lMf7KD+muDrcf5566RkCa7LKsHNr0R1FUUOHCeIg8VxFwQIAZwMBqKnMMc5AVxO WBceqDzIaLaYqDoI8yzD+tp4MVZIEWfHe54T23ticoUvl2a6y0MWP/7wFYH7ZCE+ JdJOcU1KOTT+Vw/BfkV+DjFDxAsT1FEwOOBb/iP/YIjF4Vcb8Qy+QKIG+UYBIvcW Alhye1148Cs/vCQb8P6nmCu6iHsTQo6EAOcJJqVYNHexDywOuO1FcZinUF+yEbsC Lkt99ZwAlaFloMwSQcHDWeI+luf54ASIQH1RWsA0kOWv//myIDskhfWcqx43AUk= =+sck -----END PGP SIGNATURE----- --aeKQ3o3ktSNpH7mO-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: broonie@kernel.org (Mark Brown) Date: Mon, 27 Jun 2016 16:10:08 +0100 Subject: [PATCH v2 04/14] regulator: SY8106A regulator driver In-Reply-To: <20160625034511.7966-5-megous@megous.com> References: <20160625034511.7966-1-megous@megous.com> <20160625034511.7966-5-megous@megous.com> Message-ID: <20160627151008.GZ28202@sirena.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, Jun 25, 2016 at 05:45:01AM +0200, megous at megous.com wrote: > + config.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg); > + if (!config.init_data) { > + return -EINVAL; > + } This is broken, constraints are entirely optional - the driver should just instantiate no matter what (if anything) the constraints are. This means people can read the current state even if there is no need for runtime configuration. > + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + > + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK)); This is just noise, remove it - if we want to list the voltage for regulators at probe we should be doing it consistently for all regulators in the core rather than in individual drivers. > +/* > + * This is useless for OF-enabled devices, but it is needed by I2C subsystem > + */ > +static const struct i2c_device_id sy8106a_i2c_id[] = { > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); No, this is not the case - supporting DT does not mean that other enumeration mechanisms can't be supported and there's no reason not to list a sensible ID here. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: