linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: (tmp401) Add support for TI TMP461
@ 2016-05-31 16:27 Andrew F. Davis
  2016-06-04  4:41 ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew F. Davis @ 2016-05-31 16:27 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare, Jonathan Corbet
  Cc: linux-hwmon, linux-doc, linux-kernel, Andrew F . Davis

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 Documentation/hwmon/tmp401 | 18 +++++++++--
 drivers/hwmon/Kconfig      |  2 +-
 drivers/hwmon/tmp401.c     | 81 ++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 92 insertions(+), 9 deletions(-)

diff --git a/Documentation/hwmon/tmp401 b/Documentation/hwmon/tmp401
index 711f75e..eaa2d3b 100644
--- a/Documentation/hwmon/tmp401
+++ b/Documentation/hwmon/tmp401
@@ -22,6 +22,9 @@ Supported chips:
     Prefix: 'tmp435'
     Addresses scanned: I2C 0x48 - 0x4f
     Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp435.html
+  * Texas Instruments TMP461
+    Prefix: 'tmp461'
+    Datasheet: http://www.ti.com/product/tmp461
 
 Authors:
          Hans de Goede <hdegoede@redhat.com>
@@ -31,8 +34,8 @@ Description
 -----------
 
 This driver implements support for Texas Instruments TMP401, TMP411,
-TMP431, TMP432 and TMP435 chips. These chips implement one or two remote
-and one local temperature sensors. Temperature is measured in degrees
+TMP431, TMP432, TMP435, and TMP461 chips. These chips implement one or two
+remote and one local temperature sensors. Temperature is measured in degrees
 Celsius. Resolution of the remote sensor is 0.0625 degree. Local
 sensor resolution can be set to 0.5, 0.25, 0.125 or 0.0625 degree (not
 supported by the driver so far, so using the default resolution of 0.5
@@ -55,3 +58,14 @@ some additional features.
 
 TMP432 is compatible with TMP401 and TMP431. It supports two external
 temperature sensors.
+
+TMP461 is compatible with TMP401. It supports offset and n-factor correction
+that is applied to the remote sensor.
+
+* Sensor offset values are temperature values
+
+  Exported via sysfs attribute tempX_offset
+
+* n-factor correction, values are in raw form as described in the datasheet
+
+  Exported via sysfs attribute tempX_nfactor
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ff94007..c571dcf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1561,7 +1561,7 @@ config SENSORS_TMP401
 	depends on I2C
 	help
 	  If you say yes here you get support for Texas Instruments TMP401,
-	  TMP411, TMP431, TMP432 and TMP435 temperature sensor chips.
+	  TMP411, TMP431, TMP432, TMP435, and TMP461 temperature sensor chips.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called tmp401.
diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index ccf4cff..280065b 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -47,7 +47,8 @@
 static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4c, 0x4d,
 	0x4e, 0x4f, I2C_CLIENT_END };
 
-enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
+enum chips { tmp401, tmp411, tmp431, tmp432, tmp435, tmp461 };
+
 
 /*
  * The TMP401 registers, note some registers have different addresses for
@@ -62,31 +63,37 @@ enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
 #define TMP401_MANUFACTURER_ID_REG		0xFE
 #define TMP401_DEVICE_ID_REG			0xFF
 
-static const u8 TMP401_TEMP_MSB_READ[6][2] = {
+static const u8 TMP401_TEMP_MSB_READ[8][2] = {
 	{ 0x00, 0x01 },	/* temp */
 	{ 0x06, 0x08 },	/* low limit */
 	{ 0x05, 0x07 },	/* high limit */
 	{ 0x20, 0x19 },	/* therm (crit) limit */
 	{ 0x30, 0x34 },	/* lowest */
 	{ 0x32, 0x36 },	/* highest */
+	{ 0, 0x11 },	/* offset */
+	{ 0, 0x23 },	/* nfactor */
 };
 
-static const u8 TMP401_TEMP_MSB_WRITE[6][2] = {
+static const u8 TMP401_TEMP_MSB_WRITE[8][2] = {
 	{ 0, 0 },	/* temp (unused) */
 	{ 0x0C, 0x0E },	/* low limit */
 	{ 0x0B, 0x0D },	/* high limit */
 	{ 0x20, 0x19 },	/* therm (crit) limit */
 	{ 0x30, 0x34 },	/* lowest */
 	{ 0x32, 0x36 },	/* highest */
+	{ 0, 0x11 },	/* offset */
+	{ 0, 0x23 },	/* nfactor */
 };
 
-static const u8 TMP401_TEMP_LSB[6][2] = {
+static const u8 TMP401_TEMP_LSB[8][2] = {
 	{ 0x15, 0x10 },	/* temp */
 	{ 0x17, 0x14 },	/* low limit */
 	{ 0x16, 0x13 },	/* high limit */
 	{ 0, 0 },	/* therm (crit) limit (unused) */
 	{ 0x31, 0x35 },	/* lowest */
 	{ 0x33, 0x37 },	/* highest */
+	{ 0, 0x12 },	/* offset */
+	{ 0, 0 },	/* nfactor (unused) */
 };
 
 static const u8 TMP432_TEMP_MSB_READ[4][3] = {
@@ -149,6 +156,7 @@ static const struct i2c_device_id tmp401_id[] = {
 	{ "tmp431", tmp431 },
 	{ "tmp432", tmp432 },
 	{ "tmp435", tmp435 },
+	{ "tmp461", tmp461 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tmp401_id);
@@ -170,7 +178,7 @@ struct tmp401_data {
 	/* register values */
 	u8 status[4];
 	u8 config;
-	u16 temp[6][3];
+	u16 temp[8][3];
 	u8 temp_crit_hyst;
 };
 
@@ -445,6 +453,44 @@ static ssize_t reset_temp_history(struct device *dev,
 	return count;
 }
 
+static ssize_t show_nfactor(struct device *dev,
+			    struct device_attribute *devattr, char *buf)
+{
+	int nr = to_sensor_dev_attr_2(devattr)->nr;
+	int index = to_sensor_dev_attr_2(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return sprintf(buf, "%d\n", data->temp[nr][index]);
+}
+
+static ssize_t store_nfactor(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 tmp401_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	long val;
+	u8 regaddr;
+
+	if (kstrtol(buf, 10, &val))
+		return -EINVAL;
+	val = clamp_val(val, -128, 127);
+
+	regaddr = TMP401_TEMP_MSB_WRITE[nr][index];
+
+	mutex_lock(&data->update_lock);
+	i2c_smbus_write_byte_data(client, regaddr, val);
+	data->temp[nr][index] = val;
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
 static ssize_t show_update_interval(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
@@ -613,6 +659,26 @@ static const struct attribute_group tmp432_group = {
 };
 
 /*
+ * Additional features of the TMP461 chip.
+ * The TMP461 η-factor correction and temperature offset
+ * for the remote channel.
+ */
+static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 6, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_nfactor, S_IWUSR | S_IRUGO, show_nfactor,
+			    store_nfactor, 7, 1);
+
+static struct attribute *tmp461_attributes[] = {
+	&sensor_dev_attr_temp2_offset.dev_attr.attr,
+	&sensor_dev_attr_temp2_nfactor.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group tmp461_group = {
+	.attrs = tmp461_attributes,
+};
+
+/*
  * Begin non sysfs callback code (aka Real code)
  */
 
@@ -714,7 +780,7 @@ static int tmp401_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	static const char * const names[] = {
-		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
+		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435", "TMP461"
 	};
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
@@ -745,6 +811,9 @@ static int tmp401_probe(struct i2c_client *client,
 	if (data->kind == tmp432)
 		data->groups[groups++] = &tmp432_group;
 
+	if (data->kind == tmp461)
+		data->groups[groups++] = &tmp461_group;
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   data, data->groups);
 	if (IS_ERR(hwmon_dev))
-- 
2.8.3

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

* Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461
  2016-05-31 16:27 [PATCH] hwmon: (tmp401) Add support for TI TMP461 Andrew F. Davis
@ 2016-06-04  4:41 ` Guenter Roeck
  2016-06-08 16:56   ` Andrew F. Davis
  0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2016-06-04  4:41 UTC (permalink / raw)
  To: Andrew F. Davis, Jean Delvare, Jonathan Corbet
  Cc: linux-hwmon, linux-doc, linux-kernel

On 05/31/2016 09:27 AM, Andrew F. Davis wrote:
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>   Documentation/hwmon/tmp401 | 18 +++++++++--
>   drivers/hwmon/Kconfig      |  2 +-
>   drivers/hwmon/tmp401.c     | 81 ++++++++++++++++++++++++++++++++++++++++++----
>   3 files changed, 92 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/hwmon/tmp401 b/Documentation/hwmon/tmp401
> index 711f75e..eaa2d3b 100644
> --- a/Documentation/hwmon/tmp401
> +++ b/Documentation/hwmon/tmp401
> @@ -22,6 +22,9 @@ Supported chips:
>       Prefix: 'tmp435'
>       Addresses scanned: I2C 0x48 - 0x4f
>       Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp435.html
> +  * Texas Instruments TMP461
> +    Prefix: 'tmp461'
> +    Datasheet: http://www.ti.com/product/tmp461
>
>   Authors:
>            Hans de Goede <hdegoede@redhat.com>
> @@ -31,8 +34,8 @@ Description
>   -----------
>
>   This driver implements support for Texas Instruments TMP401, TMP411,
> -TMP431, TMP432 and TMP435 chips. These chips implement one or two remote
> -and one local temperature sensors. Temperature is measured in degrees
> +TMP431, TMP432, TMP435, and TMP461 chips. These chips implement one or two
> +remote and one local temperature sensors. Temperature is measured in degrees
>   Celsius. Resolution of the remote sensor is 0.0625 degree. Local
>   sensor resolution can be set to 0.5, 0.25, 0.125 or 0.0625 degree (not
>   supported by the driver so far, so using the default resolution of 0.5
> @@ -55,3 +58,14 @@ some additional features.
>
>   TMP432 is compatible with TMP401 and TMP431. It supports two external
>   temperature sensors.
> +
> +TMP461 is compatible with TMP401. It supports offset and n-factor correction
> +that is applied to the remote sensor.
> +
> +* Sensor offset values are temperature values
> +
> +  Exported via sysfs attribute tempX_offset
> +
> +* n-factor correction, values are in raw form as described in the datasheet
> +

If you don't mind, I would prefer if you would drop this attribute, at least
for now, for a number of reasons. It should not really be controlled by a sysfs
attribute, but either with devicetree data or platform data. Exposing a raw
register value through sysfs isn't really desirable; sysfs is supposed
to provide some kind of abstraction. It enables setting the n-factor,
but not beta-compensation which is probably equally desirable.
Last but not least, other chips supported by this driver, as well as other chips
supported by hwmon, also support  n-factor correction and beta-compensation.
If we provide this functionality we should provide it for all chips in a
generic way. In short, this needs more discussion.

Couple of additional additional comments below.

Thanks,
Guenter


> +  Exported via sysfs attribute tempX_nfactor
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index ff94007..c571dcf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1561,7 +1561,7 @@ config SENSORS_TMP401
>   	depends on I2C
>   	help
>   	  If you say yes here you get support for Texas Instruments TMP401,
> -	  TMP411, TMP431, TMP432 and TMP435 temperature sensor chips.
> +	  TMP411, TMP431, TMP432, TMP435, and TMP461 temperature sensor chips.
>
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called tmp401.
> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
> index ccf4cff..280065b 100644
> --- a/drivers/hwmon/tmp401.c
> +++ b/drivers/hwmon/tmp401.c
> @@ -47,7 +47,8 @@
>   static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4c, 0x4d,
>   	0x4e, 0x4f, I2C_CLIENT_END };
>
> -enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
> +enum chips { tmp401, tmp411, tmp431, tmp432, tmp435, tmp461 };
> +
>
Please no double empty lines.

>   /*
>    * The TMP401 registers, note some registers have different addresses for
> @@ -62,31 +63,37 @@ enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
>   #define TMP401_MANUFACTURER_ID_REG		0xFE
>   #define TMP401_DEVICE_ID_REG			0xFF
>
> -static const u8 TMP401_TEMP_MSB_READ[6][2] = {
> +static const u8 TMP401_TEMP_MSB_READ[8][2] = {
>   	{ 0x00, 0x01 },	/* temp */
>   	{ 0x06, 0x08 },	/* low limit */
>   	{ 0x05, 0x07 },	/* high limit */
>   	{ 0x20, 0x19 },	/* therm (crit) limit */
>   	{ 0x30, 0x34 },	/* lowest */
>   	{ 0x32, 0x36 },	/* highest */
> +	{ 0, 0x11 },	/* offset */
> +	{ 0, 0x23 },	/* nfactor */
>   };
>
> -static const u8 TMP401_TEMP_MSB_WRITE[6][2] = {
> +static const u8 TMP401_TEMP_MSB_WRITE[8][2] = {
>   	{ 0, 0 },	/* temp (unused) */
>   	{ 0x0C, 0x0E },	/* low limit */
>   	{ 0x0B, 0x0D },	/* high limit */
>   	{ 0x20, 0x19 },	/* therm (crit) limit */
>   	{ 0x30, 0x34 },	/* lowest */
>   	{ 0x32, 0x36 },	/* highest */
> +	{ 0, 0x11 },	/* offset */
> +	{ 0, 0x23 },	/* nfactor */
>   };
>
> -static const u8 TMP401_TEMP_LSB[6][2] = {
> +static const u8 TMP401_TEMP_LSB[8][2] = {
>   	{ 0x15, 0x10 },	/* temp */
>   	{ 0x17, 0x14 },	/* low limit */
>   	{ 0x16, 0x13 },	/* high limit */
>   	{ 0, 0 },	/* therm (crit) limit (unused) */
>   	{ 0x31, 0x35 },	/* lowest */
>   	{ 0x33, 0x37 },	/* highest */
> +	{ 0, 0x12 },	/* offset */
> +	{ 0, 0 },	/* nfactor (unused) */
>   };
>
>   static const u8 TMP432_TEMP_MSB_READ[4][3] = {
> @@ -149,6 +156,7 @@ static const struct i2c_device_id tmp401_id[] = {
>   	{ "tmp431", tmp431 },
>   	{ "tmp432", tmp432 },
>   	{ "tmp435", tmp435 },
> +	{ "tmp461", tmp461 },

Please also provide code in the detect function to auto-detect the chip.

>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, tmp401_id);
> @@ -170,7 +178,7 @@ struct tmp401_data {
>   	/* register values */
>   	u8 status[4];
>   	u8 config;
> -	u16 temp[6][3];
> +	u16 temp[8][3];
>   	u8 temp_crit_hyst;
>   };
>
> @@ -445,6 +453,44 @@ static ssize_t reset_temp_history(struct device *dev,
>   	return count;
>   }
>
> +static ssize_t show_nfactor(struct device *dev,
> +			    struct device_attribute *devattr, char *buf)
> +{
> +	int nr = to_sensor_dev_attr_2(devattr)->nr;
> +	int index = to_sensor_dev_attr_2(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	return sprintf(buf, "%d\n", data->temp[nr][index]);
> +}
> +
> +static ssize_t store_nfactor(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 tmp401_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	long val;
> +	u8 regaddr;
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +	val = clamp_val(val, -128, 127);
> +
> +	regaddr = TMP401_TEMP_MSB_WRITE[nr][index];
> +
> +	mutex_lock(&data->update_lock);
> +	i2c_smbus_write_byte_data(client, regaddr, val);
> +	data->temp[nr][index] = val;
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
>   static ssize_t show_update_interval(struct device *dev,
>   				    struct device_attribute *attr, char *buf)
>   {
> @@ -613,6 +659,26 @@ static const struct attribute_group tmp432_group = {
>   };
>
>   /*
> + * Additional features of the TMP461 chip.
> + * The TMP461 η-factor correction and temperature offset
> + * for the remote channel.
> + */
> +static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp,
> +			    store_temp, 6, 1);
> +static SENSOR_DEVICE_ATTR_2(temp2_nfactor, S_IWUSR | S_IRUGO, show_nfactor,
> +			    store_nfactor, 7, 1);
> +
> +static struct attribute *tmp461_attributes[] = {
> +	&sensor_dev_attr_temp2_offset.dev_attr.attr,
> +	&sensor_dev_attr_temp2_nfactor.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group tmp461_group = {
> +	.attrs = tmp461_attributes,
> +};
> +
> +/*
>    * Begin non sysfs callback code (aka Real code)
>    */
>
> @@ -714,7 +780,7 @@ static int tmp401_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
>   	static const char * const names[] = {
> -		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
> +		"TMP401", "TMP411", "TMP431", "TMP432", "TMP435", "TMP461"
>   	};
>   	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
> @@ -745,6 +811,9 @@ static int tmp401_probe(struct i2c_client *client,
>   	if (data->kind == tmp432)
>   		data->groups[groups++] = &tmp432_group;
>
> +	if (data->kind == tmp461)
> +		data->groups[groups++] = &tmp461_group;
> +
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>   							   data, data->groups);
>   	if (IS_ERR(hwmon_dev))
>

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

* Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461
  2016-06-04  4:41 ` Guenter Roeck
@ 2016-06-08 16:56   ` Andrew F. Davis
  2016-06-08 17:43     ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew F. Davis @ 2016-06-08 16:56 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare, Jonathan Corbet
  Cc: linux-hwmon, linux-doc, linux-kernel

On 06/03/2016 11:41 PM, Guenter Roeck wrote:
> On 05/31/2016 09:27 AM, Andrew F. Davis wrote:
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   Documentation/hwmon/tmp401 | 18 +++++++++--
>>   drivers/hwmon/Kconfig      |  2 +-
>>   drivers/hwmon/tmp401.c     | 81
>> ++++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 92 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/hwmon/tmp401 b/Documentation/hwmon/tmp401
>> index 711f75e..eaa2d3b 100644
>> --- a/Documentation/hwmon/tmp401
>> +++ b/Documentation/hwmon/tmp401
>> @@ -22,6 +22,9 @@ Supported chips:
>>       Prefix: 'tmp435'
>>       Addresses scanned: I2C 0x48 - 0x4f
>>       Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp435.html
>> +  * Texas Instruments TMP461
>> +    Prefix: 'tmp461'
>> +    Datasheet: http://www.ti.com/product/tmp461
>>
>>   Authors:
>>            Hans de Goede <hdegoede@redhat.com>
>> @@ -31,8 +34,8 @@ Description
>>   -----------
>>
>>   This driver implements support for Texas Instruments TMP401, TMP411,
>> -TMP431, TMP432 and TMP435 chips. These chips implement one or two remote
>> -and one local temperature sensors. Temperature is measured in degrees
>> +TMP431, TMP432, TMP435, and TMP461 chips. These chips implement one
>> or two
>> +remote and one local temperature sensors. Temperature is measured in
>> degrees
>>   Celsius. Resolution of the remote sensor is 0.0625 degree. Local
>>   sensor resolution can be set to 0.5, 0.25, 0.125 or 0.0625 degree (not
>>   supported by the driver so far, so using the default resolution of 0.5
>> @@ -55,3 +58,14 @@ some additional features.
>>
>>   TMP432 is compatible with TMP401 and TMP431. It supports two external
>>   temperature sensors.
>> +
>> +TMP461 is compatible with TMP401. It supports offset and n-factor
>> correction
>> +that is applied to the remote sensor.
>> +
>> +* Sensor offset values are temperature values
>> +
>> +  Exported via sysfs attribute tempX_offset
>> +
>> +* n-factor correction, values are in raw form as described in the
>> datasheet
>> +
> 
> If you don't mind, I would prefer if you would drop this attribute, at
> least
> for now, for a number of reasons. It should not really be controlled by
> a sysfs
> attribute, but either with devicetree data or platform data. Exposing a raw
> register value through sysfs isn't really desirable; sysfs is supposed
> to provide some kind of abstraction. It enables setting the n-factor,
> but not beta-compensation which is probably equally desirable.
> Last but not least, other chips supported by this driver, as well as
> other chips
> supported by hwmon, also support  n-factor correction and
> beta-compensation.
> If we provide this functionality we should provide it for all chips in a
> generic way. In short, this needs more discussion.
> 

Makes sense, will drop this attribute for now.

> Couple of additional additional comments below.
> 
> Thanks,
> Guenter
> 
> 
>> +  Exported via sysfs attribute tempX_nfactor
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index ff94007..c571dcf 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1561,7 +1561,7 @@ config SENSORS_TMP401
>>       depends on I2C
>>       help
>>         If you say yes here you get support for Texas Instruments TMP401,
>> -      TMP411, TMP431, TMP432 and TMP435 temperature sensor chips.
>> +      TMP411, TMP431, TMP432, TMP435, and TMP461 temperature sensor
>> chips.
>>
>>         This driver can also be built as a module.  If so, the module
>>         will be called tmp401.
>> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
>> index ccf4cff..280065b 100644
>> --- a/drivers/hwmon/tmp401.c
>> +++ b/drivers/hwmon/tmp401.c
>> @@ -47,7 +47,8 @@
>>   static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4c,
>> 0x4d,
>>       0x4e, 0x4f, I2C_CLIENT_END };
>>
>> -enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
>> +enum chips { tmp401, tmp411, tmp431, tmp432, tmp435, tmp461 };
>> +
>>
> Please no double empty lines.
> 

ACK

>>   /*
>>    * The TMP401 registers, note some registers have different
>> addresses for
>> @@ -62,31 +63,37 @@ enum chips { tmp401, tmp411, tmp431, tmp432,
>> tmp435 };
>>   #define TMP401_MANUFACTURER_ID_REG        0xFE
>>   #define TMP401_DEVICE_ID_REG            0xFF
>>
>> -static const u8 TMP401_TEMP_MSB_READ[6][2] = {
>> +static const u8 TMP401_TEMP_MSB_READ[8][2] = {
>>       { 0x00, 0x01 },    /* temp */
>>       { 0x06, 0x08 },    /* low limit */
>>       { 0x05, 0x07 },    /* high limit */
>>       { 0x20, 0x19 },    /* therm (crit) limit */
>>       { 0x30, 0x34 },    /* lowest */
>>       { 0x32, 0x36 },    /* highest */
>> +    { 0, 0x11 },    /* offset */
>> +    { 0, 0x23 },    /* nfactor */
>>   };
>>
>> -static const u8 TMP401_TEMP_MSB_WRITE[6][2] = {
>> +static const u8 TMP401_TEMP_MSB_WRITE[8][2] = {
>>       { 0, 0 },    /* temp (unused) */
>>       { 0x0C, 0x0E },    /* low limit */
>>       { 0x0B, 0x0D },    /* high limit */
>>       { 0x20, 0x19 },    /* therm (crit) limit */
>>       { 0x30, 0x34 },    /* lowest */
>>       { 0x32, 0x36 },    /* highest */
>> +    { 0, 0x11 },    /* offset */
>> +    { 0, 0x23 },    /* nfactor */
>>   };
>>
>> -static const u8 TMP401_TEMP_LSB[6][2] = {
>> +static const u8 TMP401_TEMP_LSB[8][2] = {
>>       { 0x15, 0x10 },    /* temp */
>>       { 0x17, 0x14 },    /* low limit */
>>       { 0x16, 0x13 },    /* high limit */
>>       { 0, 0 },    /* therm (crit) limit (unused) */
>>       { 0x31, 0x35 },    /* lowest */
>>       { 0x33, 0x37 },    /* highest */
>> +    { 0, 0x12 },    /* offset */
>> +    { 0, 0 },    /* nfactor (unused) */
>>   };
>>
>>   static const u8 TMP432_TEMP_MSB_READ[4][3] = {
>> @@ -149,6 +156,7 @@ static const struct i2c_device_id tmp401_id[] = {
>>       { "tmp431", tmp431 },
>>       { "tmp432", tmp432 },
>>       { "tmp435", tmp435 },
>> +    { "tmp461", tmp461 },
> 
> Please also provide code in the detect function to auto-detect the chip.
> 

It looks like the ID reg has been removed (at least from the datasheet)
so I'm not sure there is any good way to ID this part.

Andrew

>>       { }
>>   };
>>   MODULE_DEVICE_TABLE(i2c, tmp401_id);
>> @@ -170,7 +178,7 @@ struct tmp401_data {
>>       /* register values */
>>       u8 status[4];
>>       u8 config;
>> -    u16 temp[6][3];
>> +    u16 temp[8][3];
>>       u8 temp_crit_hyst;
>>   };
>>
>> @@ -445,6 +453,44 @@ static ssize_t reset_temp_history(struct device
>> *dev,
>>       return count;
>>   }
>>
>> +static ssize_t show_nfactor(struct device *dev,
>> +                struct device_attribute *devattr, char *buf)
>> +{
>> +    int nr = to_sensor_dev_attr_2(devattr)->nr;
>> +    int index = to_sensor_dev_attr_2(devattr)->index;
>> +    struct tmp401_data *data = tmp401_update_device(dev);
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    return sprintf(buf, "%d\n", data->temp[nr][index]);
>> +}
>> +
>> +static ssize_t store_nfactor(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 tmp401_data *data = dev_get_drvdata(dev);
>> +    struct i2c_client *client = data->client;
>> +    long val;
>> +    u8 regaddr;
>> +
>> +    if (kstrtol(buf, 10, &val))
>> +        return -EINVAL;
>> +    val = clamp_val(val, -128, 127);
>> +
>> +    regaddr = TMP401_TEMP_MSB_WRITE[nr][index];
>> +
>> +    mutex_lock(&data->update_lock);
>> +    i2c_smbus_write_byte_data(client, regaddr, val);
>> +    data->temp[nr][index] = val;
>> +    mutex_unlock(&data->update_lock);
>> +
>> +    return count;
>> +}
>> +
>>   static ssize_t show_update_interval(struct device *dev,
>>                       struct device_attribute *attr, char *buf)
>>   {
>> @@ -613,6 +659,26 @@ static const struct attribute_group tmp432_group = {
>>   };
>>
>>   /*
>> + * Additional features of the TMP461 chip.
>> + * The TMP461 η-factor correction and temperature offset
>> + * for the remote channel.
>> + */
>> +static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp,
>> +                store_temp, 6, 1);
>> +static SENSOR_DEVICE_ATTR_2(temp2_nfactor, S_IWUSR | S_IRUGO,
>> show_nfactor,
>> +                store_nfactor, 7, 1);
>> +
>> +static struct attribute *tmp461_attributes[] = {
>> +    &sensor_dev_attr_temp2_offset.dev_attr.attr,
>> +    &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group tmp461_group = {
>> +    .attrs = tmp461_attributes,
>> +};
>> +
>> +/*
>>    * Begin non sysfs callback code (aka Real code)
>>    */
>>
>> @@ -714,7 +780,7 @@ static int tmp401_probe(struct i2c_client *client,
>>               const struct i2c_device_id *id)
>>   {
>>       static const char * const names[] = {
>> -        "TMP401", "TMP411", "TMP431", "TMP432", "TMP435"
>> +        "TMP401", "TMP411", "TMP431", "TMP432", "TMP435", "TMP461"
>>       };
>>       struct device *dev = &client->dev;
>>       struct device *hwmon_dev;
>> @@ -745,6 +811,9 @@ static int tmp401_probe(struct i2c_client *client,
>>       if (data->kind == tmp432)
>>           data->groups[groups++] = &tmp432_group;
>>
>> +    if (data->kind == tmp461)
>> +        data->groups[groups++] = &tmp461_group;
>> +
>>       hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>> client->name,
>>                                  data, data->groups);
>>       if (IS_ERR(hwmon_dev))
>>
> 

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

* Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461
  2016-06-08 16:56   ` Andrew F. Davis
@ 2016-06-08 17:43     ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2016-06-08 17:43 UTC (permalink / raw)
  To: Andrew F. Davis
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel

On Wed, Jun 08, 2016 at 11:56:29AM -0500, Andrew F. Davis wrote:

[ ... ]

> >>   static const u8 TMP432_TEMP_MSB_READ[4][3] = {
> >> @@ -149,6 +156,7 @@ static const struct i2c_device_id tmp401_id[] = {
> >>       { "tmp431", tmp431 },
> >>       { "tmp432", tmp432 },
> >>       { "tmp435", tmp435 },
> >> +    { "tmp461", tmp461 },
> > 
> > Please also provide code in the detect function to auto-detect the chip.
> > 
> 
> It looks like the ID reg has been removed (at least from the datasheet)
> so I'm not sure there is any good way to ID this part.
> 
Outch :-(

Well, nothing we can do about that. Even if the register was there and provides
a value, it would be undocumented and thus unreliable and unusable. Too bad.

Thanks,
Guenter

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

end of thread, other threads:[~2016-06-08 17:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-31 16:27 [PATCH] hwmon: (tmp401) Add support for TI TMP461 Andrew F. Davis
2016-06-04  4:41 ` Guenter Roeck
2016-06-08 16:56   ` Andrew F. Davis
2016-06-08 17:43     ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).