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] [day] [month] [year] [list]

Message-ID: <20250108025305.60705-1-wenliang202407@163.com>
Date: Wed,  8 Jan 2025 10:53:05 +0800
From: Wenliang Yan <wenliang202407@....com>
To: linux@...ck-us.net
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,
	wenliang202407@....com
Subject: Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

At 2025-01-07 23:02:09, "Guenter Roeck" <linux@...ck-us.net> wrote:
>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.
>

I understand, I will modify.

>>>> -#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.
>

This is due to my format alignment, I will adjust it back.

>>>>    
>>>>    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

Thanks,
Wenliang Yan