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