Messages in this thread | | | Date | Thu, 5 Nov 2015 14:52:35 +0000 | From | Mark Brown <> | Subject | Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage regulator driver |
| |
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 = ®ulator->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. [unhandled content-type:application/pgp-signature] | |