Hi Guenter,
On Tue, 15 Jan 2013 11:25:04 -0800, Guenter Roeck wrote:
> Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693,
> MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors
>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v2:
> - Add suppport for platform data and devicetree based chip initialization
> - Drop S_IRUGOWU macro: s/S_IRUGOWU/S_IRUGO | S_IWUSR/
> v3:
> - Clarify devicetree documentation and platform data, indicating that all
> configuration information must be specified
> - Fix alert-mask and over-temperature-mask values in devicetree example
> - Align bit masks in configuration data with tempX index values
> - Fix memset size in initialization code
> - Handle extended temperature range for MAX6581 correctly
> - Make data caching time dependent on chip type and configuration data
> - Fix have_ext bit map for MAX6581
> - Sort entries in max6697_chip_data[] array alphabetically
> - Fix race condition when reading alarms
> - Define and use constants for second index in data->temp array
> - Rename show_temp11 to show_temp_input. Drop second index (always zero)
> - Use DIV_ROUND_CLOSEST to convert milli-degrees to degrees in set_temp
> - Handle error return in set_temp correctly
> - Make max6697_attributes a two-dimensional array to reduce risk of errors when
> accessing it
> - Drop temp1_fault as it is never used (local temperature sensor can not be
> faulty)
> - Fix reading extended-range-enable property
> - If no configuration data is specified, read extended range and resistance
> cancellation configuration from chip
> - Drop some unnecessary comments
> - Include linux/types.h in platform_data/max6697.h
> - Drop unused define MAX6697_MAX_CONFIG_REG
> - Add comments to fields in struct max6697_platform_data
> - Add support to configure beta compensation on MAX6693 and MAX6694
Thanks for the update. I have a few more comments:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/max6697.txt
> (...)
> +- extended-range-enable
> + Only valid for MAX6581. Set to enable extended temperature range.
> + Extended temperature will be disabled if not specified.
> +- beta-compensation-enable
> + Only valid for MAX6693 and MX6694. Set to enable beta compensation on
> + remote temperature channel 1.
> + beta compensation will be disabled if not specified.
Capital B.
> (...)
> +static ssize_t show_temp_input(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct max6697_data *data = max6697_update_device(dev);
> + int temp;
> +
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + temp = data->temp[index][MAX6697_TEMP_INPUT];
> + temp <<= 3;
> + temp |= data->temp[index][MAX6697_TEMP_EXT] >> 5;
> + temp -= data->temp_offset << 3;
You could save one shifting by reordering the operations.
> +
> + return sprintf(buf, "%d\n", temp * 125);
> +}
> (...)
> +static ssize_t set_temp(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + int nr = to_sensor_dev_attr_2(devattr)->nr;
> + int index = to_sensor_dev_attr_2(devattr)->index;
> + struct i2c_client *client = to_i2c_client(dev);
> + struct max6697_data *data = i2c_get_clientdata(client);
> + long temp;
> + int ret;
> +
> + ret = kstrtol(buf, 10, &temp);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&data->update_lock);
> + temp = clamp_val(DIV_ROUND_CLOSEST(temp, 1000), -data->temp_offset,
> + 127);
This 127 is OK for all chips but the MAX6851. I agree that the
temperature data format table in the datasheet looks wrong, but the
description of the EXTRANGE configuration bit description sheds some
light:
"Extended-Range Enable Bit. Set bit 1 to logic 1 to set the temperature and limit data
range to -64°C to +191°C. Set bit 1 to logic 0 to set the range to 0°C to +255°C."
> + temp += data->temp_offset;
> + data->temp[nr][index] = temp;
> + ret = i2c_smbus_write_byte_data(client,
> + index == 2 ? MAX6697_REG_MAX[nr]
> + : MAX6697_REG_CRIT[nr],
> + temp);
> + mutex_unlock(&data->update_lock);
> +
> + return ret < 0 ? ret : count;
> +}
> (...)
> +static int max6697_init_chip(struct i2c_client *client)
> +{
> + struct max6697_data *data = i2c_get_clientdata(client);
> + struct max6697_platform_data *pdata = dev_get_platdata(&client->dev);
> + struct max6697_platform_data p;
> + const struct max6697_chip_data *chip = data->chip;
> + int factor = chip->channels;
> + int ret;
> + u8 reg;
> +
> + /*
> + * Don't touch configuration if neither platform data nor OF
> + * configuration was specified. If that is the case, use the
> + * current chip configuration.
> + */
> + if (!pdata && !client->dev.of_node) {
> + reg = i2c_smbus_read_byte_data(client, MAX6697_REG_CONFIG);
> + if (reg < 0)
reg is unsigned. I don't get why gcc doesn't complain...
> + return reg;
> + if (data->type == max6581) {
> + data->temp_offset =
> + (reg & MAX6581_CONF_EXTENDED) ? 64 : 0;
data->temp_offset is 0 at this point so an "if" construct should be
more efficient.
> + reg = i2c_smbus_read_byte_data(client,
> + MAX6581_REG_RESISTANCE);
reg is unsigned.
> + if (reg < 0)
> + return reg;
> + factor += hweight8(reg);
> + } else {
> + if (reg & MAX6697_CONF_RESISTANCE)
> + factor++;
> + }
> + goto done;
> + }
> (...)
> --- /dev/null
> +++ b/include/linux/platform_data/max6697.h
> @@ -0,0 +1,36 @@
> +/*
> + * max6697.h
> + * Copyright (c) 2012 Guenter Roeck <linux@xxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef MAX6697_H
> +#define MAX6697_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * For all bit masks:
> + * bit 0: local temperature
> + * bit 1..7: remote temperatures
> + */
> +struct max6697_platform_data {
> + bool smbus_timeout_disable; /* set to disable smbus timeouts */
"SMBus"
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
