drivers: Add fuel gauge max17043 #93021
Conversation
github-actions
bot
requested review from
aaronemassey,
DBS06,
galak,
MaureenHelm,
microbuilder,
ozersa,
teburd,
ttmut and
yasinustunerg
Add support for fuel gauge max17043. This device is similar to the max17048 and so it was used as the basis. There are slight differences in the registers which are accounted for in the driver. Functionality was tested using a Thingy 91x. Signed-off-by: Mohammed Billoo <mab@mab-labs.com>
|
| int rc = max17043_read_register(dev, REGISTER_VERSION, &version); | ||
|
|
||
| if (!device_is_ready(cfg->i2c.bus)) { | ||
| LOG_ERR("Bus device is not ready"); | ||
| return -ENODEV; | ||
| } |
There was a problem hiding this comment.
Operations seem to be out of order. The bus should be checked before using it.
There was a problem hiding this comment.
We should also change this in drivers/fuel_gauge/max17048.
I never really looked closely into the max17048 driver which @mabembedded took as a template for the max17043, but to see all this minor issues which are the same in the max17048 I am thinking about "refactoring" the max17048 driver and extend it with some missing functionality.
There was a problem hiding this comment.
Thanks - should I create another PR for fixes to the max17048 after addressing issues in this PR?
There was a problem hiding this comment.
@mabembedded I think you can do it both ways. If you decide to do it in the same PR, split them up in 2 commits one for the max17043 and one for the changes in max17048.
| case FUEL_GAUGE_RELATIVE_STATE_OF_CHARGE: | ||
| rc = max17043_percent(dev, &data->charge); | ||
|
|
||
| if (rc < 0) { | ||
| return rc; | ||
| } | ||
|
|
||
| val->relative_state_of_charge = data->charge; | ||
| break; | ||
| case FUEL_GAUGE_VOLTAGE: | ||
| rc = max17043_voltage(dev, &data->voltage); | ||
| if (rc < 0) { | ||
| return rc; | ||
| } | ||
| val->voltage = data->voltage; | ||
| break; |
There was a problem hiding this comment.
data->charge and data->voltage are set but never actually used.
The function uses them as local variables.
Did I miss something?
| * obtain µV | ||
| */ | ||
|
|
||
| *response = (uint32_t)raw_voltage * 78.125; |
There was a problem hiding this comment.
Is there a way to not use floating point unless CONFIG_FPU=y?
E.g. the following change should give the same result but without floating point calculations.
- *response = (uint32_t)raw_voltage * 78.125;
+ *response = ((uint32_t)raw_voltage * 78125) / 1000;There was a problem hiding this comment.
We should change this in drivers/fuel_gauge/max17048 too
| return rc; | ||
| } | ||
|
|
||
| static const struct fuel_gauge_driver_api max17043_driver_api = { |
There was a problem hiding this comment.
-static const struct fuel_gauge_driver_api max17043_driver_api = {
+static DEVICE_API(fuel_gauge, max17043_driver_api) = {| #include <zephyr/pm/device.h> | ||
| #include <zephyr/sys/byteorder.h> | ||
| #include <zephyr/sys/__assert.h> |
There was a problem hiding this comment.
It seems that zephyr/pm/device.h and zephyr/sys/__assert.h are not used by the driver.
There was a problem hiding this comment.
First of all, thanks for your contribution!
I only did a quick overlook, most likely I will do a deeper review next week.
So far I noticed that the test and the max17043-emulation is missing, which would be nice to add, see tests/drivers/fuel_gauge and drivers/fuel_gauge/max17048/emul_max17048.c.
| #define RESET_COMMAND 0x5400 | ||
| #define QUICKSTART_MODE 0x4000 | ||
|
|
||
| struct max17043_config { |
There was a problem hiding this comment.
The max17043_config struct can be moved inside the max17043.c, because I don't see a need to have it in the header.
| /** | ||
| * Storage for the fuel gauge basic information | ||
| */ | ||
| struct max17043_data { |
|
|
||
| #define REGISTER_VCELL 0x02 | ||
| #define REGISTER_SOC 0x04 | ||
| #define REGISTER_MODE 0x06 |
There was a problem hiding this comment.
Does it make sense from your perspective to implement/add this and the following registers?
Can the existing driver be updated to support both models? |
Add support for the fuel gauge max17043. This device is similar to the max17048, and so it was used as the basis. There are slight differences in the registers, which are accounted for in the driver. Functionality was tested on a Nordic Thingy91x.