linux-kernel - Re: [PATCH v3] regulator: pfuze100: add pfuze100 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: <20130719173047.GQ9858@sirena.org.uk>
Date:	Fri, 19 Jul 2013 18:30:47 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Robin Gong <b38343@...escale.com>
Cc:	grant.likely@...aro.org, rob.herring@...xeda.com, rob@...dley.net,
	lgirdwood@...il.com, shawn.guo@...aro.org,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] regulator: pfuze100: add pfuze100 regulator driver

On Fri, Jul 19, 2013 at 06:14:51PM +0800, Robin Gong wrote:

> Add pfuze100 regulator driver.

Looks good, there's some minor issues below but they should be small.

> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -158,6 +158,13 @@ config REGULATOR_MC13892
>  	  Say y here to support the regulators found on the Freescale MC13892
>  	  PMIC.
>  
> +config REGULATOR_PFUZE100
> +	tristate "Support regulators on Freescale PFUZE100 PMIC"
> +	depends on I2C
> +	help
> +	  Say y here to support the regulators found on the Freescale PFUZE100
> +	  PMIC.
> +

Please keep this and the Makefile sorted - if there's issues merging
then please use my topic/kconfig as a base.

> +		if (val & 0x40)
> +			ramp_delay = 50000 / (4 * ramp_delay);
> +		else
> +			ramp_delay = 25000 / (2 * ramp_delay);
> +		if (id <= PFUZE100_SW1C)
> +			ramp_delay = 25000 / (2 * ramp_delay);

This is a bit confusing as there's a cumalaitve modification applied to
ramp_delay in the case that id is less than SW1C but it's not obvious
that this is intentional given the calculations.  Please change this to
make it clearer what the intended logic is (assuming it's not buggy) -
something like if ... else if ... else for example.

> +	ret = regmap_read(pfuze_chip->regmap, PFUZE100_REVID, &value);
> +	if (ret)
> +		return ret;
> +	dev_info(pfuze_chip->dev,
> +		 "Full lay: %x ,Metal lay: %x\n",
> +		 (value & 0xf0) >> 4, value & 0x0f);

The space should be after not before the comma in the displayed string.

> +	dev_info(pfuze_chip->dev, "FAB: %x , FIN: %x\n",
> +		 (value & 0xc) >> 2, value & 0x3);

Only a space after the , for normal formatting.  ie both should look
like "%x, bar".

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