[lm-sensors] [PATCH v2 1/4] hwmon: (lm95245) Fix hysteresis temperatures

[lm-sensors] [PATCH v2 1/4] hwmon: (lm95245) Fix hysteresis temperatures

[Guenter Roeck]

Hysteresis temperatures are defined as absolute temperatures,
not as delta value from the critical temperatures.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Don't change indentation
    Don't call lm95245_update_device from set function
    In set function, actually write the calculated hysteresis
    instead of the entered value.

 drivers/hwmon/lm95245.c |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
index 3f0956e..d7bcea1 100644
--- a/drivers/hwmon/lm95245.c
+++ b/drivers/hwmon/lm95245.c
@@ -272,27 +272,39 @@ static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t show_crit_hyst(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct lm95245_data *data = lm95245_update_device(dev);
+	int index = to_sensor_dev_attr(attr)->index;
+	int hyst = data->regs[index] - data->regs[8];
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", hyst * 1000);
+}
+
 static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
 			const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm95245_data *data = i2c_get_clientdata(client);
+	int index = to_sensor_dev_attr(attr)->index;
 	unsigned long val;
+	long hyst;
+	int limit;
 
 	if (kstrtoul(buf, 10, &val) < 0)
 		return -EINVAL;
 
-	val /= 1000;
-
-	val = clamp_val(val, 0, 31);
-
 	mutex_lock(&data->update_lock);
 
+	limit = i2c_smbus_read_byte_data(client, lm95245_reg_address[index]);
+	hyst = limit - val / 1000;
+	hyst = clamp_val(hyst, 0, 31);
 	data->valid = 0;
 
 	/* shared crit hysteresis */
 	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
-		val);
+		hyst);
 
 	mutex_unlock(&data->update_lock);
 
@@ -378,16 +390,16 @@ static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
 		set_limit, 6);
-static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
-		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
+		set_crit_hyst, 6);
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
 		STATUS1_LOC);
 
 static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
 		set_limit, 7);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
-		set_crit_hyst, 8);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
+		set_crit_hyst, 7);
 static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
 		STATUS1_RTCRIT);
 static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
-- 
1.7.9.7


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

Re: [lm-sensors] [PATCH v2 1/4] hwmon: (lm95245) Fix hysteresis temperatures

[Jean Delvare]

Hi Guenter,

On Sat, 22 Feb 2014 09:30:09 -0800, Guenter Roeck wrote:
> Hysteresis temperatures are defined as absolute temperatures,
> not as delta value from the critical temperatures.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Don't change indentation
>     Don't call lm95245_update_device from set function
>     In set function, actually write the calculated hysteresis
>     instead of the entered value.
> 
>  drivers/hwmon/lm95245.c |   30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
> index 3f0956e..d7bcea1 100644
> --- a/drivers/hwmon/lm95245.c
> +++ b/drivers/hwmon/lm95245.c
> @@ -272,27 +272,39 @@ static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static ssize_t show_crit_hyst(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct lm95245_data *data = lm95245_update_device(dev);
> +	int index = to_sensor_dev_attr(attr)->index;
> +	int hyst = data->regs[index] - data->regs[8];
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", hyst * 1000);
> +}
> +
>  static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
>  			const char *buf, size_t count)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm95245_data *data = i2c_get_clientdata(client);
> +	int index = to_sensor_dev_attr(attr)->index;
>  	unsigned long val;
> +	long hyst;
> +	int limit;

Nitpicking: it makes little sense to have hyst and limit as different
types.

>  
>  	if (kstrtoul(buf, 10, &val) < 0)
>  		return -EINVAL;
>  
> -	val /= 1000;
> -
> -	val = clamp_val(val, 0, 31);
> -
>  	mutex_lock(&data->update_lock);
>  
> +	limit = i2c_smbus_read_byte_data(client, lm95245_reg_address[index]);
> +	hyst = limit - val / 1000;
> +	hyst = clamp_val(hyst, 0, 31);

Note that you don't actually need to hold the lock for that. Access to
the SMBus itself is already serialized at the bus driver level, and
you're not touching any field of the data structure (that's what the
lock is protecting.)

>  	data->valid = 0;

BTW do you have any idea why the whole cache is invalidated here? I'd
think that storing hyst as data->regs[8] would be enough?

>  
>  	/* shared crit hysteresis */
>  	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
> -		val);
> +		hyst);
>  
>  	mutex_unlock(&data->update_lock);
>  
> @@ -378,16 +390,16 @@ static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
>  		set_limit, 6);
> -static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
> -		set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
> +		set_crit_hyst, 6);
>  static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
>  		STATUS1_LOC);
>  
>  static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
>  static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
>  		set_limit, 7);
> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
> -		set_crit_hyst, 8);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
> +		set_crit_hyst, 7);
>  static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
>  		STATUS1_RTCRIT);
>  static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
Suse L3 Support

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

Re: [lm-sensors] [PATCH v2 1/4] hwmon: (lm95245) Fix hysteresis temperatures

[Guenter Roeck]

Hi Jean,

On 02/23/2014 01:49 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 22 Feb 2014 09:30:09 -0800, Guenter Roeck wrote:
>> Hysteresis temperatures are defined as absolute temperatures,
>> not as delta value from the critical temperatures.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Don't change indentation
>>      Don't call lm95245_update_device from set function
>>      In set function, actually write the calculated hysteresis
>>      instead of the entered value.
>>
>>   drivers/hwmon/lm95245.c |   30 +++++++++++++++++++++---------
>>   1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
>> index 3f0956e..d7bcea1 100644
>> --- a/drivers/hwmon/lm95245.c
>> +++ b/drivers/hwmon/lm95245.c
>> @@ -272,27 +272,39 @@ static ssize_t set_limit(struct device *dev, struct device_attribute *attr,
>>   	return count;
>>   }
>>
>> +static ssize_t show_crit_hyst(struct device *dev, struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	struct lm95245_data *data = lm95245_update_device(dev);
>> +	int index = to_sensor_dev_attr(attr)->index;
>> +	int hyst = data->regs[index] - data->regs[8];
>> +
>> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", hyst * 1000);
>> +}
>> +
>>   static ssize_t set_crit_hyst(struct device *dev, struct device_attribute *attr,
>>   			const char *buf, size_t count)
>>   {
>>   	struct i2c_client *client = to_i2c_client(dev);
>>   	struct lm95245_data *data = i2c_get_clientdata(client);
>> +	int index = to_sensor_dev_attr(attr)->index;
>>   	unsigned long val;
>> +	long hyst;
>> +	int limit;
>
> Nitpicking: it makes little sense to have hyst and limit as different
> types.
>
Ok, I'll change that.

>>
>>   	if (kstrtoul(buf, 10, &val) < 0)
>>   		return -EINVAL;
>>
>> -	val /= 1000;
>> -
>> -	val = clamp_val(val, 0, 31);
>> -
>>   	mutex_lock(&data->update_lock);
>>
>> +	limit = i2c_smbus_read_byte_data(client, lm95245_reg_address[index]);
>> +	hyst = limit - val / 1000;
>> +	hyst = clamp_val(hyst, 0, 31);
>
> Note that you don't actually need to hold the lock for that. Access to
> the SMBus itself is already serialized at the bus driver level, and
> you're not touching any field of the data structure (that's what the
> lock is protecting.)
>
I think it was because the valid flag is cleared, to make sure the data
is only re-read after the hysteresis register is written to.

>>   	data->valid = 0;
>
> BTW do you have any idea why the whole cache is invalidated here? I'd
> think that storing hyst as data->regs[8] would be enough?
>

It also ensures that the limit register value in the cache is current,
but since that is not volatile it should not really make a difference.

I'll change that and only update regs[8]. But then I do need the lock,
or at least I think so.

>>
>>   	/* shared crit hysteresis */
>>   	i2c_smbus_write_byte_data(client, LM95245_REG_RW_COMMON_HYSTERESIS,
>> -		val);
>> +		hyst);
>>
>>   	mutex_unlock(&data->update_lock);
>>
>> @@ -378,16 +390,16 @@ static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
>>   static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>>   static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_limit,
>>   		set_limit, 6);
>> -static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
>> -		set_crit_hyst, 8);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
>> +		set_crit_hyst, 6);
>>   static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL,
>>   		STATUS1_LOC);
>>
>>   static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
>>   static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_limit,
>>   		set_limit, 7);
>> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_limit,
>> -		set_crit_hyst, 8);
>> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_crit_hyst,
>> +		set_crit_hyst, 7);
>>   static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL,
>>   		STATUS1_RTCRIT);
>>   static SENSOR_DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type,
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

Thanks!

Guenter

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

Re: [lm-sensors] [PATCH v2 1/4] hwmon: (lm95245) Fix hysteresis temperatures

[Jean Delvare]

On Sun, 23 Feb 2014 08:26:53 -0800, Guenter Roeck wrote:
> On 02/23/2014 01:49 AM, Jean Delvare wrote:
> > On Sat, 22 Feb 2014 09:30:09 -0800, Guenter Roeck wrote:
> >>
> >>   	if (kstrtoul(buf, 10, &val) < 0)
> >>   		return -EINVAL;
> >>
> >> -	val /= 1000;
> >> -
> >> -	val = clamp_val(val, 0, 31);
> >> -
> >>   	mutex_lock(&data->update_lock);
> >>
> >> +	limit = i2c_smbus_read_byte_data(client, lm95245_reg_address[index]);
> >> +	hyst = limit - val / 1000;
> >> +	hyst = clamp_val(hyst, 0, 31);
> >
> > Note that you don't actually need to hold the lock for that. Access to
> > the SMBus itself is already serialized at the bus driver level, and
> > you're not touching any field of the data structure (that's what the
> > lock is protecting.)
>
> I think it was because the valid flag is cleared, to make sure the data
> is only re-read after the hysteresis register is written to.

I can't think of a scenario where it would make a difference. Trying to
set the limit and the hysteresis at the same time is racy by nature, I
don't think any amount of locking can help.

> >>   	data->valid = 0;
> >
> > BTW do you have any idea why the whole cache is invalidated here? I'd
> > think that storing hyst as data->regs[8] would be enough?
> 
> It also ensures that the limit register value in the cache is current,
> but since that is not volatile it should not really make a difference.

Agreed.

> I'll change that and only update regs[8]. But then I do need the lock,
> or at least I think so.

Yes, you do.

Thanks,
-- 
Jean Delvare
Suse L3 Support

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