* [PATCH 1/4] power: supply: add support for max77759 fuel gauge
@ 2024-12-02 13:07 ` Thomas Antoine via B4 Relay
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Antoine via B4 Relay @ 2024-12-02 13:07 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Dimitri Fedrau, Catalin Marinas, Will Deacon, Peter Griffin,
Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc, Thomas Antoine
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>
#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);
--
2.47.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-02 13:07 ` Thomas Antoine via B4 Relay
(?)
@ 2024-12-03 6:47 ` André Draszik
2024-12-03 7:23 ` André Draszik
2024-12-03 9:08 ` Thomas Antoine
-1 siblings, 2 replies; 38+ messages in thread
From: André Draszik @ 2024-12-03 6:47 UTC (permalink / raw)
To: t.antoine, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Dimitri Fedrau, Catalin Marinas, Will Deacon,
Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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);
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 6:47 ` André Draszik
@ 2024-12-03 7:23 ` André Draszik
2024-12-03 9:45 ` André Draszik
2024-12-03 9:08 ` Thomas Antoine
1 sibling, 1 reply; 38+ messages in thread
From: André Draszik @ 2024-12-03 7:23 UTC (permalink / raw)
To: t.antoine, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Dimitri Fedrau, Catalin Marinas, Will Deacon,
Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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'
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 7:23 ` André Draszik
@ 2024-12-03 9:45 ` André Draszik
2024-12-03 10:30 ` Thomas Antoine
0 siblings, 1 reply; 38+ messages in thread
From: André Draszik @ 2024-12-03 9:45 UTC (permalink / raw)
To: t.antoine, Sebastian Reichel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Dimitri Fedrau, Catalin Marinas, Will Deacon,
Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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'
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 9:45 ` André Draszik
@ 2024-12-03 10:30 ` Thomas Antoine
2024-12-03 11:06 ` André Draszik
0 siblings, 1 reply; 38+ messages in thread
From: Thomas Antoine @ 2024-12-03 10:30 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
On 12/3/24 10:45, André Draszik wrote:
> 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),
Should I explicitly deny their use in the code for the max77759 or is it
just for information?
Best regards,
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 10:30 ` Thomas Antoine
@ 2024-12-03 11:06 ` André Draszik
2024-12-04 8:49 ` Thomas Antoine
0 siblings, 1 reply; 38+ messages in thread
From: André Draszik @ 2024-12-03 11:06 UTC (permalink / raw)
To: Thomas Antoine, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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'
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 11:06 ` André Draszik
@ 2024-12-04 8:49 ` Thomas Antoine
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Antoine @ 2024-12-04 8:49 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
On 12/3/24 12:06, André Draszik wrote:
> 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.
Thank you very much, I'll try this out.
Best regards,
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 6:47 ` André Draszik
2024-12-03 7:23 ` André Draszik
@ 2024-12-03 9:08 ` Thomas Antoine
2024-12-03 9:31 ` André Draszik
1 sibling, 1 reply; 38+ messages in thread
From: Thomas Antoine @ 2024-12-03 9:08 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 9:08 ` Thomas Antoine
@ 2024-12-03 9:31 ` André Draszik
2024-12-03 10:11 ` Thomas Antoine
0 siblings, 1 reply; 38+ messages in thread
From: André Draszik @ 2024-12-03 9:31 UTC (permalink / raw)
To: Thomas Antoine, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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'
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 9:31 ` André Draszik
@ 2024-12-03 10:11 ` Thomas Antoine
2024-12-03 10:50 ` Peter Griffin
2024-12-03 11:02 ` André Draszik
0 siblings, 2 replies; 38+ messages in thread
From: Thomas Antoine @ 2024-12-03 10:11 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 10:11 ` Thomas Antoine
@ 2024-12-03 10:50 ` Peter Griffin
2024-12-03 11:02 ` André Draszik
1 sibling, 0 replies; 38+ messages in thread
From: Peter Griffin @ 2024-12-03 10:50 UTC (permalink / raw)
To: Thomas Antoine
Cc: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Alim Akhtar, linux-pm, linux-kernel,
devicetree, linux-arm-kernel, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 10:11 ` Thomas Antoine
2024-12-03 10:50 ` Peter Griffin
@ 2024-12-03 11:02 ` André Draszik
2024-12-04 8:53 ` Thomas Antoine
1 sibling, 1 reply; 38+ messages in thread
From: André Draszik @ 2024-12-03 11:02 UTC (permalink / raw)
To: Thomas Antoine, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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'
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 11:02 ` André Draszik
@ 2024-12-04 8:53 ` Thomas Antoine
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Antoine @ 2024-12-04 8:53 UTC (permalink / raw)
To: André Draszik, Sebastian Reichel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Dimitri Fedrau,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar
Cc: linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
On 12/3/24 12:02, André Draszik wrote:
> 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.
Will implement this for v2, thank you.
Best regards,
Thomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-02 13:07 ` Thomas Antoine via B4 Relay
(?)
(?)
@ 2024-12-03 7:35 ` Dimitri Fedrau
2024-12-03 9:25 ` Thomas Antoine
-1 siblings, 1 reply; 38+ messages in thread
From: Dimitri Fedrau @ 2024-12-03 7:35 UTC (permalink / raw)
To: t.antoine
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar,
linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge
2024-12-03 7:35 ` Dimitri Fedrau
@ 2024-12-03 9:25 ` Thomas Antoine
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Antoine @ 2024-12-03 9:25 UTC (permalink / raw)
To: Dimitri Fedrau
Cc: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Catalin Marinas, Will Deacon, Peter Griffin, Alim Akhtar,
linux-pm, linux-kernel, devicetree, linux-arm-kernel,
linux-samsung-soc
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
^ permalink raw reply [flat|nested] 38+ messages in thread