lkml.org 
[lkml]   [2016]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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

\
 
 \ /
  Last update: 2016-11-21 17:53    [W:0.086 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site