* [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-05-19 14:10 ` sdevrien
0 siblings, 0 replies; 15+ messages in thread
From: sdevrien @ 2011-05-19 14:10 UTC (permalink / raw)
To: khali, lm-sensors, linux-kernel; +Cc: Stijn Devriendt
From: Stijn Devriendt <sdevrien@cisco.com>
Add support for the Philips SA56004, an LM86
compatible temperature sensor.
Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
---
drivers/hwmon/Kconfig | 2 +-
drivers/hwmon/lm90.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 50e40db..11c9339 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -620,7 +620,7 @@ config SENSORS_LM90
LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor NCT1008,
- and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
+ Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor chips.
This driver can also be built as a module. If so, the module
will be called lm90.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2f94f95..2910791 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -54,6 +54,9 @@
* and extended mode. They are mostly compatible with LM90 except for a data
* format difference for the temperature value registers.
*
+ * This driver also supports the SA56004 from Philips. This device is
+ * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
+ *
* Since the LM90 was the first chipset supported by this driver, most
* comments will refer to this chipset, but are actually general and
* concern all supported chipsets, unless mentioned otherwise.
@@ -96,13 +99,15 @@
* MAX6659 can have address 0x4c, 0x4d or 0x4e.
* MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
* 0x4c, 0x4d or 0x4e.
+ * SA56004 can have address 0x48 through 0x4F.
*/
static const unsigned short normal_i2c[] = {
- 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
+ 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
+ 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
- max6646, w83l771, max6696 };
+ max6646, w83l771, max6696, sa56004 };
/*
* The LM90 registers
@@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
#define MAX6659_REG_R_LOCAL_EMERG 0x17
#define MAX6659_REG_W_LOCAL_EMERG 0x17
+/* SA56004 registers */
+
+#define SA56004_REG_R_LOCAL_TEMPL 0x22
+
#define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register value */
#define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
@@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
{ "max6696", max6696 },
{ "nct1008", adt7461 },
{ "w83l771", w83l771 },
+ { "sa56004", sa56004 },
{ }
};
MODULE_DEVICE_TABLE(i2c, lm90_id);
@@ -204,6 +214,8 @@ struct lm90_params {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate register value */
+ u8 local_ext_offset; /* Local extension register if
+ LM90_HAVE_LOCAL_EXT is set*/
};
static const struct lm90_params lm90_params[] = {
@@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT,
.alert_alarms = 0x7c,
.max_convrate = 6,
+ .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6657] = {
.flags = LM90_HAVE_LOCAL_EXT,
@@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
.alert_alarms = 0x7c,
.max_convrate = 8,
+ .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6680] = {
.flags = LM90_HAVE_OFFSET,
@@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
| LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
.alert_alarms = 0x187c,
.max_convrate = 6,
+ .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
},
[w83l771] = {
.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
.alert_alarms = 0x7c,
.max_convrate = 8,
},
+ [sa56004] = {
+ .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
+ | LM90_HAVE_LOCAL_EXT,
+ .alert_alarms = 0x7b,
+ .max_convrate = 9,
+ .local_ext_offset = SA56004_REG_R_LOCAL_TEMPL,
+ },
};
/*
@@ -286,6 +308,7 @@ struct lm90_data {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate */
+ u8 local_ext_offset; /* local extension register offset */
/* registers values */
s8 temp8[8]; /* 0: local low limit
@@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (data->flags & LM90_HAVE_LOCAL_EXT) {
lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
- MAX6657_REG_R_LOCAL_TEMPL,
+ data->local_ext_offset,
&data->temp11[4]);
} else {
if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
@@ -1263,6 +1286,12 @@ static int lm90_detect(struct i2c_client *new_client,
name = "w83l771";
}
}
+ } else
+ if (man_id == 0xA1) { /* NXP Semiconductor/Philips */
+ if (chip_id == 0x00
+ && address >= 0x48 && address <= 0x4F) {
+ name = "sa56004";
+ }
}
if (!name) { /* identification failed */
@@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client *new_client,
/* Set maximum conversion rate */
data->max_convrate = lm90_params[data->kind].max_convrate;
+ if (data->flags & LM90_HAVE_LOCAL_EXT) {
+ if (lm90_params[data->kind].local_ext_offset > 0)
+ data->local_ext_offset =
+ lm90_params[data->kind].local_ext_offset;
+ else {
+ dev_err(&new_client->dev,
+ "Invalid temperature extension register. "
+ "Accuracy may be limited.\n");
+ data->flags &= (~LM90_HAVE_LOCAL_EXT);
+ }
+ }
+
/* Initialize the LM90 chip */
lm90_init_client(new_client);
--
1.7.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-05-19 14:10 ` [PATCH] Add support for the Philips SA56004 temperature sensor sdevrien
@ 2011-05-23 4:37 ` Guenter Roeck
-1 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-05-23 4:37 UTC (permalink / raw)
To: sdevrien@cisco.com
Cc: khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Thu, May 19, 2011 at 10:10:53AM -0400, sdevrien@cisco.com wrote:
> From: Stijn Devriendt <sdevrien@cisco.com>
>
> Add support for the Philips SA56004, an LM86
> compatible temperature sensor.
>
> Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
> ---
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/lm90.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 50e40db..11c9339 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -620,7 +620,7 @@ config SENSORS_LM90
> LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
> Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
> MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor NCT1008,
> - and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
> + Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor chips.
>
Please also update Documentation/hwmon/lm90.
> This driver can also be built as a module. If so, the module
> will be called lm90.
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2f94f95..2910791 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -54,6 +54,9 @@
> * and extended mode. They are mostly compatible with LM90 except for a data
> * format difference for the temperature value registers.
> *
> + * This driver also supports the SA56004 from Philips. This device is
> + * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
> + *
> * Since the LM90 was the first chipset supported by this driver, most
> * comments will refer to this chipset, but are actually general and
> * concern all supported chipsets, unless mentioned otherwise.
> @@ -96,13 +99,15 @@
> * MAX6659 can have address 0x4c, 0x4d or 0x4e.
> * MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
> * 0x4c, 0x4d or 0x4e.
> + * SA56004 can have address 0x48 through 0x4F.
> */
>
> static const unsigned short normal_i2c[] = {
> - 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
> + 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
> + 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
>
> enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> - max6646, w83l771, max6696 };
> + max6646, w83l771, max6696, sa56004 };
>
> /*
> * The LM90 registers
> @@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> #define MAX6659_REG_R_LOCAL_EMERG 0x17
> #define MAX6659_REG_W_LOCAL_EMERG 0x17
>
> +/* SA56004 registers */
> +
> +#define SA56004_REG_R_LOCAL_TEMPL 0x22
> +
> #define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register value */
> #define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
>
> @@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
> { "max6696", max6696 },
> { "nct1008", adt7461 },
> { "w83l771", w83l771 },
> + { "sa56004", sa56004 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, lm90_id);
> @@ -204,6 +214,8 @@ struct lm90_params {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate register value */
> + u8 local_ext_offset; /* Local extension register if
> + LM90_HAVE_LOCAL_EXT is set*/
I really dislike this variable name. "offset" is used as "temperature offset"
in the driver, and means a temperature offset, not a register offset (from 0).
Something like reg_local_ext or reg_local_templ would be better.
> };
>
> static const struct lm90_params lm90_params[] = {
> @@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 6,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6657] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> @@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6680] = {
> .flags = LM90_HAVE_OFFSET,
> @@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
> | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
> .alert_alarms = 0x187c,
> .max_convrate = 6,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [w83l771] = {
> .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> },
> + [sa56004] = {
> + .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
> + | LM90_HAVE_LOCAL_EXT,
> + .alert_alarms = 0x7b,
> + .max_convrate = 9,
> + .local_ext_offset = SA56004_REG_R_LOCAL_TEMPL,
> + },
> };
>
> /*
> @@ -286,6 +308,7 @@ struct lm90_data {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate */
> + u8 local_ext_offset; /* local extension register offset */
>
Same here (variable name).
> /* registers values */
> s8 temp8[8]; /* 0: local low limit
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
> if (data->flags & LM90_HAVE_LOCAL_EXT) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> - MAX6657_REG_R_LOCAL_TEMPL,
> + data->local_ext_offset,
> &data->temp11[4]);
> } else {
> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1263,6 +1286,12 @@ static int lm90_detect(struct i2c_client *new_client,
> name = "w83l771";
> }
> }
> + } else
> + if (man_id = 0xA1) { /* NXP Semiconductor/Philips */
> + if (chip_id = 0x00
> + && address >= 0x48 && address <= 0x4F) {
This multi-line split is unnecessary.
> + name = "sa56004";
> + }
> }
>
> if (!name) { /* identification failed */
> @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client *new_client,
> /* Set maximum conversion rate */
> data->max_convrate = lm90_params[data->kind].max_convrate;
>
> + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> + if (lm90_params[data->kind].local_ext_offset > 0)
> + data->local_ext_offset =
> + lm90_params[data->kind].local_ext_offset;
> + else {
> + dev_err(&new_client->dev,
> + "Invalid temperature extension register. "
> + "Accuracy may be limited.\n");
> + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> + }
Either { } in both branches of the if statement, or none.
( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset isn't.
That should be found during coding (or code review), and not be exported
to the user. So, from my perspective, the check is unnecessary. I'll leave
that up to Jean to decide, though.
In addition to the above, your patch generates several checkpatch errors
(trailing whitespace). Please fix.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-05-23 4:37 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-05-23 4:37 UTC (permalink / raw)
To: sdevrien@cisco.com
Cc: khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Thu, May 19, 2011 at 10:10:53AM -0400, sdevrien@cisco.com wrote:
> From: Stijn Devriendt <sdevrien@cisco.com>
>
> Add support for the Philips SA56004, an LM86
> compatible temperature sensor.
>
> Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
> ---
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/lm90.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 50e40db..11c9339 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -620,7 +620,7 @@ config SENSORS_LM90
> LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
> Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
> MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor NCT1008,
> - and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
> + Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor chips.
>
Please also update Documentation/hwmon/lm90.
> This driver can also be built as a module. If so, the module
> will be called lm90.
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2f94f95..2910791 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -54,6 +54,9 @@
> * and extended mode. They are mostly compatible with LM90 except for a data
> * format difference for the temperature value registers.
> *
> + * This driver also supports the SA56004 from Philips. This device is
> + * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
> + *
> * Since the LM90 was the first chipset supported by this driver, most
> * comments will refer to this chipset, but are actually general and
> * concern all supported chipsets, unless mentioned otherwise.
> @@ -96,13 +99,15 @@
> * MAX6659 can have address 0x4c, 0x4d or 0x4e.
> * MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
> * 0x4c, 0x4d or 0x4e.
> + * SA56004 can have address 0x48 through 0x4F.
> */
>
> static const unsigned short normal_i2c[] = {
> - 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
> + 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
> + 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
>
> enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> - max6646, w83l771, max6696 };
> + max6646, w83l771, max6696, sa56004 };
>
> /*
> * The LM90 registers
> @@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> #define MAX6659_REG_R_LOCAL_EMERG 0x17
> #define MAX6659_REG_W_LOCAL_EMERG 0x17
>
> +/* SA56004 registers */
> +
> +#define SA56004_REG_R_LOCAL_TEMPL 0x22
> +
> #define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register value */
> #define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
>
> @@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
> { "max6696", max6696 },
> { "nct1008", adt7461 },
> { "w83l771", w83l771 },
> + { "sa56004", sa56004 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, lm90_id);
> @@ -204,6 +214,8 @@ struct lm90_params {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate register value */
> + u8 local_ext_offset; /* Local extension register if
> + LM90_HAVE_LOCAL_EXT is set*/
I really dislike this variable name. "offset" is used as "temperature offset"
in the driver, and means a temperature offset, not a register offset (from 0).
Something like reg_local_ext or reg_local_templ would be better.
> };
>
> static const struct lm90_params lm90_params[] = {
> @@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 6,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6657] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> @@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6680] = {
> .flags = LM90_HAVE_OFFSET,
> @@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
> | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
> .alert_alarms = 0x187c,
> .max_convrate = 6,
> + .local_ext_offset = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [w83l771] = {
> .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> },
> + [sa56004] = {
> + .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
> + | LM90_HAVE_LOCAL_EXT,
> + .alert_alarms = 0x7b,
> + .max_convrate = 9,
> + .local_ext_offset = SA56004_REG_R_LOCAL_TEMPL,
> + },
> };
>
> /*
> @@ -286,6 +308,7 @@ struct lm90_data {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate */
> + u8 local_ext_offset; /* local extension register offset */
>
Same here (variable name).
> /* registers values */
> s8 temp8[8]; /* 0: local low limit
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
> if (data->flags & LM90_HAVE_LOCAL_EXT) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> - MAX6657_REG_R_LOCAL_TEMPL,
> + data->local_ext_offset,
> &data->temp11[4]);
> } else {
> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1263,6 +1286,12 @@ static int lm90_detect(struct i2c_client *new_client,
> name = "w83l771";
> }
> }
> + } else
> + if (man_id == 0xA1) { /* NXP Semiconductor/Philips */
> + if (chip_id == 0x00
> + && address >= 0x48 && address <= 0x4F) {
This multi-line split is unnecessary.
> + name = "sa56004";
> + }
> }
>
> if (!name) { /* identification failed */
> @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client *new_client,
> /* Set maximum conversion rate */
> data->max_convrate = lm90_params[data->kind].max_convrate;
>
> + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> + if (lm90_params[data->kind].local_ext_offset > 0)
> + data->local_ext_offset =
> + lm90_params[data->kind].local_ext_offset;
> + else {
> + dev_err(&new_client->dev,
> + "Invalid temperature extension register. "
> + "Accuracy may be limited.\n");
> + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> + }
Either { } in both branches of the if statement, or none.
( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset isn't.
That should be found during coding (or code review), and not be exported
to the user. So, from my perspective, the check is unnecessary. I'll leave
that up to Jean to decide, though.
In addition to the above, your patch generates several checkpatch errors
(trailing whitespace). Please fix.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-05-23 4:37 ` [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor Guenter Roeck
@ 2011-05-23 7:08 ` Stijn Devriendt (sdevrien)
-1 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt (sdevrien) @ 2011-05-23 7:08 UTC (permalink / raw)
To: Guenter Roeck; +Cc: khali, lm-sensors, linux-kernel
> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
>
> > if (!name) { /* identification failed */
> > @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client
> *new_client,
> > /* Set maximum conversion rate */
> > data->max_convrate = lm90_params[data->kind].max_convrate;
> >
> > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > + if (lm90_params[data->kind].local_ext_offset > 0)
> > + data->local_ext_offset > > +
lm90_params[data->kind].local_ext_offset;
> > + else {
> > + dev_err(&new_client->dev,
> > + "Invalid temperature extension register. "
> > + "Accuracy may be limited.\n");
> > + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> > + }
>
> Either { } in both branches of the if statement, or none.
> ( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
>
> I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset
isn't.
> That should be found during coding (or code review), and not be
exported
> to the user. So, from my perspective, the check is unnecessary. I'll
leave
> that up to Jean to decide, though.
>
Do you think a BUG_ON() would be better suited here?
> In addition to the above, your patch generates several checkpatch
errors
> (trailing whitespace). Please fix.
I recall letting checkpatch yell at me... I'll have another round of it
to
be sure.
Thanks,
Stijn
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-05-23 7:08 ` Stijn Devriendt (sdevrien)
0 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt (sdevrien) @ 2011-05-23 7:08 UTC (permalink / raw)
To: Guenter Roeck; +Cc: khali, lm-sensors, linux-kernel
> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
>
> > if (!name) { /* identification failed */
> > @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client
> *new_client,
> > /* Set maximum conversion rate */
> > data->max_convrate = lm90_params[data->kind].max_convrate;
> >
> > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > + if (lm90_params[data->kind].local_ext_offset > 0)
> > + data->local_ext_offset =
> > +
lm90_params[data->kind].local_ext_offset;
> > + else {
> > + dev_err(&new_client->dev,
> > + "Invalid temperature extension register. "
> > + "Accuracy may be limited.\n");
> > + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> > + }
>
> Either { } in both branches of the if statement, or none.
> ( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
>
> I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset
isn't.
> That should be found during coding (or code review), and not be
exported
> to the user. So, from my perspective, the check is unnecessary. I'll
leave
> that up to Jean to decide, though.
>
Do you think a BUG_ON() would be better suited here?
> In addition to the above, your patch generates several checkpatch
errors
> (trailing whitespace). Please fix.
I recall letting checkpatch yell at me... I'll have another round of it
to
be sure.
Thanks,
Stijn
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-05-23 7:08 ` [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor Stijn Devriendt (sdevrien)
@ 2011-05-23 13:52 ` Guenter Roeck
-1 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-05-23 13:52 UTC (permalink / raw)
To: Stijn Devriendt (sdevrien)
Cc: khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Mon, May 23, 2011 at 03:08:42AM -0400, Stijn Devriendt (sdevrien) wrote:
> > -----Original Message-----
> > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> >
> > > if (!name) { /* identification failed */
> > > @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client
> > *new_client,
> > > /* Set maximum conversion rate */
> > > data->max_convrate = lm90_params[data->kind].max_convrate;
> > >
> > > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > > + if (lm90_params[data->kind].local_ext_offset > 0)
> > > + data->local_ext_offset > > > +
> lm90_params[data->kind].local_ext_offset;
> > > + else {
> > > + dev_err(&new_client->dev,
> > > + "Invalid temperature extension register. "
> > > + "Accuracy may be limited.\n");
> > > + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> > > + }
> >
> > Either { } in both branches of the if statement, or none.
> > ( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
> >
> > I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset
> isn't.
> > That should be found during coding (or code review), and not be
> exported
> > to the user. So, from my perspective, the check is unnecessary. I'll
> leave
> > that up to Jean to decide, though.
> >
> Do you think a BUG_ON() would be better suited here?
>
I would just use
data->local_ext_offset = lm90_params[data->kind].local_ext_offset;
without any conditionals (the if statements just add code without real value),
followed by
BUG_ON((data->flags & LM90_HAVE_LOCAL_EXT) && data->local_ext_offset = 0);
if you want to be sure.
> > In addition to the above, your patch generates several checkpatch
> errors
> > (trailing whitespace). Please fix.
> I recall letting checkpatch yell at me... I'll have another round of it
> to
> be sure.
>
Try to apply your own patch, and you'll see git complain about whitespace errors.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-05-23 13:52 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-05-23 13:52 UTC (permalink / raw)
To: Stijn Devriendt (sdevrien)
Cc: khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Mon, May 23, 2011 at 03:08:42AM -0400, Stijn Devriendt (sdevrien) wrote:
> > -----Original Message-----
> > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> >
> > > if (!name) { /* identification failed */
> > > @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client
> > *new_client,
> > > /* Set maximum conversion rate */
> > > data->max_convrate = lm90_params[data->kind].max_convrate;
> > >
> > > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > > + if (lm90_params[data->kind].local_ext_offset > 0)
> > > + data->local_ext_offset =
> > > +
> lm90_params[data->kind].local_ext_offset;
> > > + else {
> > > + dev_err(&new_client->dev,
> > > + "Invalid temperature extension register. "
> > > + "Accuracy may be limited.\n");
> > > + data->flags &= (~LM90_HAVE_LOCAL_EXT);
> > > + }
> >
> > Either { } in both branches of the if statement, or none.
> > ( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary.
> >
> > I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset
> isn't.
> > That should be found during coding (or code review), and not be
> exported
> > to the user. So, from my perspective, the check is unnecessary. I'll
> leave
> > that up to Jean to decide, though.
> >
> Do you think a BUG_ON() would be better suited here?
>
I would just use
data->local_ext_offset = lm90_params[data->kind].local_ext_offset;
without any conditionals (the if statements just add code without real value),
followed by
BUG_ON((data->flags & LM90_HAVE_LOCAL_EXT) && data->local_ext_offset == 0);
if you want to be sure.
> > In addition to the above, your patch generates several checkpatch
> errors
> > (trailing whitespace). Please fix.
> I recall letting checkpatch yell at me... I'll have another round of it
> to
> be sure.
>
Try to apply your own patch, and you'll see git complain about whitespace errors.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* [lm-sensors] [PATCH] Add support for the Philips SA56004
@ 2011-06-04 10:37 ` Stijn Devriendt
0 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt @ 2011-06-04 10:37 UTC (permalink / raw)
To: khali, lm-sensors, linux-kernel; +Cc: guenter.roeck, Stijn Devriendt
Add support for the Philips SA56004, an LM86
compatible temperature sensor.
Changes since v1:
- Updated documentation
- Trace replaced by BUG_ON
- style updates
Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
---
Documentation/hwmon/lm90 | 9 ++++++++-
drivers/hwmon/Kconfig | 2 +-
drivers/hwmon/lm90.c | 39 ++++++++++++++++++++++++++++++++++++---
3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
index f3efd18..9cd14cfe 100644
--- a/Documentation/hwmon/lm90
+++ b/Documentation/hwmon/lm90
@@ -113,7 +113,11 @@ Supported chips:
Prefix: 'w83l771'
Addresses scanned: I2C 0x4c
Datasheet: Not publicly available, can be requested from Nuvoton
-
+ * Philips/NXP SA56004X
+ Prefix: 'sa56004'
+ Addresses scanned: I2C 0x48 through 0x4F
+ Datasheet: Publicly available at NXP website
+ http://ics.nxp.com/products/interface/datasheet/sa56004x.pdf
Author: Jean Delvare <khali@linux-fr.org>
@@ -193,6 +197,9 @@ W83L771AWG/ASG
* The AWG and ASG variants only differ in package format.
* Diode ideality factor configuration (remote sensor) at 0xE3
+SA56004X:
+ * Better local resolution
+
All temperature values are given in degrees Celsius. Resolution
is 1.0 degree for the local temperature, 0.125 degree for the remote
temperature, except for the MAX6657, MAX6658 and MAX6659 which have a
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 50e40db..11c9339 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -620,7 +620,7 @@ config SENSORS_LM90
LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor NCT1008,
- and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
+ Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor chips.
This driver can also be built as a module. If so, the module
will be called lm90.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2f94f95..01b38a0 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -54,6 +54,9 @@
* and extended mode. They are mostly compatible with LM90 except for a data
* format difference for the temperature value registers.
*
+ * This driver also supports the SA56004 from Philips. This device is
+ * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
+ *
* Since the LM90 was the first chipset supported by this driver, most
* comments will refer to this chipset, but are actually general and
* concern all supported chipsets, unless mentioned otherwise.
@@ -96,13 +99,15 @@
* MAX6659 can have address 0x4c, 0x4d or 0x4e.
* MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
* 0x4c, 0x4d or 0x4e.
+ * SA56004 can have address 0x48 through 0x4F.
*/
static const unsigned short normal_i2c[] = {
- 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
+ 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
+ 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
- max6646, w83l771, max6696 };
+ max6646, w83l771, max6696, sa56004 };
/*
* The LM90 registers
@@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
#define MAX6659_REG_R_LOCAL_EMERG 0x17
#define MAX6659_REG_W_LOCAL_EMERG 0x17
+/* SA56004 registers */
+
+#define SA56004_REG_R_LOCAL_TEMPL 0x22
+
#define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register value */
#define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
@@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
{ "max6696", max6696 },
{ "nct1008", adt7461 },
{ "w83l771", w83l771 },
+ { "sa56004", sa56004 },
{ }
};
MODULE_DEVICE_TABLE(i2c, lm90_id);
@@ -204,6 +214,8 @@ struct lm90_params {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate register value */
+ u8 reg_local_ext; /* Local extension register if
+ LM90_HAVE_LOCAL_EXT is set*/
};
static const struct lm90_params lm90_params[] = {
@@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT,
.alert_alarms = 0x7c,
.max_convrate = 6,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6657] = {
.flags = LM90_HAVE_LOCAL_EXT,
@@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
.alert_alarms = 0x7c,
.max_convrate = 8,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6680] = {
.flags = LM90_HAVE_OFFSET,
@@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
| LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
.alert_alarms = 0x187c,
.max_convrate = 6,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[w83l771] = {
.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
.alert_alarms = 0x7c,
.max_convrate = 8,
},
+ [sa56004] = {
+ .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
+ | LM90_HAVE_LOCAL_EXT,
+ .alert_alarms = 0x7b,
+ .max_convrate = 9,
+ .reg_local_ext = SA56004_REG_R_LOCAL_TEMPL,
+ },
};
/*
@@ -286,6 +308,7 @@ struct lm90_data {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate */
+ u8 reg_local_ext; /* local extension register offset */
/* registers values */
s8 temp8[8]; /* 0: local low limit
@@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (data->flags & LM90_HAVE_LOCAL_EXT) {
lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
- MAX6657_REG_R_LOCAL_TEMPL,
+ data->reg_local_ext,
&data->temp11[4]);
} else {
if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
@@ -1263,6 +1286,11 @@ static int lm90_detect(struct i2c_client *new_client,
name = "w83l771";
}
}
+ } else
+ if (man_id = 0xA1) { /* NXP Semiconductor/Philips */
+ if (chip_id = 0x00 && address >= 0x48 && address <= 0x4F) {
+ name = "sa56004";
+ }
}
if (!name) { /* identification failed */
@@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
/* Set maximum conversion rate */
data->max_convrate = lm90_params[data->kind].max_convrate;
+ if (data->flags & LM90_HAVE_LOCAL_EXT) {
+ data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
+ BUG_ON(data->reg_local_ext = 0);
+ }
+
/* Initialize the LM90 chip */
lm90_init_client(new_client);
--
1.7.3.3
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-06-04 10:37 ` Stijn Devriendt
0 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt @ 2011-06-04 10:37 UTC (permalink / raw)
To: khali, lm-sensors, linux-kernel; +Cc: guenter.roeck, Stijn Devriendt
Add support for the Philips SA56004, an LM86
compatible temperature sensor.
Changes since v1:
- Updated documentation
- Trace replaced by BUG_ON
- style updates
Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
---
Documentation/hwmon/lm90 | 9 ++++++++-
drivers/hwmon/Kconfig | 2 +-
drivers/hwmon/lm90.c | 39 ++++++++++++++++++++++++++++++++++++---
3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
index f3efd18..9cd14cfe 100644
--- a/Documentation/hwmon/lm90
+++ b/Documentation/hwmon/lm90
@@ -113,7 +113,11 @@ Supported chips:
Prefix: 'w83l771'
Addresses scanned: I2C 0x4c
Datasheet: Not publicly available, can be requested from Nuvoton
-
+ * Philips/NXP SA56004X
+ Prefix: 'sa56004'
+ Addresses scanned: I2C 0x48 through 0x4F
+ Datasheet: Publicly available at NXP website
+ http://ics.nxp.com/products/interface/datasheet/sa56004x.pdf
Author: Jean Delvare <khali@linux-fr.org>
@@ -193,6 +197,9 @@ W83L771AWG/ASG
* The AWG and ASG variants only differ in package format.
* Diode ideality factor configuration (remote sensor) at 0xE3
+SA56004X:
+ * Better local resolution
+
All temperature values are given in degrees Celsius. Resolution
is 1.0 degree for the local temperature, 0.125 degree for the remote
temperature, except for the MAX6657, MAX6658 and MAX6659 which have a
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 50e40db..11c9339 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -620,7 +620,7 @@ config SENSORS_LM90
LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor NCT1008,
- and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
+ Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor chips.
This driver can also be built as a module. If so, the module
will be called lm90.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2f94f95..01b38a0 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -54,6 +54,9 @@
* and extended mode. They are mostly compatible with LM90 except for a data
* format difference for the temperature value registers.
*
+ * This driver also supports the SA56004 from Philips. This device is
+ * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
+ *
* Since the LM90 was the first chipset supported by this driver, most
* comments will refer to this chipset, but are actually general and
* concern all supported chipsets, unless mentioned otherwise.
@@ -96,13 +99,15 @@
* MAX6659 can have address 0x4c, 0x4d or 0x4e.
* MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
* 0x4c, 0x4d or 0x4e.
+ * SA56004 can have address 0x48 through 0x4F.
*/
static const unsigned short normal_i2c[] = {
- 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
+ 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
+ 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
- max6646, w83l771, max6696 };
+ max6646, w83l771, max6696, sa56004 };
/*
* The LM90 registers
@@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
#define MAX6659_REG_R_LOCAL_EMERG 0x17
#define MAX6659_REG_W_LOCAL_EMERG 0x17
+/* SA56004 registers */
+
+#define SA56004_REG_R_LOCAL_TEMPL 0x22
+
#define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register value */
#define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
@@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
{ "max6696", max6696 },
{ "nct1008", adt7461 },
{ "w83l771", w83l771 },
+ { "sa56004", sa56004 },
{ }
};
MODULE_DEVICE_TABLE(i2c, lm90_id);
@@ -204,6 +214,8 @@ struct lm90_params {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate register value */
+ u8 reg_local_ext; /* Local extension register if
+ LM90_HAVE_LOCAL_EXT is set*/
};
static const struct lm90_params lm90_params[] = {
@@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT,
.alert_alarms = 0x7c,
.max_convrate = 6,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6657] = {
.flags = LM90_HAVE_LOCAL_EXT,
@@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
.flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
.alert_alarms = 0x7c,
.max_convrate = 8,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[max6680] = {
.flags = LM90_HAVE_OFFSET,
@@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
| LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
.alert_alarms = 0x187c,
.max_convrate = 6,
+ .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
},
[w83l771] = {
.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
.alert_alarms = 0x7c,
.max_convrate = 8,
},
+ [sa56004] = {
+ .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
+ | LM90_HAVE_LOCAL_EXT,
+ .alert_alarms = 0x7b,
+ .max_convrate = 9,
+ .reg_local_ext = SA56004_REG_R_LOCAL_TEMPL,
+ },
};
/*
@@ -286,6 +308,7 @@ struct lm90_data {
u16 alert_alarms; /* Which alarm bits trigger ALERT# */
/* Upper 8 bits for max6695/96 */
u8 max_convrate; /* Maximum conversion rate */
+ u8 reg_local_ext; /* local extension register offset */
/* registers values */
s8 temp8[8]; /* 0: local low limit
@@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (data->flags & LM90_HAVE_LOCAL_EXT) {
lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
- MAX6657_REG_R_LOCAL_TEMPL,
+ data->reg_local_ext,
&data->temp11[4]);
} else {
if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
@@ -1263,6 +1286,11 @@ static int lm90_detect(struct i2c_client *new_client,
name = "w83l771";
}
}
+ } else
+ if (man_id == 0xA1) { /* NXP Semiconductor/Philips */
+ if (chip_id == 0x00 && address >= 0x48 && address <= 0x4F) {
+ name = "sa56004";
+ }
}
if (!name) { /* identification failed */
@@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
/* Set maximum conversion rate */
data->max_convrate = lm90_params[data->kind].max_convrate;
+ if (data->flags & LM90_HAVE_LOCAL_EXT) {
+ data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
+ BUG_ON(data->reg_local_ext == 0);
+ }
+
/* Initialize the LM90 chip */
lm90_init_client(new_client);
--
1.7.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-05-19 14:10 ` [PATCH] Add support for the Philips SA56004 temperature sensor sdevrien
` (2 preceding siblings ...)
(?)
@ 2011-06-04 13:42 ` anish singh
2011-06-04 16:32 ` [PATCH] Add support for the Philips SA56004 temperature sensor Stijn Devriendt (sdevrien)
-1 siblings, 1 reply; 15+ messages in thread
From: anish singh @ 2011-06-04 13:42 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1.1: Type: text/plain, Size: 8968 bytes --]
I am no expert on HWMON but just want to
add some points.
On Sat, Jun 4, 2011 at 4:07 PM, Stijn Devriendt <sdevrien@cisco.com> wrote:
> Add support for the Philips SA56004, an LM86
> compatible temperature sensor.
>
> Changes since v1:
> - Updated documentation
> - Trace replaced by BUG_ON
> - style updates
>
> Signed-off-by: Stijn Devriendt <sdevrien@cisco.com>
> ---
> Documentation/hwmon/lm90 | 9 ++++++++-
> drivers/hwmon/Kconfig | 2 +-
> drivers/hwmon/lm90.c | 39 ++++++++++++++++++++++++++++++++++++---
> 3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
> index f3efd18..9cd14cfe 100644
> --- a/Documentation/hwmon/lm90
> +++ b/Documentation/hwmon/lm90
> @@ -113,7 +113,11 @@ Supported chips:
> Prefix: 'w83l771'
> Addresses scanned: I2C 0x4c
> Datasheet: Not publicly available, can be requested from Nuvoton
> -
> + * Philips/NXP SA56004X
> + Prefix: 'sa56004'
> + Addresses scanned: I2C 0x48 through 0x4F
> + Datasheet: Publicly available at NXP website
> +
> http://ics.nxp.com/products/interface/datasheet/sa56004x.pdf
>
> Author: Jean Delvare <khali@linux-fr.org>
>
> @@ -193,6 +197,9 @@ W83L771AWG/ASG
> * The AWG and ASG variants only differ in package format.
> * Diode ideality factor configuration (remote sensor) at 0xE3
>
> +SA56004X:
> + * Better local resolution
> +
> All temperature values are given in degrees Celsius. Resolution
> is 1.0 degree for the local temperature, 0.125 degree for the remote
> temperature, except for the MAX6657, MAX6658 and MAX6659 which have a
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 50e40db..11c9339 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -620,7 +620,7 @@ config SENSORS_LM90
> LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and
> ADT7461A,
> Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658,
> MAX6659,
> MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, ON Semiconductor
> NCT1008,
> - and Winbond/Nuvoton W83L771W/G/AWG/ASG sensor chips.
> + Winbond/Nuvoton W83L771W/G/AWG/ASG and Philips SA56004 sensor
> chips.
>
> This driver can also be built as a module. If so, the module
> will be called lm90.
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2f94f95..01b38a0 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -54,6 +54,9 @@
> * and extended mode. They are mostly compatible with LM90 except for a
> data
> * format difference for the temperature value registers.
> *
> + * This driver also supports the SA56004 from Philips. This device is
> + * pin-compatible with the LM86, the ED/EDP parts are also
> address-compatible.
> + *
> * Since the LM90 was the first chipset supported by this driver, most
> * comments will refer to this chipset, but are actually general and
> * concern all supported chipsets, unless mentioned otherwise.
> @@ -96,13 +99,15 @@
> * MAX6659 can have address 0x4c, 0x4d or 0x4e.
> * MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
> * 0x4c, 0x4d or 0x4e.
> + * SA56004 can have address 0x48 through 0x4F.
> */
>
> static const unsigned short normal_i2c[] = {
> - 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e,
> I2C_CLIENT_END };
> + 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
> + 0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
>
> enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461,
> max6680,
> - max6646, w83l771, max6696 };
> + max6646, w83l771, max6696, sa56004 };
>
> /*
> * The LM90 registers
> @@ -152,6 +157,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
> max6659, adt7461, max6680,
> #define MAX6659_REG_R_LOCAL_EMERG 0x17
> #define MAX6659_REG_W_LOCAL_EMERG 0x17
>
> +/* SA56004 registers */
> +
> +#define SA56004_REG_R_LOCAL_TEMPL 0x22
> +
> #define LM90_DEF_CONVRATE_RVAL 6 /* Def conversion rate register
> value */
> #define LM90_MAX_CONVRATE_MS 16000 /* Maximum conversion rate in ms */
>
> @@ -192,6 +201,7 @@ static const struct i2c_device_id lm90_id[] = {
> { "max6696", max6696 },
> { "nct1008", adt7461 },
> { "w83l771", w83l771 },
> + { "sa56004", sa56004 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, lm90_id);
> @@ -204,6 +214,8 @@ struct lm90_params {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate register value */
> + u8 reg_local_ext; /* Local extension register if
> + LM90_HAVE_LOCAL_EXT is set*/
> };
>
> static const struct lm90_params lm90_params[] = {
> @@ -238,6 +250,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 6,
> + .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6657] = {
> .flags = LM90_HAVE_LOCAL_EXT,
> @@ -248,6 +261,7 @@ static const struct lm90_params lm90_params[] = {
> .flags = LM90_HAVE_LOCAL_EXT | LM90_HAVE_EMERGENCY,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> + .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [max6680] = {
> .flags = LM90_HAVE_OFFSET,
> @@ -259,12 +273,20 @@ static const struct lm90_params lm90_params[] = {
> | LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3,
> .alert_alarms = 0x187c,
> .max_convrate = 6,
> + .reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
> },
> [w83l771] = {
> .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT,
> .alert_alarms = 0x7c,
> .max_convrate = 8,
> },
> + [sa56004] = {
> + .flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
> + | LM90_HAVE_LOCAL_EXT,
> + .alert_alarms = 0x7b,
> + .max_convrate = 9,
> + .reg_local_ext = SA56004_REG_R_LOCAL_TEMPL,
> + },
> };
>
> /*
> @@ -286,6 +308,7 @@ struct lm90_data {
> u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> /* Upper 8 bits for max6695/96 */
> u8 max_convrate; /* Maximum conversion rate */
> + u8 reg_local_ext; /* local extension register offset */
>
> /* registers values */
> s8 temp8[8]; /* 0: local low limit
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct
> device *dev)
>
> if (data->flags & LM90_HAVE_LOCAL_EXT) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> - MAX6657_REG_R_LOCAL_TEMPL,
> + data->reg_local_ext,
> &data->temp11[4]);
>
I don't think this variable reg_local_ext should exist as
register address should be "# defined" and should not be
part of lm90_data but i do see a case here where we are
assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
flag set.So i think we should have some more branching here
to detect the device and pass the corresponding register but as
i said i am no expert.
> } else {
> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1263,6 +1286,11 @@ static int lm90_detect(struct i2c_client
> *new_client,
> name = "w83l771";
> }
> }
> + } else
> + if (man_id == 0xA1) { /* NXP Semiconductor/Philips */
> + if (chip_id == 0x00 && address >= 0x48 && address <= 0x4F)
> {
> + name = "sa56004";
> + }
> }
>
> if (!name) { /* identification failed */
> @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
> /* Set maximum conversion rate */
> data->max_convrate = lm90_params[data->kind].max_convrate;
>
> + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> + data->reg_local_ext =
> lm90_params[data->kind].reg_local_ext;
> + BUG_ON(data->reg_local_ext == 0);
> + }
> +
>
I think this BUG_ON is too harsh in probe.We generally use pr_err
to print if something which is supposed to be set is not set.As BUG_ON
will call kernel panic,right?
> /* Initialize the LM90 chip */
> lm90_init_client(new_client);
>
> --
> 1.7.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
[-- Attachment #1.2: Type: text/html, Size: 11110 bytes --]
[-- Attachment #2: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-06-04 13:42 ` [lm-sensors] [PATCH] Add support for the Philips SA56004 anish singh
@ 2011-06-04 16:32 ` Stijn Devriendt (sdevrien)
0 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt (sdevrien) @ 2011-06-04 16:32 UTC (permalink / raw)
To: anish singh; +Cc: khali, lm-sensors, linux-kernel, guenter.roeck
> From: anish singh [mailto:anish198519851985@gmail.com]
>
> I am no expert on HWMON but just want to
> add some points.
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
> if (data->flags & LM90_HAVE_LOCAL_EXT) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> - MAX6657_REG_R_LOCAL_TEMPL,
> + data->reg_local_ext,
> &data->temp11[4]);
> I don't think this variable reg_local_ext should exist as
> register address should be "# defined" and should not be
> part of lm90_data but i do see a case here where we are
> assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
> flag set.So i think we should have some more branching here
> to detect the device and pass the corresponding register but as
> i said i am no expert.
>
Only MAX6657 and SA56004 have the local temperature extension
register and unfortunately they reside at different offsets.
Therefore the probing will detect the right chip and, if supported,
use the correct register.
> } else {
> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
> /* Set maximum conversion rate */
> data->max_convrate = lm90_params[data->kind].max_convrate;
>
> + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> + data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
> + BUG_ON(data->reg_local_ext = 0);
> + }
> +
> I think this BUG_ON is too harsh in probe.We generally use pr_err
> to print if something which is supposed to be set is not set.As BUG_ON
> will call kernel panic,right?
The reason for adding the BUG_ON rather than the error was that it is
in fact a coding error when the flag is set without specifying the offset.
Such a condition should never make it into a running system and should be
caught during coding or review.
BUG_ON only does an oops, panic is optional depending on panic_on_oops being
set.
Regards,
Stijn
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-06-04 16:32 ` Stijn Devriendt (sdevrien)
0 siblings, 0 replies; 15+ messages in thread
From: Stijn Devriendt (sdevrien) @ 2011-06-04 16:32 UTC (permalink / raw)
To: anish singh; +Cc: khali, lm-sensors, linux-kernel, guenter.roeck
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2422 bytes --]
> From: anish singh [mailto:anish198519851985@gmail.com]
>
> I am no expert on HWMON but just want to
> add some points.
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
>Â Â Â Â Â Â Â Â if (data->flags & LM90_HAVE_LOCAL_EXT) {
>Â Â Â Â Â Â Â Â Â Â Â Â lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â MAX6657_REG_R_LOCAL_TEMPL,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â data->reg_local_ext,
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &data->temp11[4]);
> I don't think this variable reg_local_ext should exist as
> register address should be "# defined" and should not be
> part of lm90_data but i do see a case here where we are
> assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
> flag set.So i think we should have some more branching here
> to detect the device and pass the corresponding register but as
> i said i am no expert.
>Â
Only MAX6657 and SA56004 have the local temperature extension
register and unfortunately they reside at different offsets.
Therefore the probing will detect the right chip and, if supported,
use the correct register.
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â } else {
>Â Â Â Â Â Â Â Â Â Â Â Â if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
>Â Â Â Â /* Set maximum conversion rate */
>Â Â Â Â data->max_convrate = lm90_params[data->kind].max_convrate;
>
> + Â Â Â if (data->flags & LM90_HAVE_LOCAL_EXT) {
> + Â Â Â Â Â Â Â data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
> + Â Â Â Â Â Â Â BUG_ON(data->reg_local_ext == 0);
> + Â Â Â }
> +
> I think this BUG_ON is too harsh in probe.We generally use pr_err
> to print if something which is supposed to be set is not set.As BUG_ON
> will call kernel panic,right?
The reason for adding the BUG_ON rather than the error was that it is
in fact a coding error when the flag is set without specifying the offset.
Such a condition should never make it into a running system and should be
caught during coding or review.
BUG_ON only does an oops, panic is optional depending on panic_on_oops being
set.
Regards,
Stijn
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [lm-sensors] [PATCH] Add support for the Philips SA56004
2011-06-04 16:32 ` [PATCH] Add support for the Philips SA56004 temperature sensor Stijn Devriendt (sdevrien)
@ 2011-06-04 16:57 ` Guenter Roeck
-1 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-06-04 16:57 UTC (permalink / raw)
To: Stijn Devriendt (sdevrien)
Cc: anish singh, khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Sat, Jun 04, 2011 at 12:32:01PM -0400, Stijn Devriendt (sdevrien) wrote:
> > From: anish singh [mailto:anish198519851985@gmail.com]
> >
> > I am no expert on HWMON but just want to
> > add some points.
> > @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >
> > if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> > - MAX6657_REG_R_LOCAL_TEMPL,
> > + data->reg_local_ext,
> > &data->temp11[4]);
> > I don't think this variable reg_local_ext should exist as
> > register address should be "# defined" and should not be
> > part of lm90_data but i do see a case here where we are
> > assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
> > flag set.So i think we should have some more branching here
> > to detect the device and pass the corresponding register but as
> > i said i am no expert.
> >
>
> Only MAX6657 and SA56004 have the local temperature extension
> register and unfortunately they reside at different offsets.
> Therefore the probing will detect the right chip and, if supported,
> use the correct register.
>
> > } else {
> > if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> > @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
> > /* Set maximum conversion rate */
> > data->max_convrate = lm90_params[data->kind].max_convrate;
> >
> > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > + data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
> > + BUG_ON(data->reg_local_ext = 0);
> > + }
> > +
> > I think this BUG_ON is too harsh in probe.We generally use pr_err
> > to print if something which is supposed to be set is not set.As BUG_ON
> > will call kernel panic,right?
>
> The reason for adding the BUG_ON rather than the error was that it is
> in fact a coding error when the flag is set without specifying the offset.
> Such a condition should never make it into a running system and should be
> caught during coding or review.
> BUG_ON only does an oops, panic is optional depending on panic_on_oops being
> set.
>
Maybe use WARN_ON instead ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] Add support for the Philips SA56004 temperature sensor.
@ 2011-06-04 16:57 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2011-06-04 16:57 UTC (permalink / raw)
To: Stijn Devriendt (sdevrien)
Cc: anish singh, khali@linux-fr.org, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Sat, Jun 04, 2011 at 12:32:01PM -0400, Stijn Devriendt (sdevrien) wrote:
> > From: anish singh [mailto:anish198519851985@gmail.com]
> >
> > I am no expert on HWMON but just want to
> > add some points.
> > @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >
> > if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> > - MAX6657_REG_R_LOCAL_TEMPL,
> > + data->reg_local_ext,
> > &data->temp11[4]);
> > I don't think this variable reg_local_ext should exist as
> > register address should be "# defined" and should not be
> > part of lm90_data but i do see a case here where we are
> > assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
> > flag set.So i think we should have some more branching here
> > to detect the device and pass the corresponding register but as
> > i said i am no expert.
> >
>
> Only MAX6657 and SA56004 have the local temperature extension
> register and unfortunately they reside at different offsets.
> Therefore the probing will detect the right chip and, if supported,
> use the correct register.
>
> > } else {
> > if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> > @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
> > /* Set maximum conversion rate */
> > data->max_convrate = lm90_params[data->kind].max_convrate;
> >
> > + if (data->flags & LM90_HAVE_LOCAL_EXT) {
> > + data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
> > + BUG_ON(data->reg_local_ext == 0);
> > + }
> > +
> > I think this BUG_ON is too harsh in probe.We generally use pr_err
> > to print if something which is supposed to be set is not set.As BUG_ON
> > will call kernel panic,right?
>
> The reason for adding the BUG_ON rather than the error was that it is
> in fact a coding error when the flag is set without specifying the offset.
> Such a condition should never make it into a running system and should be
> caught during coding or review.
> BUG_ON only does an oops, panic is optional depending on panic_on_oops being
> set.
>
Maybe use WARN_ON instead ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread