Re: [PATCH v5 2/2] hwmon:(ina238)Add support for SQ52206

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

On 3/24/25 05:38, Wenliang Yan wrote:
> At 2025-03-23 01:25:11, "Guenter Roeck" <linux@roeck-us.net> wrote:
>> On 3/13/25 01:02, Wenliang Yan wrote:
>>> Add support for SQ52206 to the Ina238 driver. Add registers,
>>> add calculation formulas, increase compatibility, add
>>> compatibility programs for multiple chips.
>>>
>>> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
>>
>> The patch unfortunately combines adding support for a new chip
>> with adding the necessary infrastructure. I finally found the time
>> to look into this further and split the changes, trying to find out
>> what actually changed. Unfortunately there are some problems.
>> Some of them are listed below.
>>
>> This is not a complete review. Also, I'll have to write module tests
>> to ensure that support for existing chips is not broken.
>>
>>>    #define INA238_SHUNT_VOLTAGE_LSB	5 /* 5 uV/lsb */
>>>    #define INA238_BUS_VOLTAGE_LSB		3125 /* 3.125 mV/lsb */
>>> -#define INA238_DIE_TEMP_LSB		125 /* 125 mC/lsb */
>>> +#define INA238_DIE_TEMP_LSB			1250000 /* 125 mC/lsb */
>>
>> This is not correct. The unit is 10ths of uC.
>>
> 
> Since the temp_lsb of sq52206 is 7.8125 mC/LSB, in order to express it as an interger,
> my original intention was to use 7.8125*10000 to represent 7.8125 mC/lsb.
> At the same time, for the consistency of different devides, using 125 * 10000 to represent 125mC/lsb.
> 

Yes, I understand. The problem here is not the code, just the comment.

>>> +#define SQ52206_BUS_VOLTAGE_LSB		3750 /* 3.75 mV/lsb */
>>> +#define SQ52206_DIE_TEMP_LSB		78125 /* 7.8125 mC/lsb */
>>>    
>> ... expressed in 10ths of uC.
>>
>>>    static const struct regmap_config ina238_regmap_config = {
>>>    	.max_register = INA238_REGISTERS,
>>> @@ -102,7 +114,20 @@ static const struct regmap_config ina238_regmap_config = {
>>>    	.val_bits = 16,
>>>    };
>>>    
>>> +enum ina238_ids { ina238, ina237, sq52206 };
>>> +
>>> +struct ina238_config {
>>> +	bool has_power_highest;		/* chip detection power peak */
>>> +	bool has_energy;		/* chip detection energy */
>>> +	u8 temp_shift;
>>> +	u32 power_calculate_factor;		/*fixed parameters for power calculate*/
>>> +	u16 config_default;
>>> +	int bus_voltage_lsb;    /* uV */
>>> +	int temp_lsb;   /* mC */
>>
>> No, this is not the temperature in mC. It is the temperature in 10th of uC.
>>
> 
> This is indeed a problem, I need to handle temp_lsb appropriately.
> 
Same as above. The problem is (as far as I can see) not in the code.
The comment should state the correct units.

>>>    			/* gain of 1 -> LSB / 4 */
>>> -			*val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
>>> -			       (1000 * (4 - data->gain + 1));
>>> +			*val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
>>> +					data->gain / (1000 * 4);
>>
>> The slight benefit of this change is that the divisor is now a constant,
>> which may enable some compiler optimization. Still, it is not a necessary
>> change and just makes review more difficult.
>>
> 
> The original formula takes effect only when gain=1 or 4, but sq52206 has a gain=2.
> The principle of this formula is the same as before.
> 

Thanks for the clarification. It might make sense to separate this and the
calculation change below out into a separate patch and explain the reasoning
in the commit description.

Thanks,
Guenter