diff mbox

[U-Boot,v4,2/2] hwmon: do not init sensors on startup

Message ID 1312196898-20228-1-git-send-email-hs@denx.de
State Superseded
Headers show

Commit Message

Heiko Schocher Aug. 1, 2011, 11:08 a.m. UTC 
The U-Boot Design Principles[1] clearly say:

  Initialize devices only when they are needed within U-Boot, i.e. don't
  initialize the Ethernet interface(s) unless U-Boot performs a download
  over Ethernet; don't initialize any IDE or USB devices unless U-Boot
  actually tries to load files from these, etc. (and don't forget to
  shut down these devices after using them - otherwise nasty things may
  happen when you try to boot your OS).

So, do not initialize and read the sensors on startup.

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Wolfgang Denk <wd@denx.de>
cc: Holger Brunck <holger.brunck@keymile.com>

---
- changes since v1
  add comments from Wolfgang Denk
  use BUILD_BUG_ON to create a compileerror if there are
  defined more than 32 sensors.

- changes since v2
  don;t include genutils.h, as macro is now in common.h

- changes since v3
  do not initialize static sensor_initialized with 0

 arch/powerpc/lib/board.c |    3 ---
 common/cmd_dtt.c         |   16 ++++++++++++++--
 drivers/hwmon/adm1021.c  |   27 +++------------------------
 drivers/hwmon/adt7460.c  |    2 +-
 drivers/hwmon/ds1621.c   |   19 +------------------
 drivers/hwmon/ds1775.c   |   19 +------------------
 drivers/hwmon/lm63.c     |   19 +------------------
 drivers/hwmon/lm73.c     |   20 ++------------------
 drivers/hwmon/lm75.c     |   29 ++---------------------------
 drivers/hwmon/lm81.c     |   21 ++-------------------
 include/dtt.h            |    2 +-
 11 files changed, 28 insertions(+), 149 deletions(-)

Comments

Wolfgang Denk Aug. 1, 2011, 1:28 p.m. UTC | #1 
Dear Heiko Schocher,

In message <1312196898-20228-1-git-send-email-hs@denx.de> you wrote:
> The U-Boot Design Principles[1] clearly say:
> 
>   Initialize devices only when they are needed within U-Boot, i.e. don't
>   initialize the Ethernet interface(s) unless U-Boot performs a download
>   over Ethernet; don't initialize any IDE or USB devices unless U-Boot
>   actually tries to load files from these, etc. (and don't forget to
>   shut down these devices after using them - otherwise nasty things may
>   happen when you try to boot your OS).
> 
> So, do not initialize and read the sensors on startup.
...
> - changes since v3
>   do not initialize static sensor_initialized with 0
...
> +static unsigned long sensor_initialized = 0xffffffff;

Ummm.... no.  You failed to understand what the checkpatch error
means.

There is no need to explicitly static variables with 0, because this
is their default value if you leave them unitialized - unintialized
data is allocated in the BSS segment, and thus get automatically
initialized as 0.

The code above enforces allocation ion the data segment, i. e. it
increses the (flash) memory footprint.

Please undo this.  Just omit the "= 0" par tin the declaration all
together.

Thanks.

Best regards,

Wolfgang Denk
Heiko Schocher Aug. 1, 2011, 1:54 p.m. UTC | #2 
Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <1312196898-20228-1-git-send-email-hs@denx.de> you wrote:
>> The U-Boot Design Principles[1] clearly say:
>>
>>   Initialize devices only when they are needed within U-Boot, i.e. don't
>>   initialize the Ethernet interface(s) unless U-Boot performs a download
>>   over Ethernet; don't initialize any IDE or USB devices unless U-Boot
>>   actually tries to load files from these, etc. (and don't forget to
>>   shut down these devices after using them - otherwise nasty things may
>>   happen when you try to boot your OS).
>>
>> So, do not initialize and read the sensors on startup.
> ...
>> - changes since v3
>>   do not initialize static sensor_initialized with 0
> ...
>> +static unsigned long sensor_initialized = 0xffffffff;
> 
> Ummm.... no.  You failed to understand what the checkpatch error
> means.
> 
> There is no need to explicitly static variables with 0, because this
> is their default value if you leave them unitialized - unintialized
> data is allocated in the BSS segment, and thus get automatically
> initialized as 0.

Ah! Ok, I undo this change.

> The code above enforces allocation ion the data segment, i. e. it
> increses the (flash) memory footprint.
> 
> Please undo this.  Just omit the "= 0" par tin the declaration all
> together.

bye,
Heiko
diff mbox

Patch

diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c
index aaa5add..e84cd52 100644
--- a/arch/powerpc/lib/board.c
+++ b/arch/powerpc/lib/board.c
@@ -946,9 +946,6 @@  void board_init_r (gd_t *id, ulong dest_addr)
 
 	WATCHDOG_RESET ();
 
-#if defined(CONFIG_DTT)		/* Digital Thermometers and Thermostats */
-	dtt_init ();
-#endif
 #if defined(CONFIG_CMD_SCSI)
 	WATCHDOG_RESET ();
 	puts ("SCSI:  ");
diff --git a/common/cmd_dtt.c b/common/cmd_dtt.c
index 3388e43..31aa420 100644
--- a/common/cmd_dtt.c
+++ b/common/cmd_dtt.c
@@ -28,12 +28,16 @@ 
 #include <dtt.h>
 #include <i2c.h>
 
+static unsigned long sensor_initialized = 0xffffffff;
+
 int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
 	unsigned char sensors[] = CONFIG_DTT_SENSORS;
 	int old_bus;
 
+	/* Force a compilation error, if there are more then 32 sensors */
+	BUILD_BUG_ON(sizeof(sensors) > 32);
 	/* switch to correct I2C bus */
 	old_bus = I2C_GET_BUS();
 	I2C_SET_BUS(CONFIG_SYS_DTT_BUS_NUM);
@@ -42,8 +46,16 @@  int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 	 * Loop through sensors, read
 	 * temperature, and output it.
 	 */
-	for (i = 0; i < sizeof (sensors); i++)
-		printf ("DTT%d: %i C\n", i + 1, dtt_get_temp (sensors[i]));
+	for (i = 0; i < sizeof(sensors); i++) {
+		if ((sensor_initialized & (1 << i)) != 0) {
+			if (dtt_init_one(sensors[i]) != 0) {
+				printf("DTT%d: Failed init!\n", i);
+				continue;
+			}
+			sensor_initialized &= ~(1 << i);
+		}
+		printf("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
+	}
 
 	/* switch back to original I2C bus */
 	I2C_SET_BUS(old_bus);
diff --git a/drivers/hwmon/adm1021.c b/drivers/hwmon/adm1021.c
index d753e9a..d074cb7 100644
--- a/drivers/hwmon/adm1021.c
+++ b/drivers/hwmon/adm1021.c
@@ -109,8 +109,8 @@  dtt_write (int sensor, int reg, int val)
 	return 0;
 } /* dtt_write() */
 
-static int
-_dtt_init (int sensor)
+int
+dtt_init_one(int sensor)
 {
 	dtt_cfg_t *dcp = &dttcfg[sensor >> 1];
 	int reg, val;
@@ -164,28 +164,7 @@  _dtt_init (int sensor)
 		return 1;
 
 	return 0;
-} /* _dtt_init() */
-
-int
-dtt_init (void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-	const char *const header = "DTT:   ";
-
-	/* switch to correct I2C bus */
-	I2C_SET_BUS(CONFIG_SYS_DTT_BUS_NUM);
-
-	for (i = 0; i < sizeof(sensors); i++) {
-		if (_dtt_init(sensors[i]) != 0)
-			printf ("%s%d FAILED INIT\n", header, i+1);
-		else
-			printf ("%s%d is %i C\n", header, i+1,
-				dtt_get_temp(sensors[i]));
-	}
-
-	return (0);
-} /* dtt_init() */
+} /* dtt_init_one() */
 
 int
 dtt_get_temp (int sensor)
diff --git a/drivers/hwmon/adt7460.c b/drivers/hwmon/adt7460.c
index caef70a..b7e36fe 100644
--- a/drivers/hwmon/adt7460.c
+++ b/drivers/hwmon/adt7460.c
@@ -50,7 +50,7 @@  int dtt_write(int sensor, int reg, int val)
 	return 0;
 }
 
-int dtt_init(void)
+int dtt_init_one(int sensor)
 {
 	printf("ADT7460 at I2C address 0x%2x\n", ADT7460_ADDRESS);
 
diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index 5a2ea62..4e1db6d 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -126,7 +126,7 @@  int dtt_write(int sensor, int reg, int val)
 }
 
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int val;
 
@@ -155,23 +155,6 @@  static int _dtt_init(int sensor)
 	return 0;
 }
 
-
-int dtt_init (void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-
-	for (i = 0; i < sizeof(sensors); i++) {
-		if (_dtt_init(sensors[i]) != 0)
-			printf("DTT%d:  FAILED\n", i + 1);
-		else
-			printf("DTT%d:  %i C\n", i + 1, dtt_get_temp(sensors[i]));
-	}
-
-	return (0);
-}
-
-
 int dtt_get_temp(int sensor)
 {
 	int i;
diff --git a/drivers/hwmon/ds1775.c b/drivers/hwmon/ds1775.c
index 80fb26f..feabdf2 100644
--- a/drivers/hwmon/ds1775.c
+++ b/drivers/hwmon/ds1775.c
@@ -98,7 +98,7 @@  int dtt_write(int sensor, int reg, int val)
 }
 
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int val;
 
@@ -133,23 +133,6 @@  static int _dtt_init(int sensor)
 	return 0;
 }
 
-
-int dtt_init (void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-
-	for (i = 0; i < sizeof(sensors); i++) {
-		if (_dtt_init(sensors[i]) != 0)
-			printf("DTT%d:  FAILED\n", i+1);
-		else
-			printf("DTT%d:  %i C\n", i+1, dtt_get_temp(sensors[i]));
-	}
-
-	return (0);
-}
-
-
 int dtt_get_temp(int sensor)
 {
 	return (dtt_read(sensor, DTT_READ_TEMP) / 256);
diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 2f1f3cf..f3adf64 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -101,7 +101,7 @@  static int is_lm64(int sensor)
 	return sensor && (sensor != DTT_I2C_LM63_ADDR);
 }
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int i;
 	int val;
@@ -175,20 +175,3 @@  int dtt_get_temp(int sensor)
 	/* Ignore LSB for now, U-Boot only prints natural numbers */
 	return temp >> 8;
 }
-
-int dtt_init(void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-	const char *const header = "DTT:   ";
-
-	for (i = 0; i < sizeof(sensors); i++) {
-		if (_dtt_init(sensors[i]) != 0)
-			printf("%s%d FAILED INIT\n", header, i + 1);
-		else
-			printf("%s%d is %i C\n", header, i + 1,
-			       dtt_get_temp(sensors[i]));
-	}
-
-	return 0;
-}
diff --git a/drivers/hwmon/lm73.c b/drivers/hwmon/lm73.c
index 7b5d893..45cc6db 100644
--- a/drivers/hwmon/lm73.c
+++ b/drivers/hwmon/lm73.c
@@ -112,7 +112,7 @@  int dtt_write(int const sensor, int const reg, int const val)
 			      dlen);
 } /* dtt_write() */
 
-static int _dtt_init(int const sensor)
+int dtt_init_one(int const sensor)
 {
 	int val;
 
@@ -148,23 +148,7 @@  static int _dtt_init(int const sensor)
 
 	dtt_read(sensor, DTT_CONTROL);	/* clear temperature flags */
 	return 0;
-} /* _dtt_init() */
-
-int dtt_init(void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-	const char *const header = "DTT:   ";
-
-	for (i = 0; i < sizeof(sensors); i++) {
-		if (0 != _dtt_init(sensors[i]))
-			printf("%s%d FAILED INIT\n", header, i + 1);
-		else
-			printf("%s%d is %i C\n", header, i + 1,
-			       dtt_get_temp(sensors[i]));
-	}
-	return 0;
-} /* dtt_init() */
+} /* dtt_init_one() */
 
 int dtt_get_temp(int const sensor)
 {
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 8119821..29c1a39 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -119,7 +119,7 @@  int dtt_write(int sensor, int reg, int val)
 } /* dtt_write() */
 
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int val;
 
@@ -145,32 +145,7 @@  static int _dtt_init(int sensor)
 		return 1;
 
 	return 0;
-} /* _dtt_init() */
-
-
-int dtt_init (void)
-{
-	int i;
-	unsigned char sensors[] = CONFIG_DTT_SENSORS;
-	const char *const header = "DTT:   ";
-	int old_bus;
-
-	/* switch to correct I2C bus */
-	old_bus = I2C_GET_BUS();
-	I2C_SET_BUS(CONFIG_SYS_DTT_BUS_NUM);
-
-	for (i = 0; i < sizeof(sensors); i++) {
-	if (_dtt_init(sensors[i]) != 0)
-		printf("%s%d FAILED INIT\n", header, i+1);
-	else
-		printf("%s%d is %i C\n", header, i+1,
-		dtt_get_temp(sensors[i]));
-	}
-	/* switch back to original I2C bus */
-	I2C_SET_BUS(old_bus);
-
-	return (0);
-} /* dtt_init() */
+} /* dtt_init_one() */
 
 int dtt_get_temp(int sensor)
 {
diff --git a/drivers/hwmon/lm81.c b/drivers/hwmon/lm81.c
index afe5b0d..f1572ba 100644
--- a/drivers/hwmon/lm81.c
+++ b/drivers/hwmon/lm81.c
@@ -89,7 +89,7 @@  int dtt_write(int sensor, int reg, int val)
 #define DTT_CONFIG	0x40
 #define DTT_ADR		0x48
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int	man;
 	int	adr;
@@ -111,26 +111,9 @@  static int _dtt_init(int sensor)
 
 	debug ("DTT:   Found LM81@%x Rev: %d\n", adr, rev);
 	return 0;
-} /* _dtt_init() */
+} /* dtt_init_one() */
 
 
-int dtt_init (void)
-{
-    int i;
-    unsigned char sensors[] = CONFIG_DTT_SENSORS;
-    const char *const header = "DTT:   ";
-
-    for (i = 0; i < sizeof(sensors); i++) {
-	if (_dtt_init(sensors[i]) != 0)
-	    printf("%s%d FAILED INIT\n", header, i+1);
-	else
-	    printf("%s%d is %i C\n", header, i+1,
-		   dtt_get_temp(sensors[i]));
-    }
-
-    return (0);
-} /* dtt_init() */
-
 #define TEMP_FROM_REG(temp) \
    ((temp)<256?((((temp)&0x1fe) >> 1) * 10)	 + ((temp) & 1) * 5:  \
 	       ((((temp)&0x1fe) >> 1) -255) * 10 - ((temp) & 1) * 5)  \
diff --git a/include/dtt.h b/include/dtt.h
index 399b64a..9e6c08c 100644
--- a/include/dtt.h
+++ b/include/dtt.h
@@ -52,7 +52,7 @@ 
 #endif
 #endif /* CONFIG_DTT_ADM1021 */
 
-extern int dtt_init (void);
+extern int dtt_init_one(int);
 extern int dtt_read(int sensor, int reg);
 extern int dtt_write(int sensor, int reg, int val);
 extern int dtt_get_temp(int sensor);