Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

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

On 1/8/25 16:50, Leo Yang wrote:
> Hi Guenter,
> 
> On Mon, Jan 6, 2025 at 11:31 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Besides, while I did point out a number of problems, but I did not suggest
>> to "rewrite the driver".
>>
>> Since this is v2 of this driver, the submission should have been versioned,
>> and a change log should have been provided.
>>
> 
> Sorry this is my first v2 patch,
> I should have been more aware of this problem, thank you.
> 
>>
>> Why not just pass the power coefficient directly as parameter ?
>>
>>> +     if (1000000 % *m) {
>>
>> I fail to understand the logic here. Why scale if and only if m is a clean
>> multiple of 1000000 ? Scale if m == 1000001 but not if m == 1000000 ?
>> Please explain.
>>
>>> +             /* Default value, Scaling to keep integer precision,
>>> +              * Change it if you need
>>
>> This comment does not provide any actual information and thus does not
>> add any value. Change to what ? Why ? And, again, why not scale if
>> m is a multiple of 1000000, no matter how large or small it is ?
>>
> 
> When we calculate the Telemetry and Warning Conversion Coefficients,
> the m-value of the current needs to be calculated via Equation:
> 1(A)/Current_LSB(A).
> 
> The number 1000000 comes from A->uA to match the unit uA of Current_LSB.
> Try to prevent the loss of fractional precision in integer.
> 
> But this is not enough,
> according to spec 7.5.4 Reading Telemetry Data and Warning Thresholds
> If there is decimal information in m, we should try to move the decimal point
> so that the value of m is between -32768 and 32767 and maximize it as much
> as possible to minimize rounding errors.
> 
> Therefore, if m does not have decimal information, even if the value of m is
> scaled up, it is not possible to minimize rounding errors.
> 
> But my comments are not clear enough, I'll fix it.
> 
>>> +
>>> +     /* Maximize while keeping it bounded.*/
>>> +     while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
>>> +             scaled_m /= 10;
>>
>> This looks wrong. If scaled_m < MIN_M_VAL it doesn't make sense
>> to decrease it even more.
>>
> 
> In this part, I try to move the decimal point so that the value of m is between
> -32768 and 32767.
> Assuming scaled_m = -40001, I can scale it to m = -4000 and adjust it by R++
> 
Sorry, I missed that MIN_M_VAL is negative.

Guenter

>>> +             scale_factor++;
>>> +     }
>>> +     /* Scale up only if fractional part exists. */
>>> +     while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
>>
>> This looks just as wrong. If scaled_m > 10 * MIN_M_VAL, why increase it even more ?
>>
> 
> I think the purpose of spec is to keep as many integers as possible in m, and
> then save the information in decimals via R to minimize rounding errors.
> So here I keep the positive numbers as close to 32767 as possible, and the
> negative numbers as close to -32768 as possible.
> 
> And thank you for the suggestions, they are very helpful and I will
> try to fix them.
> 
> 
> Best Regards,
> 
> Leo Yang