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

[Thomas Antoine <t.antoine@xxxxxxxxxxxx>]

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