linux-kernel - Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

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 linux-hardening linux-cve-announce 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]

Message-ID: <3353d7477aec9b78fb6e5aa3f145fec1@walle.cc>
Date:   Mon, 21 Nov 2016 17:53:01 +0100
From:   Michael Walle <michael@...le.cc>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-hwmon@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC,2/2] hwmon: adt7411: add min, max and alarm attributes

Am 2016-11-19 19:05, schrieb Guenter Roeck:
> Hi Michael,
> 
> On Fri, Oct 14, 2016 at 11:43:35AM +0200, Michael Walle wrote:
>> This patch adds support for the min, max and alarm attributes of the
>> voltage and temperature channels. Additionally, the temp2_fault 
>> attribute
>> is supported which indicates a fault of the external temperature 
>> diode.
>> 
>> Signed-off-by: Michael Walle <michael@...le.cc>
> 
> Sorry for the late reply. Mostly looks ok.
> Couple of comments below.

thanks for the review. I will send an updated version, soon.


[snip]

>> +static int adt7411_write_temp(struct device *dev, u32 attr, int 
>> channel,
>> +			      long val)
>> +{
>> +	struct adt7411_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	int reg;
>> +
>> +	val = DIV_ROUND_CLOSEST(val, 1000);
>> +	val = clamp_val(val, -128, 127);
>> +	val = val < 0 ? 0x100 + val : val;
> 
> Does this add any value ? It doesn't change the low byte.

mh? if val is negative 256 will be added.


>>  static umode_t adt7411_is_visible(const void *_data,
>>  				  enum hwmon_sensor_types type,
>>  				  u32 attr, int channel)
>>  {
>>  	const struct adt7411_data *data = _data;
>> +	bool visible;
>> 
>>  	switch (type) {
>>  	case hwmon_in:
>> -		if (channel > 0 && channel < 3)
>> -			return data->use_ext_temp ? 0 : S_IRUGO;
>> -		else
>> -			return S_IRUGO;
>> +		visible = channel == 0 || channel >= 2 || !data->use_ext_temp;
> 
> in2 is now visible even if external temperature is measured.
> This is not correct. Yes, one can read the register, but the
> external pin (AIN2) is connected to the temperature sensor.

i guess

   visible = channel == 0 || channel >= 3 || !data->use_ext_temp;

makes more sense.

-michael