RE: [PATCH v6 01/10] OMAP3: PM: Adding voltage driver support.

[Anand Sawant <sawant@ti.com>]

Thara,

Here are some comments on the voltage layer; many of them are
cosmetic in nature.

> -----Original Message-----
> From: Thara Gopinath [mailto:thara@ti.com]
> Sent: Monday, December 20, 2010 10:29 PM
> To: linux-omap@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Cc: khilman@deeprootsystems.com; paul@pwsan.com; b-
> cousson@ti.com; vishwanath.bs@ti.com; sawant@ti.com;
> nm@ti.com; Thara Gopinath
> Subject: [PATCH v6 01/10] OMAP3: PM: Adding voltage driver
> support.
>
> This patch adds voltage driver support for OMAP3. The driver
> allows  configuring the voltage controller and voltage
> processors during init and exports APIs to enable/disable
> voltage processors, scale voltage and reset voltage.
> The driver maintains the global voltage table on a per
> VDD basis which contains the various voltages supported by
> the
> VDD along with per voltage dependent data like smartreflex
> efuse offset, errminlimit and voltage processor errorgain.
> The driver also allows the voltage parameters dependent on
> the
> PMIC to be passed from the PMIC file through an API.
> The driver allows scaling of VDD voltages either through
> "vc bypass method" or through "vp forceupdate method" the
> choice being configurable through the board file.
>
> This patch contains code originally in linux omap pm branch
> smartreflex driver.  Major contributors to this driver are
> Lesly A M, Rajendra Nayak, Kalle Jokiniemi, Paul Walmsley,
> Nishant Menon, Kevin Hilman. The separation of PMIC
> parameters
> into a separate structure which can be populated from
> the PMIC file is based on the work of Lun Chang from
> Motorola
> in an internal tree.
>
> Signed-off-by: Thara Gopinath <thara@ti.com>
> ---
> This patch has 14 checkpatch.pl above 80-chars warnings for
> the definitions of omap34xx_vddmpu_volt_data,
> omap36xx_vddmpu_volt_data,
> omap34xx_vddcore_volt_data and omap36xx_vddcore_volt_data
> structures.
> IMHO splitting of the entries in these structures affects
> readability and looks very ugly. Hence they are left as is.
>
>  arch/arm/mach-omap2/Makefile              |    3 +-
>  arch/arm/mach-omap2/control.h             |   17 +
>  arch/arm/mach-omap2/pm.c                  |    8 +
>  arch/arm/mach-omap2/voltage.c             | 1226
> +++++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/voltage.h |  134 ++++
>  5 files changed, 1387 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/voltage.c
>  create mode 100644 arch/arm/plat-
> omap/include/plat/voltage.h
>
> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-
> omap2/Makefile
> index 7c79683..5034797 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -57,7 +57,8 @@ endif
>  ifeq ($(CONFIG_PM),y)
>  obj-$(CONFIG_ARCH_OMAP2)		+= pm24xx.o
>  obj-$(CONFIG_ARCH_OMAP2)		+= sleep24xx.o pm_bus.o
> -obj-$(CONFIG_ARCH_OMAP3)		+= pm34xx.o sleep34xx.o
> cpuidle34xx.o pm_bus.o
> +obj-$(CONFIG_ARCH_OMAP3)		+= pm34xx.o sleep34xx.o
> voltage.o \
> +					   cpuidle34xx.o pm_bus.o
>  obj-$(CONFIG_ARCH_OMAP4)		+= pm44xx.o pm_bus.o
>  obj-$(CONFIG_PM_DEBUG)			+= pm-debug.o
>
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-
> omap2/control.h
> index 5289461..9fe32dc 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -148,6 +148,15 @@
>  #define OMAP343X_CONTROL_TEST_KEY_11
> 	(OMAP2_CONTROL_GENERAL + 0x00f4)
>  #define OMAP343X_CONTROL_TEST_KEY_12
> 	(OMAP2_CONTROL_GENERAL + 0x00f8)
>  #define OMAP343X_CONTROL_TEST_KEY_13
> 	(OMAP2_CONTROL_GENERAL + 0x00fc)
> +#define OMAP343X_CONTROL_FUSE_OPP1_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0110)
> +#define OMAP343X_CONTROL_FUSE_OPP2_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0114)
> +#define OMAP343X_CONTROL_FUSE_OPP3_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0118)
> +#define OMAP343X_CONTROL_FUSE_OPP4_VDD1
> (OMAP2_CONTROL_GENERAL + 0x011c)
> +#define OMAP343X_CONTROL_FUSE_OPP5_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0120)
> +#define OMAP343X_CONTROL_FUSE_OPP1_VDD2
> (OMAP2_CONTROL_GENERAL + 0x0124)
> +#define OMAP343X_CONTROL_FUSE_OPP2_VDD2
> (OMAP2_CONTROL_GENERAL + 0x0128)
> +#define OMAP343X_CONTROL_FUSE_OPP3_VDD2
> (OMAP2_CONTROL_GENERAL + 0x012c)
> +#define OMAP343X_CONTROL_FUSE_SR
> (OMAP2_CONTROL_GENERAL + 0x0130)
>  #define OMAP343X_CONTROL_IVA2_BOOTADDR
> 	(OMAP2_CONTROL_GENERAL + 0x0190)
>  #define OMAP343X_CONTROL_IVA2_BOOTMOD
> 	(OMAP2_CONTROL_GENERAL + 0x0194)
>  #define OMAP343X_CONTROL_DEBOBS(i)	(OMAP2_CONTROL_GENERAL +
> 0x01B0 \
> @@ -164,6 +173,14 @@
>  #define OMAP343X_CONTROL_SRAMLDO5	(OMAP2_CONTROL_GENERAL +
> 0x02C0)
>  #define OMAP343X_CONTROL_CSI		(OMAP2_CONTROL_GENERAL +
> 0x02C4)
>
> +/* OMAP3630 only CONTROL_GENERAL register offsets */
> +#define OMAP3630_CONTROL_FUSE_OPP1G_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0110)
> +#define OMAP3630_CONTROL_FUSE_OPP50_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0114)
> +#define OMAP3630_CONTROL_FUSE_OPP100_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0118)
> +#define OMAP3630_CONTROL_FUSE_OPP120_VDD1
> (OMAP2_CONTROL_GENERAL + 0x0120)
> +#define OMAP3630_CONTROL_FUSE_OPP50_VDD2
> (OMAP2_CONTROL_GENERAL + 0x0128)
> +#define OMAP3630_CONTROL_FUSE_OPP100_VDD2
> (OMAP2_CONTROL_GENERAL + 0x012C)
> +
>  /* AM35XX only CONTROL_GENERAL register offsets */
>  #define AM35XX_CONTROL_MSUSPENDMUX_6
> (OMAP2_CONTROL_GENERAL + 0x0038)
>  #define AM35XX_CONTROL_DEVCONF2
> (OMAP2_CONTROL_GENERAL + 0x0310)
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-
> omap2/pm.c
> index 227a211..22adfb2 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -17,6 +17,7 @@
>  #include <plat/omap-pm.h>
>  #include <plat/omap_device.h>
>  #include <plat/common.h>
> +#include <plat/voltage.h>
>
>  #include "powerdomain.h"
>  #include "clockdomain.h"
> @@ -145,3 +146,10 @@ static int __init
> omap2_common_pm_init(void)
>  }
>  postcore_initcall(omap2_common_pm_init);
>
> +static int __init omap2_common_pm_late_init(void)
> +{
> +	omap_voltage_late_init();
> +
> +	return 0;
> +}
> +late_initcall(omap2_common_pm_late_init);
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-
> omap2/voltage.c
> new file mode 100644
> index 0000000..875667f
> --- /dev/null
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -0,0 +1,1226 @@
> +/*
> + * OMAP3/OMAP4 Voltage Management Routines
> + *
> + * Author: Thara Gopinath	<thara@ti.com>
> + *
> + * Copyright (C) 2007 Texas Instruments, Inc.
> + * Rajendra Nayak <rnayak@ti.com>
> + * Lesly A M <x0080970@ti.com>
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Kalle Jokiniemi
> + *
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + * Thara Gopinath <thara@ti.com>
> + *
> + * This program is free software; you can redistribute it
> and/or modify
> + * it under the terms of the GNU General Public License
> version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +#include <linux/slab.h>
> +
> +#include <plat/common.h>
> +#include <plat/voltage.h>
> +
> +#include "prm-regbits-34xx.h"
> +#include "control.h"
> +
> +#define VP_IDLE_TIMEOUT		200
> +#define VP_TRANXDONE_TIMEOUT	300
> +#define VOLTAGE_DIR_SIZE	16

Add comments for the above #defines.

> +
> +/* Voltage processor register offsets */

Add comments for the data members.

> +struct vp_reg_offs {
> +	u8 vpconfig;
> +	u8 vstepmin;
> +	u8 vstepmax;
> +	u8 vlimitto;
> +	u8 vstatus;
> +	u8 voltage;
> +};
> +
> +/* Voltage Processor bit field values, shifts and masks */

Add comments for the data members.

> +struct vp_reg_val {
> +	/* PRM module */
> +	u16 prm_mod;
> +	/* VPx_VPCONFIG */
> +	u32 vpconfig_erroroffset;
> +	u16 vpconfig_errorgain;
> +	u32 vpconfig_errorgain_mask;
> +	u8 vpconfig_errorgain_shift;
> +	u32 vpconfig_initvoltage_mask;
> +	u8 vpconfig_initvoltage_shift;
> +	u32 vpconfig_timeouten;
> +	u32 vpconfig_initvdd;
> +	u32 vpconfig_forceupdate;
> +	u32 vpconfig_vpenable;
> +	/* VPx_VSTEPMIN */
> +	u8 vstepmin_stepmin;
> +	u16 vstepmin_smpswaittimemin;
> +	u8 vstepmin_stepmin_shift;
> +	u8 vstepmin_smpswaittimemin_shift;
> +	/* VPx_VSTEPMAX */
> +	u8 vstepmax_stepmax;
> +	u16 vstepmax_smpswaittimemax;
> +	u8 vstepmax_stepmax_shift;
> +	u8 vstepmax_smpswaittimemax_shift;
> +	/* VPx_VLIMITTO */
> +	u8 vlimitto_vddmin;
> +	u8 vlimitto_vddmax;
> +	u16 vlimitto_timeout;
> +	u8 vlimitto_vddmin_shift;
> +	u8 vlimitto_vddmax_shift;
> +	u8 vlimitto_timeout_shift;
> +	/* PRM_IRQSTATUS*/
> +	u32 tranxdone_status;
> +};
> +
> +/* Voltage controller registers and offsets */

Add comments for the data members.

> +struct vc_reg_info {
> +	/* PRM module */
> +	u16 prm_mod;
> +	/* VC register offsets */
> +	u8 smps_sa_reg;
> +	u8 smps_volra_reg;
> +	u8 bypass_val_reg;
> +	u8 cmdval_reg;
> +	u8 voltsetup_reg;
> +	/*VC_SMPS_SA*/
> +	u8 smps_sa_shift;
> +	u32 smps_sa_mask;
> +	/* VC_SMPS_VOL_RA */
> +	u8 smps_volra_shift;
> +	u32 smps_volra_mask;
> +	/* VC_BYPASS_VAL */
> +	u8 data_shift;
> +	u8 slaveaddr_shift;
> +	u8 regaddr_shift;
> +	u32 valid;
> +	/* VC_CMD_VAL */
> +	u8 cmd_on_shift;
> +	u8 cmd_onlp_shift;
> +	u8 cmd_ret_shift;
> +	u8 cmd_off_shift;
> +	u32 cmd_on_mask;
> +	/* PRM_VOLTSETUP */
> +	u8 voltsetup_shift;
> +	u32 voltsetup_mask;
> +};
> +
> +/**
> + * omap_vdd_info - Per Voltage Domain info
> + *
> + * @volt_data		: voltage table having the distinct
> voltages supported
> + *			  by the domain and other associated per
> voltage data.
> + * @pmic_info		: pmic specific parameters which
> should be populted by
> + *			  the pmic drivers.
> + * @vp_offs		: structure containing the offsets
> for various
> + *			  vp registers
> + * @vp_reg		: the register values, shifts, masks for
> various
> + *			  vp registers
> + * @vc_reg		: structure containing various various vc
> registers,
> + *			  shifts, masks etc.
> + * @voltdm		: pointer to the voltage domain structure
> + * @debug_dir		: debug directory for this voltage
> domain.
> + * @curr_volt		: current voltage for this vdd.
> + * @ocp_mod		: The prm module for accessing the
> prm irqstatus reg.
> + * @prm_irqst_reg	: prm irqstatus register.
> + * @vp_enabled		: flag to keep track of whether vp
> is enabled or not

Add comments for read_reg & write_reg.

> + * @volt_scale		: API to scale the voltage of the
> vdd.

Add more explanation about the structure - like its
significance, how it is populated and used etc.

> + */
> +struct omap_vdd_info {
> +	struct omap_volt_data *volt_data;
> +	struct omap_volt_pmic_info *pmic_info;
> +	struct vp_reg_offs vp_offs;
> +	struct vp_reg_val vp_reg;
> +	struct vc_reg_info vc_reg;
> +	struct voltagedomain voltdm;

Make voltdm the first member of the structure as it's a key identifier.

> +	struct dentry *debug_dir;
> +	u32 curr_volt;
> +	u16 ocp_mod;
> +	u8 prm_irqst_reg;
> +	bool vp_enabled;
> +	u32 (*read_reg) (u16 mod, u8 offset);
> +	void (*write_reg) (u32 val, u16 mod, u8 offset);
> +	int (*volt_scale) (struct omap_vdd_info *vdd,
> +		unsigned long target_volt);
> +};
> +
> +static struct omap_vdd_info *vdd_info;

Add comment. It's an important pointer.

> +/*
> + * Number of scalable voltage domains.
> + */
> +static int nr_scalable_vdd;
> +
> +/* OMAP3 VDD sturctures */

Add comment to mention that partial initialization is
done here & the rest is done in omap3_vdd_data_configure()
function.

> +static struct omap_vdd_info omap3_vdd_info[] = {
> +	{
> +		.vp_offs = {
> +			.vpconfig = OMAP3_PRM_VP1_CONFIG_OFFSET,
> +			.vstepmin = OMAP3_PRM_VP1_VSTEPMIN_OFFSET,
> +			.vstepmax = OMAP3_PRM_VP1_VSTEPMAX_OFFSET,
> +			.vlimitto = OMAP3_PRM_VP1_VLIMITTO_OFFSET,
> +			.vstatus = OMAP3_PRM_VP1_STATUS_OFFSET,
> +			.voltage = OMAP3_PRM_VP1_VOLTAGE_OFFSET,
> +		},

Recommend to use DEFINE_VP_REG_OFFS macro similar to
VOLT_DATA_DEFINE to make it more readable.

> +		.voltdm = {
> +			.name = "mpu",
> +		},

Make this initialization the first initialization.

> +	},
> +	{
> +		.vp_offs = {
> +			.vpconfig = OMAP3_PRM_VP2_CONFIG_OFFSET,
> +			.vstepmin = OMAP3_PRM_VP2_VSTEPMIN_OFFSET,
> +			.vstepmax = OMAP3_PRM_VP2_VSTEPMAX_OFFSET,
> +			.vlimitto = OMAP3_PRM_VP2_VLIMITTO_OFFSET,
> +			.vstatus = OMAP3_PRM_VP2_STATUS_OFFSET,
> +			.voltage = OMAP3_PRM_VP2_VOLTAGE_OFFSET,
> +		},
> +		.voltdm = {
> +			.name = "core",
> +		},
> +	},
> +};
> +
> +#define OMAP3_NR_SCALABLE_VDD ARRAY_SIZE(omap3_vdd_info)
> +
> +/*
> + * Structures containing OMAP3430/OMAP3630 voltage
> supported and various
> + * voltage dependent data for each VDD.
> + */
> +#define VOLT_DATA_DEFINE(_v_nom, _efuse_offs, _errminlimit,
> _errgain)	\
> +{									\
> +	.volt_nominal	= _v_nom,
> 	\
> +	.sr_efuse_offs	= _efuse_offs,
> 	\
> +	.sr_errminlimit	= _errminlimit,
> 	\
> +	.vp_errgain	= _errgain					\
> +}
> +
> +/* VDD1 */

Add a comment stressing the need to keep
VOLT_DATA_DEFINE(0, 0, 0, 0) as always the last member.
Otherwise, the loop in omap_voltage_get_voltdata() will
not function properly.

> +static struct omap_volt_data omap34xx_vddmpu_volt_data[] =
> {
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP1_UV,
> OMAP343X_CONTROL_FUSE_OPP1_VDD1, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP2_UV,
> OMAP343X_CONTROL_FUSE_OPP2_VDD1, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP3_UV,
> OMAP343X_CONTROL_FUSE_OPP3_VDD1, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP4_UV,
> OMAP343X_CONTROL_FUSE_OPP4_VDD1, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_MPU_OPP5_UV,
> OMAP343X_CONTROL_FUSE_OPP5_VDD1, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(0, 0, 0, 0),
> +};
> +
> +static struct omap_volt_data omap36xx_vddmpu_volt_data[] =
> {
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP50_UV,
> OMAP3630_CONTROL_FUSE_OPP50_VDD1, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP100_UV,
> OMAP3630_CONTROL_FUSE_OPP100_VDD1, 0xf9, 0x16),
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP120_UV,
> OMAP3630_CONTROL_FUSE_OPP120_VDD1, 0xfa, 0x23),
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP1G_UV,
> OMAP3630_CONTROL_FUSE_OPP1G_VDD1, 0xfa, 0x27),
> +	VOLT_DATA_DEFINE(0, 0, 0, 0),
> +};
> +
> +/* VDD2 */
> +static struct omap_volt_data omap34xx_vddcore_volt_data[] =
> {
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_CORE_OPP1_UV,
> OMAP343X_CONTROL_FUSE_OPP1_VDD2, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_CORE_OPP2_UV,
> OMAP343X_CONTROL_FUSE_OPP2_VDD2, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3430_VDD_CORE_OPP3_UV,
> OMAP343X_CONTROL_FUSE_OPP3_VDD2, 0xf9, 0x18),
> +	VOLT_DATA_DEFINE(0, 0, 0, 0),
> +};
> +
> +static struct omap_volt_data omap36xx_vddcore_volt_data[] =
> {
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_CORE_OPP50_UV,
> OMAP3630_CONTROL_FUSE_OPP50_VDD2, 0xf4, 0x0c),
> +	VOLT_DATA_DEFINE(OMAP3630_VDD_CORE_OPP100_UV,
> OMAP3630_CONTROL_FUSE_OPP100_VDD2, 0xf9, 0x16),
> +	VOLT_DATA_DEFINE(0, 0, 0, 0),
> +};
> +
> +static struct dentry *voltage_dir;

Add comment.

> +
> +/* Init function pointers */

Add comment mentioning how they are populated.

> +static void (*vc_init) (struct omap_vdd_info *vdd);
> +static int (*vdd_data_configure) (struct omap_vdd_info
> *vdd);

Recommend to define a similar init function pointer for
vp initialization. At present, it will be initialized with
the generic vp_init(). But it will allow easy migration
in case vp initialization is different for future OMAPs.

Also, highlight the difference between init (actually writes
to module registers) and data_configure (populates data
structures; no writing to registers) for clarity.

> +
> +static u32 omap3_voltage_read_reg(u16 mod, u8 offset)
> +{
> +	return omap2_prm_read_mod_reg(mod, offset);
> +}
> +
> +static void omap3_voltage_write_reg(u32 val, u16 mod, u8
> offset)
> +{
> +	omap2_prm_write_mod_reg(val, mod, offset);
> +}
> +

Add comment for vp_latch_vsel.

> +static void vp_latch_vsel(struct omap_vdd_info *vdd)
> +{
> +	u32 vpconfig;
> +	u16 mod;
> +	unsigned long uvdc;
> +	char vsel;
> +
> +	uvdc = omap_voltage_get_nom_volt(&vdd->voltdm);
> +	if (!uvdc) {
> +		pr_warning("%s: unable to find current voltage
> for vdd_%s\n",
> +			__func__, vdd->voltdm.name);
> +		return;
> +	}
> +
> +	if (!vdd->pmic_info || !vdd->pmic_info->uv_to_vsel) {
> +		pr_warning("%s: PMIC function to convert voltage
> in uV to"
> +			" vsel not registered\n", __func__);
> +		return;
> +	}
> +
> +	mod = vdd->vp_reg.prm_mod;
> +
> +	vsel = vdd->pmic_info->uv_to_vsel(uvdc);
> +
> +	vpconfig = vdd->read_reg(mod, vdd->vp_offs.vpconfig);
> +	vpconfig &= ~(vdd->vp_reg.vpconfig_initvoltage_mask |
> +			vdd->vp_reg.vpconfig_initvdd);
> +	vpconfig |= vsel << vdd-
> >vp_reg.vpconfig_initvoltage_shift;
> +
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	/* Trigger initVDD value copy to voltage processor */
> +	vdd->write_reg((vpconfig | vdd-
> >vp_reg.vpconfig_initvdd), mod,
> +			vdd->vp_offs.vpconfig);
> +

Check if any delay is needed between triggering initVDD copy
& clearing initVDD copy. Check if the two need to be atomic.

> +	/* Clear initVDD copy trigger bit */
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +}
> +
> +/* Generic voltage init functions */
> +static void __init vp_init(struct omap_vdd_info *vdd)

Recommend to change the return type to int to allow for
better error handling & diagnostic messages.

> +{
> +	u32 vp_val;
> +	u16 mod;
> +
> +	if (!vdd->read_reg || !vdd->write_reg) {
> +		pr_err("%s: No read/write API for accessing
> vdd_%s regs\n",
> +			__func__, vdd->voltdm.name);
> +		return;
> +	}
> +
> +	mod = vdd->vp_reg.prm_mod;
> +
> +	vp_val = vdd->vp_reg.vpconfig_erroroffset |
> +		(vdd->vp_reg.vpconfig_errorgain <<
> +		vdd->vp_reg.vpconfig_errorgain_shift) |
> +		vdd->vp_reg.vpconfig_timeouten;
> +	vdd->write_reg(vp_val, mod, vdd->vp_offs.vpconfig);
> +
> +	vp_val = ((vdd->vp_reg.vstepmin_smpswaittimemin <<
> +		vdd->vp_reg.vstepmin_smpswaittimemin_shift) |
> +		(vdd->vp_reg.vstepmin_stepmin <<
> +		vdd->vp_reg.vstepmin_stepmin_shift));
> +	vdd->write_reg(vp_val, mod, vdd->vp_offs.vstepmin);
> +
> +	vp_val = ((vdd->vp_reg.vstepmax_smpswaittimemax <<
> +		vdd->vp_reg.vstepmax_smpswaittimemax_shift) |
> +		(vdd->vp_reg.vstepmax_stepmax <<
> +		vdd->vp_reg.vstepmax_stepmax_shift));
> +	vdd->write_reg(vp_val, mod, vdd->vp_offs.vstepmax);
> +
> +	vp_val = ((vdd->vp_reg.vlimitto_vddmax <<
> +		vdd->vp_reg.vlimitto_vddmax_shift) |
> +		(vdd->vp_reg.vlimitto_vddmin <<
> +		vdd->vp_reg.vlimitto_vddmin_shift) |
> +		(vdd->vp_reg.vlimitto_timeout <<
> +		vdd->vp_reg.vlimitto_timeout_shift));
> +	vdd->write_reg(vp_val, mod, vdd->vp_offs.vlimitto);
> +}
> +

Add comment for vdd_debugfs_init.

> +static void __init vdd_debugfs_init(struct omap_vdd_info
> *vdd)
> +{
> +	char *name;
> +
> +	name = kzalloc(VOLTAGE_DIR_SIZE, GFP_KERNEL);
> +	if (!name) {
> +		pr_warning("%s: Unable to allocate memory for
> debugfs"
> +			" directory name for vdd_%s",
> +			__func__, vdd->voltdm.name);
> +		return;
> +	}
> +	strcpy(name, "vdd_");
> +	strcat(name, vdd->voltdm.name);
> +
> +	vdd->debug_dir = debugfs_create_dir(name,
> voltage_dir);
> +	if (IS_ERR(vdd->debug_dir)) {
> +		pr_warning("%s: Unable to create debugfs
> directory for"
> +			" vdd_%s\n", __func__, vdd->voltdm.name);
> +		vdd->debug_dir = NULL;
> +	}
> +}
> +
> +/* Voltage scale and accessory APIs */

Add comment for _pre_volt_scale.

> +static int _pre_volt_scale(struct omap_vdd_info *vdd,
> +		unsigned long target_volt, u8 *target_vsel, u8
> *current_vsel)
> +{
> +	struct omap_volt_data *volt_data;
> +	u32 vc_cmdval, vp_errgain_val;
> +	u16 vp_mod, vc_mod;
> +
> +	/* Check if suffiecient pmic info is available for
> this vdd */
> +	if (!vdd->pmic_info) {
> +		pr_err("%s: Insufficient pmic info to scale the
> vdd_%s\n",
> +			__func__, vdd->voltdm.name);
> +		return -EINVAL;
> +	}
> +
> +	if (!vdd->pmic_info->uv_to_vsel) {
> +		pr_err("%s: PMIC function to convert voltage in
> uV to"
> +			"vsel not registered. Hence unable to
> scale voltage"
> +			"for vdd_%s\n", __func__, vdd-
> >voltdm.name);
> +		return -ENODATA;
> +	}
> +
> +	if (!vdd->read_reg || !vdd->write_reg) {
> +		pr_err("%s: No read/write API for accessing
> vdd_%s regs\n",
> +			__func__, vdd->voltdm.name);
> +		return -EINVAL;
> +	}
> +
> +	vp_mod = vdd->vp_reg.prm_mod;
> +	vc_mod = vdd->vc_reg.prm_mod;
> +
> +	/* Get volt_data corresponding to target_volt */
> +	volt_data = omap_voltage_get_voltdata(&vdd->voltdm,
> target_volt);
> +	if (IS_ERR(volt_data))
> +		volt_data = NULL;

If there is an error on volt_data; return an error code.
No need to continue with the rest of the processing.

> +
> +	*target_vsel = vdd->pmic_info-
> >uv_to_vsel(target_volt);
> +	*current_vsel = vdd->read_reg(vp_mod, vdd-
> >vp_offs.voltage);
> +
> +	/* Setting the ON voltage to the new target voltage */
> +	vc_cmdval = vdd->read_reg(vc_mod, vdd-
> >vc_reg.cmdval_reg);
> +	vc_cmdval &= ~vdd->vc_reg.cmd_on_mask;
> +	vc_cmdval |= (*target_vsel << vdd-
> >vc_reg.cmd_on_shift);
> +	vdd->write_reg(vc_cmdval, vc_mod, vdd-
> >vc_reg.cmdval_reg);
> +
> +	/* Setting vp errorgain based on the voltage */
> +	if (volt_data) {
> +		vp_errgain_val = vdd->read_reg(vp_mod,
> +				vdd->vp_offs.vpconfig);
> +		vdd->vp_reg.vpconfig_errorgain = volt_data-
> >vp_errgain;
> +		vp_errgain_val &= ~vdd-
> >vp_reg.vpconfig_errorgain_mask;
> +		vp_errgain_val |= vdd->vp_reg.vpconfig_errorgain
> <<
> +				vdd-
> >vp_reg.vpconfig_errorgain_shift;
> +		vdd->write_reg(vp_errgain_val, vp_mod,
> +				vdd->vp_offs.vpconfig);
> +	}

Recommend to take a snapshot of current vsel,
cmdval_reg & errgain before setting the new values. Think of
a mechanism to restore them in case the scaling function
(called after _pre_volt_scale()) fails midway.

Check related comments in vp_forceupdate_scale_voltage()
& vc_bypass_scale_voltage().

> +
> +	return 0;
> +}
> +

Add comment for _post_volt_scale.

> +static void _post_volt_scale(struct omap_vdd_info *vdd,
> +		unsigned long target_volt, u8 target_vsel, u8
> current_vsel)
> +{
> +	u32 smps_steps = 0, smps_delay = 0;
> +
> +	smps_steps = abs(target_vsel - current_vsel);
> +	/* SMPS slew rate / step size. 2us added as buffer. */
> +	smps_delay = ((smps_steps * vdd->pmic_info->step_size)
> /
> +			vdd->pmic_info->slew_rate) + 2;
> +	udelay(smps_delay);
> +
> +	vdd->curr_volt = target_volt;
> +}
> +
> +/* vc_bypass_scale_voltage - VC bypass method of voltage
> scaling */
> +static int vc_bypass_scale_voltage(struct omap_vdd_info
> *vdd,
> +		unsigned long target_volt)
> +{
> +	u32 loop_cnt = 0, retries_cnt = 0;
> +	u32 vc_valid, vc_bypass_val_reg, vc_bypass_value;
> +	u16 mod;
> +	u8 target_vsel, current_vsel;
> +	int ret;
> +
> +	ret = _pre_volt_scale(vdd, target_volt, &target_vsel,
> &current_vsel);
> +	if (ret)
> +		return ret;

This check is ineffective to handle error in case volt_data
corresponding to the target voltage is not found as it
returns 0.

> +
> +	mod = vdd->vc_reg.prm_mod;
> +
> +	vc_valid = vdd->vc_reg.valid;
> +	vc_bypass_val_reg = vdd->vc_reg.bypass_val_reg;
> +	vc_bypass_value = (target_vsel << vdd-
> >vc_reg.data_shift) |
> +			(vdd->pmic_info->pmic_reg <<
> +			vdd->vc_reg.regaddr_shift) |
> +			(vdd->pmic_info->i2c_slave_addr <<
> +			vdd->vc_reg.slaveaddr_shift);
> +
> +	vdd->write_reg(vc_bypass_value, mod,
> vc_bypass_val_reg);
> +	vdd->write_reg(vc_bypass_value | vc_valid, mod,
> vc_bypass_val_reg);
> +
> +	vc_bypass_value = vdd->read_reg(mod,
> vc_bypass_val_reg);
> +	/*
> +	 * Loop till the bypass command is acknowledged from
> the SMPS.
> +	 * NOTE: This is legacy code. The loop count and retry
> count needs
> +	 * to be revisited.
> +	 */
> +	while (!(vc_bypass_value & vc_valid)) {
> +		loop_cnt++;
> +
> +		if (retries_cnt > 10) {
> +			pr_warning("%s: Retry count exceeded\n",
> __func__);

Recommend to restore the snapshot of vsel, cmdval_reg &
errgain in _pre_volt_scale before returning an error.

This can be bit complicated as HW's exact state is not
known.

> +			return -ETIMEDOUT;
> +		}
> +
> +		if (loop_cnt > 50) {
> +			retries_cnt++;
> +			loop_cnt = 0;
> +			udelay(10);
> +		}
> +		vc_bypass_value = vdd->read_reg(mod,
> vc_bypass_val_reg);
> +	}
> +
> +	_post_volt_scale(vdd, target_volt, target_vsel,
> current_vsel);
> +	return 0;
> +}
> +
> +/* VP force update method of voltage scaling */
> +static int vp_forceupdate_scale_voltage(struct
> omap_vdd_info *vdd,
> +		unsigned long target_volt)
> +{
> +	u32 vpconfig;
> +	u16 mod, ocp_mod;
> +	u8 target_vsel, current_vsel, prm_irqst_reg;
> +	int ret, timeout = 0;
> +
> +	ret = _pre_volt_scale(vdd, target_volt, &target_vsel,
> &current_vsel);
> +	if (ret)
> +		return ret;

This check is ineffective to handle error in case volt_data
corresponding to the target voltage is not found as it
returns 0.

> +
> +	mod = vdd->vp_reg.prm_mod;
> +	ocp_mod = vdd->ocp_mod;
> +	prm_irqst_reg = vdd->prm_irqst_reg;
> +
> +	/*
> +	 * Clear all pending TransactionDone interrupt/status.
> Typical latency
> +	 * is <3us
> +	 */
> +	while (timeout++ < VP_TRANXDONE_TIMEOUT) {
> +		vdd->write_reg(vdd->vp_reg.tranxdone_status,
> +				ocp_mod, prm_irqst_reg);
> +		if (!(vdd->read_reg(ocp_mod, prm_irqst_reg) &
> +				vdd->vp_reg.tranxdone_status))
> +				break;
> +		udelay(1);
> +	}
> +	if (timeout >= VP_TRANXDONE_TIMEOUT) {
> +		pr_warning("%s: vdd_%s TRANXDONE timeout
> exceeded."
> +			"Voltage change aborted", __func__, vdd-
> >voltdm.name);

Recommend to restore the snapshot of vsel, cmdval_reg &
errgain in _pre_volt_scale before returning an error.

This can be bit complicated as HW's exact state is not
known.

> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Configure for VP-Force Update */
> +	vpconfig = vdd->read_reg(mod, vdd->vp_offs.vpconfig);
> +	vpconfig &= ~(vdd->vp_reg.vpconfig_initvdd |
> +			vdd->vp_reg.vpconfig_forceupdate |
> +			vdd->vp_reg.vpconfig_initvoltage_mask);
> +	vpconfig |= ((target_vsel <<
> +			vdd->vp_reg.vpconfig_initvoltage_shift));
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	/* Trigger initVDD value copy to voltage processor */
> +	vpconfig |= vdd->vp_reg.vpconfig_initvdd;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	/* Force update of voltage */
> +	vpconfig |= vdd->vp_reg.vpconfig_forceupdate;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	/*
> +	 * Wait for TransactionDone. Typical latency is
> <200us.
> +	 * Depends on SMPSWAITTIMEMIN/MAX and voltage change
> +	 */
> +	timeout = 0;
> +	omap_test_timeout((vdd->read_reg(ocp_mod,
> prm_irqst_reg) &
> +			vdd->vp_reg.tranxdone_status),
> +			VP_TRANXDONE_TIMEOUT, timeout);
> +	if (timeout >= VP_TRANXDONE_TIMEOUT)
> +		pr_err("%s: vdd_%s TRANXDONE timeout exceeded."
> +			"TRANXDONE never got set after the voltage
> update\n",
> +			__func__, vdd->voltdm.name);
> +
> +	_post_volt_scale(vdd, target_volt, target_vsel,
> current_vsel);

It is risky to assume voltage change has been successful in
case TRANXDONE never got set after voltage update. Recommend
to do a read of SMPS voltage for confirmation before
proceeding further.

> +
> +	/*
> +	 * Disable TransactionDone interrupt , clear all
> status, clear
> +	 * control registers
> +	 */

Check whether it is safe to disable TRANXDONE interrupt even
if it didn't get set.

> +	timeout = 0;
> +	while (timeout++ < VP_TRANXDONE_TIMEOUT) {
> +		vdd->write_reg(vdd->vp_reg.tranxdone_status,
> +				ocp_mod, prm_irqst_reg);
> +		if (!(vdd->read_reg(ocp_mod, prm_irqst_reg) &
> +				vdd->vp_reg.tranxdone_status))
> +				break;
> +		udelay(1);
> +	}
> +
> +	if (timeout >= VP_TRANXDONE_TIMEOUT)
> +		pr_warning("%s: vdd_%s TRANXDONE timeout
> exceeded while trying"
> +			"to clear the TRANXDONE status\n",
> +			__func__, vdd->voltdm.name);
> +
> +	vpconfig = vdd->read_reg(mod, vdd->vp_offs.vpconfig);
> +	/* Clear initVDD copy trigger bit */
> +	vpconfig &= ~vdd->vp_reg.vpconfig_initvdd;;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +	/* Clear force bit */
> +	vpconfig &= ~vdd->vp_reg.vpconfig_forceupdate;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	return 0;
> +}
> +
> +/* OMAP3 specific voltage init functions */
> +
> +/*
> + * Intializes the voltage controller registers with the
> PMIC and board
> + * specific parameters and voltage setup times for OMAP3.
> + */
> +static void __init omap3_vc_init(struct omap_vdd_info *vdd)
> +{
> +	u32 vc_val;
> +	u16 mod;
> +	u8 on_vsel, onlp_vsel, ret_vsel, off_vsel;
> +	static bool is_initialized;
> +
> +	if (!vdd->pmic_info || !vdd->pmic_info->uv_to_vsel) {
> +		pr_err("%s: PMIC info requried to configure vc
> for"
> +			"vdd_%s not populated.Hence cannot
> initialize vc\n",
> +			__func__, vdd->voltdm.name);
> +		return;
> +	}
> +
> +	if (!vdd->read_reg || !vdd->write_reg) {
> +		pr_err("%s: No read/write API for accessing
> vdd_%s regs\n",
> +			__func__, vdd->voltdm.name);
> +		return;
> +	}
> +
> +	mod = vdd->vc_reg.prm_mod;
> +
> +	/* Set up the SMPS_SA(i2c slave address in VC */
> +	vc_val = vdd->read_reg(mod, vdd->vc_reg.smps_sa_reg);
> +	vc_val &= ~vdd->vc_reg.smps_sa_mask;
> +	vc_val |= vdd->pmic_info->i2c_slave_addr << vdd-
> >vc_reg.smps_sa_shift;
> +	vdd->write_reg(vc_val, mod, vdd->vc_reg.smps_sa_reg);
> +
> +	/* Setup the VOLRA(pmic reg addr) in VC */
> +	vc_val = vdd->read_reg(mod, vdd-
> >vc_reg.smps_volra_reg);
> +	vc_val &= ~vdd->vc_reg.smps_volra_mask;
> +	vc_val |= vdd->pmic_info->pmic_reg << vdd-
> >vc_reg.smps_volra_shift;
> +	vdd->write_reg(vc_val, mod, vdd-
> >vc_reg.smps_volra_reg);
> +
> +	/*Configure the setup times */
> +	vc_val = vdd->read_reg(mod, vdd-
> >vc_reg.voltsetup_reg);
> +	vc_val &= ~vdd->vc_reg.voltsetup_mask;
> +	vc_val |= vdd->pmic_info->volt_setup_time <<
> +			vdd->vc_reg.voltsetup_shift;
> +	vdd->write_reg(vc_val, mod, vdd-
> >vc_reg.voltsetup_reg);
> +
> +	/* Set up the on, inactive, retention and off voltage
> */
> +	on_vsel = vdd->pmic_info->uv_to_vsel(vdd->pmic_info-
> >on_volt);
> +	onlp_vsel = vdd->pmic_info->uv_to_vsel(vdd->pmic_info-
> >onlp_volt);
> +	ret_vsel = vdd->pmic_info->uv_to_vsel(vdd->pmic_info-
> >ret_volt);
> +	off_vsel = vdd->pmic_info->uv_to_vsel(vdd->pmic_info-
> >off_volt);
> +	vc_val	= ((on_vsel << vdd->vc_reg.cmd_on_shift) |
> +		(onlp_vsel << vdd->vc_reg.cmd_onlp_shift) |
> +		(ret_vsel << vdd->vc_reg.cmd_ret_shift) |
> +		(off_vsel << vdd->vc_reg.cmd_off_shift));
> +	vdd->write_reg(vc_val, mod, vdd->vc_reg.cmdval_reg);
> +
> +	if (is_initialized)
> +		return;
> +
> +	/* Generic VC parameters init */
> +	vdd->write_reg(OMAP3430_CMD1_MASK |
> OMAP3430_RAV1_MASK, mod,
> +			OMAP3_PRM_VC_CH_CONF_OFFSET);
> +	vdd->write_reg(OMAP3430_MCODE_SHIFT |
> OMAP3430_HSEN_MASK, mod,
> +			OMAP3_PRM_VC_I2C_CFG_OFFSET);
> +	vdd->write_reg(OMAP3_CLKSETUP, mod,
> OMAP3_PRM_CLKSETUP_OFFSET);
> +	vdd->write_reg(OMAP3_VOLTOFFSET, mod,
> OMAP3_PRM_VOLTOFFSET_OFFSET);
> +	vdd->write_reg(OMAP3_VOLTSETUP2, mod,
> OMAP3_PRM_VOLTSETUP2_OFFSET);
> +	is_initialized = true;
> +}
> +
> +/* Sets up all the VDD related info for OMAP3 */
> +static int __init omap3_vdd_data_configure(struct
> omap_vdd_info *vdd)
> +{
> +	struct clk *sys_ck;
> +	u32 sys_clk_speed, timeout_val, waittime;
> +
> +	if (!vdd->pmic_info) {
> +		pr_err("%s: PMIC info requried to configure
> vdd_%s not"
> +			"populated.Hence cannot initialize
> vdd_%s\n",
> +			__func__, vdd->voltdm.name, vdd-
> >voltdm.name);
> +		return -EINVAL;
> +	}
> +
> +	if (!strcmp(vdd->voltdm.name, "mpu")) {
> +		if (cpu_is_omap3630())
> +			vdd->volt_data =
> omap36xx_vddmpu_volt_data;
> +		else
> +			vdd->volt_data =
> omap34xx_vddmpu_volt_data;
> +
> +		vdd->vp_reg.tranxdone_status =
> OMAP3430_VP1_TRANXDONE_ST_MASK;
> +		vdd->vc_reg.cmdval_reg =
> OMAP3_PRM_VC_CMD_VAL_0_OFFSET;
> +		vdd->vc_reg.smps_sa_shift =
> OMAP3430_PRM_VC_SMPS_SA_SA0_SHIFT;
> +		vdd->vc_reg.smps_sa_mask =
> OMAP3430_PRM_VC_SMPS_SA_SA0_MASK;
> +		vdd->vc_reg.smps_volra_shift =
> OMAP3430_VOLRA0_SHIFT;
> +		vdd->vc_reg.smps_volra_mask =
> OMAP3430_VOLRA0_MASK;
> +		vdd->vc_reg.voltsetup_shift =
> OMAP3430_SETUP_TIME1_SHIFT;
> +		vdd->vc_reg.voltsetup_mask =
> OMAP3430_SETUP_TIME1_MASK;
> +	} else if (!strcmp(vdd->voltdm.name, "core")) {
> +		if (cpu_is_omap3630())
> +			vdd->volt_data =
> omap36xx_vddcore_volt_data;
> +		else
> +			vdd->volt_data =
> omap34xx_vddcore_volt_data;
> +
> +		vdd->vp_reg.tranxdone_status =
> OMAP3430_VP2_TRANXDONE_ST_MASK;
> +		vdd->vc_reg.cmdval_reg =
> OMAP3_PRM_VC_CMD_VAL_1_OFFSET;
> +		vdd->vc_reg.smps_sa_shift =
> OMAP3430_PRM_VC_SMPS_SA_SA1_SHIFT;
> +		vdd->vc_reg.smps_sa_mask =
> OMAP3430_PRM_VC_SMPS_SA_SA1_MASK;
> +		vdd->vc_reg.smps_volra_shift =
> OMAP3430_VOLRA1_SHIFT;
> +		vdd->vc_reg.smps_volra_mask =
> OMAP3430_VOLRA1_MASK;
> +		vdd->vc_reg.voltsetup_shift =
> OMAP3430_SETUP_TIME2_SHIFT;
> +		vdd->vc_reg.voltsetup_mask =
> OMAP3430_SETUP_TIME2_MASK;
> +	} else {
> +		pr_warning("%s: vdd_%s does not exisit in
> OMAP3\n",
> +			__func__, vdd->voltdm.name);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Sys clk rate is require to calculate vp timeout
> value and
> +	 * smpswaittimemin and smpswaittimemax.
> +	 */
> +	sys_ck = clk_get(NULL, "sys_ck");
> +	if (IS_ERR(sys_ck)) {
> +		pr_warning("%s: Could not get the sys clk to
> calculate"
> +			"various vdd_%s params\n", __func__, vdd-
> >voltdm.name);
> +		return -EINVAL;
> +	}
> +	sys_clk_speed = clk_get_rate(sys_ck);
> +	clk_put(sys_ck);
> +	/* Divide to avoid overflow */
> +	sys_clk_speed /= 1000;
> +
> +	/* Generic voltage parameters */
> +	vdd->curr_volt = 1200000;
> +	vdd->ocp_mod = OCP_MOD;
> +	vdd->prm_irqst_reg = OMAP3_PRM_IRQSTATUS_MPU_OFFSET;
> +	vdd->read_reg = omap3_voltage_read_reg;
> +	vdd->write_reg = omap3_voltage_write_reg;
> +	vdd->volt_scale = vp_forceupdate_scale_voltage;
> +	vdd->vp_enabled = false;
> +
> +	/* VC parameters */
> +	vdd->vc_reg.prm_mod = OMAP3430_GR_MOD;
> +	vdd->vc_reg.smps_sa_reg = OMAP3_PRM_VC_SMPS_SA_OFFSET;
> +	vdd->vc_reg.smps_volra_reg =
> OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET;
> +	vdd->vc_reg.bypass_val_reg =
> OMAP3_PRM_VC_BYPASS_VAL_OFFSET;
> +	vdd->vc_reg.voltsetup_reg =
> OMAP3_PRM_VOLTSETUP1_OFFSET;
> +	vdd->vc_reg.data_shift = OMAP3430_DATA_SHIFT;
> +	vdd->vc_reg.slaveaddr_shift =
> OMAP3430_SLAVEADDR_SHIFT;
> +	vdd->vc_reg.regaddr_shift = OMAP3430_REGADDR_SHIFT;
> +	vdd->vc_reg.valid = OMAP3430_VALID_MASK;
> +	vdd->vc_reg.cmd_on_shift = OMAP3430_VC_CMD_ON_SHIFT;
> +	vdd->vc_reg.cmd_on_mask = OMAP3430_VC_CMD_ON_MASK;
> +	vdd->vc_reg.cmd_onlp_shift =
> OMAP3430_VC_CMD_ONLP_SHIFT;
> +	vdd->vc_reg.cmd_ret_shift = OMAP3430_VC_CMD_RET_SHIFT;
> +	vdd->vc_reg.cmd_off_shift = OMAP3430_VC_CMD_OFF_SHIFT;
> +
> +	vdd->vp_reg.prm_mod = OMAP3430_GR_MOD;
> +
> +	/* VPCONFIG bit fields */
> +	vdd->vp_reg.vpconfig_erroroffset = (vdd->pmic_info-
> >vp_erroroffset <<
> +				 OMAP3430_ERROROFFSET_SHIFT);
> +	vdd->vp_reg.vpconfig_errorgain_mask =
> OMAP3430_ERRORGAIN_MASK;
> +	vdd->vp_reg.vpconfig_errorgain_shift =
> OMAP3430_ERRORGAIN_SHIFT;
> +	vdd->vp_reg.vpconfig_initvoltage_shift =
> OMAP3430_INITVOLTAGE_SHIFT;
> +	vdd->vp_reg.vpconfig_initvoltage_mask =
> OMAP3430_INITVOLTAGE_MASK;
> +	vdd->vp_reg.vpconfig_timeouten =
> OMAP3430_TIMEOUTEN_MASK;
> +	vdd->vp_reg.vpconfig_initvdd = OMAP3430_INITVDD_MASK;
> +	vdd->vp_reg.vpconfig_forceupdate =
> OMAP3430_FORCEUPDATE_MASK;
> +	vdd->vp_reg.vpconfig_vpenable =
> OMAP3430_VPENABLE_MASK;
> +
> +	/* VSTEPMIN VSTEPMAX bit fields */
> +	waittime = ((vdd->pmic_info->step_size / vdd-
> >pmic_info->slew_rate) *
> +				sys_clk_speed) / 1000;
> +	vdd->vp_reg.vstepmin_smpswaittimemin = waittime;
> +	vdd->vp_reg.vstepmax_smpswaittimemax = waittime;
> +	vdd->vp_reg.vstepmin_stepmin = vdd->pmic_info-
> >vp_vstepmin;
> +	vdd->vp_reg.vstepmax_stepmax = vdd->pmic_info-
> >vp_vstepmax;
> +	vdd->vp_reg.vstepmin_smpswaittimemin_shift =
> +				OMAP3430_SMPSWAITTIMEMIN_SHIFT;
> +	vdd->vp_reg.vstepmax_smpswaittimemax_shift =
> +				OMAP3430_SMPSWAITTIMEMAX_SHIFT;
> +	vdd->vp_reg.vstepmin_stepmin_shift =
> OMAP3430_VSTEPMIN_SHIFT;
> +	vdd->vp_reg.vstepmax_stepmax_shift =
> OMAP3430_VSTEPMAX_SHIFT;
> +
> +	/* VLIMITTO bit fields */
> +	timeout_val = (sys_clk_speed * vdd->pmic_info-
> >vp_timeout_us) / 1000;
> +	vdd->vp_reg.vlimitto_timeout = timeout_val;
> +	vdd->vp_reg.vlimitto_vddmin = vdd->pmic_info-
> >vp_vddmin;
> +	vdd->vp_reg.vlimitto_vddmax = vdd->pmic_info-
> >vp_vddmax;
> +	vdd->vp_reg.vlimitto_vddmin_shift =
> OMAP3430_VDDMIN_SHIFT;
> +	vdd->vp_reg.vlimitto_vddmax_shift =
> OMAP3430_VDDMAX_SHIFT;
> +	vdd->vp_reg.vlimitto_timeout_shift =
> OMAP3430_TIMEOUT_SHIFT;
> +
> +	return 0;
> +}

Big function. Recommend to simplify it by creating two data
configure functions, one for vp and one for vc. And call them
from omap3_vdd_data_configure().

> +
> +/* Public functions */
> +/**
> + * omap_voltage_get_nom_volt() - Gets the current non-auto-
> compensated voltage
> + * @voltdm:	pointer to the VDD for which current
> voltage info is needed
> + *
> + * API to get the current non-auto-compensated voltage for
> a VDD.
> + * Returns 0 in case of error else returns the current
> voltage for the VDD.
> + */
> +unsigned long omap_voltage_get_nom_volt(struct
> voltagedomain *voltdm)
> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return 0;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	return vdd->curr_volt;
> +}
> +
> +/**
> + * omap_vp_get_curr_volt() - API to get the current vp
> voltage.
> + * @voltdm:	pointer to the VDD.
> + *
> + * This API returns the current voltage for the specified
> voltage processor
> + */
> +unsigned long omap_vp_get_curr_volt(struct voltagedomain
> *voltdm)
> +{
> +	struct omap_vdd_info *vdd;
> +	u8 curr_vsel;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return 0;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +	if (!vdd->read_reg) {
> +		pr_err("%s: No read API for reading vdd_%s
> regs\n",
> +			__func__, voltdm->name);
> +		return 0;
> +	}
> +
> +	curr_vsel = vdd->read_reg(vdd->vp_reg.prm_mod,
> +			vdd->vp_offs.voltage);
> +
> +	if (!vdd->pmic_info || !vdd->pmic_info->vsel_to_uv) {
> +		pr_warning("%s: PMIC function to convert vsel to
> voltage"
> +			"in uV not registerd\n", __func__);
> +		return 0;
> +	}
> +
> +	return vdd->pmic_info->vsel_to_uv(curr_vsel);
> +}
> +
> +/**
> + * omap_vp_enable() - API to enable a particular VP
> + * @voltdm:	pointer to the VDD whose VP is to be
> enabled.
> + *
> + * This API enables a particular voltage processor. Needed
> by the smartreflex
> + * class drivers.
> + */
> +void omap_vp_enable(struct voltagedomain *voltdm)
> +{
> +	struct omap_vdd_info *vdd;
> +	u32 vpconfig;
> +	u16 mod;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +	if (!vdd->read_reg || !vdd->write_reg) {
> +		pr_err("%s: No read/write API for accessing
> vdd_%s regs\n",
> +			__func__, voltdm->name);
> +		return;
> +	}
> +
> +	mod = vdd->vp_reg.prm_mod;
> +
> +	/* If VP is already enabled, do nothing. Return */
> +	if (vdd->vp_enabled)
> +		return;
> +
> +	vp_latch_vsel(vdd);
> +
> +	/* Enable VP */
> +	vpconfig = vdd->read_reg(mod, vdd->vp_offs.vpconfig);
> +	vpconfig |= vdd->vp_reg.vpconfig_vpenable;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);

Check if there is a need for delay or status check (similar to
omap_vp_disable) before flipping the status to true.

> +	vdd->vp_enabled = true;
> +}
> +
> +/**
> + * omap_vp_disable() - API to disable a particular VP
> + * @voltdm:	pointer to the VDD whose VP is to be
> disabled.
> + *
> + * This API disables a particular voltage processor. Needed
> by the smartreflex
> + * class drivers.
> + */
> +void omap_vp_disable(struct voltagedomain *voltdm)
> +{
> +	struct omap_vdd_info *vdd;
> +	u32 vpconfig;
> +	u16 mod;
> +	int timeout;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +	if (!vdd->read_reg || !vdd->write_reg) {
> +		pr_err("%s: No read/write API for accessing
> vdd_%s regs\n",
> +			__func__, voltdm->name);
> +		return;
> +	}
> +
> +	mod = vdd->vp_reg.prm_mod;
> +
> +	/* If VP is already disabled, do nothing. Return */
> +	if (!vdd->vp_enabled) {
> +		pr_warning("%s: Trying to disable VP for vdd_%s
> when"
> +			"it is already disabled\n", __func__,
> voltdm->name);
> +		return;
> +	}
> +
> +	/* Disable VP */
> +	vpconfig = vdd->read_reg(mod, vdd->vp_offs.vpconfig);
> +	vpconfig &= ~vdd->vp_reg.vpconfig_vpenable;
> +	vdd->write_reg(vpconfig, mod, vdd->vp_offs.vpconfig);
> +
> +	/*
> +	 * Wait for VP idle Typical latency is <2us. Maximum
> latency is ~100us
> +	 */
> +	omap_test_timeout((vdd->read_reg(mod, vdd-
> >vp_offs.vstatus)),
> +				VP_IDLE_TIMEOUT, timeout);
> +
> +	if (timeout >= VP_IDLE_TIMEOUT)
> +		pr_warning("%s: vdd_%s idle timedout\n",
> +			__func__, voltdm->name);
> +
> +	vdd->vp_enabled = false;
> +
> +	return;
> +}
> +
> +/**
> + * omap_voltage_scale_vdd() - API to scale voltage of a
> particular
> + *				voltage domain.
> + * @voltdm:	pointer to the VDD which is to be scaled.
> + * @target_volt:	The target voltage of the voltage domain
> + *
> + * This API should be called by the kernel to do the
> voltage scaling
> + * for a particular voltage domain during dvfs or any other
> situation.
> + */
> +int omap_voltage_scale_vdd(struct voltagedomain *voltdm,
> +		unsigned long target_volt)
> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	if (!vdd->volt_scale) {
> +		pr_err("%s: No voltage scale API registered for
> vdd_%s\n",
> +			__func__, voltdm->name);
> +		return -ENODATA;
> +	}
> +
> +	return vdd->volt_scale(vdd, target_volt);
> +}
> +
> +/**
> + * omap_voltage_reset() - Resets the voltage of a
> particular voltage domain
> + *			to that of the current OPP.
> + * @voltdm:	pointer to the VDD whose voltage is to be
> reset.
> + *
> + * This API finds out the correct voltage the voltage
> domain is supposed
> + * to be at and resets the voltage to that level. Should be
> used expecially
> + * while disabling any voltage compensation modules.
> + */
> +void omap_voltage_reset(struct voltagedomain *voltdm)
> +{
> +	unsigned long target_uvdc;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return;
> +	}
> +
> +	target_uvdc = omap_voltage_get_nom_volt(voltdm);
> +	if (!target_uvdc) {
> +		pr_err("%s: unable to find current voltage for
> vdd_%s\n",
> +			__func__, voltdm->name);
> +		return;
> +	}
> +
> +	omap_voltage_scale_vdd(voltdm, target_uvdc);
> +}
> +
> +/**
> + * omap_voltage_get_volttable() - API to get the voltage
> table associated with a
> + *				particular voltage domain.
> + * @voltdm:	pointer to the VDD for which the voltage
> table is required
> + * @volt_data:	the voltage table for the particular vdd
> which is to be
> + *		populated by this API
> + *
> + * This API populates the voltage table associated with a
> VDD into the
> + * passed parameter pointer. Returns the count of distinct
> voltages
> + * supported by this vdd.
> + *
> + */
> +void omap_voltage_get_volttable(struct voltagedomain
> *voltdm,
> +		struct omap_volt_data **volt_data)
> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	*volt_data = vdd->volt_data;
> +}
> +
> +/**
> + * omap_voltage_get_voltdata() - API to get the voltage
> table entry for a
> + *				particular voltage
> + * @voltdm:	pointer to the VDD whose voltage table has
> to be searched
> + * @volt:	the voltage to be searched in the voltage table
> + *
> + * This API searches through the voltage table for the
> required voltage
> + * domain and tries to find a matching entry for the passed
> voltage volt.
> + * If a matching entry is found volt_data is populated with
> that entry.
> + * This API searches only through the non-compensated
> voltages int the
> + * voltage table.
> + * Returns pointer to the voltage table entry corresponding
> to volt on
> + * sucess. Returns -ENODATA if no voltage table exisits for
> the passed voltage
> + * domain or if there is no matching entry.
> + */
> +struct omap_volt_data *omap_voltage_get_voltdata(struct
> voltagedomain *voltdm,
> +		unsigned long volt)
> +{
> +	struct omap_vdd_info *vdd;
> +	int i;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	if (!vdd->volt_data) {
> +		pr_warning("%s: voltage table does not exist for
> vdd_%s\n",
> +			__func__, voltdm->name);
> +		return ERR_PTR(-ENODATA);
> +	}
> +
> +	for (i = 0; vdd->volt_data[i].volt_nominal != 0; i++)
> {
> +		if (vdd->volt_data[i].volt_nominal == volt)
> +			return &vdd->volt_data[i];
> +	}
> +
> +	pr_notice("%s: Unable to match the current voltage
> with the voltage"
> +		"table for vdd_%s\n", __func__, voltdm->name);
> +
> +	return ERR_PTR(-ENODATA);
> +}
> +
> +/**
> + * omap_voltage_register_pmic() - API to register PMIC
> specific data
> + * @voltdm:	pointer to the VDD for which the PMIC
> specific data is
> + *		to be registered
> + * @pmic_info:	the structure containing pmic info
> + *
> + * This API is to be called by the SOC/PMIC file to specify
> the
> + * pmic specific info as present in omap_volt_pmic_info
> structure.
> + */
> +int omap_voltage_register_pmic(struct voltagedomain
> *voltdm,
> +		struct omap_volt_pmic_info *pmic_info)
> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	vdd->pmic_info = pmic_info;
> +
> +	return 0;
> +}
> +
> +/**
> + * omap_voltage_get_dbgdir() - API to get pointer to the
> debugfs directory
> + *				corresponding to a voltage domain.
> + *
> + * @voltdm:	pointer to the VDD whose debug directory
> is required.
> + *
> + * This API returns pointer to the debugfs directory
> corresponding
> + * to the voltage domain. Should be used by drivers
> requiring to
> + * add any debug entry for a particular voltage domain.
> Returns NULL
> + * in case of error.
> + */
> +struct dentry *omap_voltage_get_dbgdir(struct voltagedomain
> *voltdm)
> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return NULL;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	return vdd->debug_dir;
> +}
> +
> +/**
> + * omap_change_voltscale_method() - API to change the
> voltage scaling method.
> + * @voltdm:	pointer to the VDD whose voltage scaling
> method
> + *		has to be changed.
> + * @voltscale_method:	the method to be used for voltage
> scaling.
> + *
> + * This API can be used by the board files to change the
> method of voltage
> + * scaling between vpforceupdate and vcbypass. The
> parameter values are
> + * defined in voltage.h
> + */
> +void omap_change_voltscale_method(struct voltagedomain
> *voltdm,
> +		int voltscale_method)

Recommend to change the return type to int for better error
handling.

> +{
> +	struct omap_vdd_info *vdd;
> +
> +	if (!voltdm || IS_ERR(voltdm)) {
> +		pr_warning("%s: VDD specified does not
> exist!\n", __func__);
> +		return;
> +	}
> +
> +	vdd = container_of(voltdm, struct omap_vdd_info,
> voltdm);
> +
> +	switch (voltscale_method) {
> +	case VOLTSCALE_VPFORCEUPDATE:
> +		vdd->volt_scale = vp_forceupdate_scale_voltage;
> +		return;
> +	case VOLTSCALE_VCBYPASS:
> +		vdd->volt_scale = vc_bypass_scale_voltage;
> +		return;
> +	default:
> +		pr_warning("%s: Trying to change the method of
> voltage scaling"
> +			"to an unsupported one!\n", __func__);
> +	}
> +}
> +
> +/**
> + * omap_voltage_domain_lookup() - API to get the voltage
> domain pointer
> + * @name:	Name of the voltage domain
> + *
> + * This API looks up in the global vdd_info struct for the
> + * existence of voltage domain <name>. If it exists, the
> API returns
> + * a pointer to the voltage domain structure corresponding
> to the
> + * VDD<name>. Else retuns error pointer.
> + */
> +struct voltagedomain *omap_voltage_domain_lookup(char
> *name)
> +{
> +	int i;
> +
> +	if (!vdd_info) {
> +		pr_err("%s: Voltage driver init not yet
> happened.Faulting!\n",
> +			__func__);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!name) {
> +		pr_err("%s: No name to get the votage
> domain!\n", __func__);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	for (i = 0; i < nr_scalable_vdd; i++) {
> +		if (!(strcmp(name, vdd_info[i].voltdm.name)))
> +			return &vdd_info[i].voltdm;
> +	}

Print an error before returning.

> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +/**
> + * omap_voltage_late_init() - Init the various voltage
> parameters
> + *
> + * This API is to be called in the later stages of the
> + * system boot to init the voltage controller and
> + * voltage processors.
> + */
> +int __init omap_voltage_late_init(void)
> +{
> +	int i;
> +
> +	if (!vdd_info) {
> +		pr_err("%s: Voltage driver support not added\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	voltage_dir = debugfs_create_dir("voltage", NULL);
> +	if (IS_ERR(voltage_dir))
> +		pr_err("%s: Unable to create voltage debugfs
> main dir\n",
> +			__func__);
> +	for (i = 0; i < nr_scalable_vdd; i++) {
> +		if (vdd_data_configure(&vdd_info[i]))
> +			continue;

Do error checking and print success/ fail message (especially
fail) for each of vc_init, vp_init & vdd_debugfs_init.
May need to change the return types of those functions.

A failure should stop further initialization.

> +		vc_init(&vdd_info[i]);
> +		vp_init(&vdd_info[i]);
> +		vdd_debugfs_init(&vdd_info[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * omap_voltage_early_init()- Volatage driver early init
> + */
> +static int __init omap_voltage_early_init(void)
> +{
> +	if (cpu_is_omap34xx()) {
> +		vdd_info = omap3_vdd_info;
> +		nr_scalable_vdd = OMAP3_NR_SCALABLE_VDD;
> +		vc_init = omap3_vc_init;
> +		vdd_data_configure = omap3_vdd_data_configure;
> +	} else {
> +		pr_warning("%s: voltage driver support not
> added\n", __func__);
> +	}
> +
> +	return 0;
> +}

Recommend to reorder the functions based on logical grouping
of related functionalities e.g. all init/ configure functions
can be listed together. A comment explaining these different
groups and their constituent functions (located near the
beginning) will be useful.

> +core_initcall(omap_voltage_early_init);
> diff --git a/arch/arm/plat-omap/include/plat/voltage.h
> b/arch/arm/plat-omap/include/plat/voltage.h
> new file mode 100644
> index 0000000..2f4f59a
> --- /dev/null
> +++ b/arch/arm/plat-omap/include/plat/voltage.h
> @@ -0,0 +1,134 @@
> +/*
> + * OMAP Voltage Management Routines
> + *
> + * Author: Thara Gopinath	<thara@ti.com>
> + *
> + * Copyright (C) 2009 Texas Instruments, Inc.
> + * Thara Gopinath <thara@ti.com>
> + *
> + * This program is free software; you can redistribute it
> and/or modify
> + * it under the terms of the GNU General Public License
> version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ARCH_ARM_MACH_OMAP2_VOLTAGE_H
> +#define __ARCH_ARM_MACH_OMAP2_VOLTAGE_H
> +
> +#define VOLTSCALE_VPFORCEUPDATE		1
> +#define VOLTSCALE_VCBYPASS		2

Add comment explaining the above #defines

> +
> +/*
> + * OMAP3 GENERIC setup times. Revisit to see if these needs
> to be
> + * passed from board or PMIC file
> + */
> +#define OMAP3_CLKSETUP		0xff
> +#define OMAP3_VOLTOFFSET	0xff
> +#define OMAP3_VOLTSETUP2	0xff
> +
> +/* Voltage value defines */
> +#define OMAP3430_VDD_MPU_OPP1_UV		975000
> +#define OMAP3430_VDD_MPU_OPP2_UV		1075000
> +#define OMAP3430_VDD_MPU_OPP3_UV		1200000
> +#define OMAP3430_VDD_MPU_OPP4_UV		1270000
> +#define OMAP3430_VDD_MPU_OPP5_UV		1350000
> +
> +#define OMAP3430_VDD_CORE_OPP1_UV		975000
> +#define OMAP3430_VDD_CORE_OPP2_UV		1050000
> +#define OMAP3430_VDD_CORE_OPP3_UV		1150000
> +
> +#define OMAP3630_VDD_MPU_OPP50_UV		1012500
> +#define OMAP3630_VDD_MPU_OPP100_UV		1200000
> +#define OMAP3630_VDD_MPU_OPP120_UV		1325000
> +#define OMAP3630_VDD_MPU_OPP1G_UV		1375000
> +
> +#define OMAP3630_VDD_CORE_OPP50_UV		1000000
> +#define OMAP3630_VDD_CORE_OPP100_UV		1200000
> +
> +/**
> + * struct voltagedomain - omap voltage domain global
> structure.
> + * @name:	Name of the voltage domain which can be used as
> a unique
> + *		identifier.
> + */
> +struct voltagedomain {
> +	char *name;
> +};
> +
> +/* API to get the voltagedomain pointer */
> +struct voltagedomain *omap_voltage_domain_lookup(char
> *name);
> +
> +/**
> + * struct omap_volt_data - Omap voltage specific data.
> + * @voltage_nominal:	The possible voltage value in uV
> + * @sr_efuse_offs:	The offset of the efuse
> register(from system
> + *			control module base address) from where to
> read
> + *			the n-target value for the smartreflex
> module.
> + * @sr_errminlimit:	Error min limit value for
> smartreflex. This value
> + *			differs at differnet opp and thus is
> linked
> + *			with voltage.
> + * @vp_errorgain:	Error gain value for the voltage
> processor. This
> + *			field also differs according to the
> voltage/opp.
> + */
> +struct omap_volt_data {
> +	u32	volt_nominal;
> +	u32	sr_efuse_offs;
> +	u8	sr_errminlimit;
> +	u8	vp_errgain;
> +};
> +
> +/**
> + * struct omap_volt_pmic_info - PMIC specific data required
> by voltage driver.
> + * @slew_rate:	PMIC slew rate (in uv/us)
> + * @step_size:	PMIC voltage step size (in uv)
> + * @vsel_to_uv:	PMIC API to convert vsel value to actual
> voltage in uV.
> + * @uv_to_vsel:	PMIC API to convert voltage in uV to vsel
> value.

Add comments for the remaining data members.

Best Regards,

Anand Sawant

> + */
> +struct omap_volt_pmic_info {
> +	int slew_rate;
> +	int step_size;
> +	u32 on_volt;
> +	u32 onlp_volt;
> +	u32 ret_volt;
> +	u32 off_volt;
> +	u16 volt_setup_time;
> +	u8 vp_erroroffset;
> +	u8 vp_vstepmin;
> +	u8 vp_vstepmax;
> +	u8 vp_vddmin;
> +	u8 vp_vddmax;
> +	u8 vp_timeout_us;
> +	u8 i2c_slave_addr;
> +	u8 pmic_reg;
> +	unsigned long (*vsel_to_uv) (const u8 vsel);
> +	u8 (*uv_to_vsel) (unsigned long uV);
> +};
> +
> +unsigned long omap_vp_get_curr_volt(struct voltagedomain
> *voltdm);
> +void omap_vp_enable(struct voltagedomain *voltdm);
> +void omap_vp_disable(struct voltagedomain *voltdm);
> +int omap_voltage_scale_vdd(struct voltagedomain *voltdm,
> +		unsigned long target_volt);
> +void omap_voltage_reset(struct voltagedomain *voltdm);
> +void omap_voltage_get_volttable(struct voltagedomain
> *voltdm,
> +		struct omap_volt_data **volt_data);
> +struct omap_volt_data *omap_voltage_get_voltdata(struct
> voltagedomain *voltdm,
> +		unsigned long volt);
> +unsigned long omap_voltage_get_nom_volt(struct
> voltagedomain *voltdm);
> +struct dentry *omap_voltage_get_dbgdir(struct voltagedomain
> *voltdm);
> +#ifdef CONFIG_PM
> +int omap_voltage_register_pmic(struct voltagedomain
> *voltdm,
> +		struct omap_volt_pmic_info *pmic_info);
> +void omap_change_voltscale_method(struct voltagedomain
> *voltdm,
> +		int voltscale_method);
> +int omap_voltage_late_init(void);
> +#else
> +static inline int omap_voltage_register_pmic(struct
> voltagedomain *voltdm,
> +		struct omap_volt_pmic_info *pmic_info) {}
> +static inline  void omap_change_voltscale_method(struct
> voltagedomain *voltdm,
> +		int voltscale_method) {}
> +static inline int omap_voltage_late_init(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +#endif
> --
> 1.7.0.4