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

[Michael Walle <michael@walle.cc>]

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@walle.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