Message-ID: <afe3f2e6-703b-4d9f-ae88-99da1321d1fc@roeck-us.net>
Date: Mon, 27 Oct 2025 09:52:53 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Igor Reznichenko <igor@...nichenko.net>
Cc: conor+dt@...nel.org, corbet@....net, david.hunter.linux@...il.com,
 devicetree@...r.kernel.org, krzk+dt@...nel.org, linux-doc@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org, robh@...nel.org,
 skhan@...uxfoundation.org
Subject: Re: [PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

On 10/26/25 23:41, Igor Reznichenko wrote:
>> 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?
> 

I said "used by the chip", and referred to the value written into TSC1641_RSHUNT_LSB_UOHM.

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

Please add a respective comment into the code.

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

My unit test code bails out on negative values, returning an error from regmap when
trying to write negative values. I had seen that before. Masking the value passed
to regmap with 0xffff solved the problem.

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

Isn't that great ? You can always find an example for everything in the Linux kernel
if you are looking for it.

Guenter

Powered by blists - more mailing lists