linux-kernel - Re: [PATCH v2 1/4] power: supply: add support for max77759 fuel gauge

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: <c28c8894-125c-4d1f-bdec-16977cc98e4d@uclouvain.be>
Date: Fri, 10 Jan 2025 17:01:50 +0100
From: Thomas Antoine <t.antoine@...ouvain.be>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, Peter Griffin <peter.griffin@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 André Draszik <andre.draszik@...aro.org>,
 linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v2 1/4] power: supply: add support for max77759 fuel gauge

Hi,

>> [...]
>>  
>>  struct max1720x_device_info {
>>  	struct regmap *regmap;
>>  	struct regmap *regmap_nv;
>>  	struct i2c_client *ancillary;
>>  	int rsense;
>> +	int has_nvmem;
> 
> Switch to bool and why is needed twice ? Here and in chip_data. Better
> keep it in chip_data.

It is useless in device_info at the moment, I'm not sure why I thought I
neeed it there at the time. I will remove it.

>> [...]
>> +// Use 64 bits because computation on 32 bits leads to an overflow
>> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
>> +{
>> +	u64 val = reg;
>> +
>> +	return val * 78125 / 1000;	/* in uV */
> 
> You could avoid the overflow with:
> 	return val * 625 / 8;	/* in uV */

Indeed, I will change that.

>> +}
>> +
>>  static int max172xx_capacity_to_ps(unsigned int reg)
>>  {
>>  	return reg * 500;	/* in uAh */
>> @@ -303,6 +383,33 @@ static int max172xx_current_to_voltage(unsigned int reg)
>>  	return val * 156252;
>>  }
>>  
>> +static int max1720x_devname_to_model(unsigned int reg_val,
>> +					union power_supply_propval *val,
>> +					struct max1720x_device_info *info)
> 
> nit: Align with open parenthesis

Will fix.

>>  static int max1720x_battery_get_property(struct power_supply *psy,
>>  					 enum power_supply_property psp,
>>  					 union power_supply_propval *val)
>> @@ -332,7 +439,12 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>>  		break;
>>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>  		ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
>> -		val->intval = max172xx_voltage_to_ps(reg_val);
>> +		if (!ret)
>> +			val->intval = max172xx_voltage_to_ps(reg_val);
>> +		else {
>> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
>> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
>> +		}
> 
> Would be better to read the right register, since we know which device
> we are. You could differentiate like in max1720x_devname_to_model.

Will do.

>> [...]
>> +static int max1720x_get_rsense(struct device *dev,
>> +					 struct max1720x_device_info *info)
> 
> nit: Align with open parenthesis.
> 
>> [...]
>> +		ret = of_property_read_u32(dev->of_node,
>> +					"shunt-resistor-micro-ohms", &val);
> 
> nit: Align with open parenthesis.

I will fix those too. Thanks for the feedback.

Best regards,
Thomas