Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Oct 23, 2025 at 12:50:50AM -0700, Igor Reznichenko wrote:
> Guenter,
> Thanks for the detailed feedback. I will address it.
> 
> > Please send a register dump.
> 
> Here's register dump after init during run: 
> 
> tsc1641 1-0040: 0x00: 0x003f
> tsc1641 1-0040: 0x01: 0x0253
> tsc1641 1-0040: 0x02: 0x0dc0
> tsc1641 1-0040: 0x03: 0x0053
> tsc1641 1-0040: 0x04: 0x0250
> tsc1641 1-0040: 0x05: 0x0033
> tsc1641 1-0040: 0x06: 0x0000
> tsc1641 1-0040: 0x07: 0x0000
> tsc1641 1-0040: 0x08: 0x01f4
> tsc1641 1-0040: 0x09: 0x0000
> tsc1641 1-0040: 0x0a: 0x0000
> tsc1641 1-0040: 0x0b: 0x0000
> tsc1641 1-0040: 0x0c: 0x0000
> tsc1641 1-0040: 0x0d: 0x0000
> tsc1641 1-0040: 0x0e: 0x0000
> tsc1641 1-0040: 0xfe: 0x0006
> tsc1641 1-0040: 0xff: 0x1000
> 
Great, thanks a lot!

> > > +
> > > +	/*
> > > +	 * Disable alert mask first, then write the value and enable alert mask
> > Why ? 
> 
> The idea was to prevent potential previous alert from propagating when changing 
> the value, plus to only enable alert when crit/lcrit value is non-zero. 
> But given your response below this is not the right thing to do.
> 
> > Disabling alerts if the limit is 0 is wrong: The limit can be set
> > to 0 on purpose. Only unmasking the limit if a limit is set is just as wrong.
> > Either limits are enabled and reported, or they are disabled and the attributes
> > must not be generated. Mis-using the ABI to declare "If the limit value is
> > 0, mask the limit. Otherwise set the limit and unmask it" is unacceptable.
> 
> Thanks for clarification. So would you recommend then that all alerts should 
> be always on/unmasked for this chip or to add custom sysfs attributes to control 
> them, since it has this capability?
> 

Almost every chip has that capability. That does not warrant a custom sysfs
attribute. I'd suggest to just enable them all.

Guenter