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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f707f3f-8929-4453-8b18-285594b5f99d@roeck-us.net>
Date: Tue, 7 Jan 2025 07:02:09 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>
Cc: conor+dt@...nel.org, corbet@....net, jdelvare@...e.com,
 krzk+dt@...nel.org, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org, robh@...nel.org
Subject: Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

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