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

[Michael Walle]

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@xxxxxxxx>
>
> 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