[PATCH v3] hwmon: Driver for Maxim MAX6697 and compatibles

[Guenter Roeck]

On Tue, Jan 15, 2013 at 10:50:34PM +0100, Jean Delvare wrote:
> 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 at roeck-us.net>
> > ---
> > 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...
> 
It does complain if one compiles with W=1 ... I keep forgetting to do that.

Thanks a lot again!

Guenter

> > +			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 at roeck-us.net>
> > + *
> > + * 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
>