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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151105145235.GL18409@sirena.org.uk>
Date:	Thu, 5 Nov 2015 14:52:35 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Chen Feng <puck.chen@...ilicon.com>
Cc:	w.f@...wei.com, sameo@...ux.intel.com, lee.jones@...aro.org,
	linux-kernel@...r.kernel.org, lgirdwood@...il.com,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	joro@...tes.org, iommu@...ts.linux-foundation.org,
	haojian.zhuang@...aro.org, devicetree@...r.kernel.org,
	xuwei5@...ilicon.com, xuyiping@...ilicon.com,
	kong.kongxinwei@...ilicon.com, z.liuxinliang@...ilicon.com,
	yudongbin@...ilicon.com, weidong2@...ilicon.com,
	saberlily.xia@...ilicon.com, haojian.zhuang@...look.com,
	leo.yan@...aro.org, linuxarm@...wei.com, dan.zhao@...ilicon.com,
	peter.panshilin@...ilicon.com, qijiwen@...ilicon.com
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 = &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.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)