Date: Wed, 16 Dec 2015 19:16:48 +0000
From: Mark Brown <broonie@kernel.org>
To: Chen Feng <puck.chen@hisilicon.com>
Cc: lee.jones@linaro.org, linux-kernel@vger.kernel.org, lgirdwood@gmail.com,
        robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
        xuwei5@hisilicon.com, puck.chen@aliyun.com, yudongbin@hisilicon.com,
        saberlily.xia@hisilicon.com, suzhuangluan@hisilicon.com,
        kong.kongxinwei@hisilicon.com, xuyiping@hisilicon.com,
        z.liuxinliang@hisilicon.com, weidong2@hisilicon.com, w.f@huawei.com,
        qijiwen@hisilicon.com, peter.panshilin@hisilicon.com,
        dan.zhao@hisilicon.com, linuxarm@huawei.com
Message-ID: <20151216191648.GP5727@sirena.org.uk>
References: <1450184056-79851-1-git-send-email-puck.chen@hisilicon.com>
 <1450184056-79851-5-git-send-email-puck.chen@hisilicon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="epRuUhiNkW/O9uqo"
Content-Disposition: inline
In-Reply-To: <1450184056-79851-5-git-send-email-puck.chen@hisilicon.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Subject: Re: [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2883
Lines: 77


--epRuUhiNkW/O9uqo
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:

> +config REGULATOR_HI655X
> +	tristate "Hisilicon HI655X PMIC regulators support"
> +	depends on ARCH_HISI
> +	depends on MFD_HI655X_PMIC && OF
> +	help
> +	  This driver provides support for the voltage regulators of the
> +	  Hisilicon Hi655x PMIC device.
> +

On the previous version of this patch I said:

| For both of these we should have an || COMPILE_TEST and there's no need
| for either to be bool I can see, they should be tristate.

I see you have made this a tristate which is good but you've not enabled
COMPILE_TEST or indicated why - there may be a very good reason for
doing this but nobody has said what it is.  Please don't ignore review
comments, people are generally making them for a reason and are likely
to have the same concerns if issues remain unaddressed.  Having to
repeat the same comments can get repetitive and make people question the
value of time spent reviewing.  If you disagree with the review comments
that's fine but you need to reply and discuss your concerns so that the
reviewer can understand your decisions.

> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> +	unsigned int value = 0;
> +
> +	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;

The style here is not what we normally do - normally the struct lookups
would be first, then any other variables and there wouldn't be any blank
lines in the variable declarations.  The same applies to quite a few
functions.

> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> +				int min_uV, int max_uV, unsigned *selector)
> +{

As I commented on the previous version of this driver:

| Use the standard helpers, including one of the map_voltage()s and
| set_voltage_sel_regmap(), don't open code them.

You need to at least split the map and set_voltage_sel operations.

I've stopped reviewing here.

--epRuUhiNkW/O9uqo
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWcbifAAoJECTWi3JdVIfQyisH/j/gwhdfKMGUyj/LdPiWKbT8
CYE8Bg1Q/Na1ySZBv/HjTY6k9IvbIzuuVFobH6USz5ZPjU9p77KfLg7y4RSOUUoW
vOBQnmyDCeoEcgWrjSgvUcI7H1yNobDvtnpyCZnYZoBuAzLyC/Ly6ZQy9TMTGaJF
h3+skdYLxrUVPSIBRlKjLd2Pgj5LtphHDq2pu8b2Goyi+chynK0zFdWJnDSc1EdS
FumwyE8Xh3ZZCgof1BZ5dTnsCsS9JYC7NPzOVZTXU2KhzhV86K2WN7KSnYwTgfq6
MFJQbecYLgm3qnsAztVvppElKDbWJf0LpARI1Gzz9qXAQj8H7Ab+EN37qfyF9v4=
=rksG
-----END PGP SIGNATURE-----

--epRuUhiNkW/O9uqo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/