Re: [PATCH] Add support for the Philips SA56004 temperature sensor.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> 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@xxxxxxxxx>
---
Â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@xxxxxxxxxxxx>

@@ -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@xxxxxxxxxxxxxxx
More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
Please read the FAQ at Âhttp://www.tux.org/lkml/

_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux