[RFC] OMAP1 PM Core, PM Core Implementation 2/2

[RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Eugeny S. Mints]

OMAP1 PM Core reference implementation

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index f9362ee..05cd7f1 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -745,6 +745,21 @@ config CPU_FREQ_INTEGRATOR
 
 endmenu
 
+if (ARCH_OMAP)
+
+menu "PM Core"
+
+config OMAP1_PM_CORE
+	tristate "OMAP1 PM Core"
+	depends POWEROP
+	help
+	This enables OMAP1 PM Core to control platform clocks/power via
+	PowerOP interface
+
+endmenu
+
+endif
+
 endif
 
 menu "Floating point emulation"
diff --git a/arch/arm/mach-omap1/Makefile b/arch/arm/mach-omap1/Makefile
index 7165f74..de61ab1 100644
--- a/arch/arm/mach-omap1/Makefile
+++ b/arch/arm/mach-omap1/Makefile
@@ -38,3 +38,6 @@ led-$(CONFIG_MACH_OMAP_PERSEUS2)	+= leds
 led-$(CONFIG_MACH_OMAP_OSK)		+= leds-osk.o
 obj-$(CONFIG_LEDS)			+= $(led-y)
 
+#PM Core
+obj-$(CONFIG_OMAP1_PM_CORE)		+=pm_core.o
+
diff --git a/arch/arm/mach-omap1/pm_core.c b/arch/arm/mach-omap1/pm_core.c
new file mode 100644
index 0000000..d144928
--- /dev/null
+++ b/arch/arm/mach-omap1/pm_core.c
@@ -0,0 +1,530 @@
+/*
+ * arch/arm/mach-omap1/pm_core.c
+ *
+ * PM Core implementation by Eugeny S. Mints <eugeny.mints@gmail.com>
+ *
+ * Copyright (C) 2006, Nomad Global Solutions, Inc.
+ *
+ * Based on code by Todd Poynor, Matthew Locke, Dmitry Chigirev, and 
+ * Bishop Brock.
+ *
+ * Copyright (C) 2002, 2004 MontaVista Software <source@mvista.com>.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+#include <linux/config.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/kmod.h>
+#include <linux/module.h>
+#include <linux/stat.h>
+#include <linux/string.h>
+#include <linux/device.h>
+#include <linux/pm.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/powerop.h>
+#include <linux/powerop_sysfs.h>
+#include <linux/pm_core.h>
+#include <linux/plat_pwr_param.h>
+
+#include <asm/hardirq.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
+#include <asm/processor.h>
+#include <asm/uaccess.h>
+#include <asm/io.h>
+#include <asm/mach-types.h>
+#include <asm/arch/hardware.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/board.h>
+#include <asm/arch/mux.h>
+#include <asm/arch/pm.h>
+#include <asm/arch/system.h>
+#include <asm/mach/time.h>
+#include <asm/arch/pm_core.h>
+
+#if defined(CONFIG_MACH_OMAP_H2)
+#define V_CORE_HIGH		1500
+#define V_CORE_LOW		1300
+#define DVS_SUPPORT
+#elif defined(CONFIG_MACH_OMAP_H3)
+#define V_CORE_HIGH		1300
+#define V_CORE_LOW		1050
+#define DVS_SUPPORT
+#else
+#define V_CORE_HIGH		1500
+#endif
+
+#define ULPD_MIN_MAX_REG (1 << 11)
+#define ULPD_DVS_ENABLE  (1 << 10)
+#define ULPD_LOW_PWR_REQ (1 << 1)
+#define LOW_POWER (ULPD_MIN_MAX_REG | ULPD_DVS_ENABLE | ULPD_LOW_PWR_REQ | \
+		   ULPD_LOW_PWR_EN)
+
+/* 
+ * REVISIT: this function is implemented here temporary.
+ * Once Clock/Voltage Framework is in  place this routine
+ * implementation will be hided in the framework (along with 
+ * struct voltage)
+ */ 
+struct voltage {
+	int v; /* mV */
+};
+static struct voltage vhandle;
+
+static unsigned int
+vtg_get_voltage(struct voltage *handle)
+{
+	unsigned int v;
+
+#ifdef DVS_SUPPORT
+	if (omap_readw(ULPD_POWER_CTRL) & (1 << 1))
+		v = V_CORE_LOW;
+	else
+#endif
+		v = V_CORE_HIGH;
+
+	return v;
+}
+
+/* 
+ * auxiliary routines to get/set rate of various clocks 
+ * RETURN: rates in KHz
+ */
+static long 
+get_clk_rate(const char *clk_name)
+{
+	struct clk *clk;
+	long        ret;
+
+	clk = clk_get(0, clk_name);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_get_rate(clk);
+	clk_put(clk);
+
+	return ret/1000;
+}
+
+static long 
+set_clk_rate(const char *clk_name, unsigned int rate)
+{
+	struct clk *clk;
+	long        ret;
+
+	clk = clk_get(0, clk_name);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_set_rate(clk, rate);
+	clk_put(clk);
+
+	return ret;
+}
+
+/* 
+ * auxiliary routines to get/set rate voltages 
+ * RETURN: mV
+ */
+static long 
+get_vtg(const char *vdomain)
+{
+	long ret = 0;
+#ifdef VOLTAGE_FRAMEWORK
+	struct voltage *v;
+
+	v = vtg_get(0, vdomain);
+	if (IS_ERR(v))
+		return PTR_ERR(v);
+
+	ret = vtg_get_voltage(v);
+	vtg_put(v);
+#else
+	ret = vtg_get_voltage(&vhandle);
+#endif /* VOLTAGE_FRAMEWORK */	
+	return ret;
+}
+
+static long 
+set_vtg(const char *vdomain, int val)
+{
+	long ret = 0;
+#ifdef VOLTAGE_FRAMEWORK
+	struct voltage *v;
+
+	v = vtg_get(0, vdomain);
+	if (IS_ERR(v))
+		return PTR_ERR(v);
+
+	ret = vtg_request(v, val);
+	vtg_put(v);
+#endif /* VOLTAGE_FRAMEWORK */
+	return ret;
+}
+
+/* FIXME: verification is incomplete */
+static int
+omap_pm_core_verify_opt(struct pm_core_point *opt)
+{
+	/* 
+	 * Let's do some upfront error checking. If we fail any of these, 
+	 * then the whole operating point is suspect and therefore invalid.
+	 *
+	 * Vrification includes but is not limited to checking that passed
+	 * parameters are withing ranges.
+	 *
+	 * FIXME: who should be responsible for checking whether the values
+	 * are consistent in regard to clock modes (fully sycn/scalable sync/
+	 * etc)?
+	 */
+	if (opt == NULL)
+		return -EINVAL;
+
+#ifdef DVS_SUPPORT
+	if ((opt->cpu_vltg != -1) && ((opt->cpu_vltg != V_CORE_LOW) && 
+	    (opt->cpu_vltg != V_CORE_HIGH))) {
+		printk(KERN_WARNING "%s: Core voltage can not be other"
+		       "than %dmV or %dmV\n"
+		       "Core voltage %d out of range!", __FUNCTION__, 
+			V_CORE_LOW, V_CORE_HIGH, opt->cpu_vltg);
+		return -EINVAL;
+	}
+#endif
+
+	/* TODO: implement all necessary checking */
+
+	return 0;
+}
+
+#define PWR_PARAM_MAX_LENGTH	20
+/* REVISIT: with the current approach which allows only subset of platform 
+ * power parameters to be explicitly listed at an operating point creation the
+ * following aspect needs attention. PM Core puts '-1' to all power parameters
+ * which are not listed explicitly. omap_pm_core_verify_opt() replaces '-1'
+ * with values inherited from current operating point and resulting full set of  * power parameters may appear
+ * to be invalid (for example ivalid combination of dsp and dspmmu frequencies)
+ * The are two options. First one is to leave it upto to an upper layer to 
+ * provide correct resulting operating point regadless of how many power 
+ * parameters are listed explicitly. The other option is to provide an
+ * interface to a caller which in case of resulting point after replacement of 
+ * all implicit parameters occurs to be invalid returns a valid operating point
+ * with the original value for explicitly set power parameters but with
+ * implicit parameters set to complement of explicit parameter upto a valid
+ * operating point instead of just inherited implicit parameters from the 
+ * current operatin point. Of course just if such combination exists.
+ *
+ * NOTE: implicit changes of various power parameters underneath PM Core 
+ * layer like set_clk_rate() for "mpu" clock does will be prohibited and
+ * removed in the future since otherwise PM Core will validate one operating
+ * point while system will actually switch to completely different one
+ */
+static void *
+omap_pm_create_point(struct powerop_pwr_param *pwr_params, int size)
+{
+	struct pm_core_point *opt;
+	int i = 0;
+	int err;
+
+	opt = kmalloc(sizeof(struct pm_core_point), GFP_KERNEL);
+	if (opt == NULL)
+		return NULL;
+	/* 
+	 * PM Core puts '-1' for all power parameters which are not listed
+	 * explicitly
+	 */
+	OMAP_PM_CORE_INIT_OPT(opt);
+
+	for (i = 0; i < size; i++)
+	{
+		struct platform_pwr_param *p = 
+				to_platform_pwr_param(pwr_params[i].attr);
+		p->store(opt, pwr_params[i].value); 
+	}
+
+	err = omap_pm_core_verify_opt(opt);
+	if (err != 0)
+		goto out;
+
+	return (void *)opt;
+out:
+	kfree(opt);
+	return NULL;
+}
+
+/* FIXME: synchronization aspect may need attention */
+/*
+ * FIXME: notifiers  aspect is not highlighted here. There are 3 options:
+ *  1) let an upper layer to handle notifiers
+ *  2) handle notifiers at PM Core layer
+ *  3) handle notifiers at clock(voltage) framework layer
+ */ 
+static int
+omap_pm_core_set_opt(void *md_opt)
+{
+	int ret = 0;
+	struct pm_core_point *new = (struct pm_core_point *)md_opt;
+
+	ret = omap_pm_core_verify_opt(new);
+	if (ret != 0)
+		return ret;
+
+#define PM_CORE_USE_MPU_CLOCK
+#ifdef PM_CORE_USE_MPU_CLOCK
+	/* 
+	 * OMAP1 clock framework exports "smart" virtual clock "mpu". 
+	 * "mpu" clock hides and hardcodes interdependencies between
+	 * ck_dpll1, arm_ck, dsp_ck, dspmmu_ck, tc_ck via the
+	 * rate_table[].
+	 * "mpu" clock rate needs to be set to desired cpu clock rate 
+	 *
+	 * current implementation of "mpu" clock allows to switch between
+	 * only those cpu frequencies defined in the rate_table[] which 
+	 * have the same ck_ref and dpll_ck1 rates.
+	 *
+	 * since CONFIG_OMAP_ARM_192MHZ provides the widest range of 
+	 * available frequencies which meet the above constraints we 
+	 * use this config for pm_core reference code    
+	 */
+
+	ret = set_clk_rate("mpu", new->cpu * 1000);
+	if (ret != 0)
+		return ret;
+#else
+	/* 
+	 * go through op parameters and set as requested if changed 
+	 * if at least one failed then restore cur_opt
+	 *
+	 * NOTE: auxiliary access to hw registers may occur here for
+	 * example for proper handling of coupled simultaneous clock and 
+	 * voltage changes  
+	 *
+	 * FIXME: not all parameters are implemented so far
+	 * REVISIT: '-1' option is not implemented yet
+	 */
+	if (cur_opt.cpu_vltg != new->cpu_vltg)
+		if ((ret = set_vtg("v1", new->cpu_vltg)) != 0)
+			goto failed;
+
+	if (cur_opt.dpll != new->dpll)
+		if ((ret = set_clk_rate("ck_dpll1", new->dpll)) != 0)
+			goto failed;
+
+	if (cur_opt.cpu != new->cpu)
+		if ((ret = set_clk_rate("arm_ck", new->cpu)) != 0)
+			goto failed;
+
+	if (cur_opt.tc != new->tc)
+		if ((ret = set_clk_rate("tc_ck", new->tc)) != 0)
+			goto failed;
+
+	if (cur_opt.dsp != new->dsp)
+		if ((ret = set_clk_rate("dsp_ck", new->dsp)) != 0)
+			goto failed;
+
+	if (cur_opt.dspmmu != new->dspmmu)
+		if ((ret = set_clk_rate("dspmmu_ck", new->dspmmu)) != 0)
+			goto failed;
+#endif /* PM_CORE_USE_MPU_CLOCK */ 
+	return ret;
+}
+
+/* 
+ * Fully determine the current machine-dependent operating point, and fill in 
+ * a structure presented by a caller.
+ *
+ * RETURN: -ENOENT on error; in error case one should not rely on any 
+ * power parameter value returned
+ */
+static int
+omap_pm_core_access_opt(void *md_opt, struct powerop_pwr_param *pwr_params,
+			int size, int op)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	struct platform_pwr_param *p;
+	int i = 0;
+
+	if ((pwr_params == NULL) && (size != 0))
+		return -EINVAL;
+
+	if (op == POWEROP_GET_PARAMS) {
+		for (i = 0; i < size; i++) {
+			p = to_platform_pwr_param(pwr_params[i].attr);
+			if (p->show(opt, &pwr_params[i].value))
+				return -EIO;
+		} 
+	} else {
+		for (i = 0; i < size; i++) {
+			p = to_platform_pwr_param(pwr_params[i].attr);					p->store(opt, pwr_params[i].value);
+		}
+	} 
+
+	return 0;
+}
+
+static struct powerop_driver omap_pm_core_driver = {
+	.owner                  = THIS_MODULE,
+	.name                   = "omap1 pm core driver",
+	.create_point           = omap_pm_create_point,
+	.access_point           = omap_pm_core_access_opt,
+	.set_point              = omap_pm_core_set_opt, 
+};
+
+static int cpu_vltg_show(void *md_opt, int *value)
+{
+	int rc = 0;
+	if (md_opt == NULL) {
+		if ((*value = get_vtg("v1")) <= 0)
+			return -EIO;
+	}
+	else {
+		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+		*value = opt->cpu_vltg;
+	}
+
+	return rc;
+}
+
+/* FIXME: in all *_store routines we can check that a value is within range */
+static void cpu_vltg_store(void *md_opt, int value)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	opt->cpu_vltg = value;
+}
+
+static int dpll_show(void *md_opt, int *value)
+{
+	int rc = 0;
+	if (md_opt == NULL) {
+		if ((*value = get_clk_rate("ck_dpll1")) <= 0)
+			return -EIO;
+	}
+	else {
+		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+		*value = opt->dpll;
+	}
+
+	return rc;
+}
+
+static void dpll_store(void *md_opt, int value)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	opt->dpll = value;
+}
+
+static int cpu_show(void *md_opt, int *value)
+{
+	int rc = 0;
+	if (md_opt == NULL) {
+		if ((*value = get_clk_rate("arm_ck")) <= 0)
+			return -EIO;
+	}
+	else {
+		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+		*value = opt->cpu;
+	}
+
+	return rc;
+}
+
+static void cpu_store(void *md_opt, int value)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	opt->cpu = value;
+}
+
+static int tc_show(void *md_opt, int *value)
+{
+	int rc = 0;
+	if (md_opt == NULL) {
+		if ((*value = get_clk_rate("tc_ck")) <= 0)
+			return -EIO;
+	}
+	else {
+		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+		*value = opt->tc;
+	}
+
+	return rc;
+}
+
+static void tc_store(void *md_opt, int value)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	opt->tc = value;
+}
+
+static int per_show(void *md_opt, int *value)
+{
+	int rc = 0;
+	if (md_opt == NULL) {
+		if ((*value = get_clk_rate("armper_ck")) <= 0)
+			return -EIO;
+	}
+	else {
+		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+		*value = opt->per;
+	}
+
+	return rc;
+}
+
+static void per_store(void *md_opt, int value)
+{
+	struct pm_core_point *opt = (struct pm_core_point *)md_opt;
+	opt->per = value;
+}
+
+platform_pwr_param_init(cpu_vltg);
+platform_pwr_param_init(dpll);
+platform_pwr_param_init(cpu);
+platform_pwr_param_init(tc);
+platform_pwr_param_init(per);
+
+static struct attribute *platform_pwr_params[] = {
+	&cpu_vltg_attr.attr,
+	&dpll_attr.attr,
+	&cpu_attr.attr,
+	&tc_attr.attr,
+	&per_attr.attr,
+	NULL,
+};
+
+int 
+platform_pwr_param_get_names(struct attribute ***platform_pwr_params_names)
+{
+	*platform_pwr_params_names = platform_pwr_params;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_pwr_param_get_names);
+
+int __init
+omap_pm_core_init(void)
+{
+	int rc;
+
+	rc = powerop_driver_register(&omap_pm_core_driver);
+	if (rc != 0)
+		return rc;
+
+	return powerop_sysfs_register_pwr_params(platform_pwr_params);
+}
+
+postcore_initcall(omap_pm_core_init);
+
diff --git a/include/asm-arm/arch-omap/pm_core.h b/include/asm-arm/arch-omap/pm_core.h
new file mode 100644
index 0000000..fd406d8
--- /dev/null
+++ b/include/asm-arm/arch-omap/pm_core.h
@@ -0,0 +1,48 @@
+/*
+ * Set of power parameters supported for OMAP1
+ *
+ * Based on DPM OMAP code by Matthew Locke, Dmitry Chigirev, Vladimir
+ * Barinov, and Todd Poynor.
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#ifndef __ASM_ARCH_OMAP_PM_CORE_H__
+#define __ASM_ARCH_OMAP_PM_CORE_H__
+
+#define PM_CORE_PRE_NOTIFICATION    1
+#define PM_CORE_POST_NOTIFICATION   2
+/* 
+ * Instances of this structure define operating points
+ *
+ * -1 for any following fields indicates no change from current op 
+ */
+
+struct pm_core_point {
+	int cpu_vltg; /* voltage in mV */
+	int dpll;     /* in KHz */
+	int cpu;      /* CPU frequency in KHz */
+	int tc;       /* in KHz */
+	int per;      /* in KHz */
+	int dsp;      /* in KHz */
+	int dspmmu;   /* in KHz */
+	int lcd;      /* in KHz */
+};
+
+#define OMAP_PM_CORE_INIT_OPT(point)    \
+	do {                            \
+		(point)->cpu_vltg = -1; \
+		(point)->dpll = -1;     \
+		(point)->cpu = -1;      \
+		(point)->tc = -1;       \
+		(point)->per = -1;      \
+		(point)->dsp = -1;      \
+		(point)->dspmmu = -1;   \
+		(point)->lcd = -1;      \
+	} while (0)   
+
+#endif /* __ASM_ARCH_OMAP_PM_CORE_H__ */
+

Re: [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

Some nitpicking about the patch follows..

On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> +static long 
> +get_vtg(const char *vdomain)
> +{
> +	long ret = 0;

Unnecessary initialisation.

> +static long 
> +set_vtg(const char *vdomain, int val)
> +{
> +	long ret = 0;

here too. This and 'int i = 0;' happens in many functions.

> +	err = omap_pm_core_verify_opt(opt);
> +	if (err != 0)
> +		goto out;
> +
> +	return (void *)opt;
> +out:
> +	kfree(opt);
> +	return NULL;
> +}

Maybe use if (err != 0) { kfree(opt); return NULL; } because goto out is 
only used once?

> +static int cpu_vltg_show(void *md_opt, int *value)
> +{
> +	int rc = 0;
> +	if (md_opt == NULL) {
> +		if ((*value = get_vtg("v1")) <= 0)
> +			return -EIO;
> +	}
> +	else {
> +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
> +		*value = opt->cpu_vltg;
> +	}
> +
> +	return rc;
> +}

int rc is unnecessary because the function always returns 0. This 
happens in many places.

> +	int dpll;     /* in KHz */
> +	int cpu;      /* CPU frequency in KHz */
> +	int tc;       /* in KHz */
> +	int per;      /* in KHz */
> +	int dsp;      /* in KHz */
> +	int dspmmu;   /* in KHz */
> +	int lcd;      /* in KHz */
> +};

The SI multiplier is 'k', not 'K'.

- Heikki

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
> Some nitpicking about the patch follows..
> 
> On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > +static long 
> > +get_vtg(const char *vdomain)
> > +{
> > +	long ret = 0;
> 
> Unnecessary initialisation.

No, sorry.

> > +static long 
> > +set_vtg(const char *vdomain, int val)
> > +{
> > +	long ret = 0;
> 
> here too. This and 'int i = 0;' happens in many functions.

Wrong again. automatic variables are not zero initialized.

> > +	err = omap_pm_core_verify_opt(opt);
> > +	if (err != 0)
> > +		goto out;
> > +
> > +	return (void *)opt;
> > +out:
> > +	kfree(opt);
> > +	return NULL;
> > +}
> 
> Maybe use if (err != 0) { kfree(opt); return NULL; } because goto out is 
> only used once?

This is actually common kernel idiom, so it is okay to leave like
this. Your other points are ok.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

On Sun, Oct 01, 2006 at 07:10:32PM +0200, Pavel Machek wrote:
> On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
> > Some nitpicking about the patch follows..
> > 
> > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > > +static long 
> > > +get_vtg(const char *vdomain)
> > > +{
> > > +	long ret = 0;
> > 
> > Unnecessary initialisation.
> 
> No, sorry.

In get_vtg(), if VOLTAGE_FRAMEWORK is defined then

	ret = vtg_get_voltage(v);

is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:

	ret = vtg_get_voltage(&vhandle);

Then "return ret;" follows. I cannot see a path where 
pre-initialisation of ret does anything useful. If someone removed the
#else part, the compiler would bark.

> 
> > > +static long 
> > > +set_vtg(const char *vdomain, int val)
> > > +{
> > > +	long ret = 0;
> > 
> > here too.
> 
> Wrong again. automatic variables are not zero initialized.

My bad, this was a mistake. If VOLTAGE_FRAMEWORK is not defined, ret 
must be initialised. (the compiler would have noticed this one :-)

>> 'int i = 0;' happens in many functions.

for example, omap_pm_create_point() does this.

- Heikki

Re: [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> > > Some nitpicking about the patch follows..
> > > 
> > > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > > > +static long 
> > > > +get_vtg(const char *vdomain)
> > > > +{
> > > > +	long ret = 0;
> > > 
> > > Unnecessary initialisation.
> > 
> > No, sorry.
> 
> In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
> 
> 	ret = vtg_get_voltage(v);
> 
> is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
> 
> 	ret = vtg_get_voltage(&vhandle);
> 
> Then "return ret;" follows. I cannot see a path where 
> pre-initialisation of ret does anything useful. If someone removed the
> #else part, the compiler would bark.

Ahha, sorry, I did not look at this kind of context.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| Some nitpicking about the patch follows..
| 
| On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > +static long 
| > +get_vtg(const char *vdomain)
| > +{
| > +	long ret = 0;
| 
| Unnecessary initialisation.
---

Many of us work in environments where initialization is in the coding
standard. It also helps with static analysis tools. CodingStyle seems to
be silent on the point, but points to Kernighan and Ritchie, who say
"These initializations are actually unnecessary, since all are zero, but
it's a good idea to make them explicit anyway."

Any reasonable compiler will avoid doing the initialization twice...

---
| ...
| > +static int cpu_vltg_show(void *md_opt, int *value)
| > +{
| > +	int rc = 0;
| > +	if (md_opt == NULL) {
| > +		if ((*value = get_vtg("v1")) <= 0)
| > +			return -EIO;
| > +	}
| > +	else {
| > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
| > +		*value = opt->cpu_vltg;
| > +	}
| > +
| > +	return rc;
| > +}
| 
| int rc is unnecessary because the function always returns 0. This 
| happens in many places.
---

Wonder if he wrote it for a coding standard that requires single return
(so that the "return -EIO" would have been "rc=-EIO") and converted
it...

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
> It also helps with static analysis tools.

I'd say those analysis tools are pretty useless if they can not handle 
trivial cases like this.

> CodingStyle seems to
> be silent on the point, but points to Kernighan and Ritchie, who say
> "These initializations are actually unnecessary, since all are zero, but
> it's a good idea to make them explicit anyway."

It was a local variable. They are not autoinitialised. Are you perhaps 
mixing this with statics and globals?

- Heikki

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> | Some nitpicking about the patch follows..
> | 
> | On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> | > +static long 
> | > +get_vtg(const char *vdomain)
> | > +{
> | > +	long ret = 0;
> | 
> | Unnecessary initialisation.
> 
> Many of us work in environments where initialization is in the coding
> standard. 

Well, l-k is not _this_ kind of environment.

> | > +static int cpu_vltg_show(void *md_opt, int *value)
> | > +{
> | > +	int rc = 0;
> | > +	if (md_opt == NULL) {
> | > +		if ((*value = get_vtg("v1")) <= 0)
> | > +			return -EIO;
> | > +	}
> | > +	else {
> | > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
> | > +		*value = opt->cpu_vltg;
> | > +	}
> | > +
> | > +	return rc;
> | > +}
> | 
> | int rc is unnecessary because the function always returns 0. This 
> | happens in many places.
> ---
> 
> Wonder if he wrote it for a coding standard that requires single return
> (so that the "return -EIO" would have been "rc=-EIO") and converted
> it...

We sometimes do that in l-k, too. But having rc=-EIO; return rc; with
single return is little extreme. Just fix it, it is easier than
debating codingstyle.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Sun, Oct 01, 2006 at 07:10:32PM +0200, Pavel Machek wrote:
| > On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
| > > Some nitpicking about the patch follows..
| > > 
| > > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > > > +static long 
| > > > +get_vtg(const char *vdomain)
| > > > +{
| > > > +	long ret = 0;
| > > 
| > > Unnecessary initialisation.
| > 
| > No, sorry.
| 
| In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
| 
| 	ret = vtg_get_voltage(v);
| 
| is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
| 
| 	ret = vtg_get_voltage(&vhandle);
| 
| Then "return ret;" follows. I cannot see a path where 
| pre-initialisation of ret does anything useful. If someone removed the
| #else part, the compiler would bark.
---

True, but a good compiler should remove the dead initialization...

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
| > It also helps with static analysis tools.
| 
| I'd say those analysis tools are pretty useless if they can not handle 
| trivial cases like this.
| 
| > CodingStyle seems to
| > be silent on the point, but points to Kernighan and Ritchie, who say
| > "These initializations are actually unnecessary, since all are zero, but
| > it's a good idea to make them explicit anyway."
| 
| It was a local variable. They are not autoinitialised. Are you perhaps 
| mixing this with statics and globals?
---

Indeed - I answered too quickly.

However, as noted, there's no practical cost to including the
initializations and some of us do work in places where it's normallly
required.

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com