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

[Thomas Antoine <t.antoine@uclouvain.be>]

On 12/3/24 07:47, André Draszik wrote:
> Hi Thomas,
> 
> Thanks for looking into this!

Hi,

With pleasure! This is my first time trying to contribute to the kernel
so sorry for any beginner mistakes I might do.
 
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
>> except for the non-volatile memory slave address which is not available.
> 
> It is not fully compatible, and it also has a lot more registers.
> 
> For example, the voltage now is not in register 0xda as this driver assumes.
> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> doesn't exist in max77759
> 
> I haven't compared in depth yet, though.

Is the voltage necessary for the driver? If so, we could just not
read the voltage. If it is necessary, I can try to kook into it and try
to find in which register it is located (if there is one).

>>  static const char *const max17205_model = "MAX17205";
>> +static const char *const max77759_model = "MAX77759";
>>
>>  struct max1720x_device_info {
>>       struct regmap *regmap;
>> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>>       int rsense;
>>  };
>>
>> +struct chip_data {
>> +     u16 default_nrsense; /* in regs in 10^-5 */
>> +     u8 has_nvmem;
>> +};
>> +
>> +static const struct chip_data max1720x_data  = {
>> +     .default_nrsense = 1000,
>> +     .has_nvmem = 1,
>> +};
>> +
>> +static const struct chip_data max77759_data = {
>> +     .default_nrsense = 500,
>> +     .has_nvmem = 0,
>> +};
> 
> This should be made a required devicetree property instead, at least for
> max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
> is widely used.
> 
> I also don't think there should be a default. The driver should just fail
> to probe if not specified in DT (for max77759).

I hesitated to do this but I didn't know what would be better. Will change
for v2.

>> +
>>  /*
>>   * Model Gauge M5 Algorithm output register
>>   * Volatile data (must not be cached)
>> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
>> power_supply *psy,
>>                       val->strval = max17201_model;
>>               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>>                       val->strval = max17205_model;
>> +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
>> +                     val->strval = max77759_model;
>>               else
> 
> This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> cares about the bottom 4 bits, the register is described as 'Firmware
> Version Information'.
> 
> But maybe it's ok to do it like that, at least for now.

I thought this method would be ok as long as there is no collision on
values. I hesitated to change the model evaluation method based on chip
model, where the max77759 would thus have an hard-coded value and the
max1720x would still evaluate the register value. I did not do it because
it led to a lot more changes for no difference.

>> &max77759_data},
> 
> missing space before }

Will change for v2.

Best regards,
Thomas