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

[Guenter Roeck]

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