All of lore.kernel.org
 help / color / mirror / Atom feed
* [lm-sensors] [PATCH] hwmon: Driver for ADT7410
@ 2012-07-31 20:17 Hartmut Knaack
  2012-07-31 23:24 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Hartmut Knaack @ 2012-07-31 20:17 UTC (permalink / raw)
  To: lm-sensors

Hi,
this patch brings basic support for the Analog Devices ADT7410 temperature sensor, based on the lm75 hwmon driver and the adt7410 iio driver. The following functionality has been implemented:

  * get current temperature
  * get/set minimum, maximum and critical temperature
  * get/set hysteresis
  * get alarm events for minimum, maximum and critical temperature

The ADT7410 provides some more configuration options, but would need non-standard sysfs symbols, so I prefer to get some feedback before implementing any:

  * get/set resolution (13/16 bits): temp#_resolution (13/16)?
  * get/set event mode (comparator/interrupt): temp#_eventmode (0/1)?
  * get/set polarity of INT pin (active-high/active-low): temp#_INT (0/1)?
  * get/set polarity of CT pin (active-high/active-low): temp#_CT (0/1)?
  * get/set sampling mode (continous/one sample per second/one-shot/power-down): temp#_mode (0/1/2/3)?
  * get/set fault queue (1 to 4 faults to trigger event): temp#_faultqueue (1/2/3/4)?
  * get device id: temp#_id?

Besides, I saw that there are several sensors (including the ADT7410) which have just one hysteresis register, that stores a delta value that applies to min, max and critical temperature. How are the chances to create a standard symbol "temp#_hyst"?

All implemented sysfs-symbols have been sucessfully tested at temperatures of 15°C to 40°C.

Signed-off-by: Hartmut Knaack <knaack.h <at> gmx.de>
---
diff --git a/Documentation/hwmon/adt7410 b/Documentation/hwmon/adt7410
new file mode 100644
index 0000000..5ccae11
--- /dev/null
+++ b/Documentation/hwmon/adt7410
@@ -0,0 +1,47 @@
+Kernel driver adt7410
+=====================
+
+Supported chips:
+  * Analog Devices ADT7410
+    Prefix: 'adt7410'
+    Addresses scanned: I2C 0x48 - 0x4B
+    Datasheet: Publicly available at the Analog Devices website
+               http://www.analog.com/static/imported-files/data_sheets/ADT7410.pdf
+
+Author: Hartmut Knaack <knaack.h <at> gmx.de>
+
+Description
+-----------
+
+The ADT7410 is a temperature sensor with rated temperature range of -55°C to
++150°C. It has a high accuracy of +/-0.5°C and can be operated at a resolution
+of 13 bits (0.0625°C) or 16 bits (0.0078°C). The sensor provides an INT pin to
+indicate an excession of a set minimum or maximum temperature, as well as a
+critical temperature (CT) pin to indicate an excession of a set critical
+temperature. Both pins can be set up with a common hysteresis of 0°C - 15°C
+and a fault queue ranging from 1 to 4 events. Both pins can individually set
+to be active-low or active-high, while the whole device can either run in
+comparator mode or interrupt mode. The ADT7410 supports continous temperature
+sampling, as well as sampling one temperature value per second or even just
+get one sample on demand for power saving. Besides, it can completely power
+down its ADC, if power management is required.
+
+Configuration Notes
+-------------------
+
+Since the device uses one hysteresis value, which is a delta to minimum,
+maximum and critical temperature, the driver will represent it as
+temp#_max_hyst.
+The device is set to 16 bit resolution and comparator mode by default.
+
+sysfs-Interface
+---------------
+
+temp#_input        - temperature input
+temp#_min        - temperature minimum setpoint
+temp#_max        - temperature maximum setpoint
+temp#_crit        - critical temperature setpoint
+temp#_max_hyst        - hysteresis for temperature maximum
+temp#_min_alarm        - temperature minimum alarm flag
+temp#_max_alarm        - temperature maximum alarm flag
+temp#_crit_alarm    - critical temperature alarm flag
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 6f1d167..7ed1989 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -179,6 +179,16 @@ config SENSORS_ADM9240
       This driver can also be built as a module.  If so, the module
       will be called adm9240.
 
+config SENSORS_ADT7410
+    tristate "Analog Devices ADT7410"
+    depends on I2C && EXPERIMENTAL
+    help
+      If you say yes here you get support for the Analog Devices
+      ADT7410 temperature monitoring chip.
+
+      This driver can also be built as a module. If so, the module
+      will be called adt7410.
+
 config SENSORS_ADT7411
     tristate "Analog Devices ADT7411"
     depends on I2C && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e1eeac1..112ce3a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_ADM9240)    += adm9240.o
 obj-$(CONFIG_SENSORS_ADS1015)    += ads1015.o
 obj-$(CONFIG_SENSORS_ADS7828)    += ads7828.o
 obj-$(CONFIG_SENSORS_ADS7871)    += ads7871.o
+obj-$(CONFIG_SENSORS_ADT7410)    += adt7410.o
 obj-$(CONFIG_SENSORS_ADT7411)    += adt7411.o
 obj-$(CONFIG_SENSORS_ADT7462)    += adt7462.o
 obj-$(CONFIG_SENSORS_ADT7470)    += adt7470.o
diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c
new file mode 100644
index 0000000..49f3c10
--- /dev/null
+++ b/drivers/hwmon/adt7410.c
@@ -0,0 +1,539 @@
+/*
+ * adt7410.c - Part of lm_sensors, Linux kernel modules for hardware
+ *     monitoring
+ * Hartmut Knaack <knaack.h <at> gmx.de> 2012-07-22
+ * based on lm75.c by Frodo Looijaard <frodol <at> dds.nl>
+ * and adt7410.c from iio-staging by Sonic Zhang <sonic.zhang <at> analog.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+
+/*
+ * ADT7410 registers definition
+ */
+
+#define ADT7410_TEMPERATURE        0
+#define ADT7410_STATUS            2
+#define ADT7410_CONFIG            3
+#define ADT7410_T_ALARM_HIGH        4
+#define ADT7410_T_ALARM_LOW        6
+#define ADT7410_T_CRIT            8
+#define ADT7410_T_HYST            0xA
+#define ADT7410_ID            0xB
+#define ADT7410_RESET            0x2F
+
+/*
+ * ADT7410 status
+ */
+#define ADT7410_STAT_T_LOW        0x10
+#define ADT7410_STAT_T_HIGH        0x20
+#define ADT7410_STAT_T_CRIT        0x40
+#define ADT7410_STAT_NOT_RDY        0x80
+
+/*
+ * ADT7410 config
+ */
+#define ADT7410_FAULT_QUEUE_MASK    0x3
+#define ADT7410_CT_POLARITY        0x4
+#define ADT7410_INT_POLARITY        0x8
+#define ADT7410_EVENT_MODE        0x10
+#define ADT7410_MODE_MASK        0x60
+#define ADT7410_FULL            0x0
+#define ADT7410_ONESHOT            0x20
+#define ADT7410_SPS            0x40
+#define ADT7410_PD            0x60
+#define ADT7410_RESOLUTION        0x80
+
+/*
+ * ADT7410 masks
+ */
+#define ADT7410_T16_VALUE_SIGN            0x8000
+#define ADT7410_T16_VALUE_FLOAT_OFFSET        7
+#define ADT7410_T16_VALUE_FLOAT_MASK        0x7F
+#define ADT7410_T13_VALUE_MASK            0xFFF8
+#define ADT7410_T_HYST_MASK            0xF
+#define ADT7410_DEVICE_ID_MASK            0x7
+#define ADT7410_MANUFACTORE_ID_OFFSET        3
+#define ADT7410_MANUFACTURE_ID            0x19
+#define ADT7410_IRQS                2
+
+
+/* straight from the datasheet */
+#define ADT7410_TEMP_MIN (-55000)
+#define ADT7410_TEMP_MAX 150000
+
+/*
+ * This driver handles the ADT7410 and compatible digital temperature sensors.
+ */
+
+enum adt7410_type {        /* keep sorted in alphabetical order */
+    adt7410,
+};
+
+/* Addresses scanned */
+static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
+                    I2C_CLIENT_END };
+
+static const u8 ADT7410_REG_TEMP[4] = {
+    ADT7410_TEMPERATURE,        /* input */
+    ADT7410_T_ALARM_HIGH,        /* high */
+    ADT7410_T_ALARM_LOW,        /* low */
+    ADT7410_T_CRIT,            /* critical */
+};
+
+/* Each client has this additional data */
+struct adt7410_data {
+    struct device        *hwmon_dev;
+    struct mutex        update_lock;
+    u8            config;
+    char            valid;        /* !=0 if registers are valid */
+    unsigned long        last_updated;    /* In jiffies */
+    u16            temp[4];    /* Register values,
+                           0 = input
+                           1 = high
+                           2 = low
+                           3 = critical */
+};
+
+/*
+ * adt7410 register access by I2C
+ */
+
+static int adt7410_i2c_read_word(struct i2c_client *client, u8 reg, u16 *data)
+{
+    int ret = 0;
+    ret = i2c_smbus_read_word_data(client, reg);
+    if (ret < 0) {
+        dev_err(&client->dev, "I2C read error\n");
+        return ret;
+    }
+
+    *data = swab16((u16)ret);
+
+    return 0;
+}
+
+static int adt7410_i2c_write_word(struct i2c_client *client, u8 reg, u16 data)
+{
+    int ret = 0;
+    ret = i2c_smbus_write_word_data(client, reg, swab16(data));
+    if (ret < 0)
+        dev_err(&client->dev, "I2C write error\n");
+
+    return ret;
+}
+
+static int adt7410_i2c_read_byte(struct i2c_client *client, u8 reg, u8 *data)
+{
+    int ret = 0;
+    ret = i2c_smbus_read_byte_data(client, reg);
+    if (ret < 0) {
+        dev_err(&client->dev, "I2C read error\n");
+        return ret;
+    }
+
+    *data = (u8)ret;
+
+    return 0;
+}
+
+static int adt7410_i2c_write_byte(struct i2c_client *client, u8 reg, u8 data)
+{
+    int ret = 0;
+    ret = i2c_smbus_write_byte_data(client, reg, data);
+    if (ret < 0)
+        dev_err(&client->dev, "I2C write error\n");
+
+    return ret;
+}
+
+static struct adt7410_data *adt7410_update_device(struct device *dev)
+{
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    struct adt7410_data *ret = data;
+    mutex_lock(&data->update_lock);
+
+    if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
+        || !data->valid) {
+        int i;
+        dev_dbg(&client->dev, "Starting adt7410 update\n");
+
+        for (i = 0; i < ARRAY_SIZE(data->temp); i++) {
+            int status;
+            if (i == 0) {        /* wait for register to */
+                int j = 0;    /* contain valid data */
+                u8 sreg;
+                do {
+                    status = adt7410_i2c_read_byte(client,
+                          ADT7410_STATUS, &sreg);
+                    if (status)
+                        return ERR_PTR(-EIO);
+                    j++;
+                    if (j == 10000)
+                        return ERR_PTR(-EIO);
+                    } while (sreg & ADT7410_STAT_NOT_RDY);
+            }
+            status = adt7410_i2c_read_word(client,
+                        ADT7410_REG_TEMP[i],
+                        &data->temp[i]);
+            if (unlikely(status < 0)) {
+                dev_dbg(dev,
+                    "ADT7410: Failed to read value: reg %d, error %d\n",
+                    ADT7410_REG_TEMP[i], status);
+                ret = ERR_PTR(status);
+                data->valid = 0;
+                goto abort;
+            }
+        }
+        data->last_updated = jiffies;
+        data->valid = 1;
+    }
+
+abort:
+    mutex_unlock(&data->update_lock);
+    return ret;
+}
+
+static u16 ADT7410_TEMP_TO_REG(long temp)
+{
+    return (u16)((SENSORS_LIMIT(temp, ADT7410_TEMP_MIN,
+            ADT7410_TEMP_MAX) * 128) / 1000);
+}
+
+static int ADT7410_REG_TO_TEMP(struct adt7410_data *data,
+        u16 reg)
+{
+    /* in 13 bit mode, bits 0-2 are status flags - mask them out */
+    if (!(data->config & ADT7410_RESOLUTION))
+        reg &= ADT7410_T13_VALUE_MASK;
+    /* temperature is stored in twos complement format, in steps of
+     * 1/128°C */
+    return (((int)reg * 1000) / 128);
+}
+
+
+
+/*-----------------------------------------------------------------------*/
+
+/* sysfs attributes for hwmon */
+
+static ssize_t adt7410_show_temp(struct device *dev,
+        struct device_attribute *da,
+        char *buf)
+{
+    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+    struct adt7410_data *data = adt7410_update_device(dev);
+    if (IS_ERR(data))
+        return PTR_ERR(data);
+
+    return sprintf(buf, "%d\n", ADT7410_REG_TO_TEMP(data,
+            data->temp[attr->index]));
+}
+
+static ssize_t adt7410_set_temp(struct device *dev,
+        struct device_attribute *da,
+        const char *buf, size_t count)
+{
+    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    int nr = attr->index;
+    long temp;
+    int ret;
+    ret = kstrtol(buf, 10, &temp);
+    if (ret)
+        return -EINVAL;
+
+    mutex_lock(&data->update_lock);
+    data->temp[nr] = ADT7410_TEMP_TO_REG(temp);
+    adt7410_i2c_write_word(client, ADT7410_REG_TEMP[nr], data->temp[nr]);
+    mutex_unlock(&data->update_lock);
+    return count;
+}
+
+static ssize_t adt7410_show_t_hyst(struct device *dev,
+        struct device_attribute *attr,
+        char *buf)
+{
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    int ret;
+    u8 t_hyst;
+    ret = adt7410_i2c_read_byte(client, ADT7410_T_HYST, &t_hyst);
+    if (ret)
+        return -EIO;
+    /* hysteresis is stored as a 4 bit delta in the device, convert it
+     * to an absolute value in relation to t_max */
+    return sprintf(buf, "%d\n", ADT7410_REG_TO_TEMP(data, data->temp[1] -
+            ((u16)(t_hyst & ADT7410_T_HYST_MASK) << 7)));
+}
+
+static ssize_t adt7410_set_t_hyst(struct device *dev,
+        struct device_attribute *da,
+        const char *buf, size_t count)
+{
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    int ret;
+    long hyst;
+    u8 t_hyst;
+    ret = kstrtol(buf, 10, &hyst);
+    if ((hyst < ADT7410_TEMP_MIN) || (hyst > ADT7410_TEMP_MAX))
+        return -EINVAL;
+    /* convert absolute hysteresis value to a 4 bit delta value */
+    t_hyst = (u8)(((s32)((data->temp[1] * 1000) / 128) - hyst) / 1000);
+    if (ret || (t_hyst > ADT7410_T_HYST_MASK))
+        return -EINVAL;
+
+    ret = adt7410_i2c_write_byte(client, ADT7410_T_HYST, t_hyst);
+    if (ret)
+        return -EIO;
+
+    return count;
+}
+
+static ssize_t adt7410_show_alarm(struct device *dev,
+        u8 status,
+        char *buf)
+{
+    struct i2c_client *client = to_i2c_client(dev);
+    u8 reg;
+    int ret = 0;
+    ret = adt7410_i2c_read_byte(client, ADT7410_STATUS, &reg);
+    if (ret)
+        return -EIO;
+
+    return sprintf(buf, "%d\n", (reg & status) ? 1 : 0);
+}
+
+static ssize_t adt7410_show_min_alarm(struct device *dev,
+        struct device_attribute *attr,
+        char *buf)
+{
+    return adt7410_show_alarm(dev, ADT7410_STAT_T_LOW, buf);
+}
+
+static ssize_t adt7410_show_max_alarm(struct device *dev,
+        struct device_attribute *attr,
+        char *buf)
+{
+    return adt7410_show_alarm(dev, ADT7410_STAT_T_HIGH, buf);
+}
+
+static ssize_t adt7410_show_crit_alarm(struct device *dev,
+        struct device_attribute *attr,
+        char *buf)
+{
+    return adt7410_show_alarm(dev, ADT7410_STAT_T_CRIT, buf);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adt7410_show_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
+            adt7410_show_temp, adt7410_set_temp, 1);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
+            adt7410_show_temp, adt7410_set_temp, 2);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO,
+            adt7410_show_temp, adt7410_set_temp, 3);
+static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
+            adt7410_show_t_hyst, adt7410_set_t_hyst, 4);
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IWUSR | S_IRUGO,
+            adt7410_show_min_alarm, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IWUSR | S_IRUGO,
+            adt7410_show_max_alarm, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IWUSR | S_IRUGO,
+            adt7410_show_crit_alarm, NULL, 7);
+
+static struct attribute *adt7410_attributes[] = {
+    &sensor_dev_attr_temp1_input.dev_attr.attr,
+    &sensor_dev_attr_temp1_max.dev_attr.attr,
+    &sensor_dev_attr_temp1_min.dev_attr.attr,
+    &sensor_dev_attr_temp1_crit.dev_attr.attr,
+    &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
+    &sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+    &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+    &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+    NULL
+};
+
+static const struct attribute_group adt7410_group = {
+    .attrs = adt7410_attributes,
+};
+
+/*-----------------------------------------------------------------------*/
+
+/* device probe and removal */
+
+static int
+adt7410_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+    struct adt7410_data *data;
+    int ret = 0;
+    u8 set_mask, clr_mask;
+    int new;
+    if (!i2c_check_functionality(client->adapter,
+            I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+        return -EIO;
+
+    data = kzalloc(sizeof(struct adt7410_data), GFP_KERNEL);
+    if (!data)
+        return -ENOMEM;
+
+    i2c_set_clientdata(client, data);
+    mutex_init(&data->update_lock);
+
+    /* Set to 16 bit resolution, continous conversion and comparator mode.
+     * Then tweak to be more precise when appropriate.
+     */
+    set_mask = ADT7410_FULL | ADT7410_RESOLUTION | ADT7410_EVENT_MODE;
+    clr_mask = 0;
+
+    /* configure as specified */
+    ret = adt7410_i2c_read_byte(client, ADT7410_CONFIG, &data->config);
+    if (ret) {
+        dev_dbg(&client->dev, "Can't read config? %d\n", ret);
+        goto exit_free;
+    }
+    new = (data->config & ~clr_mask) | set_mask;
+    if (data->config != new) {
+        adt7410_i2c_write_byte(client, ADT7410_CONFIG, new);
+        data->config = new;
+    }
+    dev_dbg(&client->dev, "Config %02x\n", new);
+
+    /* Register sysfs hooks */
+    ret = sysfs_create_group(&client->dev.kobj, &adt7410_group);
+    if (ret)
+        goto exit_free;
+
+    data->hwmon_dev = hwmon_device_register(&client->dev);
+    if (IS_ERR(data->hwmon_dev)) {
+        ret = PTR_ERR(data->hwmon_dev);
+        goto exit_remove;
+    }
+
+    dev_info(&client->dev, "%s: sensor '%s'\n",
+         dev_name(data->hwmon_dev), client->name);
+
+    return 0;
+
+exit_remove:
+    sysfs_remove_group(&client->dev.kobj, &adt7410_group);
+exit_free:
+    kfree(data);
+    return ret;
+}
+
+static int adt7410_remove(struct i2c_client *client)
+{
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    hwmon_device_unregister(data->hwmon_dev);
+    sysfs_remove_group(&client->dev.kobj, &adt7410_group);
+    adt7410_i2c_write_byte(client, ADT7410_CONFIG, data->config);
+    kfree(data);
+    return 0;
+}
+
+static const struct i2c_device_id adt7410_ids[] = {
+    { "adt7410", adt7410, },
+    { /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i2c, adt7410_ids);
+
+/* Return 0 if detection is successful, -ENODEV otherwise
+ * very basic detection */
+static int adt7410_detect(struct i2c_client *new_client,
+               struct i2c_board_info *info)
+{
+    struct i2c_adapter *adapter = new_client->adapter;
+    int ret = 0;
+    u8 mid;
+    if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+                     I2C_FUNC_SMBUS_WORD_DATA))
+        return -ENODEV;
+
+    ret = adt7410_i2c_read_byte(new_client, ADT7410_ID, &mid);
+    if (ret)
+        return -EIO;
+
+    if ((mid >> ADT7410_MANUFACTORE_ID_OFFSET) != ADT7410_MANUFACTURE_ID)
+        return -ENODEV;
+
+    strlcpy(info->type, "adt7410", I2C_NAME_SIZE);
+
+    return 0;
+}
+
+#ifdef CONFIG_PM
+static int adt7410_suspend(struct device *dev)
+{
+    int ret = 0;
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    ret = adt7410_i2c_write_byte(client, ADT7410_CONFIG,
+                     (data->config | ADT7410_PD));
+    if (ret)
+        return -EIO;
+    return 0;
+}
+
+static int adt7410_resume(struct device *dev)
+{
+    int ret = 0;
+    struct i2c_client *client = to_i2c_client(dev);
+    struct adt7410_data *data = i2c_get_clientdata(client);
+    ret = adt7410_i2c_write_byte(client, ADT7410_CONFIG, data->config);
+    if (ret)
+        return -EIO;
+    return 0;
+}
+
+static const struct dev_pm_ops adt7410_dev_pm_ops = {
+    .suspend    = adt7410_suspend,
+    .resume        = adt7410_resume,
+};
+#define ADT7410_DEV_PM_OPS (&adt7410_dev_pm_ops)
+#else
+#define ADT7410_DEV_PM_OPS NULL
+#endif /* CONFIG_PM */
+
+static struct i2c_driver adt7410_driver = {
+    .class        = I2C_CLASS_HWMON,
+    .driver = {
+        .name    = "adt7410",
+        .pm    = ADT7410_DEV_PM_OPS,
+    },
+    .probe        = adt7410_probe,
+    .remove        = adt7410_remove,
+    .id_table    = adt7410_ids,
+    .detect        = adt7410_detect,
+    .address_list    = normal_i2c,
+};
+
+module_i2c_driver(adt7410_driver);
+
+MODULE_AUTHOR("Hartmut Knaack <knaack.h <at> gmx.de>");
+MODULE_DESCRIPTION("ADT7410 driver");
+MODULE_LICENSE("GPL");


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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [lm-sensors] [PATCH] hwmon: Driver for ADT7410
  2012-07-31 20:17 [lm-sensors] [PATCH] hwmon: Driver for ADT7410 Hartmut Knaack
@ 2012-07-31 23:24 ` Guenter Roeck
  2012-08-03 11:20 ` Hartmut Knaack
  2012-08-04  5:30 ` Guenter Roeck
  2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-07-31 23:24 UTC (permalink / raw)
  To: lm-sensors

On Tue, Jul 31, 2012 at 10:17:18PM +0200, Hartmut Knaack wrote:
> Hi,
> this patch brings basic support for the Analog Devices ADT7410 temperature sensor, based on the lm75 hwmon driver and the adt7410 iio driver. The following functionality has been implemented:
> 
>   * get current temperature
>   * get/set minimum, maximum and critical temperature
>   * get/set hysteresis
>   * get alarm events for minimum, maximum and critical temperature
> 
> The ADT7410 provides some more configuration options, but would need non-standard sysfs symbols, so I prefer to get some feedback before implementing any:
> 
>   * get/set resolution (13/16 bits): temp#_resolution (13/16)?
>   * get/set event mode (comparator/interrupt): temp#_eventmode (0/1)?
>   * get/set polarity of INT pin (active-high/active-low): temp#_INT (0/1)?
>   * get/set polarity of CT pin (active-high/active-low): temp#_CT (0/1)?
>   * get/set sampling mode (continous/one sample per second/one-shot/power-down): temp#_mode (0/1/2/3)?
>   * get/set fault queue (1 to 4 faults to trigger event): temp#_faultqueue (1/2/3/4)?
>   * get device id: temp#_id?
> 
> Besides, I saw that there are several sensors (including the ADT7410) which have just one hysteresis register, that stores a delta value that applies to min, max and critical temperature. How are the chances to create a standard symbol "temp#_hyst"?
> 
> All implemented sysfs-symbols have been sucessfully tested at temperatures of 15°C to 40°C.
> 
> Signed-off-by: Hartmut Knaack <knaack.h <at> gmx.de>

Hi,

your mailer replaced all tabs with spaces, making a real review quite difficult.
Please fix that.

Couple of comments (not a complete review):

- For configuration, we commonly assume it is taken care of by the BIOS. Other
  that that, if you think anything needs to be configurable, use platform data
  and/or device tree information. No sysfs attributes, please.
- Hysteresis is an absolute temperature, not a relative one. So the question for
  a generis attribute would be what it applies to. The current approach does not
  have that problem.
- Please use devm_ functions to allocate resources.
- The wrappers for adt7410_i2c_write_byte and adt7410_i2c_read_byte are really
  unnecessary. Please drop. You don't need error messages here, hopefully.
- Use i2c_smbus_read_word_swapped and i2c_smbus_write_word_swapped for the 16
  bit functions instead of the local wrapper functions.
- Using "char" for "valid" makes the code more complex for most platforms.
  Consider bool instead.
- Your code hides error returns from i2c access functions by returning EIO
  instead of the actual error. Please don't do that.
- The access retry code in adt7410_update_device is really quite complex.
  Is this really needed ? In general, the sensor should be configured for
  continuous conversion, and it should not be necessary to wait for a conversion
  to complete. besides, we can not afford an active 240 ms wait loop, so if the
  loop should _really_ be needed, you would have to add a sleep function call.
- The detect function is quite weak. you should definitely also check bits 0..3
  of the status register (must be 0), and consider checking against known chip
  revision numbers. Question is also if the chip is expected to show up
  in a PC type system; if not, it might make sense to drop the detect function
  entirely, given its weakness.
- Use SENSOR_DEVICE_ATTR_2 to encode secondary index values (instead of multiple
  show_xxx_alarm functions).
- Don't initialize variables unnecessarily.
- set_mask and clr_mask in the probe function are really not necessary.
- Please align function parameters to the opening (.
- No unnecessary ( ), please [no, it does not make the code easier to read,
  it just hides the really important ( )s].
- Please run the patch through checkpatch.pl. I can not really say for sure
  due to the tab problem how many issues there  are, but I see at least one
  checkpatch error.

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] 4+ messages in thread

* Re: [lm-sensors] [PATCH] hwmon: Driver for ADT7410
  2012-07-31 20:17 [lm-sensors] [PATCH] hwmon: Driver for ADT7410 Hartmut Knaack
  2012-07-31 23:24 ` Guenter Roeck
@ 2012-08-03 11:20 ` Hartmut Knaack
  2012-08-04  5:30 ` Guenter Roeck
  2 siblings, 0 replies; 4+ messages in thread
From: Hartmut Knaack @ 2012-08-03 11:20 UTC (permalink / raw)
  To: lm-sensors

Guenter Roeck schrieb:
> Hi,
>
> your mailer replaced all tabs with spaces, making a real review quite difficult.
> Please fix that.
It likes to do that, but I fixed it :-)
>
> Couple of comments (not a complete review):
>
> - For configuration, we commonly assume it is taken care of by the BIOS. Other
>   that that, if you think anything needs to be configurable, use platform data
>   and/or device tree information. No sysfs attributes, please.
I tend to regard this from the users point of view. My aim is to have it supported in openwrt, where you get your ready-to-flash image and install your custom kernel modules and software. It's fine to have some valid default configuration, maybe set by those ways you mentioned. But I would prefer to have some chance of runtime configuration. BIOS/bootloader is something you don't really want to mess with, platform data requires kernel (module) compilation, from what I know. Device tree might work, though I'm pretty unexperienced with that, yet.
> - Hysteresis is an absolute temperature, not a relative one. So the question for
>   a generis attribute would be what it applies to. The current approach does not
>   have that problem.
From what I see, most sensors store hysteresis as absolute temperature (like lm75). But some store one offset values (like adt7410, adt7475), which have to get converted by the driver to appear like absolute values. In case of the adt7410, the offset applies to min, max and crit, and in my driver it is set with max_hyst (since I think that is the most common use). But if someone would only want to use alarm for min, it would be necessary to check/set max and set max_hyst, which I think is a bit confusing.
> - Please use devm_ functions to allocate resources.
fixed
> - The wrappers for adt7410_i2c_write_byte and adt7410_i2c_read_byte are really
>   unnecessary. Please drop. You don't need error messages here, hopefully.
fixed
> - Use i2c_smbus_read_word_swapped and i2c_smbus_write_word_swapped for the 16
>   bit functions instead of the local wrapper functions.
fixed
> - Using "char" for "valid" makes the code more complex for most platforms.
>   Consider bool instead.
fixed
> - Your code hides error returns from i2c access functions by returning EIO
>   instead of the actual error. Please don't do that.
fixed
> - The access retry code in adt7410_update_device is really quite complex.
>   Is this really needed ? In general, the sensor should be configured for
>   continuous conversion, and it should not be necessary to wait for a conversion
>   to complete. besides, we can not afford an active 240 ms wait loop, so if the
>   loop should _really_ be needed, you would have to add a sleep function call.
Changed to sleep up to 4 times for 100 ms.
> - The detect function is quite weak. you should definitely also check bits 0..3
>   of the status register (must be 0), and consider checking against known chip
>   revision numbers. Question is also if the chip is expected to show up
>   in a PC type system; if not, it might make sense to drop the detect function
>   entirely, given its weakness.
Added those checks. I personally would not need the detect function, just added it for completeness.
> - Use SENSOR_DEVICE_ATTR_2 to encode secondary index values (instead of multiple
>   show_xxx_alarm functions).
fixed
> - Don't initialize variables unnecessarily.
fixed
> - set_mask and clr_mask in the probe function are really not necessary.
fixed
> - Please align function parameters to the opening (.
fixed
> - No unnecessary ( ), please [no, it does not make the code easier to read,
>   it just hides the really important ( )s].
hopefully fixed.
> - Please run the patch through checkpatch.pl. I can not really say for sure
>   due to the tab problem how many issues there  are, but I see at least one
>   checkpatch error.
It complained about unneccessary parenthesis, before, but I thought it was safer to keep them.
> 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] 4+ messages in thread

* Re: [lm-sensors] [PATCH] hwmon: Driver for ADT7410
  2012-07-31 20:17 [lm-sensors] [PATCH] hwmon: Driver for ADT7410 Hartmut Knaack
  2012-07-31 23:24 ` Guenter Roeck
  2012-08-03 11:20 ` Hartmut Knaack
@ 2012-08-04  5:30 ` Guenter Roeck
  2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-08-04  5:30 UTC (permalink / raw)
  To: lm-sensors

Hi Hartmut,

On Fri, Aug 03, 2012 at 01:20:13PM +0200, Hartmut Knaack wrote:
> Guenter Roeck schrieb:
> > Hi,
> >
> > your mailer replaced all tabs with spaces, making a real review quite difficult.
> > Please fix that.
> It likes to do that, but I fixed it :-)
> >
> > Couple of comments (not a complete review):
> >
> > - For configuration, we commonly assume it is taken care of by the BIOS. Other
> >   that that, if you think anything needs to be configurable, use platform data
> >   and/or device tree information. No sysfs attributes, please.
> I tend to regard this from the users point of view. My aim is to have it supported in openwrt, where you get your ready-to-flash image and install your custom kernel modules and software. It's fine to have some valid default configuration, maybe set by those ways you mentioned. But I would prefer to have some chance of runtime configuration. BIOS/bootloader is something you don't really want to mess with, platform data requires kernel (module) compilation, from what I know. Device tree might work, though I'm pretty unexperienced with that, yet.

Me too, but that is not an excuse. Ultimately, it is platform data, device tree,
or nothing.

> > - Hysteresis is an absolute temperature, not a relative one. So the question for
> >   a generis attribute would be what it applies to. The current approach does not
> >   have that problem.
> From what I see, most sensors store hysteresis as absolute temperature (like lm75). But some store one offset values (like adt7410, adt7475), which have to get converted by the driver to appear like absolute values. In case of the adt7410, the offset applies to min, max and crit, and in my driver it is set with max_hyst (since I think that is the most common use). But if someone would only want to use alarm for min, it would be necessary to check/set max and set max_hyst, which I think is a bit confusing.

I suggested to provide the hyst attribute for min, max, and crit. This way you
cover it all. Sure, there are three attributes for one register, but that is ok.
See the lm77 driver for an example.

> > - The access retry code in adt7410_update_device is really quite complex.
> >   Is this really needed ? In general, the sensor should be configured for
> >   continuous conversion, and it should not be necessary to wait for a conversion
> >   to complete. besides, we can not afford an active 240 ms wait loop, so if the
> >   loop should _really_ be needed, you would have to add a sleep function call.
> Changed to sleep up to 4 times for 100 ms.

I'll really want to know why you think this is necessary before I accept it. 

> > - The detect function is quite weak. you should definitely also check bits 0..3
> >   of the status register (must be 0), and consider checking against known chip
> >   revision numbers. Question is also if the chip is expected to show up
> >   in a PC type system; if not, it might make sense to drop the detect function
> >   entirely, given its weakness.
> Added those checks. I personally would not need the detect function, just added it for completeness.

I still don't feel comfortable with it. Do you have a use case where it is
needed, or, in other words, do you know of a PC type system with such a sensor ?
Otherwise it might be better to just drop it.

> > - No unnecessary ( ), please [no, it does not make the code easier to read,
> >   it just hides the really important ( )s].
> hopefully fixed.

Not all of them, though.

> > - Please run the patch through checkpatch.pl. I can not really say for sure
> >   due to the tab problem how many issues there  are, but I see at least one
> >   checkpatch error.
> It complained about unneccessary parenthesis, before, but I thought it was safer to keep them.

Extra parenthesis make the code more difficult to read and understand.
It gets difficult to find out what the coder really wanted to implement.
So, really, no, it is not safer, it adds risk.

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] 4+ messages in thread

end of thread, other threads:[~2012-08-04  5:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-31 20:17 [lm-sensors] [PATCH] hwmon: Driver for ADT7410 Hartmut Knaack
2012-07-31 23:24 ` Guenter Roeck
2012-08-03 11:20 ` Hartmut Knaack
2012-08-04  5:30 ` Guenter Roeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.