Re: [PATCH v3 4/5] regulator: add regulator driver of hi655x PMIC

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]