| Message ID | 20241202-b4-gs101_max77759_fg-v1-1-98d2fa7bfe30@uclouvain.be |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] power: supply: add support for max77759 fuel gauge | expand |
Hi Thomas, Thanks for looking into this! > 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. > No slave is available at address 0xb of the i2c bus, which is coherent > with the following driver from google: line 5836 disables non-volatile > memory for m5 gauge. > > Link: > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c > > Add support for the max77759 by allowing to use the non-volatile > memory or not based on the chip. Value for RSense comes from the following > stock devicetree: > > Link: > https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi > > Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be> > --- > drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++- > ----- > 1 file changed, 59 insertions(+), 12 deletions(-) > > diff --git a/drivers/power/supply/max1720x_battery.c > b/drivers/power/supply/max1720x_battery.c > index > 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca8 > 0ad41a9f 100644 > --- a/drivers/power/supply/max1720x_battery.c > +++ b/drivers/power/supply/max1720x_battery.c > @@ -13,6 +13,7 @@ > #include <linux/nvmem-provider.h> > #include <linux/power_supply.h> > #include <linux/regmap.h> > +#include <linux/types.h> Please keep file names in alphabetical order. > > #include <linux/unaligned.h> > > @@ -39,6 +40,7 @@ > #define MAX172XX_DEV_NAME_TYPE_MASK GENMASK(3, 0) > #define MAX172XX_DEV_NAME_TYPE_MAX17201 BIT(0) > #define MAX172XX_DEV_NAME_TYPE_MAX17205 (BIT(0) | BIT(2)) > +#define MAX172XX_DEV_NAME_TYPE_MAX77759 0 > #define MAX172XX_QR_TABLE10 0x22 > #define MAX172XX_BATT 0xDA /* Battery voltage */ > #define MAX172XX_ATAVCAP 0xDF > @@ -46,6 +48,7 @@ > static const char *const max1720x_manufacturer = "Maxim Integrated"; > static const char *const max17201_model = "MAX17201"; > 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). > + > /* > * 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. > return -ENODEV; > break; > @@ -416,7 +436,6 @@ static int max1720x_probe_nvmem(struct i2c_client > *client, > .priv = info, > }; > struct nvmem_device *nvmem; > - unsigned int val; > int ret; > > info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb); > @@ -438,6 +457,27 @@ static int max1720x_probe_nvmem(struct i2c_client > *client, > return PTR_ERR(info->regmap_nv); > } > > + nvmem = devm_nvmem_register(dev, &nvmem_config); > + if (IS_ERR(nvmem)) { > + dev_err(dev, "Could not register nvmem!"); > + return PTR_ERR(nvmem); > + } > + > + return 0; > +} > + > +static int max1720x_get_rsense(struct device *dev, > + struct max1720x_device_info > *info, > + const struct chip_data *data) > +{ > + unsigned int val; > + int ret; > + > + if (!data->has_nvmem) { > + info->rsense = data->default_nrsense; > + return 0; > + } > + > ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val); > if (ret < 0) { > dev_err(dev, "Failed to read sense resistor value\n"); > @@ -446,14 +486,9 @@ static int max1720x_probe_nvmem(struct i2c_client > *client, > > info->rsense = val; > if (!info->rsense) { > - dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n"); > - info->rsense = 1000; /* in regs in 10^-5 */ > - } > - > - nvmem = devm_nvmem_register(dev, &nvmem_config); > - if (IS_ERR(nvmem)) { > - dev_err(dev, "Could not register nvmem!"); > - return PTR_ERR(nvmem); > + dev_warn(dev, "RSense not calibrated, set %d mOhms!\n", > + data- > >default_nrsense/100); > + info->rsense = data->default_nrsense; > } > > return 0; > @@ -474,6 +509,7 @@ static int max1720x_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct max1720x_device_info *info; > struct power_supply *bat; > + const struct chip_data *data; > int ret; > > info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > @@ -488,9 +524,19 @@ static int max1720x_probe(struct i2c_client *client) > return dev_err_probe(dev, PTR_ERR(info->regmap), > "regmap initialization failed\n"); > > - ret = max1720x_probe_nvmem(client, info); > + data = device_get_match_data(dev); > + if (!data) > + return dev_err_probe(dev, ret, "Failed to get chip > data\n"); > + > + if (data->has_nvmem) { > + ret = max1720x_probe_nvmem(client, info); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to probe > nvmem\n"); > + } > + > + ret = max1720x_get_rsense(dev, info, data); > if (ret) > - return dev_err_probe(dev, ret, "Failed to probe > nvmem\n"); > + return dev_err_probe(dev, ret, "Failed to get RSense"); > > bat = devm_power_supply_register(dev, &max1720x_bat_desc, > &psy_cfg); > if (IS_ERR(bat)) > @@ -501,7 +547,8 @@ static int max1720x_probe(struct i2c_client *client) > } > > static const struct of_device_id max1720x_of_match[] = { > - { .compatible = "maxim,max17201" }, > + { .compatible = "maxim,max17201", .data = (void *) &max1720x_data > }, > + { .compatible = "maxim,max77759-fg", .data = (void *) > &max77759_data}, missing space before } Cheers, Andre' > {} > }; > MODULE_DEVICE_TABLE(of, max1720x_of_match); >
On Tue, 2024-12-03 at 06:47 +0000, André Draszik wrote: > Hi Thomas, > > Thanks for looking into this! > > > 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. Regarding the regmap in this driver, please see below comparison I had collected some time ago: regmap_reg_range(0x24, 0x26), // exists regmap_reg_range(0x30, 0x31), // exists regmap_reg_range(0x33, 0x34), // exists regmap_reg_range(0x37, 0x37), // exists regmap_reg_range(0x3B, 0x3C), // exists regmap_reg_range(0x40, 0x41), // exists regmap_reg_range(0x43, 0x44), // exists regmap_reg_range(0x47, 0x49), // exists regmap_reg_range(0x4B, 0x4C), // exists regmap_reg_range(0x4E, 0xAF), // 0x4e 0x4f exists regmap_reg_range(0xB1, 0xB3), // exists regmap_reg_range(0xB5, 0xB7), // exists regmap_reg_range(0xBF, 0xD0), // 0xd0 exists 0xd1 .. 0xdb don't exist regmap_reg_range(0xDB, 0xDB), regmap_reg_range(0xE0, 0xFF), // 0xfb 0xff exist the comments refer to whether or not the register exists in max77759 > > > No slave is available at address 0xb of the i2c bus, which is coherent > > with the following driver from google: line 5836 disables non-volatile > > memory for m5 gauge. > > > > Link: > > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c > > > > Add support for the max77759 by allowing to use the non-volatile > > memory or not based on the chip. Value for RSense comes from the following > > stock devicetree: > > > > Link: > > https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi > > > > Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be> > > --- > > drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++- > > ----- > > 1 file changed, 59 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/power/supply/max1720x_battery.c > > b/drivers/power/supply/max1720x_battery.c > > index > > 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca8 > > 0ad41a9f 100644 > > --- a/drivers/power/supply/max1720x_battery.c > > +++ b/drivers/power/supply/max1720x_battery.c > > @@ -13,6 +13,7 @@ > > #include <linux/nvmem-provider.h> > > #include <linux/power_supply.h> > > #include <linux/regmap.h> > > +#include <linux/types.h> > > Please keep file names in alphabetical order. Ignore that, it's too early :-) Cheers, Andre'
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
On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote: > 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. No worries :-) > > > > 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). 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. > > > 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. > > > + > > > /* > > > * 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. Cheers, Andre'
On Tue, 2024-12-03 at 07:23 +0000, André Draszik wrote: > On Tue, 2024-12-03 at 06:47 +0000, André Draszik wrote: > > Hi Thomas, > > > > Thanks for looking into this! > > > > > 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. > > Regarding the regmap in this driver, please see below comparison I had > collected some time ago: > > regmap_reg_range(0x24, 0x26), // exists > regmap_reg_range(0x30, 0x31), // exists > regmap_reg_range(0x33, 0x34), // exists > regmap_reg_range(0x37, 0x37), // exists > regmap_reg_range(0x3B, 0x3C), // exists > regmap_reg_range(0x40, 0x41), // exists > regmap_reg_range(0x43, 0x44), // exists > regmap_reg_range(0x47, 0x49), // exists > regmap_reg_range(0x4B, 0x4C), // exists > regmap_reg_range(0x4E, 0xAF), // 0x4e 0x4f exists > regmap_reg_range(0xB1, 0xB3), // exists > regmap_reg_range(0xB5, 0xB7), // exists > regmap_reg_range(0xBF, 0xD0), // 0xd0 exists > 0xd1 .. 0xdb don't exist > regmap_reg_range(0xDB, 0xDB), > regmap_reg_range(0xE0, 0xFF), // 0xfb 0xff exist > > the comments refer to whether or not the register exists in max77759 I think this should make it more clear: allow: regmap_reg_range(0x00, 0xff), deny: regmap_reg_range(0x50, 0xaf), regmap_reg_range(0xc0, 0xcf), regmap_reg_range(0xd1, 0xdb), regmap_reg_range(0xe0, 0xfa), regmap_reg_range(0xfc, 0xfe), Cheers, Andre'
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@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). > > 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@uclouvain.be/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
Hi Thomas, Thanks for your contribution :) On Tue, 3 Dec 2024 at 10:12, Thomas Antoine <t.antoine@uclouvain.be> wrote: > > 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@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). > > > > 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@uclouvain.be/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. The definition of this register for max77759 is Register name(addr): DevName (0x21) Rest value: 0x6200 Bitfield: DevName Bits: 15:0 Description: Firmware Version Information So I don't think you can rely on the bottom bits always being zero regards, Peter
On Tue, 2024-12-03 at 11:30 +0100, Thomas Antoine wrote: > > Should I explicitly deny their use in the code for the max77759 or is it > just for information? I'd probably do something like this, which will indeed deny their reading and/or writing, both via debugfs, and also normal driver access via readmap_read()/write() etc: static const struct regmap_range max77759_registers[] = { regmap_reg_range(0x00, 0x4f), regmap_reg_range(0xb0, 0xbf), regmap_reg_range(0xd0, 0xd0), regmap_reg_range(0xdc, 0xdf), regmap_reg_range(0xfb, 0xfb), regmap_reg_range(0xff, 0xff), }; static const struct regmap_range max77759_ro_registers[] = { regmap_reg_range(0x3d, 0x3d), regmap_reg_range(0xfb, 0xfb), regmap_reg_range(0xff, 0xff), }; static const struct regmap_access_table max77759_write_table = { .yes_ranges = max77759_registers, .n_yes_ranges = ARRAY_SIZE(max77759_registers), .no_ranges = max77759_ro_registers, .n_no_ranges = ARRAY_SIZE(max77759_ro_registers), }; static const struct regmap_access_table max77759_rd_table = { .yes_ranges = max77759_registers, .n_yes_ranges = ARRAY_SIZE(max77759_registers), }; static const struct regmap_config max77759_regmap_config = { .reg_bits = 8, .val_bits = 8, .max_register = 0xff, .wr_table = &max77759_write_table, .rd_table = &max77759_rd_table, .cache_type = REGCACHE_NONE, }; And maybe without cache for now. Most are probably not cacheable anyway. Cheers, Andre'
diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c index 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca80ad41a9f 100644 --- a/drivers/power/supply/max1720x_battery.c +++ b/drivers/power/supply/max1720x_battery.c @@ -13,6 +13,7 @@ #include <linux/nvmem-provider.h> #include <linux/power_supply.h> #include <linux/regmap.h> +#include <linux/types.h> #include <linux/unaligned.h> @@ -39,6 +40,7 @@ #define MAX172XX_DEV_NAME_TYPE_MASK GENMASK(3, 0) #define MAX172XX_DEV_NAME_TYPE_MAX17201 BIT(0) #define MAX172XX_DEV_NAME_TYPE_MAX17205 (BIT(0) | BIT(2)) +#define MAX172XX_DEV_NAME_TYPE_MAX77759 0 #define MAX172XX_QR_TABLE10 0x22 #define MAX172XX_BATT 0xDA /* Battery voltage */ #define MAX172XX_ATAVCAP 0xDF @@ -46,6 +48,7 @@ static const char *const max1720x_manufacturer = "Maxim Integrated"; static const char *const max17201_model = "MAX17201"; 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, +}; + /* * 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 return -ENODEV; break; @@ -416,7 +436,6 @@ static int max1720x_probe_nvmem(struct i2c_client *client, .priv = info, }; struct nvmem_device *nvmem; - unsigned int val; int ret; info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb); @@ -438,6 +457,27 @@ static int max1720x_probe_nvmem(struct i2c_client *client, return PTR_ERR(info->regmap_nv); } + nvmem = devm_nvmem_register(dev, &nvmem_config); + if (IS_ERR(nvmem)) { + dev_err(dev, "Could not register nvmem!"); + return PTR_ERR(nvmem); + } + + return 0; +} + +static int max1720x_get_rsense(struct device *dev, + struct max1720x_device_info *info, + const struct chip_data *data) +{ + unsigned int val; + int ret; + + if (!data->has_nvmem) { + info->rsense = data->default_nrsense; + return 0; + } + ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val); if (ret < 0) { dev_err(dev, "Failed to read sense resistor value\n"); @@ -446,14 +486,9 @@ static int max1720x_probe_nvmem(struct i2c_client *client, info->rsense = val; if (!info->rsense) { - dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n"); - info->rsense = 1000; /* in regs in 10^-5 */ - } - - nvmem = devm_nvmem_register(dev, &nvmem_config); - if (IS_ERR(nvmem)) { - dev_err(dev, "Could not register nvmem!"); - return PTR_ERR(nvmem); + dev_warn(dev, "RSense not calibrated, set %d mOhms!\n", + data->default_nrsense/100); + info->rsense = data->default_nrsense; } return 0; @@ -474,6 +509,7 @@ static int max1720x_probe(struct i2c_client *client) struct device *dev = &client->dev; struct max1720x_device_info *info; struct power_supply *bat; + const struct chip_data *data; int ret; info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); @@ -488,9 +524,19 @@ static int max1720x_probe(struct i2c_client *client) return dev_err_probe(dev, PTR_ERR(info->regmap), "regmap initialization failed\n"); - ret = max1720x_probe_nvmem(client, info); + data = device_get_match_data(dev); + if (!data) + return dev_err_probe(dev, ret, "Failed to get chip data\n"); + + if (data->has_nvmem) { + ret = max1720x_probe_nvmem(client, info); + if (ret) + return dev_err_probe(dev, ret, "Failed to probe nvmem\n"); + } + + ret = max1720x_get_rsense(dev, info, data); if (ret) - return dev_err_probe(dev, ret, "Failed to probe nvmem\n"); + return dev_err_probe(dev, ret, "Failed to get RSense"); bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg); if (IS_ERR(bat)) @@ -501,7 +547,8 @@ static int max1720x_probe(struct i2c_client *client) } static const struct of_device_id max1720x_of_match[] = { - { .compatible = "maxim,max17201" }, + { .compatible = "maxim,max17201", .data = (void *) &max1720x_data }, + { .compatible = "maxim,max77759-fg", .data = (void *) &max77759_data}, {} }; MODULE_DEVICE_TABLE(of, max1720x_of_match);