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

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature