Re: [PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko <igor@reznichenko.net>]

>In some way this is inconsistent: It accepts a shunt resistor value of, say, 105
>even though the chip can only accept multiples of 10 uOhm. In situations like this
>I suggest to expect devicetree values to be accurate and to clamp values entered
>through sysfs. More on that below.
>
>> +	return 0;
>> +}
>> +
>> +static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	long rshunt_reg;
>> +
>> +	if (tsc1641_validate_shunt(val) < 0)
>> +		return -EINVAL;
>> +
>> +	data->rshunt_uohm = val;
>> +	data->current_lsb_ua = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NVOLT * 1000,
>> +						 data->rshunt_uohm);
>> +	/* RSHUNT register LSB is 10uOhm so need to divide further*/
>> +	rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
>
>This means that all calculations do not use the actual shunt resistor values used
>by the chip, but an approximation. I would suggest to store and use the actual shunt
>resistor value instead, not the one entered by the user.

By "actual shunt" you mean defined in devicetree? Then does it mean disabling 
writing value by user via sysfs and making "shunt_resistor" read-only or leaving it
writable and clamping to devicetree value, thus discarding the user provided value?

>See below - clamping is insufficient for negative values, and it is not clear to me if
>the limit register is signed or unsigned.

>Also, the datasheet doesn't say that the limit value would be signed. Did you verify
>that negative temperature limit values are actually treated as negative values ?

SUL, SOL, TOL are signed, I verified. The negative limits for current and temperature
work well based on my testing.

>This doesn't work as intended for negative values. regmap doesn't expect to see
>negative register values and returns an error if trying to write one, so clamping
>against SHRT_MIN and SHRT_MAX is insufficient. You also need to mask the result
>against 0xffff.

I was under impression regmap would handle this masking correctly when defining
.val_bits = 16. E.g. in regmap.c:973 it selects formatting function for 16bit values.
I can mask explicitly if it's required.
It certainly doesn't throw error since negative alerts work as mentioned.

>Why did you choose lcrit/crit attributes instead of min/max ? If there is only
>one alert limit, that usually means the first level of alert, not a critical level.
>Raising an alert does not mean it is a critical alert. Please reconsider.

I used hwmon/ina2xx.c as a reference. It covers many similar power monitors which
have single threshold alerts and defines only lcrit/crit. If this is a wrong approach
I'll change to min/max.

The rest of the things are clear, I'll fix those.

Thanks, Igor