diff mbox

[U-Boot,NEXT] hwmon: do not init sensors on startup

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

Commit Message

Heiko Schocher Dec. 9, 2010, 2 p.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>
---
 arch/powerpc/lib/board.c |    3 ---
 common/cmd_dtt.c         |   14 ++++++++++++--
 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, 26 insertions(+), 149 deletions(-)

Comments

Wolfgang Denk Dec. 9, 2010, 2:26 p.m. UTC | #1 
Dear Heiko Schocher,

In message <1291903238-29071-1-git-send-email-hs@denx.de> you wrote:
>
> --- a/common/cmd_dtt.c
> +++ b/common/cmd_dtt.c
> @@ -28,6 +28,8 @@
>  #include <dtt.h>
>  #include <i2c.h>
>  
> +static unsigned long sensors_init_done = 0;

What if there are more then 32 sensors?

>  int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  {
>  	int i;
> @@ -42,8 +44,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 ((sensors_init_done & (1 << i)) != (1 << i)) {
> +			if (dtt_init_one(sensors[i]) == 0)
> +				sensors_init_done |= (1 << i);
> +			else
> +				printf("DTT%d: Failed init!\n", i);
> +		}
> +		if ((sensors_init_done & (1 << i)) == (1 << i))
> +			printf ("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
> +	}

This is overly complicated, it seems.  Why not:

	for (i = 0; i < sizeof(sensors); i++) {
		if ((sensors_init_done & (1 << i)) == 0) {
			if (dtt_init_one(sensors[i]) != 0) {
				printf("DTT%d: init failed\n", i);
				continue;
			}
			sensors_init_done |= (1 << i);
		}

		printf("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
	}


Best regards,

Wolfgang Denk
Heiko Schocher Dec. 9, 2010, 3:45 p.m. UTC | #2 
Hello Wolfgang,

Wolfgang Denk wrote:
> In message <1291903238-29071-1-git-send-email-hs@denx.de> you wrote:
>> --- a/common/cmd_dtt.c
>> +++ b/common/cmd_dtt.c
>> @@ -28,6 +28,8 @@
>>  #include <dtt.h>
>>  #include <i2c.h>
>>  
>> +static unsigned long sensors_init_done = 0;
> 
> What if there are more then 32 sensors?

Ok, that would not work ... Hmm.. I can get the number of DTTs on a board
with ARRAY_SIZE(CONFIG_DTT_SENSORS) ... should I use this info to allocate
an array, which stores the info, if the DTTs are initialized?

>>  int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>  {
>>  	int i;
>> @@ -42,8 +44,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 ((sensors_init_done & (1 << i)) != (1 << i)) {
>> +			if (dtt_init_one(sensors[i]) == 0)
>> +				sensors_init_done |= (1 << i);
>> +			else
>> +				printf("DTT%d: Failed init!\n", i);
>> +		}
>> +		if ((sensors_init_done & (1 << i)) == (1 << i))
>> +			printf ("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
>> +	}
> 
> This is overly complicated, it seems.  Why not:
> 
> 	for (i = 0; i < sizeof(sensors); i++) {
> 		if ((sensors_init_done & (1 << i)) == 0) {
> 			if (dtt_init_one(sensors[i]) != 0) {
> 				printf("DTT%d: init failed\n", i);
> 				continue;
> 			}
> 			sensors_init_done |= (1 << i);
> 		}
> 
> 		printf("DTT%d: %i C\n", i + 1, dtt_get_temp(sensors[i]));
> 	}

Yep, that looks better, change this in v2.

bye,
Heiko
Wolfgang Denk Dec. 9, 2010, 6:50 p.m. UTC | #3 
Dear Heiko Schocher,

In message <4D00F98B.1010507@denx.de> you wrote:
> 
> >> +static unsigned long sensors_init_done = 0;
> > 
> > What if there are more then 32 sensors?
> 
> Ok, that would not work ... Hmm.. I can get the number of DTTs on a board
> with ARRAY_SIZE(CONFIG_DTT_SENSORS) ... should I use this info to allocate
> an array, which stores the info, if the DTTs are initialized?

No - it's only a theoretical case. But I think you should add a
compile-time test to catch such errors.


Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c
index b21c1d6..f9412a9 100644
--- a/arch/powerpc/lib/board.c
+++ b/arch/powerpc/lib/board.c
@@ -917,9 +917,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..37ce4df 100644
--- a/common/cmd_dtt.c
+++ b/common/cmd_dtt.c
@@ -28,6 +28,8 @@ 
 #include <dtt.h>
 #include <i2c.h>
 
+static unsigned long sensors_init_done = 0;
+
 int do_dtt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
@@ -42,8 +44,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 ((sensors_init_done & (1 << i)) != (1 << i)) {
+			if (dtt_init_one(sensors[i]) == 0)
+				sensors_init_done |= (1 << i);
+			else
+				printf("DTT%d: Failed init!\n", i);
+		}
+		if ((sensors_init_done & (1 << i)) == (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..3ef80af 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 03616e1..2e49258 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -93,7 +93,7 @@  int dtt_write(int sensor, int reg, int val)
 	return 0;
 }				/* dtt_write() */
 
-static int _dtt_init(int sensor)
+int dtt_init_one(int sensor)
 {
 	int i;
 	int val;
@@ -155,20 +155,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..bbaa033 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);