lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 3 Jun 2016 21:41:55 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	"Andrew F. Davis" <afd@...com>, Jean Delvare <jdelvare@...e.com>,
	Jonathan Corbet <corbet@....net>
Cc:	linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461

On 05/31/2016 09:27 AM, Andrew F. Davis wrote:
> Signed-off-by: Andrew F. Davis <afd@...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@...hat.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))
>

Powered by blists - more mailing lists