Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver

[Mark Brown <broonie@kernel.org>]

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

On Thu, Nov 05, 2015 at 09:34:47PM +0800, Chen Feng wrote:

> +config REGULATOR_HI6220_MTCMOS
> +        bool "Hisilicon Hi6220 mtcmos support"
> +        depends on ARCH_HISI
> +        help
> +          This driver provides support for the mtcmos regulators of Hi6220 Soc.
> +

The Kconfig and Makefile updates for MCTMOS should have been in the
patch adding the driver for that.

> +config REGULATOR_HI655X
> +        bool "HiSilicon Hi655x PMIC voltage regulator support"
> +        depends on ARCH_HISI

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.

> +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;
> +
> +	regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
> +	return (value & BIT(regulator->ctrl_mask));
> +}

Use the standard regmap helpers, don't open code them.

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

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

> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> +	/* hi655x pmic on hi6220 SoC only support normal mode */
> +	if (mode == REGULATOR_MODE_NORMAL)
> +		return REGULATOR_MODE_NORMAL;
> +	else
> +		return -EINVAL;
> +}

If the device only has one mode it should not have any mode operations,
they're only meaningful if there are multiple modes to set and they are
optional.

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