New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fuel_gauge: Add battery cutoff support #61435
fuel_gauge: Add battery cutoff support #61435
Conversation
136ea21
to
564483b
Compare
* Why not devicetree: Finding the maximum length of all the battery cutoff payloads in a devicetree | ||
* at compile-time would require labyrinthine amount of macro-batics. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a fine trade off for this use case.
As an alternative, you could allocate an array of cutoff_payload
for each fuel gauge based on a devictree property length and then link to the array from the driver's static config
. This example uses the DT_INST_FOREACH_PROP_ELEM
to allocate the array. However, this approach would likely consume a little more memory than what you've done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Will keep current solution for now. Not resolving comment so other reviewers have visibility on this.
battery cutoff support. See the default fuel gauge SBS Gauge compatible as an example. | ||
|
||
properties: | ||
battery_cutoff_support: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devicetree property names should not use underscores.
battery_cutoff_support: | |
battery-cutoff-support: |
type: int | ||
battery_cutoff_payload: | ||
description: | | ||
Payload to write to cutoff battery. This is often a sequence of two duplicate values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Payload to write to cutoff battery. This is often a sequence of two duplicate values. | |
Payload to write to cutoff battery register. This must be an array of 2 integers. | |
If your battery only requires a single payload, duplicate the value. This will | |
cause the driver to issue the cutoff twice. |
8155d86
to
c6f5791
Compare
/* FIXME: fix init priority */ | ||
#ifdef SBS_GAUGE_CUTOFF_SUPPORT | ||
#define SBS_GAUGE_CONFIG_INIT(index) \ | ||
.cutoff_support = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_support, false), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For boolean types, I believe DT_NODE_HAS_PROP
is always true. So I think this can be simplified:
.cutoff_support = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_support, false), \ | |
.cutoff_support = DT_INST_PROP(index), battery_cutoff_support), \ |
Payload to write to cutoff battery register. This must be array of 2 integers. If your | ||
battery only requires a single payload, duplicate the value. This will cause the driver to | ||
issue the cutoff twice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking closer at your macros, you actually support a payload length of 1 or 2. Change this help to note you can specify a maximum of 2 integers and drop the text about setting a duplicate value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, though I was fine with being restrictive at first and just relaxing that as needed.
Changing.
#define SBS_GAUGE_VALIDATE_CUTOFF_PAYLOAD_SIZE(index) \ | ||
(true _SBS_GAUGE_VALIDATE_CUTOFF_PAYLOAD_SIZE(index)) | ||
|
||
BUILD_ASSERT(DT_INST_FOREACH_STATUS_OKAY(SBS_GAUGE_VALIDATE_CUTOFF_PAYLOAD_SIZE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified as:
#define CUTOFF_PAYLOAD_SIZE_ASSERT(inst) \
BUILD_ASSERT(DT_INST_PROP_OR(inst, battery_cutoff_payload, 0) <= \
SBS_GAUGE_CUTOFF_PAYLOAD_MAX_SIZE)
DT_INST_FOREACH_STATUS_OKAY(CUTOFF_PAYLOAD_SIZE_ASSERT)
.cutoff_reg_addr = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_reg_addr, 0), \ | ||
.cutoff_payload = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_payload, {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.cutoff_reg_addr = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_reg_addr, 0), \ | |
.cutoff_payload = DT_PROP_OR(DT_DRV_INST(index), battery_cutoff_payload, {}), | |
.cutoff_reg_addr = DT_INST_PROP_OR(battery_cutoff_reg_addr, 0), \ | |
.cutoff_payload = DT_INST_PROP_OR(battery_cutoff_payload, {}), |
* gauge. For the case where it's a singular value that must only be written to the fuel gauge only | ||
* once, retransmitting the duplicate write has no significant negative consequences. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete reference to retransmitting duplicate value, since your code doesn't require that.
return -ENOTSUP; | ||
} | ||
|
||
for (int i = 0; i < ARRAY_SIZE(cfg->cutoff_payload); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int i = 0; i < ARRAY_SIZE(cfg->cutoff_payload); i++) { | |
for (int i = 0; i < cfg->cutoff_payload_size; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually cutoff_payload_size
only exists in the emulator as a helper to prevent usages of ARRAY_SIZE()
.
But I realize that's a bit confusing so I'll remove the emulator's cutoff_payload_size in favor of ARRAY_SIZE()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver should capture a cutoff_payload_size
. ARRAY_SIZE(cfg->cutoff_payload)
will always be equal to SBS_GAUGE_CUTOFF_PAYLOAD_MAX_SIZE
.
This code will write a 0 to the cutoff register if the user only provides a single payload value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh jeez. I should've jumped off earlier looking back at that this morning. Thanks!
} | ||
|
||
for (int i = 0; i < ARRAY_SIZE(cfg->cutoff_payload); i++) { | ||
rc = sbs_cmd_reg_write(dev, cfg->cutoff_reg_addr, cfg->cutoff_payload[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need a test case where the payload value is not duplicated.
rc = sbs_cmd_reg_write(dev, cfg->cutoff_reg_addr, cfg->cutoff_payload[0]); | |
rc = sbs_cmd_reg_write(dev, cfg->cutoff_reg_addr, cfg->cutoff_payload[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some doc nits
@@ -46,8 +57,48 @@ struct sbs_gauge_emul_data { | |||
struct sbs_gauge_emul_cfg { | |||
/** I2C address of emulator */ | |||
uint16_t addr; | |||
bool cutoff_support; | |||
uint32_t cutoff_reg_addr; | |||
int cutoff_payload[SBS_GAUGE_CUTOFF_PAYLOAD_MAX_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comments for these?
How about s/cutoff_reg_addr/cutoff_reg/ ?
@@ -46,8 +57,48 @@ struct sbs_gauge_emul_data { | |||
struct sbs_gauge_emul_cfg { | |||
/** I2C address of emulator */ | |||
uint16_t addr; | |||
bool cutoff_support; | |||
uint32_t cutoff_reg_addr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u16, or int?
* We thoroughly check bounds elsewhere, so we can be confident we're not indexing | ||
* past the end of the array. | ||
*/ | ||
uint16_t target_payload_elem_val = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is u16 but above it uses int
|
||
description: | | ||
|
||
Properties for fuel-gauges that may control battery cutoff, this is common in SBS compliant or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SBS-compliant
description: | | ||
|
||
Properties for fuel-gauges that may control battery cutoff, this is common in SBS compliant or | ||
similarly smart battery fuel gauges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the word 'battery' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to differentiate from other future forms of cutoff
such as with charging.
Alternatively, I could rename all of this to ship_mode. I prefer the battery cutoff nomenclature because it's more self-defining.
similarly smart battery fuel gauges. | ||
|
||
Note: These properties are to be used with meaningful defaults in fuel gauge ICs that implement | ||
battery cutoff support. See the default fuel gauge SBS Gauge compatible as an example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the word 'support' ?
What does 'default fuel gauge SBS Gauge compatible' mean? It might be missing hyphens, but I'm not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (hope) I made it more clear in the following push.
properties: | ||
battery-cutoff-support: | ||
description: | | ||
Helper prop that indicates whether this device can cutoff the battery, this is also often |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
battery. This
(or use a semicolon?)
include: ["sbs,sbs-gauge-new-api.yaml", "battery_cutoff.yaml"] | ||
|
||
description: | | ||
Default generic smart battery fuel gauge driver. Includes support for battery cutoff if enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think nearly everywhere you use this it should be 'battery-cutoff'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you should just say 'cutoff' an then mention in one place that it means cutting off the battery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep it to battery cutoff to be more explicit and differentiating from charge cutoff.
@@ -34,6 +34,7 @@ extern "C" { | |||
*/ | |||
__subsystem struct fuel_gauge_emul_driver_api { | |||
int (*set_battery_charging)(const struct emul *emul, uint32_t uV, int uA); | |||
int (*is_battery_cutoff)(const struct emul *emul, bool *cutoff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these not get comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically no. The client does not typically interact with the API struct directly but through functions defined in the same header.
@@ -66,6 +67,26 @@ static inline int emul_fuel_gauge_set_battery_charging(const struct emul *target | |||
return backend_api->set_battery_charging(target, uV, uA); | |||
} | |||
|
|||
/** | |||
* @brief Check if the underlying emulator has battery cutoff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is checking whether the battery is actually cut off, not whether the emulator has the 'cutoff' feature.
Perhaps it would be easier if you just said 'Check if the battery is cut off' ?
The regulator APIs have a call for shipping mode: zephyr/include/zephyr/drivers/regulator.h Line 67 in 03905f7
No arguments or anything fancy, just a place to put the code, would it make sense to use that one instead? |
c6f5791
to
76c320c
Compare
So doing this would require creating multiple device instances and them having them coordinate and be cohesive. This seems like more trouble than it'd be worth over duplicating this in the fuel gauge API given the current constraints on Zephyr's device driver model. I'd appreciate @keith-zephyr adding a word here. |
A battery fuel gauge is not a regulator, so I don't see adding 'cutoff' to batteries as redundant with regulators. It just so happens both device types provide a similar function. Trying to shoehorn this into the regulator parent would also mean the fuel gauge would need to stub out |
76c320c
to
0229d63
Compare
Ah you could leave that unset and let the API stub check it. Was thinking you could do a child device just for that function but yeah that does seem a bit overkill. Thing is that this functionality can go to few different places, for instance https://www.ti.com/product/BQ25180 is a lipo charger and also has a ship mode, so we'd end up with an api for that in regulator, one in fuel gauge (which probably applies only for SBS batteries?) and one for chargers. |
bb7d099
to
c5b8846
Compare
battery-cutoff-support: | ||
description: | | ||
Helper prop that indicates whether this device can cutoff the battery; this is also often | ||
referred to as ship or sleep mode. Any non-zero value means battery cutoff is supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now a boolean property, so the "non-zero" comment no longer applies.
referred to as ship or sleep mode. Any non-zero value means battery cutoff is supported. | |
referred to as ship or sleep mode. |
return -ENOTSUP; | ||
} | ||
|
||
for (int i = 0; i < ARRAY_SIZE(cfg->cutoff_payload); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver should capture a cutoff_payload_size
. ARRAY_SIZE(cfg->cutoff_payload)
will always be equal to SBS_GAUGE_CUTOFF_PAYLOAD_MAX_SIZE
.
This code will write a 0 to the cutoff register if the user only provides a single payload value.
c5b8846
to
1322276
Compare
|
Should be fixed, reran CI for you. |
1322276
to
3e4b314
Compare
Whoops accidentally combined with a prior commit and pushed. Fixed in https://github.com/zephyrproject-rtos/zephyr/compare/1322276e2bf9f6985a9ad58e5ad1bd81e95db701..3e4b314c7c183762e38a9f4a3ef0c670a9c1ae1c |
3e4b314
to
323c4f4
Compare
#define _SBS_GAUGE_INST_CUTOFF_SUPPORTED(inst) \ | ||
DT_PROP_OR(DT_DRV_INST(inst), battery_cutoff_support, false) | ||
#define SBS_GAUGE_CUTOFF_SUPPORT() \ | ||
(false || DT_INST_FOREACH_STATUS_OKAY(_SBS_GAUGE_INST_CUTOFF_SUPPORTED)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach :-)
How about instead of this you split the cutoff fields in a separate config struct, only instantiate it if the specific instance has the property and then leave the handler there all the time (it only takes few bytes of flash) and return an error instantly if this structure is not set? (meaning the battery does not support it)
Or I guess you could rip off the multi instance support if we don't need it, though that'd be sad, I really miss my X250 :_(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you already have cutoff_support
, so yeah you could replace that with a struct pointer, if it's null you don't have cutoff support, if it's set you do and the data is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sgtm, having the extra struct is easier to manage, gives grouping to the associated properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiobaltieri But allocating a different struct makes no change to needing SBS_GAUGE_CUTOFF_SUPPORT()
unless I'm missing something?
You're talking about just collecting constant parameters as a separate struct, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to drop SBS_GAUGE_CUTOFF_SUPPORT()
entirely if you only allocate the extra structure if battery_cutoff_support
is there, then sbs_gauge_do_battery_cutoff
is always compiled in but validates that the structure is not there and returns -ENOSUP
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's certainly cleaner. I'm totally happy to do that if we don't care about sbs_gauge_do_battery_cutoff
always being compiled in and nominally increasing the code size.
However, I think it's more broadly suitable if we keep it compiled out. Battery cutoff is NOT standard SBS behavior so many folks might be willing to use this driver without wanting the battery cutoff feature.
I'll do that then. We can always revert it back to using the helper macro if requested :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always have a Kconfig option to compile that out if one really wants to save those few bytes, but it's just flash so I would not worry too much about it.
zephyrproject-rtos/zephyr#61435 Many fuel gauge ICs offer a battery cutoff/shipping mode functionality that cutoff charge from the battery. This is often useful for preserving battery charge on devices while in storage. Add battery cutoff support to the fuel gauge API with a generic default SBS driver showing an example of support in tests. b:266855499 Change-Id: Ic3dcff8a5a1f6afc116da901daaf23a25a14681b Signed-off-by: Aaron Massey <aaronmassey@google.com> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4785449 Reviewed-by: Simon Glass <sjg@chromium.org> Commit-Queue: Simon Glass <sjg@chromium.org>
323c4f4
to
d6c78fd
Compare
Last push was rebase only. |
d6c78fd
to
1c3f2f0
Compare
1c3f2f0
to
4df450a
Compare
Many fuel gauge ICs offer a battery cutoff/shipping mode functionality that cutoff charge from the battery. This is often useful for preserving battery charge on devices while in storage. Add battery cutoff support to the fuel gauge API with a generic default SBS driver showing an example of support in tests. Signed-off-by: Aaron Massey <aaronmassey@google.com>
4df450a
to
adee51b
Compare
Many fuel gauge ICs offer a battery cutoff/shipping mode functionality that cutoff charge from the battery. This is often useful for preserving battery charge on devices while in storage.
Add battery cutoff support to the fuel gauge API with a generic default SBS driver showing an example of support in tests.
Please see #61434 for more information on motivation and a graphic to gain some quick context on this PR.