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

From: Mark Brown
Date: Thu Nov 05 2015 - 10:28:22 EST


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: signature.asc
Description: PGP signature