| Message ID | 20241202-b4-gs101_max77759_fg-v1-1-98d2fa7bfe30@uclouvain.be |
|---|---|
| State | New |
| Headers | show |
| Series | Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support | 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); >
Hi Antoine, Am Mon, Dec 02, 2024 at 02:07:15PM +0100 schrieb Thomas Antoine via B4 Relay: > 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. > 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..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> > No need to include it, it is done by <linux/i2.c> which includes <linux/device.h> which includes <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, > +}; > + You can get rid of chip_data by reading rsense from DT and moving has_nvmem to max1720x_device_info. By doing so you don't have to rely on default values. Either it is specified by DT or by rsense value in nvmem. Best regards, Dimitri Fedrau
On 12/3/24 08:35, Dimitri Fedrau wrote: > Hi Antoine, > > Am Mon, Dec 02, 2024 at 02:07:15PM +0100 schrieb Thomas Antoine via B4 Relay: >> From: Thomas Antoine <t.antoine@uclouvain.be> >> +#include <linux/types.h> >> > No need to include it, it is done by <linux/i2.c> which includes > <linux/device.h> which includes <linux/types.h> Hi, Will remove for v2. >> 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, >> +}; >> + > You can get rid of chip_data by reading rsense from DT and moving > has_nvmem to max1720x_device_info. By doing so you don't have to rely on > default values. Either it is specified by DT or by rsense value in > nvmem. I guess I can just get has_nvmem by seeing if of_property_read of the rsense value fails. As long as the binding is well defined as to not allow a rsense value for the max1720x, there should be no problem. Will do that for v2. Thanks, best regards, Thomas
On Tue, 2024-12-03 at 11:11 +0100, Thomas Antoine 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> > > > > > [...] > > > > > /* > > > > > * 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. Maybe the best way forward is to go by the compatible (from DT), and if max77759 to then print a warning if the upper bits are != 0x62 and != 0x63. And maybe even refuse to load in that case. 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);