Re: [PATCH v9 2/3] hwmon: ltc4283: Add support for the LTC4283 Swap Controller

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

On 4/11/26 05:38, Nuno Sá wrote:
> On Fri, 2026-04-10 at 16:27 -0700, Guenter Roeck wrote:
>> On 4/6/26 07:31, Nuno Sá via B4 Relay wrote:
>>> From: Nuno Sá <nuno.sa@analog.com>
>>>
>>> Support the LTC4283 Hot Swap Controller. The device features programmable
>>> current limit with foldback and independently adjustable inrush current to
>>> optimize the MOSFET safe operating area (SOA). The SOA timer limits MOSFET
>>> temperature rise for reliable protection against overstresses.
>>>
>>> An I2C interface and onboard ADC allow monitoring of board current,
>>> voltage, power, energy, and fault status.
>>>
>>> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
>>
>> The patch still has some issues. Please see
>>
>> https://sashiko.dev/#/patchset/20260406-ltc4283-support-v9-0-b66cfc749261%40analog.com
>>
>> Specifically:
>>
>> - regmap_clear_bits() may not cause problems, but it is not the best
>>     choice either because the register was already read.
>>     It might be better to just write the value to be masked since
>>     both the register value and the mask are known.
> 
> Fair enough.
> 
>>
>> - I can't comment on the energy accuracy lost. That is your call.
>>
> 
> The AI might have a point. Maybe you know better but if I understood correctly,
> mul_u64_u64_div_u64() will handle the multiplication by using 128bits (when
> available) or if not, using clever tricks. And it should also handle overflows.
> 
> So my feeling is that we can simplify all of those check_overflow paths with the
> suggested API.
> 
>> - Clamping before multiplying is indeed wrong.
>>     You'll need to clamp before multiplying (and then possibly
>>     clamp again).
> 
> Yeah, the clamp change was just nonsense from me. What about about
> 
> val = clamp_val((u64)val * MILLI, ...)
> 
> ?
> 

I don't think that will work on systems where sizeof(long) == 64.
I'd suggest to just bite the bullet and clamp against LONG_MAX/MILLI
first.

> 
>> -  %*ph: The AI seems to have a point.
> 
> Indeed!
> 
> FWIW, I was already aware of the AI feedback but I'll just setup things locally and
> run the review before submitting again.
> 

The AI now copies you on new revisions. Please feel free to rely on that
(unless you have tokens to burn, of course ;-). Those AI reviews are cheap
for what they do, but they are expensive in absolute terms.

Thanks,
Guenter