[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi Jim,

>> +static ssize_t
>> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
>> +			const char *buf, size_t count)
>> +{
>>   
>> +	if (data->pwm_enable[nr] != val) {
>>   
> I guess *here* is where you mean non-trivial.
> 
> why not take the lock here -> instead of in both branches of if ?
> 
>> +		if (!val) {
>>   
> you specifically and knowingly dont protect this call,
> and advised us of it in your msg.  It certainly warrants a comment here too.

The call protect itself because it locks on its own. If the lock would be 
before, it tries to lock 2 times and will deadlock.

> 
>> +			store_pwm(dev, attr, "255", 3);
>>   
> and now take the lock.
>> +			mutex_lock(&data->update_lock);
>> +			data->pwm_enable[nr] = val;
>> +		}
>> +		else {
>>   
> repeating, this lock could be hoisted, but for the missing/unstated reason.

I know this code is not perfect, but how can it be done without more locking or 
more confusing temporary variables?

Thanks,

regards
Rudolf