On 12/3/24 10:31, André Draszik wrote: > On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote: >> On 12/3/24 07:47, André Draszik wrote: >>>> 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). > > Downstream reports it in > https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.c#2400 > > so upstream should do, too. I should have checked before answering. Indeed, I will try to see the best way to choose the register based on the chip. From what I see, it will also be necessary to change the translation of the reg value to microvolt as downstream does *78125/1000 when it is currently *1250 in mainline: https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.h#49 >>>> 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. > > Just to clarify, has_nvmem can stay here in the driver, just rsense should > go into DT is what I mean. It was clear don't worry. This answer is related to the same subject: https://lore.kernel.org/linux-devicetree/20241202-b4-gs101_max77759_fg-v1-0-98d2fa7bfe30@xxxxxxxxxxxx/T/#ma55f41d42bf39be826d6cbf8987138bcc4eb52e3 >>>> + >>>> /* >>>> * 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. > > Downstream uses the upper bits for max77759: > https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135 > > I don't know what the original max17201/5 report in this register > for those bits, though. Given for max77759 this register returns > the firmware version, I assume the lower bits can change. Based on this datasheet of the max1720x, the upper bits are the revision and the four lower bits are device. So it could change. https://www.analog.com/media/en/technical-documentation/data-sheets/MAX17201-MAX17215.pdf#MAX17201%20DS.indd%3A.213504%3A15892 If the four lower bits are not always 0 for the max77759 then I guess it is necessary to change this as it wouldn't work with all max77759. Best regards, Thomas
