linux-kernel - Re: [PATCH 2/2] Regulators: TPS65917: Add Regulator driver for TPS65917 PMIC

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] [day] [month] [year] [list]

Message-ID: <20140428090345.GD15125@sirena.org.uk>
Date:	Mon, 28 Apr 2014 10:03:45 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Keerthy <j-keerthy@...com>
Cc:	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	sameo@...ux.intel.com, lee.jones@...aro.org,
	grant.likely@...aro.org, robh+dt@...nel.org, lgirdwood@...il.com
Subject: Re: [PATCH 2/2] Regulators: TPS65917: Add Regulator driver for
 TPS65917 PMIC

On Mon, Apr 28, 2014 at 10:58:26AM +0530, Keerthy wrote:

> index 6a79328..5ddb220 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -384,6 +384,18 @@ config REGULATOR_PALMAS
>  	  on the muxing. This is handled automatically in the driver by
>  	  reading the mux info from OTP.
>  
> +config REGULATOR_TPS65917
> +	tristate "TI TPS65917 PMIC Regulators"
> +	depends on MFD_TPS65917

Please keep the Kconfig and Makefile sorted.

> +static const struct regs_info tps65917_regs_info[] = {
> +	{
> +		.name		= "SMPS1",
> +		.sname		= "smps1-in",
> +		.vsel_addr	= TPS65917_SMPS1_VOLTAGE,
> +		.ctrl_addr	= TPS65917_SMPS1_CTRL,
> +		.sleep_id	= TPS65917_EXTERNAL_REQSTR_ID_SMPS1,
> +	},

> +static int tps65917_is_enabled_smps(struct regulator_dev *dev)
> +{
> +	struct tps65917_pmic *pmic = rdev_get_drvdata(dev);
> +	int id = rdev_get_id(dev);
> +	unsigned int reg;
> +
> +	tps65917_smps_read(pmic->tps65917,
> +			   tps65917_regs_info[id].ctrl_addr, &reg);
> +
> +	reg &= TPS65917_SMPS1_CTRL_STATUS_MASK;
> +	reg >>= TPS65917_SMPS1_CTRL_STATUS_SHIFT;
> +
> +	return !!(reg);
> +}

A lot of these functions look like they could be implemented using the
standard helpers rather than open coded - the helpers now support
multi-bit enable fields.

> +	if (selector == 0)
> +		return 0;
> +	else if (selector < 6)
> +		return 500000 * mult;
> +	else
> +		/* Voltage is linear mapping starting from selector 6,
> +		 * volt = (0.49V + ((selector - 5) * 0.01V)) * RANGE
> +		 * RANGE is either x1 or x2
> +		 */
> +		return (490000 + ((selector - 5) * 10000)) * mult;
> +}

This looks like the driver could just use linear ranges.

> +static struct of_device_id of_tps65917_match_tbl[] = {
> +	{ .compatible = "ti,tps65917-pmic", },
> +};

Missing null entry at the end of the table here.

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