Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Guenter Roeck]

On 1/7/25 04:43, Wenliang Yan wrote:
...
> > > -#define INA238_REGISTERS		0x11
> > > +#define INA238_MAX_REGISTERS		0x20
> >
> > Why this change ?
>
> The maximum register address for SQ52206 is 0x20.

That isn't what I referred to. Th value change is ok.
The question was why change INA238_REGISTERS to INA238_MAX_REGISTERS.
That is a personal preference change. I strongly discourage such
changes because if I accept them someone else may come tomorrow
and change the name again to match their preference.

> > > -#define INA238_FIXED_SHUNT		20000
> > > +#define INA238_FIXED_SHUNT			20000
> >
> > Why this change ?
>
> Also refer to the chip manual provided in the website above.

The value didn't change. The indentation changed without reason.

> > >    
> > >    static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
> > > @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
> > >    		regval = (s16)regval;
> > >    		if (channel == 0)
> > >    			/* 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);
> >
> > Why this change ?
>
> Because INA238 only has two gains of 1 and 4, the previous formula can
> be used, but SQ52206 has a gain of 2, so I changed the formula
Ok.

> > >    		else
> > > -			*val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
> > > +			*val = (regval * data->config->bus_voltage_lsb) / 1000;
> > >    		break;
> > >    	case hwmon_in_max_alarm:
> > >    	case hwmon_in_min_alarm:
> > > @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
> > >    	case 0:
> > >    		/* signed value, clamp to max range +/-163 mV */
> > >    		regval = clamp_val(val, -163, 163);
> > > -		regval = (regval * 1000 * (4 - data->gain + 1)) /
> > > -			 INA238_SHUNT_VOLTAGE_LSB;
> > > +		regval = (regval * 1000 * 4) /
> > > +			 INA238_SHUNT_VOLTAGE_LSB * data->gain;
> >
> > Why this change ?
>
> Consistent with the reason described in the previous article.
Ok.

> > >    	data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
> > > @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
> > >    	/* load shunt gain value */
> > >    	if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
> > >    		data->gain = 4; /* Default of ADCRANGE = 0 */
> > > -	if (data->gain != 1 && data->gain != 4) {
> > > +	if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
> >
> > Chip independent changes should be in separate patch(es).
>
> SQ52206 has a gain of 2.
Ok.


Thanks,
Guenter