[PATCH] hwmon: Add basic support for lm64 to lm63.c

[PATCH] hwmon: Add basic support for lm64 to lm63.c

[Matthew Garrett]

The lm64 appears to be an lm63 with added gpio lines. Add support for the
hwmon functionality - gpio can be added at some later stage if someone
has a need for them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 Documentation/hwmon/lm63 |    7 +++++++
 drivers/hwmon/lm63.c     |   19 +++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/lm63 b/Documentation/hwmon/lm63
index 31660bf..b6f0495 100644
--- a/Documentation/hwmon/lm63
+++ b/Documentation/hwmon/lm63
@@ -7,6 +7,11 @@ Supported chips:
     Addresses scanned: I2C 0x4c
     Datasheet: Publicly available at the National Semiconductor website
                http://www.national.com/pf/LM/LM63.html
+  * National Semiconductor LM64
+    Prefix: 'lm64'
+    Addresses scanned: I2C 0x18 and 0x4e
+    Datasheet: Publicly available at the National Semiconductor website
+               http://www.national.com/pf/LM/LM64.html
 
 Author: Jean Delvare <khali@linux-fr.org>
 
@@ -55,3 +60,5 @@ The lm63 driver will not update its values more frequently than every
 second; reading them more often will do no harm, but will return 'old'
 values.
 
+The lm64 is effectively an lm63 with gpio lines. The driver does not
+support these gpio lines at present.
\ No newline at end of file
diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index bf81aff..01b23ef 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -53,7 +53,7 @@
  * Address is fully defined internally and cannot be changed.
  */
 
-static const unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
+static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 
 /*
  * The LM63 registers
@@ -131,12 +131,15 @@ static struct lm63_data *lm63_update_device(struct device *dev);
 static int lm63_detect(struct i2c_client *client, struct i2c_board_info *info);
 static void lm63_init_client(struct i2c_client *client);
 
+enum chips { lm63, lm64 };
+
 /*
  * Driver data (common to all clients)
  */
 
 static const struct i2c_device_id lm63_id[] = {
-	{ "lm63", 0 },
+	{ "lm63", lm63 },
+	{ "lm64", lm64 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, lm63_id);
@@ -177,6 +180,7 @@ struct lm63_data {
 			   2: remote high limit */
 	u8 temp2_crit_hyst;
 	u8 alarms;
+	int kind;
 };
 
 /*
@@ -422,6 +426,7 @@ static int lm63_detect(struct i2c_client *new_client,
 	struct i2c_adapter *adapter = new_client->adapter;
 	u8 man_id, chip_id, reg_config1, reg_config2;
 	u8 reg_alert_status, reg_alert_mask;
+	int address = new_client->addr;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
@@ -439,7 +444,6 @@ static int lm63_detect(struct i2c_client *new_client,
 			 LM63_REG_ALERT_MASK);
 
 	if (man_id != 0x01 /* National Semiconductor */
-	 || chip_id != 0x41 /* LM63 */
 	 || (reg_config1 & 0x18) != 0x00
 	 || (reg_config2 & 0xF8) != 0x00
 	 || (reg_alert_status & 0x20) != 0x00
@@ -450,7 +454,12 @@ static int lm63_detect(struct i2c_client *new_client,
 		return -ENODEV;
 	}
 
-	strlcpy(info->type, "lm63", I2C_NAME_SIZE);
+	if (chip_id == 0x41 && address == 0x4c)
+		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
+	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
+		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
+	else
+		return -ENODEV;
 
 	return 0;
 }
@@ -471,6 +480,8 @@ static int lm63_probe(struct i2c_client *new_client,
 	data->valid = 0;
 	mutex_init(&data->update_lock);
 
+	data->kind = id->driver_data;
+
 	/* Initialize the LM63 chip */
 	lm63_init_client(new_client);
 
-- 
1.7.0.1

Re: [lm-sensors] [PATCH] hwmon: Add basic support for lm64 to lm63.c

[Jean Delvare]

Hi Matthew,

On Wed, 17 Mar 2010 15:55:22 -0400, Matthew Garrett wrote:
> The lm64 appears to be an lm63 with added gpio lines. Add support for the
> hwmon functionality - gpio can be added at some later stage if someone
> has a need for them.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  Documentation/hwmon/lm63 |    7 +++++++
>  drivers/hwmon/lm63.c     |   19 +++++++++++++++----
>  2 files changed, 22 insertions(+), 4 deletions(-)

Can you please send me a dump of your LM64 chip?

Review:

> 
> diff --git a/Documentation/hwmon/lm63 b/Documentation/hwmon/lm63
> index 31660bf..b6f0495 100644
> --- a/Documentation/hwmon/lm63
> +++ b/Documentation/hwmon/lm63
> @@ -7,6 +7,11 @@ Supported chips:
>      Addresses scanned: I2C 0x4c
>      Datasheet: Publicly available at the National Semiconductor website
>                 http://www.national.com/pf/LM/LM63.html
> +  * National Semiconductor LM64
> +    Prefix: 'lm64'
> +    Addresses scanned: I2C 0x18 and 0x4e
> +    Datasheet: Publicly available at the National Semiconductor website
> +               http://www.national.com/pf/LM/LM64.html
>  
>  Author: Jean Delvare <khali@linux-fr.org>
>  
> @@ -55,3 +60,5 @@ The lm63 driver will not update its values more frequently than every
>  second; reading them more often will do no harm, but will return 'old'
>  values.
>  
> +The lm64 is effectively an lm63 with gpio lines. The driver does not
> +support these gpio lines at present.

Please use "LM64" and "LM63" when referring to the devices themselves.
And spell GPIO with capitals, too. Same applies to the patch
description, BTW.

> \ No newline at end of file

Please add the missing newline.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index bf81aff..01b23ef 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -53,7 +53,7 @@
>   * Address is fully defined internally and cannot be changed.
>   */
>  
> -static const unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
> +static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  
>  /*
>   * The LM63 registers
> @@ -131,12 +131,15 @@ static struct lm63_data *lm63_update_device(struct device *dev);
>  static int lm63_detect(struct i2c_client *client, struct i2c_board_info *info);
>  static void lm63_init_client(struct i2c_client *client);
>  
> +enum chips { lm63, lm64 };
> +
>  /*
>   * Driver data (common to all clients)
>   */
>  
>  static const struct i2c_device_id lm63_id[] = {
> -	{ "lm63", 0 },
> +	{ "lm63", lm63 },
> +	{ "lm64", lm64 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, lm63_id);
> @@ -177,6 +180,7 @@ struct lm63_data {
>  			   2: remote high limit */
>  	u8 temp2_crit_hyst;
>  	u8 alarms;
> +	int kind;
>  };
>  
>  /*
> @@ -422,6 +426,7 @@ static int lm63_detect(struct i2c_client *new_client,
>  	struct i2c_adapter *adapter = new_client->adapter;
>  	u8 man_id, chip_id, reg_config1, reg_config2;
>  	u8 reg_alert_status, reg_alert_mask;
> +	int address = new_client->addr;
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>  		return -ENODEV;
> @@ -439,7 +444,6 @@ static int lm63_detect(struct i2c_client *new_client,
>  			 LM63_REG_ALERT_MASK);
>  
>  	if (man_id != 0x01 /* National Semiconductor */
> -	 || chip_id != 0x41 /* LM63 */
>  	 || (reg_config1 & 0x18) != 0x00
>  	 || (reg_config2 & 0xF8) != 0x00
>  	 || (reg_alert_status & 0x20) != 0x00
> @@ -450,7 +454,12 @@ static int lm63_detect(struct i2c_client *new_client,
>  		return -ENODEV;
>  	}
>  
> -	strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> +	if (chip_id == 0x41 && address == 0x4c)
> +		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
> +	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
> +		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
> +	else
> +		return -ENODEV;
>  
>  	return 0;
>  }
> @@ -471,6 +480,8 @@ static int lm63_probe(struct i2c_client *new_client,
>  	data->valid = 0;
>  	mutex_init(&data->update_lock);
>  
> +	data->kind = id->driver_data;

You don't use this value anywhere, so why bother storing it?

> +
>  	/* Initialize the LM63 chip */
>  	lm63_init_client(new_client);
>  

Your patch is missing an update to drivers/hwmon/Kconfig.

Please submit an updated patch and I'll apply it.

-- 
Jean Delvare

[PATCH] hwmon: Add basic support for LM64 to lm63.c

[Matthew Garrett]

The LM64 appears to be an LM63 with added GPIO lines. Add support for the
hwmon functionality - GPIO can be added at some later stage if someone
has a need for them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---

Capitalisation fixes and remove an unused variable.

 Documentation/hwmon/lm63 |    7 +++++++
 drivers/hwmon/Kconfig    |    9 +++++----
 drivers/hwmon/lm63.c     |   16 ++++++++++++----
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/Documentation/hwmon/lm63 b/Documentation/hwmon/lm63
index 31660bf..b9843ea 100644
--- a/Documentation/hwmon/lm63
+++ b/Documentation/hwmon/lm63
@@ -7,6 +7,11 @@ Supported chips:
     Addresses scanned: I2C 0x4c
     Datasheet: Publicly available at the National Semiconductor website
                http://www.national.com/pf/LM/LM63.html
+  * National Semiconductor LM64
+    Prefix: 'lm64'
+    Addresses scanned: I2C 0x18 and 0x4e
+    Datasheet: Publicly available at the National Semiconductor website
+               http://www.national.com/pf/LM/LM64.html
 
 Author: Jean Delvare <khali@linux-fr.org>
 
@@ -55,3 +60,5 @@ The lm63 driver will not update its values more frequently than every
 second; reading them more often will do no harm, but will return 'old'
 values.
 
+The LM64 is effectively an LM63 with GPIO lines. The driver does not
+support these GPIO lines at present.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e4595e6..1aca601 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -450,10 +450,11 @@ config SENSORS_LM63
 	tristate "National Semiconductor LM63"
 	depends on I2C
 	help
-	  If you say yes here you get support for the National Semiconductor
-	  LM63 remote diode digital temperature sensor with integrated fan
-	  control.  Such chips are found on the Tyan S4882 (Thunder K8QS Pro)
-	  motherboard, among others.
+	  If you say yes here you get support for the National
+	  Semiconductor LM63 and LM64 remote diode digital temperature
+	  sensors with integrated fan control.  Such chips are found
+	  on the Tyan S4882 (Thunder K8QS Pro) motherboard, among
+	  others.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm63.
diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index bf81aff..776aeb3 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -53,7 +53,7 @@
  * Address is fully defined internally and cannot be changed.
  */
 
-static const unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
+static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 
 /*
  * The LM63 registers
@@ -131,12 +131,15 @@ static struct lm63_data *lm63_update_device(struct device *dev);
 static int lm63_detect(struct i2c_client *client, struct i2c_board_info *info);
 static void lm63_init_client(struct i2c_client *client);
 
+enum chips { lm63, lm64 };
+
 /*
  * Driver data (common to all clients)
  */
 
 static const struct i2c_device_id lm63_id[] = {
-	{ "lm63", 0 },
+	{ "lm63", lm63 },
+	{ "lm64", lm64 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, lm63_id);
@@ -422,6 +425,7 @@ static int lm63_detect(struct i2c_client *new_client,
 	struct i2c_adapter *adapter = new_client->adapter;
 	u8 man_id, chip_id, reg_config1, reg_config2;
 	u8 reg_alert_status, reg_alert_mask;
+	int address = new_client->addr;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
@@ -439,7 +443,6 @@ static int lm63_detect(struct i2c_client *new_client,
 			 LM63_REG_ALERT_MASK);
 
 	if (man_id != 0x01 /* National Semiconductor */
-	 || chip_id != 0x41 /* LM63 */
 	 || (reg_config1 & 0x18) != 0x00
 	 || (reg_config2 & 0xF8) != 0x00
 	 || (reg_alert_status & 0x20) != 0x00
@@ -450,7 +453,12 @@ static int lm63_detect(struct i2c_client *new_client,
 		return -ENODEV;
 	}
 
-	strlcpy(info->type, "lm63", I2C_NAME_SIZE);
+	if (chip_id == 0x41 && address == 0x4c)
+		strlcpy(info->type, "lm63", I2C_NAME_SIZE);
+	else if (chip_id == 0x51 && (address == 0x18 || address == 0x4e))
+		strlcpy(info->type, "lm64", I2C_NAME_SIZE);
+	else
+		return -ENODEV;
 
 	return 0;
 }
-- 
1.7.0.1

Re: [lm-sensors] [PATCH] hwmon: Add basic support for lm64 to lm63.c

[Matthew Garrett]

Here's the dump from the lm64.

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 34 3e 00 03 08 46 00 46 00 03 08 46 00 46 00 00    4>.??F.F.??F.F..
10: 60 10 00 00 00 00 e5 00 00 7b 1f 1f 00 00 00 00    `?....?..{??....
20: 00 64 00 00 00 00 00 00 00 00 00 00 00 00 00 00    .d..............
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
40: 00 00 00 00 00 00 af 0f ff ff 00 00 05 08 00 05    ......??....??.?
50: 00 04 32 05 46 06 5a 07 5f 0b 64 0d 69 10 69 10    .?2?F?Z?_?d?i?i?
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
b0: c0 20 2a d1 19 01 0d 00 00 00 00 00 00 00 00 00    ? *????.........
c0: 00 00 00 00 00 00 00 27 00 00 00 00 00 00 00 02    .......'.......?
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 51    ..............?Q

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] hwmon: Add basic support for LM64 to lm63.c

[Jean Delvare]

On Thu, 18 Mar 2010 09:36:59 -0400, Matthew Garrett wrote:
> The LM64 appears to be an LM63 with added GPIO lines. Add support for the
> hwmon functionality - GPIO can be added at some later stage if someone
> has a need for them.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
> 
> Capitalisation fixes and remove an unused variable.

Applied, thanks!

-- 
Jean Delvare