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

[Guenter Roeck]

On 3/24/25 05:38, Wenliang Yan wrote:
> At 2025-03-23 01:25:11, "Guenter Roeck" <linux@xxxxxxxxxxxx> 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@xxxxxxx>
> >
> > 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